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

Fri May 15 21:47:14 UTC 2020

Hi Max,

I have updated the webrev 
(http://cr.openjdk.java.net/~valeriep/8242151/webrev.05/) to address 
your suggestion below. Touched classes are NamedCurve, CurveDB, 
ConstraintsParameters, and SunEC. The result of using the single method 
looks pretty good - clean and shorter code. :)

> CurveDB.getNamesByOID is only used in ConstraintsParameters.getNamedCurveFromKey(), but we already have a NamedCurve there and you can directly use it without converting to nc.getObjectId().
>
> In fact, it looks like nc.getAliases() and nc.getName() are always used together. Can we just remove these 2 and add a new method nc.getNameAndAliases()? Then there will be no compatibility impact for getName() at all!
>
Thanks,
Valerie

On 5/13/2020 7:03 PM, Weijun Wang wrote:
>
>> On May 14, 2020, at 3:17 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> 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.
> I was OK with the $ -> / conversion, but it's up to you to decide whether to keep that.
>
>>>   - 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.
> CurveDB.getNamesByOID is only used in ConstraintsParameters.getNamedCurveFromKey(), but we already have a NamedCurve there and you can directly use it without converting to nc.getObjectId().
>
> In fact, it looks like nc.getAliases() and nc.getName() are always used together. Can we just remove these 2 and add a new method nc.getNameAndAliases()? Then there will be no compatibility impact for getName() at all! :-)
>
>>> 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.
> Fair enough.
>
> Everything else looks fine to me.
>
> Thanks,
> Max
>
>> 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