RFR: 8265372: Simplify PKCS9Attribute

Weijun Wang weijun at openjdk.org
Thu Dec 21 17:32:48 UTC 2023 

On Fri, 15 Dec 2023 20:34:21 GMT, Ben Perez <duke at openjdk.org> wrote:

> Refactored PKCS9Attribute to use a hash map instead of multiple arrays. The key for the hash map is an `ObjectIdentifier` and the values are a record `AttributeInfo` that stores the information previously contained in the arrays `PKCS9_VALUE_TAGS`, `VALUE_CLASSES`, and `SINGLE_VALUED`. 
> 
> It seems as though we should be able to get rid of constants such as `EMAIL_ADDRESS_OID` since they aren't heavily used with the hash map approach, but since the values are public it might cause compatibility issues.
> 
> Another question is how to handle `RSA DSI`, `S/MIME`, `Extended-certificate`, and `Issuer Serial Number` OIDs. The prior version threw an error but in this refactor they are treated as an "unknown OID" and only throw a debug warning. This was addressed in https://bugs.openjdk.org/browse/JDK-8011867 but prior to this refactor the aforementioned OIDs were treated differently than unknown OIDs.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 186:

> 184:     /**
> 185:      * Array of attribute OIDs defined in PKCS9, by number.
> 186:      */

I don't think `PKCS9_OIDS` is useful now. It's used in `PKCS9Attributes.getAttributes()` but this method is used nowhere. It's also used in `PKCS9Attributes.toString` but we can just iterate through `attributes` there. I don't see a reason to print the attributes in this order. If we want to print them in the order they appear in the data, we can use `LinkedHashMap` to in `PKCS9Attributes`. `Hashtable` is a little stale.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 193:

> 191:     static {
> 192:         try {
> 193:             BYTE_ARRAY_CLASS = Class.forName("[B");

Is this simply `byte[].class`?  In fact I wonder if it's worth define a variable for this. Just use the literal directly everywhere.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 220:

> 218:         ObjectIdentifier.of(KnownOIDs.IssuerAndSerialNumber);
> 219:     // [11], [12] are RSA DSI proprietary
> 220:     // [13] ==> signingDescription, S/MIME, not used anymore

No need to talk about indexes in the array now since there is no array.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 230:

> 228:         ObjectIdentifier.of(KnownOIDs.CMSAlgorithmProtection);
> 229: 
> 230:     private static final Map<ObjectIdentifier,AttributeInfo> oidMap = new LinkedHashMap<>();

Why `Linked`? Need an order somewhere?

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 232:

> 230:     private static final Map<ObjectIdentifier,AttributeInfo> oidMap = new LinkedHashMap<>();
> 231:     private static void add(ObjectIdentifier oid, boolean singleValued,
> 232:                             Class<?> valueClass, Byte[] valueTags) {

I think there is no need to use `Byte`, just using `byte` is OK. There is nowhere that the tag can be `null`. The `indexOf()` method is only used once and it needs not be generel.

Also, if you make the last argument above a vararg (`byte... valueTag`), there will be no need to write so many `new Byte[] {` in the `add` calls.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 241:

> 239:     static {
> 240:         try {
> 241:             Class<?> str = Class.forName("[Ljava.lang.String;");

`String[].class`. In fact I wonder if it's worth define a variable for this. Just use the literal directly everywhere.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 255:

> 253: 
> 254:             add(CONTENT_TYPE_OID, true,
> 255:                 Class.forName("sun.security.util.ObjectIdentifier"),

`sun.security.util.ObjectIdentifier.class`, same below.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 287:

> 285:                 new Byte[]{DerValue.tag_Sequence});
> 286: 
> 287:             add(SIGNING_CERTIFICATE_OID, true, null,

Is the `null` class here safe? `SigningCertificateInfo` is already used elsewhere.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 308:

> 306:     /**
> 307:      * The index of the OID of this attribute in <code>PKCS9_OIDS</code>,
> 308:      * or -1 if it's unknown.

Rewrite comment. Also for `value`.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 339:

> 337: 
> 338:     private void init(ObjectIdentifier oid, Object value)
> 339:         throws IllegalArgumentException {

If we inline `init` into its caller, there is a chance to make all fields `final`.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 344:

> 342:         info = oidMap.get(oid);
> 343:         Class<?> clazz = (info == null) ? BYTE_ARRAY_CLASS : info.valueClass();
> 344:         if (clazz == null) {

If we assign a class to `SIGNING_CERTIFICATE_OID`, this will never be null.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 462:

> 460:             break;
> 461: 
> 462:         default: // Can't happen

Then throw an exception.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 480:

> 478: 
> 479:         KnownOIDs knownOID = KnownOIDs.findMatch(oid.toString());
> 480:         if (knownOID == null) {

The OID might be defined in `KnownOIDs` but not supported here. Then you might fall into the `default` case below. I think a better check is to see if `info` is null.

src/java.base/share/classes/sun/security/pkcs/PKCS9Attribute.java line 574:

> 572:             break;
> 573: 
> 574:         default: // Can't happen

Then throw an exception.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434277885
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434283698
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434284316
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434284947
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434293465
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434296628
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434300444
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434309185
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434306645
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434330844
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434310225
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434331159
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434336294
PR Review Comment: https://git.openjdk.org/jdk/pull/17132#discussion_r1434336632

More information about the security-dev mailing list