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

[Kevin Driver]

On Fri, 4 Oct 2024 17:38:37 GMT, Weijun Wang <weijun at openjdk.org> wrote:

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

Maybe it is worthwhile to call out the lack of true immutability, though it may appear to be otherwise?

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

Sure. I only mention it because within the same method it is inconsistent. See the next line. `key` is not a parameter to the method, yet `this.key` is used.

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

See usage of `this.key` in this method.

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

See above reply. Maybe worth explicitly stating lack of true immutability in order to achieve this goal.

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

Thanks for the explanation.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788256865
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788258092
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788258813
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788259867
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1788260067