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.
- 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 <[email protected]> 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, ValerieThanks, Max
