On 3/9/20 8:35 PM, Weijun Wang wrote:
SunEC.java:- isJavaSupported() has a strange name - Is it better to put the javaSupportedCurves somewhere near fields and orderFields in ECOperations.java? Thus we can update it simultaneously in the future. Or at least add a comment somewhere? ECKeyPairGenerator.java: - Is it possible to implement the Java part of isSupported() based on ECParameterSpec instead of its getEncoded() form?
I'm going to try to rework it so the isSupported is replaced by ECOperations.forParameters(). This is similar to what is done for XDH and EdDSA. That would remove the isJavaSupported method.
KeySizeTest.java: - Do we need to enable native for the "SunEC EC 256" case?
No it shouldn't be necessary
jarsigner/Spec.java: - I would suggest updating "kpg.initialize(192)" to use 256 and "kpg.initialize(571)" to use 521 instead of enabling the native lib
I would rather leave them alone. The only reason is that native is still supported even though it's disabled and should still be tested. When the native library is removed, then this can be changed to 256 and 521.
SecurityTools.java: - I still don't like always enabling native and would rather only enable it when required (Ex: in test/jdk/sun/security/tools/keytool/GroupName.java). I'm now running a mach5 job to see how much impact it has.
I will look at it to see if it's a quick change, but I hesitant making many native specific changes in the tests because those same changes will be removed when the native library is removed.
Otherwise fine to me. Thanks, MaxOn Mar 7, 2020, at 4:04 AM, Anthony Scarpino <anthony.scarp...@oracle.com> wrote: Webrev is updated and the CSR is updated with a comment if the property is false. https://cr.openjdk.java.net/~ascarpino/8237219/webrev.02 thanks Tony On 3/2/20 4:40 PM, Anthony Scarpino wrote:Hi I need a review of the CSR and webrev for disabling by default the native SunEC curves from the API. With the recent verification changes in JDK-8237218, SunJCE is long dependent on the native code for verifying the constant-time curves. This disabling can be undone with setting a system property, jdk.sunec.disableNative. I'm doing a simultaneous review as changes for one will likely affect the other. CSR: https://bugs.openjdk.java.net/browse/JDK-8238911 webrev: https://cr.openjdk.java.net/~ascarpino/8237219/ The curves affected are: secp112r1, secp112r2, secp128r1, secp128r2, secp160k1, secp160r1, secp160r2, secp192k1, secp192r1, secp224k1, secp224r1, secp256k1, sect113r1, sect113r2, sect131r1, sect131r2, sect163k1, sect163r1, sect163r2, sect193r1, sect193r2, sect233k1, sect233r1, sect239k1, sect283k1, sect283r1, sect409k1, sect409r1, sect571k1, sect571r1, X9.62 c2tnb191v1, X9.62 c2tnb191v2, X9.62 c2tnb191v3, X9.62 c2tnb239v1, X9.62 c2tnb239v2, X9.62 c2tnb239v3, X9.62 c2tnb359v1, X9.62 c2tnb431r1, X9.62 prime192v2, X9.62 prime192v3, X9.62 prime239v1, X9.62 prime239v2, X9.62 prime239v3, brainpoolP256r1 brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 Tony
