On Thu, 3 Oct 2024 17:40:22 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 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
