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

[Weijun Wang]

On Fri, 2 May 2025 06:09:52 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 with a new target base due to a merge or a rebase. The pull request now contains 66 commits:
> 
>  - major code review comments update
>  - Merge branch 'master' into pem
>  - Merge branch 'master' into pem
>  - javadoc updates
>  - code review comments
>  - merge with master
>  - better comment and remove commented out code
>  - Merge branch 'master' into pem
>  - Merge branch 'pem-merge' into pem
>  - merge
>  - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327

Comments on `EncryptedPrivateKeyInfo`.

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

> 372:             SecretKeyFactory factory;
> 373:             if (provider == null) {
> 374:                 factory = SecretKeyFactory.getInstance(algorithm);

`algorithm` might be null.

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

> 395:      * arguments passed to the method.
> 396:      * @throws SecurityException on a encryption errors.
> 397:      * @throws NullPointerException when the password is null.

and when `key` is `null`.

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

> 406:     public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key,
> 407:         char[] password) {
> 408:         char[] pass = password.clone();

No need to clone password here. It will be cloned into a `PBEKeySpec` later.

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

> 456:         }
> 457: 
> 458:         if (Pem.DEFAULT_ALGO == null || Pem.DEFAULT_ALGO.length() == 0) {

Is this required if `algorithm` is already specified?

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

> 571:      *                 {@code SecretKeyFactory}, and the {@code PrivateKey},
> 572:      *                 {@code KeyFactory}.  A {@code null} value will use the
> 573:      *                 default provider configuration.

There is no `SecretKeyFactory` in this method, but there is `Cipher`.

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

> 575:      * @throws InvalidKeyException if an error occurs during parsing of the
> 576:      * encrypted data or creation of the key object.
> 577:      * @throws NullPointerException if {@code key} is null.

s/key/decryptKey/.

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

> 580:      */
> 581:     @PreviewFeature(feature = PreviewFeature.Feature.PEM_API)
> 582:     public PrivateKey getKey(Key decryptKey, Provider provider)

This method goes one step further than the existing `getKeySpec(dk, p)`. It should only throw more types of exception than the existing one. Now you put everything into a `InvalidKeyException`. Is that good?

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

PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2825666685
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080069229
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080040299
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080032269
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080067182
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080114399
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080114811
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080127359