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/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