On Mon, 7 Oct 2024 18:54:03 GMT, Weijun Wang <wei...@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
