> On Apr 28, 2020, at 6:19 AM, Valerie Peng <valerie.p...@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.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
>>>
