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

[Weijun Wang]

On Fri, 4 Oct 2024 16:10:57 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

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

This is for pure internal use and we will enforce the calling convention. Yes, the object is not immutable but we will use it in an immutable way.

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

How about `rawKey`?

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

I think `this` is necessary when there are input arguments having the same name as the instance fields. This happens a lot in a constructor when fields are being initialized from input arguments. In other methods, it's more common to avoid using `this`.

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

See above.

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

I updated the word. What other suggestion do you have? I tried my best to put each case on a single line without being too long.

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

This is all internal. So consider these classes and all its consumers belong to the same author with all responsibility. Unnecessary clones not only slow down the program but it also might leave sensitive data in the memory if not properly cleaned up. We don't want that to happen.

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

OK.

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

Same reason.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788073222
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788076498
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788078656
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788078825
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788082350
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788084634
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788084835
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788085005