[15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration

[Valerie Peng]

Hi Max,

Thanks for the review, I find your comments very useful.

Please find responses inline.

On 5/6/2020 5:48 AM, Weijun Wang wrote:
> - PBES2Parameters.java:
>
> In parseKDF(), any OID can be accepted as kdfAlgo, wonder if it would lead to unexpected result:
>
> jshell> var a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
>
> jshell> a.init(new PBEParameterSpec("hello".getBytes(), 100, new IvParameterSpec("iv".getBytes())))
>
> jshell> var b = a.getEncoded()
>
> jshell> b
> b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12, 48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42, -122, 72, -122, -9, 13, 2, 9, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4, 1, 42, 4, 2, 105, 118 }
>
> // Modify HmacSHA512("1.2.840.113549.2.11") to MD2("1.2.840.113549.2.2")
> jshell> b[41] = 2
> $44 ==> 2
>
> jshell> b
> b ==> byte[61] { 48, 59, 48, 40, 6, 9, 42, -122, 72, -122, -9, 13, 1, 5, 12, 48, 27, 4, 5, 104, 101, 108, 108, 111, 2, 1, 100, 2, 1, 32, 48, 12, 6, 8, 42, -122, 72, -122, -9, 13, 2, 2, 5, 0, 48, 15, 6, 9, 96, -122, 72, 1, 101, 3, 4, 1, 42, 4, 2, 105, 118 }
>
> jshell> a = AlgorithmParameters.getInstance("PBEWithHmacSHA256AndAES_256")
>
> jshell> a.init(b)
>
> jshell> a
> a ==> PBEWithMD2AndAES_256

Ok, good catch, I will add some check back in (webrev.03) to at least 
match exsting impl.

It seems that existing impl of PBES2Parameters class only enforces that 
the KDF algo is one of the HmacSHAxxx. But it does not throw exception 
if the instance is requested with "PBEWithHmacSHA256AndAES_256" and then 
initialized with DER encoding containing "PBEWithHmacSHA512AndAES_256". 
Perhaps it should be further tightened up?

> - PKCS9Attribute.java:
>
> It looks you can declare and initialize and at the same time make it an element in an array at the same time, like this
>
>      public static final ObjectIdentifier EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
>              ObjectIdentifier.of(KnownOIDs.EmailAddress);
>
> Then there is no need to write two lines
>
>          EMAIL_ADDRESS_OID = PKCS9_OIDS[1] =
>                  ObjectIdentifier.of(KnownOIDs.EmailAddress);
>          public static final ObjectIdentifier EMAIL_ADDRESS_OID;
>
> Also, looks like the strings are never used anywhere inside JDK
>
>          public static final String EMAIL_ADDRESS_STR = "EmailAddress";
>          ...
>
> Can we remove them? If we want them back, it can be KnownOIDs.stdName.
Ok, I removed them and updated some more source and test to use the 
other PKCS9Attribute constructor which takes an oid instead. It seems 
that the constructor which takes String then become unused with this 
change, so I commented it out (in webrev.03).
> - AlgorithmId.java:
>
> getName(): Can we use the collected map from 3rd party providers? I asked this last time.
Sure. I have just replied to your other email. Will include in webrev.03.
> - OIDMap.java:
>
> Maybe we can simplify this file later.
Yes, looks like some more house scrubbing is needed later.
> - KnownOIDs.java:
>
> register(): Are you throwing a RuntimeException only when debug is true? This sounds incorrect.
>
>              "already registered; no need to continue;"  Can this happen? You don't manually register preferred ones now.

Correct, should have removed the debug check. Missed this one.

Also removed the "already registered" part of code as it is now obsolete 
(in webrev.03).

Thanks,

Valerie

> No other comment.
>
> Thanks,
> Max
>
> p.s. JGSS uses ObjectIdentifier also, but it also has its own public Oid class. Maybe we can think about it in a different RFE.
>
>
>> On May 6, 2020, at 11:18 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> Hi Max,
>>
>> Webrev has been updated, http://cr.openjdk.java.net/~valeriep/8242151/webrev.02/.
>>
>> Major changes are:
>>
>> - Moved oidTable caching from AlgorithmId class to ObjectIdentifier class. Made ObjectIdentifier constructor private and callers have to use the of(String) method which always check the oidTable cache before creating new instances. Some more files (src files and tests) are added to the webrev due to the private ObjectIdentifier constructor change.
>>
>> - Arrays.asList() calls are now replaced with List.of() calls.
>>
>> - KnownOIDs enum has been enhanced with registerName() method for ensuring that same standard name can have at most one enum mapping. Added the method stdName() instead of relying on toString(). Added aliases support to KnownOIDs enums. Note that external aliases are in SecurityProviderConstants class. The two non-oid BASE ones are removed and keytool/Main.java is updated.
>>
>> - SecurityProviderConstants class will pick up the internal aliases and combine with external aliases and handle multiple oid enums in its store(...) method.
>>
>> - Updated SunRsaSign provider to use the same naming convention (append the letter A) and fixed its KeyFactory and KeyPairGenerator to use the same oid as before. Also update providers outside of the java.base module in a similar fashion.
>>
>> As for the comments below. Please find replies inline.
>>
>> On 5/4/2020 6:24 PM, Weijun Wang wrote:
>>> Do you want to add OIDs in CurveDB into KnownOIDs as well?
>> Sure, will do in webrev.03.
>>
>> Thanks,
>>
>> Valerie
>>
>>> Thanks,
>>> Max