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/pkcs/NamedPKCS8Key.java line 54:
> 52:
> 53: /// Ctor from family name, parameter set name, raw key bytes.
> 54: /// Key bytes won't be cloned, caller must relinquish ownership
This strikes me as an atypical contract for a constructor, especially if the
goal is to make the object immutable.
src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java line 55:
> 53: /// Ctor from family name, parameter set name, raw key bytes.
> 54: /// Key bytes won't be cloned, caller must relinquish ownership
> 55: public NamedPKCS8Key(String fname, String pname, byte[] h) {
This may be a "style" nit, but it might be better to have a name longer than
the single-character `h`. I had to refer to the comments to know what this is
rather than relying on the variable name to be sufficient.
src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java line 120:
> 118: @Override
> 119: public void destroy() throws DestroyFailedException {
> 120: Arrays.fill(h, (byte)0);
nit: `this.h`? Consistency.
src/java.base/share/classes/sun/security/pkcs/NamedPKCS8Key.java line 125:
> 123: Arrays.fill(this.encodedKey, (byte)0);
> 124: }
> 125: destroyed = true;
nit: `this.destroyed`? You use `this` in most other places. Consistency.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787937549
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787938696
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787940800
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1787941947
