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

[Kevin Driver]

On Thu, 3 Oct 2024 17:40:22 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge branch 'master' into 8340327
>  - more test, more RAW support, fix a bug on cleaning up getRawBytes output
>  - add support for private class RawKeySpec
>  - ensure key is intact after being used
>  - renames
>  - the fix

src/java.base/share/classes/sun/security/provider/NamedKeyFactory.java line 243:

> 241:             if (key instanceof AsymmetricKey pk) {
> 242:                 String name;
> 243:                 // Three case that we can find the parameter set name from a RAW key:

nit: case -> cases

This phrase may need to be reworded as well.

src/java.base/share/classes/sun/security/provider/NamedKeyPairGenerator.java line 54:

> 52: ///
> 53: /// An implementation must implement all abstract methods. For all these
> 54: /// methods, the implementation must relinquish any "ownership" of any input

See earlier comment about relinquishing "ownership". There's not really a way to enforce this, and unless there is, the resultant object cannot be said to be immutable.

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

> 61: /// also applies to abstract methods defined in [NamedKEM] and [NamedSignature].
> 62: ///
> 63: /// Also, an implementation must not keep any extra copy of a private key.

This wording may be fine. Could also say something about zeroing any copies.

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

> 70:         this.fname = Objects.requireNonNull(fname);
> 71:         if (pnames == null || pnames.length == 0) {
> 72:             throw new AssertionError("pnames cannot be null or empty");

Same question here. Curious to know why `AssertionError` vs other options.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787966627
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787970347
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787972502
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787977879