Re: 8237219: Disabling the native SunEC implementation

[Anthony Scarpino]

On 3/18/20 7:32 PM, Weijun Wang wrote:
ECKeyPairGenerator.java:
You can move the following lines to the beginning of the method:

  132         // Check if the java implementation supports this curve
  133         if (ECOperations.forParameters(ecSpec).isPresent()) {
  134             return;
  135         }

I left it after the eparams.init() as this checks CurveDB.lookup, if the 
curve not found it returns an InvalidParameterSpecException which is 
wrapped into a InvalidAlgorithmParameterSpec.  If I move the above 
check, now it only returns an InvalidAlgorithmParameterSpec.

Going forward when this native library is removed, I think changing the 
exception wrapping is less of a concern, but since the purpose of this 
change is more for older releases, I thought keeping this consistent was 
more important, does that make sense and do you agree?


Everything else looks fine.

In the CSR, how about explicitly spell out the curve names we support in Java?

ok


You also need a release note: Talk about the system property, when it should be 
enabled, and why enabling it is probably not a good idea.

Yes, when the CSR is reviewed I'll have a basis to write the Release 
Note from.


Thanks,
Max



On Mar 18, 2020, at 11:52 PM, Anthony Scarpino <anthony.scarp...@oracle.com> 
wrote:

Max,

Are you satisfied with this last update?

Thanks

Tony

On Mar 11, 2020, at 11:23 PM, Anthony Scarpino <anthony.scarp...@oracle.com> 
wrote:

Another webrev update with Max's recent comments.
https://cr.openjdk.java.net/~ascarpino/8237219/webrev.03

Also I still need a reviewer for the CSR.

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