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

Wed Apr 29 23:55:37 UTC 2020

Hi Max,

I believe most of your comments (except the two below) in this email 
should already be addressed in webrev.01.

> I caught them by looking at the "skipped" debug output. Can we add some logic to detect this? For example, for those nonpreferred OIDs, use a special constructor?
Hmm, ok, let me think about this. Will see what I can do in webrev.02.

> We can also populate the name2enum map inside the constructor, there is no need to store the aliases in each enum.
I tried this approach earlier and ran into some class initialization 
error when building JDK. So, I still populate the name2enum map in the 
static initialization block.

Thanks,
Valerie
On 4/27/2020 7:15 PM, Weijun Wang wrote:
>> On Apr 28, 2020, at 6:19 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> 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.
> I'd rather use the old name.
>
>>> 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.
> computeOidTable() is only called by name2oid() only called by name2oidStr() only called by algOID() which is in AlgorithmId.
>
> algOID() in AlgorithmId is called by AlgorithmId::get which makes it quite the core of that class.
>
>>> 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.
> I caught them by looking at the "skipped" debug output. Can we add some logic to detect this? For example, for those nonpreferred OIDs, use a special constructor?
>
>>>     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?
> Oh, I meant lines 415 - 428.
>
>> 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.
> We can also populate the name2enum map inside the constructor, there is no need to store the aliases in each enum.
>
>>> 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.
> Good to know.
>
>>> 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...
> Fair enough.
>
> If we want to eliminate the use of these objects later, is there a way to not create a new ObjectIdentifier object each time? I think I've read some C projects that defines OID as the DER encoding but not as strings. String is only best for log usages. But this is a much broader topic.
>
>>> 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,
> Max
>
>> 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
>>>>