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

[Valerie Peng]

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.peng at 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
>>