On 3/4/20 10:59 PM, Weijun Wang wrote:
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)?
I'm thinking same. Name and OID may only be needed.
2. There is a ECKeyPairGenerator::isSupportedI've changed this to support only 3 NIST curves when the library is disabled3. The native libSimilar to isSupported, if the property is false, it uses the native libraryFor 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, MaxThanks, MaxOn 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
