> On Mar 5, 2020, at 11:14 AM, Anthony Scarpino <anthony.scarp...@oracle.com>
> wrote:
>
> On 3/3/20 6:57 PM, Weijun Wang wrote:
>> 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.
>
> Currently the CurveDB is unchanged. But when the library is removed, maybe
> it should be trimmed to only the 3 NIST curves?
Maybe not. TLS uses it to find out the curve name for a ciphersuite name. if
SunEC does not support the curve name it can still find it in BC.
So there are 2 kinds of curves now: known curves, and SunEC supported curves.
Or does this mean we can move CurveDB inside TLS, and only keep the name part
(without the BigIntegers)?
>
>> 2. There is a ECKeyPairGenerator::isSupported
>
> I've changed this to support only 3 NIST curves when the library is disabled
>
>> 3. The native lib
>
> Similar to isSupported, if the property is false, it uses the native library
>
>> For a curve name rejected on each place, is there any behavior change?
>
> There should not be any with my latest changes that I have not put onto a
> webrev yet.
Great.
>
>> 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.
>
> I don't know how much BC depends on SunEC. Right now the SunEC provider is
> returning AlgorithmsParameters supportedCurves property with the 3 NIST
> curves instead of all the curves in CurveDB that are disabled`.
I think BC should not depend on SunEC, but CurveDB is in java.base.
Thanks,
Max
>
>> 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
>
