On 12/10/19 5:37 PM, Anthony Scarpino wrote:
I believe I have address all the comments in the updated CSR. I have
also added the "include" keyword for the new property along with the
description for it's use.
Great.
However, regarding the brainpool curves, the ones you say that we do not
support are in CurveDB.java. It was my impression that CurveDB provided
the parameters to create these named curves. Where do you see that
those 3 curves are not supported?
If you pass one of these curves to the following code:
KeyPairGenerator ekpg = KeyPairGenerator.getInstance("EC");
ECGenParameterSpec spec = new ECGenParameterSpec(curve);
ekpg.initialize(spec);
KeyPair kp = ekpg.generateKeyPair();
it will throw the following exception:
java.security.InvalidAlgorithmParameterException: Unsupported curve:
brainpoolP160r1 (1.3.36.3.3.2.8.1.1.1)
at
jdk.crypto.ec/sun.security.ec.ECKeyPairGenerator.ensureCurveIsSupported(ECKeyPairGenerator.java:137)
at
jdk.crypto.ec/sun.security.ec.ECKeyPairGenerator.initialize(ECKeyPairGenerator.java:114)
at
java.base/java.security.KeyPairGenerator$Delegate.initialize(KeyPairGenerator.java:699)
at
java.base/java.security.KeyPairGenerator.initialize(KeyPairGenerator.java:436)
so I don't think they are really supported. In any case, I have added my
name as Reviewer to the CSR, but feel free to try to resolve this before
you submit it.
--Sean
Tony
On 12/10/19 5:57 AM, Sean Mullan wrote:
In general, this CSR looks good. Here are my specific comments:
- The Scope should be "JDK" since these are JDK supported security
properties.
- The Fix Version should also include 7-pool.
- I would change the summary to "This change adds named elliptic
curves to the jdk.[tls|certpath|jar].disabledAlgorithms security
properties."
- In the Summary and/or Solution sections, you should add that you are
disabling these legacy curves by default, and add some rationale as to
why we are doing that. I don't see that specifically mentioned anywhere.
- In the Solution section, missing a period at end of first sentence.
- In the Solution section, there is a typo in the property name
"jdk.disabled.NamedCurve" (should be plural).
- Typo: "full property name used" -> "full property name is used"
Comments in Specification section:
----------------------------------
1. Change:
+# in jdk.[tls|certpath|jar].disabledAlgorithms. To include this list
in any
to:
+# in the jdk.[tls|certpath|jar].disabledAlgorithms properties. To
include this list in any
2. We don't support the brainpoolP160r1, brainpoolP192r1,
brainpoolP224r1 curves, so these don't need to be listed.
3. +# properities. See the property for details.
Typo: "properties"
--Sean
On 12/9/19 1:10 PM, Anthony Scarpino wrote:
I need a CSR review for the change with policy and property addition
for 8233228.
https://bugs.openjdk.java.net/browse/JDK-8235540
Tony
