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

[Weijun Wang]

On Thu, 3 Oct 2024 19:09:28 GMT, Sean Mullan <mullan 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/x509/NamedX509Key.java line 39:
> 
>> 37: import java.security.ProviderException;
>> 38: import java.security.spec.NamedParameterSpec;
>> 39: 
> 
> Class description?

I can add some. This is not public APIs after all.

> src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 48:
> 
>> 46:     private final byte[] h;
>> 47: 
>> 48:     /// Ctor from family name, parameter set name, raw key bytes.
> 
> Any reason you use 3 slashes instead of 2 for comments?

I'm trying out the https://openjdk.org/jeps/467 style javadoc. Still, this is not public API so I can take the risk for the experiment.

> src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 56:
> 
>> 54:             this.algid = AlgorithmId.get(pname);
>> 55:         } catch (NoSuchAlgorithmException e) {
>> 56:             throw new ProviderException(e);
> 
> Can this ever happen?

If we can be sure that `KnownOIDs` have the mapping then no. I do think this is not a programming error but an inconsistency inside the provider, therefore I choose `ProviderException` instead of `AssertionError`.

> src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 67:
> 
>> 65:         this.fname = fname;
>> 66:         decode(encoded);
>> 67:         paramSpec = new NamedParameterSpec(algid.getName());
> 
> Use `this` to be consistent with other ctor.

Sure.

> src/java.base/share/classes/sun/security/x509/NamedX509Key.java line 71:
> 
>> 69:             throw new InvalidKeyException("algorithm identifier has params");
>> 70:         }
>> 71:         h = getKey().toByteArray();
> 
> Use this to be consistent with other ctor.

Yes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786761967
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786762767
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786764211
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786763140
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1786763408