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

[Valerie Peng]

Hi Max,

Thanks for review~

Webrev is updated accordingly for your comments: 
http://cr.openjdk.java.net/~valeriep/8242151/webrev.04/

Please find replies inline below:

On 5/11/2020 9:24 PM, Weijun Wang wrote:
> Some files have trailing spaces.
Yes, I generally check for white spaces before integration. Now that you 
mention it, I checked and included this in webrev.04.
> KnownOIDs.java:
>
>   - Is there an order for the fields? I see they are grouped but now always ordered.

Well, the fields are grouped based on their oid with comments stating 
their parent node value. Within the same group, they are ordered so it's 
easier to spot missing/gap values.

The less used ones are placed at the end.

>
>   - Unused "import java.security.Provider;"
Removed.
>   - 1.3.14.3.2.29 used to be an alias for SHA1withRSA, now it's not. Is this intended?
Good catch. I added the necessary entry to SecurityProviderConstants 
class now.
>   - s/_/-/ in process() is now used for
>
>       SHA3_224 -> SHA3-224
>       SHA3_256 -> SHA3-256
>       SHA3_384 -> SHA3-384
>       SHA3_512 -> SHA3-512
>       HmacSHA3_224 -> HmacSHA3-224
>       HmacSHA3_256 -> HmacSHA3-256
>       HmacSHA3_384 -> HmacSHA3-384
>       HmacSHA3_512 -> HmacSHA3-512
>       SHA3_224withRSA -> SHA3-224withRSA
>       SHA3_256withRSA -> SHA3-256withRSA
>       SHA3_384withRSA -> SHA3-384withRSA
>       SHA3_512withRSA -> SHA3-512withRSA
>       RSASSA_PSS -> RSASSA-PSS
>       CHACHA20_POLY1305 -> CHACHA20-POLY1305
>
>    Can we just hardcode the stdName in constructor and remove the substitution? It looks fragile and expensive to me. What if we invent a name like AES_128overSHA3_256?
The process() code is also used as "/" is not allowed as variable names. 
Anyway, I have removed the process() method and hardcoded all relevant 
entries with a standard name.
>   - Now that you've added EC curve names starting with a lowercase letter, can we also use "serverAuth"?
Alright, if that's what you prefer. I added a comment at the top of 
KnownOIDs class to explain why some starts with lowercase.
>   - I wonder if we can split the aliases by hand, i.e. modify
>
>       secp256r1("1.2.840.10045.3.1.7",
>              "secp256r1 [NIST P-256, X9.62 prime256v1]"),
>
>     to
>
>       secp256r1("1.2.840.10045.3.1.7",
>              "secp256r1", "NIST P-256", "X9.62 prime256v1"),
>
>     After all the names will be split into pieces, and we can also use KnownOIDs inside NamedCurve.

I switched to this approach, however, there is one minor behavior change 
- to avoid the pattern parsing code (which I suppose is the main benefit 
of this approach), the NamedCurve.getName() method now returns the 
stdName of the KnownOIDs enum instead of the formatted string which 
contains both stdName and aliases. I also added NamedCurve.getAliases() 
method which returns the aliases of the KnownOIDs enum. This touchs 
SunEC, CurveDB, NamedCurve, and KnownOIDs classes.

> PBES2Parameters.java:
>
>   - It's a little pity we need to hardcode several names here. Is 'o.stdName().startsWith("HmacSHA")' enough? This looks like a hack but can save us some hassle if we support more later.

Well, I prefer to keep the current changes as it enforces the same 
restriction as the existing impl. With the "startsWith" check, it is 
looser as it won't reject HmacSHA3-xxx ones. If we support more later, 
this should error out and should be obvious that an update is required.

Thanks,

Valerie

>
> Everything else looks fine.
>
> Thanks,
> Max
>
>
>
>> On May 12, 2020, at 9:25 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> Thanks for filing the bug for PBES2Parameters class.
>>
>> Webrev for 8242151 is updated at: http://cr.openjdk.java.net/~valeriep/8242151/webrev.03/
>>
>> It addresses:
>>
>> - added KnownOIDs for CurveDB class
>> - updated the KDF parsing code in PBES2Parameters class to match existing behavior
>> - removed the String constants of PKCS9Attribute class and commented out its constructor which takes String argument
>> - use 3rd party aliasing info in AlgorithmId.getName() impl
>> - misc changes to KnownOIDs class regarding the register() impl
>>
>> Thanks,
>>
>> Valerie
>>
>> On 5/6/2020 6:59 PM, Weijun Wang wrote:
>>>> 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?
>>> I think so. There is a general "PBES2" that allows filling in the algorithms at init() but if they are already inside the algorithm name, then only the same can appear in the encoding.
>>>
>>> Filed https://bugs.openjdk.java.net/browse/JDK-8244564. Maybe we will backport it.
>>>
>>> --Max
>>>