> On May 14, 2020, at 3:17 AM, Valerie Peng <valerie.p...@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.p...@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
