[8u] RFR 8233228: Disable weak named curves by default in TLS, CertPath, and Signed JAR

[Sergey Chernyshev]

Hi Andrew,

Thank you for the review.


Thanks,

Serge


On 11.01.2021 07:18, Andrew Hughes wrote:
> On 20:49 Mon 07 Dec     , Alexander Scherbatiy wrote:
>> Hello,
>>
>> Could you review the updated backport of JDK-8233228 to 8u.
>>
>>   8u webrev:
>> http://cr.openjdk.java.net/~alexsch/sercher/8233228/webrev.02/jdk.patch
>>
>> The only difference between the updated backport and the previous webrev.01
>> version
>> is that the public modifier is removed from "static String[]
>> getNamesByOID(String oid)" method
>> of CurveDB class.
>>
>> All classes which use CurveDB.getNamesByOID(oid) method
>> are placed in the same package as CurveDB and the original jdk11u patch
>> has the package-private CurveDB.getNamesByOID(oid) method.
>>
>> Thanks,
>> Alexander.
>>
>> On 12/3/20 7:36 PM, Alexander Scherbatiy wrote:
>>> Hello,
>>>
>>> Could you review the updated backport of JDK-8233228 to 8u.
>>>
>>>   8u webrev:
>>> http://cr.openjdk.java.net/~alexsch/sercher/8233228/webrev.01
>>>
>>> The classes ECParameters, NamedCurve, and CurveDB needs to be moved from
>>> sun.security.ec packageto sun.security.util
>>> because sun.security.ec is placed in sunec.jar and these classes are not
>>> accessible
>>> from ConstraintsParameters, DisabledAlgorithmConstraints which are
>>> stored in rt.jar.
>>>
>>> Moving ECParameters, NamedCurve, and CurveDB classes is sent as a part
>>> of a separate request [1]
>>>    JDK-8035166: Remove dependency on EC classes from pkcs11 provider
>>>
>>> The patch for JDK-8035166 needs to be applied first and the patch for
>>> JDK-8233228 on top of it.
>>>
>>> The tests compact3, java_security, java_security_infra, needs_jdk, and
>>> needs_jre were run.
>>>
>>> In total they contain the following crypto and security tests:
>>>   sun/security/tools/jarsigner/*
>>>   com/sun/crypto/provider/*
>>>   com/sun/security/*
>>>   java/security/*
>>>   javax/crypto/*
>>>   javax/net/ssl/*
>>>   javax/security/*
>>>   javax/xml/crypto/*
>>>   sun/security/*
>>>   security/infra/java/security/*
>>>
>>> The are no new failures comparing to the build without the fix.
>>>
>>> [1]
>>> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-December/013171.html
>>>
>>> Thanks,
>>> Alexander
>>>
>>> On 12/2/20 8:34 AM, Andrew Hughes wrote:
>>>> On 22:14 Tue 01 Dec     , Alexander Scherbatiy wrote:
>>>>> ​Hello,
>>>>>
>>>>> Could you review the backport of P2 JDK-8233228 to 8u.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8233228
>>>>> 11u patch:
>>>>> https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/a17295342862
>>>>> 8u webrev:
>>>>> http://cr.openjdk.java.net/~alexsch/sercher/8233228/webrev.00
>>>>>
>>>>>
>>>>> 8233228 backport to 8u (compared to 11u):
>>>>> * sun.security.ec.ECParameters -> sun.security.util.ECParameters
>>>>> * sun.security.ec.NamedCurve   -> sun.security.util.NamedCurve
>>>>> * sun.security.ec.CurveDB      -> sun.security.util.CurveDB
>>>>> * security/tools/keytool fixed context difference
>>>>> * DisabledAlgorithmConstraints.java fixed context difference
>>>>> * Manual merge in ConstraintsParameters.java (XECKey,
>>>>> NamedParameterSpec are
>>>>> not available in 8u).
>>>>> * CurveDB.SPLIT_PATTERN, CurveDB.getSupportedCurves() made public
>>>>> * NamedCurve class, getName(), getObjectId() made public
>>>>> * ECParameters.getAlgorithmParameters() made public
>>>>> * files java.security-<platform> are separate in each platform, applied
>>>>> identical changes in all
>>>>>
>>>> Why is it necessary to move the package these files are in?
>>>>
>>>> If we really need to do this, it should be done as a separate backport
>>>> of JDK-8035166, but I'm not yet convinced this is necessary, given the
>>>> disruption it will cause to code that relies on the code being in the
>>>> current locations.
>>>>
>>>>> The are no new failures in hotspot and compact3 tests comparing
>>>>> to the build
>>>>> without the fix.
>>>> I'm not sure how HotSpot tests would relate to a crypto change. What
>>>> crypto
>>>> tests were run?
>>>>
>>>>> Thanks,
>>>>> Alexander.
>>>>>
>>>> Thanks,
> This looks ok to me now. Reviewed & approved.
>
> Thanks,

-- 
Best regards,
Sergey Chernyshev
Bellsoft LLC