Hi Max,
Thanks for reviewing this~ Please find my replies inline.
On 4/25/2020 3:28 AM, Weijun Wang wrote:
OidString.java
==============
1. ExtendedKeyUsage names: used to be "serverAuth", now "ServerAuth". First
letter capitalized, is this necessary?
Yes, I made a change here. Using "ServerAuth" seems more consistent with
the rest of constants in OidString. So I made a change here. It affects
the displayed out but no regression tests are affected by this change.
Well, if we want to be 100% same as before, I can change these to start
with lower case, but then we probably need comments so it's clear that
these are intentional changes.
2. Can we move name2oidStr() from OidString to AlgorithmId? The computeOidTable
process looks like an alien.
Well, I think it's better to consolidate all oid string/algorithm names
to as few places as possible.
AlgorithmId class used to contain a lot of such conversions and now that
there is OidString class (we can discuss a better name for it), it
should be updated to use OidString and just be a general impl class for
handling ASN AlgorithmId structure. Let me try putting this utility
method name2oidStr() elsewhere and see.
3. Two questions on the following lines:
415 // set extra alias mappings or specify the preferred one when
416 // one standard name maps to multiple enums
417 // NOTE: key must use UPPER CASE
418 name2enum.put("SHA1", SHA_1);
419 name2enum.put("SHA", SHA_1);
420 name2enum.put("SHA224", SHA_224);
421 name2enum.put("SHA256", SHA_256);
422 name2enum.put("SHA384", SHA_384);
423 name2enum.put("SHA512", SHA_512);
424 name2enum.put("SHA512/224", SHA_512$224);
425 name2enum.put("SHA512/256", SHA_512$256);
426 name2enum.put("DH", DiffieHellman);
427 name2enum.put("DSS", SHA1withDSA);
428 name2enum.put("RSA", RSA);
a) For line 428, is this because both RSA and ITUX509_RSA have the same stdName and you are setting the
preferred one? However, I can see that "DiffieHellman", "DSA", and
"SHA1withDSA" also appear in multiple places. Do they need special attention?
Yes, initially I was relying on the ordering of enums to handle the
1-to-N mapping. Later, I changed to explicitly define the mapping as in
the webrev as this should be more robust. I should have added other
algorithms here too. Will update.
b) For the other lines, can we make this info somewhere inside the
constructor? After all our goal is to consolidate all info in one single place,
and a single line is better than a single file, esp, a very big file.
For other lines => are you referring to the extra aliases? Instead of
specifying the aliases for each enum explicitly as in current webrev,
store them into the constructor and save them into the name2enum map
while looping through the enum values? The current approach has the
benefit of being straightforward and no need to worry about duplicate
aliases.
4. Are you sure the OID <-> name mapping is always the same everywhere (for all
primitives and in all providers)? I mean for a stdName, if one OID alias is added in
one place, should it always be added the same way in another? Have you compared the
aliases map after this change?
OID is tied to an algorithm name. Thus, it should be universal to all
JDK providers as long as the algorithm impl is interoperable. In the
past, it's very easy to miss adding the OIDs as they are hardcoded in
different places. For providers who are lower in the provider list,
registering the OIDs may not be that important if a more preferred
provider already supports the same impl. Rather than registering
identical entries, with this change, the OID support should be
consistent across providers.
5. I found KnownOIDs to be a better class name.
Sure, fine with me.
AlgorithmId.java
================
There are still many lines like
public static final ObjectIdentifier MD2_oid = algOID(OidString.MD2);
here. Can they be eliminated? I use IntelliJ IDEA to find their usages and most
are used in only one place and some are not used at all.
Yes, I debated about it. It's tricky to draw the line. My current
thought is to keep these "public static" constants intact if they are
directly referenced somewhere. There are also many places where I think
further trimming/cleanup can be made. But as it is, it is already
extensive. Maybe it's safer to be conservative and gradually clean up...
I haven't read other files yet. Will send more comment later.
Sure, these two are the key files.
I will experiment on the utility method and we can discuss it further.
Thanks,
Valerie
Thanks,
Max
On Apr 24, 2020, at 7:11 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
Hi Max,
Would you have time to review this change? The current webrev attempts to cover
all security classes where hard-coded oid strings and consolidate these known
oid string values into a single enum type. The changes are quite extensive, I
can trim back and only cover the provider algorithm oids if you prefer. There
are pros and cons for both approach.
I know that the naming convention is to use all upper case for enum constants,
but choose to use the same naming convention as standard names to simplify the
code. SecurityProviderConstants class defines the common mappings which are
general to providers. Provider-specific alias mappings are handled in specific
provider class, e.g. SunJSSE class.
RFE: https://bugs.openjdk.java.net/browse/JDK-8242151
Webrev: http://cr.openjdk.java.net/~valeriep/8242151/webrev.00/
Mach5 runs clean.
Valerie
