8237219: Disabling the native SunEC implementation

[Anthony Scarpino]

On 3/3/20 8:34 AM, Sean Mullan wrote:
> Wouldn't it be better to throw an Exception when you call 
> Signature.initSign/Verify() and KeyAgreement.init() rather than waiting 
> until you sign/verify or generateSecret? This way you bail out early 
> before you start processing data.
> 
> Also, throwing a ProviderException (a RuntimeException) could be a 
> behavioral change that an application may not be prepared for. We have 
> never done a very good job of documenting when ProviderException can be 
> thrown by the JCE APIs, however. But we should think about this and 
> whether maybe you want to throw InvalidKeyException instead which is 
> already specified in the throws clause of the init methods. In any case 
> it should be documented as a potential compatibility issue in the CSR.


Unfortunately curve support decisions are not made until the library is 
accessed. You will notice the checks that call native are not until the 
operation is starting, like engineSign().  If an unsupported curve is 
used in the existing code, this is where the error is generated from. 
The native code does generate an InvalidAlgorithmParameterException.  I 
had chose ProviderException because it was a provider support change. I 
had thought about IAPE, but choose a bit more drastic action. That said, 
I'm fine with using IAPE.

If one was to remove libsunec, the existing code disables algorithms 
like SHA256withECDSA which makes it easier to separate native from java 
implementations.  But that is not true anymore with the java 
implementation supporting the NIST curves.  Some of these same decisions 
affect both property disabling and library removal.

In the past we have had similar Signature discussions before about when 
Signature.setParameter() being called before or after init().  I have to 
wonder if the original provider design followed the same logic where 
errors are generated later in the process.  Also there are some 
decisions in this provider that look similar to SunPKCS11 as both wait 
until the last moment to ask the native library.

> 
> Did you consider documenting the system property in the API javadocs for 
> Signature, KeyPairGenerator, KeyAgreement? I realize this is specific to 
> the SunEC provider, but this would help users know how to enable the 
> system property (on the hopefully rare case) they get an exception and 
> still want to use one of these curves. It could be something like:
> 
> @implNote By default, the SunEC provider throws ...Exception if the key 
> is using a legacy curve. Set the {@systemProperty 
> jdk.sunec.disableNative} to {@code false} to disable this behavior.
> 

I don't think putting in the API about one particular provider 
implementation detail is a proper place.  I think this belongs in 
release notes, and maybe the provider doc.

> 
> --Sean
> 
> On 3/2/20 7: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