CSR RFR: 8233228: Support named curves for all disabledAlgorithms

[Sean Mullan]

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