RFR: 8298420: PEM API: Implementation (Preview) [v14]

[Weijun Wang]

On Thu, 17 Apr 2025 15:51:09 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Hi all,
>> 
>> I need a code review of the PEM API.  Privacy-Enhanced Mail (PEM) is a format for encoding and decoding cryptographic keys and certificates.  It will be integrated into JDK24 as a Preview Feature.  Preview features does not permanently define the API and it is subject to change in future releases until it is finalized.
>> 
>> Details about this change can be seen at [PEM API JEP](https://bugs.openjdk.org/browse/JDK-8300911).
>> 
>> Thanks
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - javadoc updates
>  - code review comments

Some comments on the new `EncryptedPrivateKeyInfo` APIs.

Many trailing periods in `@params` tags can be removed if the descriptions are not full sentences.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 81:

> 79: 
> 80:     /**
> 81:      * Constructs an {@code EncryptedPrivateKeyInfo} from a given Encrypted

Why is "E" capitalized? Is it a special term?

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 83:

> 81:      * Constructs an {@code EncryptedPrivateKeyInfo} from a given Encrypted
> 82:      * PKCS#8 ASN.1 encoding.
> 83:      * @param encoded the ASN.1 encoding which is cloned and then parsed.

Somehow I prefer the original "The contents of the array...". We've used this style in a lot of places.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 324:

> 322: 
> 323:     /**
> 324:      * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a given

Can we say "encrypt" an "EncryptedPrivateKeyInfo"? It sounds like an already encrypted thing.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 338:

> 336:      *
> 337:      * @param key the PrivateKey object to encrypt.
> 338:      * @param password the password used for generating the PBE key.

Do we need to mention the "PBE key"? It's just the password to encrypt the private key.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 341:

> 339:      * @param algorithm the PBE encryption algorithm.
> 340:      * @param params the parameters used with the PBE encryption.
> 341:      * @param provider the Provider that will perform the encryption.

I prefer moving (if not copying) the nullable description of `params` and `provider` here.

Also, in your implementation, the provider is used to choose both `SecretkeyFactory` and `Cipher`. I hope for each provider there always exists a pair of `SecretkeyFactory` and `Cipher` with a given algorithm name.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 351:

> 349:      * @implNote The encryption uses the algorithm set by
> 350:      * `jdk.epkcs8.defaultAlgorithm` Security Property
> 351:      *  and default the {@code AlgorithmParameterSpec} of that provider.

I think this `implNote` is not necessary here. You already required `algorithm` to be non-null.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 416:

> 414:      * {@link PrivateKey} using the {@code encKey} and given parameters.
> 415:      *
> 416:      * If {@code algorithm} is {@code null} the default algorithm will be used.

In the other `encryptKey` method using password, `algorithm` must be provided. Why the inconsistency?

In fact, since you have `encKey`, doesn't it already have an algorithm name?

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 422:

> 420:      *
> 421:      * @param key the {@code PrivateKey} object to encrypt.
> 422:      * @param encKey the encryption {@code Key}

It will be more precise if we say `key` is the key to be encrypted and `encKey` is the key used to encrypt `key`.

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

PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2788731077
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056901657
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056902408
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056903380
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056906894
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056908199
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056911191
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056949605
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056952122