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
