8237219: Disabling the native SunEC implementation

[Sean Mullan]

On 3/3/20 1:42 PM, Anthony Scarpino wrote:
> 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.

Hmm, but engineSign, engineVerify, engineGenerateSecret don't throw IAPE.

I think it should probably be the same behavior as if the library did 
not exist. What do we throw now if the library has been removed?

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

Yeah, I am kind of on the fence about that myself as I don't necessarily 
want to clutter the APIs with provider-specific stuff, but OTOH, most of 
the security-related system properties *are* provider-specific, so we 
lose the nice feature of seeing how different system properties affect 
existing APIs while you are reading the javadoc. But, I think we 
probably need to decide on a more general policy for documenting 
provider-specific system properties first. So I guess we can hold off on 
this for now.

--Sean

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