RFR: 8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview) [v4]

[Sean Mullan]

On Thu, 25 Sep 2025 23:03:11 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Hi
>> 
>> Please review the [Second Preview](https://openjdk.org/jeps/8360563) for the PEM API.  The most significant changes from [JEP 470](https://openjdk.org/jeps/470) are:
>> 
>> - Renamed the name of `PEMRecord` class to `PEM`.
>> - Revised the new `encryptKey` methods of the `EncryptedPrivateKeyInfo` class to accept `DEREncodable` objects rather than just `PrivateKey` objects so that cryptographic objects with public keys, i.e., `KeyPair` and `PKCS8EncodedKeySpec`, can also be encrypted.
>> - Enhanced the `PEMEncoder` and `PEMDecoder` classes to support the encryption and decryption of `KeyPair` and `PKCS8EncodedKeySpec` objects.
>> 
>> thanks
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
> 
>   missed some decoder comments

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

> 391: 
> 392:     /**
> 393:      * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a given

s/a given/the specified/

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

> 392:     /**
> 393:      * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a given
> 394:      * {@code DEREncodable} and password.  Default algorithm and parameters are

Need to be more specific - what algorithm and what parameters.

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

> 396:      *
> 397:      * @param de the {@code DEREncodable} to be encrypted. Usage
> 398:      *            limited to {@link PrivateKey}, {@link KeyPair}, and

"Usage limited" sounds odd. Maybe "The supported `DEREncodable types are ... An IllegalArgumentException is thrown for any other type."

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

> 400:      * @param password the password used in the PBE encryption.  This array
> 401:      *                 will be cloned before being used.
> 402:      * @return an {@code EncryptedPrivateKeyInfo}

Next line: 

"@throws IllegalArgumentException on initialization errors based on the
arguments passed to the method"

What does this mean - you need to be more specific about what are the error cases. Otherwise it will not be helpful for TCK test writers on how to test this.

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

> 411:      * {@code AlgorithmParameterSpec} are the provider's algorithm defaults.
> 412:      *
> 413:      * @since 25

Throwing a RuntimeException on encryption error is too general.

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

> 533:      * @param decryptKey the decryption key and cannot be {@code null}
> 534:      * @param provider the {@code Provider} used for Cipher decryption and
> 535:      *                 {@code PrivateKey} generation. A {@code null} value will

Same comment about provider as below.

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

> 538:      * @throws GeneralSecurityException if an error occurs parsing,
> 539:      * decrypting the data, or producing the key object.
> 540:      * @throws NullPointerException if {@code decryptKey} is {@code null}

Same comment about exceptions as below.

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

> 563:      */
> 564:     @PreviewFeature(feature = PreviewFeature.Feature.PEM_API)
> 565:     public KeyPair getKeyPair(char[] password) throws GeneralSecurityException {

This should specifically throw an `InvalidKeyException` if decryption fails. I think you want to specify the exact subclasses when it is clearly the right behavior. In this case, the method should always throw `InvalidKeyException` on decryption errors.

Also why generalize it to throw `GeneralSecurityException`? This should throw the same exceptions as `getKeySpec(Key, Provider)` since the params are the same.

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

> 583:      * @param decryptKey the decryption key and cannot be {@code null}
> 584:      * @param provider the {@code Provider} used for Cipher decryption and
> 585:      *                 key generation. A {@code null} value will

Why do you allow null? This is inconsistent with the other methods that take Provider arguments. I think consistency is important.

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

> 586:      *                 use the default provider configuration.
> 587:      * @return a {@code KeyPair} extracted from the encrypted data
> 588:      * @throws GeneralSecurityException if an error occurs parsing,

Same comment as above on the exceptions.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403250173
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403251624
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403255532
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403260574
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403264388
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403246374
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403245654
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403237211
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403243416
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403241992