We have 3 places checking for an algorithm now:
1. If CurveDB does not know a curve, you won't be able to create parameters.
2. There is a ECKeyPairGenerator::isSupported
3. The native lib
For a curve name rejected on each place, is there any behavior change?
Another thing: Even after the native lib is disabled, I think we can still
create AlgorithmParameters and use KeyFactory to load a key when the curve is
not supported, right? If the user has BouncyCastle in the provider list, I hope
the Signature provider choosing fallback still works and a BC Signature object
will be used when initialized with such a key.
Thanks,
Max
> On Mar 4, 2020, at 3:42 AM, Anthony Scarpino <anthony.scarp...@oracle.com>
> wrote:
>
> On 3/3/20 11:18 AM, Sean Mullan wrote:
>> 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.
>
> The native code throws it and it's wrapped in a SignatureException.
>
> For generateKeyPair() the existing code threw a ProviderException because the
> method doesn't specify any exceptions. That probably pushed me toward
> ProviderException in other cases too.
>
>> 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?
>
> Maybe you missed one paragraph below? Without the library in the existing
> code we never got past getInstance() because those algorithms like
> SHA256withECDSA were not available. After Max's verify change, they are now.
>
>>> 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
