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.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
