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