RFR: 8340327: A common framework to support public key algorithms with standard parameter sets [v10]

[Sean Mullan]

On Mon, 7 Oct 2024 18:54:03 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> To prepare for new PQC algorithms like ML-KEM and ML-DSA where there are only named standardized parameter sets, a common framework is introduced.
>> 
>> A example of EdDSA implementation using this framework is included as a test.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   null check as asserts, and better exception messages

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 63:

> 61: 
> 62:     private Object sk2 = null;
> 63:     private Object pk2 = null;

Nit - don't need to set these to null.

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 121:

> 119:             return implSign(name, secKey, sk2, msg, appRandom);
> 120:         } else {
> 121:             throw new IllegalStateException("No private key");

`Signature.engineSign` is not specified to throw `IllegalStateException`, even though it seems like the more correct exception to throw. So this should probably be a `SignatureException`.

Same comment for `engineVerify`.

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 146:

> 144:     @SuppressWarnings("deprecation")
> 145:     protected Object engineGetParameter(String param) throws InvalidParameterException {
> 146:         throw new UnsupportedOperationException("getParameter() not supported");

`engineGetParameter` is not specified to throw UOE, so suggest throwing `InvalidParameterException` instead. Same comment for `engineSetParameter`.

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 196:

> 194:     /// This object will be passed into the [#implVerify] method along with the raw key.
> 195:     ///
> 196:     /// The default implementation returns `null`.

If the majority of implementations are supposed to implement this method, then it may be better to make it abstract (so that it won't be accidentally missed) and add a note that subclasses should return null if they don't have any additional checks.

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 208:

> 206:     /// User-defined function to validate a private key.
> 207:     ///
> 208:     /// This method will be called in `initSign`. This gives provider a chance to

s/provider/the provider/

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 210:

> 208:     /// This method will be called in `initSign`. This gives provider a chance to
> 209:     /// reject the key so an `InvalidKeyException` can be thrown earlier.
> 210:     /// An implementation can optional return a "parsed key" as an `Object` value.

s/optional/optionally/

src/java.base/share/classes/sun/security/provider/NamedSignature.java line 213:

> 211:     /// This object will be passed into the [#implSign] method along with the raw key.
> 212:     ///
> 213:     /// The default implementation returns `null`.

Same comment as for implCheckPublicKey.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792352928
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792381631
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792386559
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792370232
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792354924
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792355642
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792369393