RFR: 8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters

[Weijun Wang]

On Thu, 10 Nov 2022 05:49:12 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> An `EncryptedPrivateKeyInfo` object can be created with an uninitialized `AlgorithmParameters`, but before you call `getEncoded` on it you need to remember to initialize the params. This is unfortunate but since this is a public API, I hesitate to make a change.
>> 
>> Instead, this code change fixes the much more widely used internal class `AlgorithmId` so that it cannot be created with an uninitialized `AlgorithmParameters`. `EncryptedPrivateKeyInfo` now works with both initialized and uninitialized params, and it's immutable.
>> 
>> No intention to make `AlgorithmId` immutable this time. It has a child class named `AlgIdDSA` which makes things complicated.
>
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 192:
> 
>> 190:             // create an AlgorithmId from it. This should usually be true
>> 191:             // since we already require its getEncoded() returning the
>> 192:             // encoded bytes of it.
> 
> It looks like the comment implies there is no need of  the catch block.  Did you want to add something like "But it may be not true if ..."?  Or, is the comment belong to AlgorithmId?

This comment is for this class. The spec mentions `{@code algParams.getEncoded()} should return` so one can say that it already implied that the params has already been initialized. This is a little subtle and given that real world developer might first create this subject with an uninitialized params and then initialize it before calling `getEncoded`, I decide to support this case as well. I'll think about how to describe it better.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 199:
> 
>> 197:         if (tmp != null) {
>> 198:             this.algid = tmp;
>> 199:             this.params = null;
> 
> There is a getAlgParameters() method in the class.  Does it make sense to cache the params?

OK. Thanks.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 437:
> 
>> 435: 
>> 436:     @SuppressWarnings("fallthrough")
>> 437:     private static PKCS8EncodedKeySpec pKCS8EncodingToSpec(byte[] encodedKey)
> 
> Does "pkcs8..." (will lowercase) look like a better name for the method?

OK.

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

PR: https://git.openjdk.org/jdk/pull/11067