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

Wed Sep 17 20:13:55 UTC 2025

On Wed, 17 Sep 2025 01:00:21 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:
> 
>   rework test & commented out code.

src/java.base/share/classes/java/security/PEMDecoder.java line 55:

> 53:  * <p> The {@link #decode(String)} and {@link #decode(InputStream)}
> 54:  * methods return an instance of a class matching the PEM type and
> 55:  * implementing {@link DEREncodable}.

Wording suggestion: "... that matches the PEM type and implements `DEREncodable`"

src/java.base/share/classes/java/security/PEMDecoder.java line 58:

> 56:  *
> 57:  * <p> Supported PEM types and their decoded {@code DEREncodable} classes
> 58:  * include:

"include" may suggest that this is not a complete list, and others are supported. Suggest using a different word. I prefer the prior wording because it is more definitive.

src/java.base/share/classes/java/security/PEMDecoder.java line 74:

> 72:  *  Decryption)</li>
> 73:  *  <li>ENCRYPTED PRIVATE KEY : {@code KeyPair} (if configured with
> 74:  *  Decryption)</li>

"Decryption" doesn't need to be capitalized. Same comment on other lines.

src/java.base/share/classes/java/security/PEMDecoder.java line 81:

> 79:  *
> 80:  * <p> For {@code PublicKey} and {@code PrivateKey} types, an algorithm-specific
> 81:  * subclass is returned if the algorithm is supported. For example an

add comma after "For example"

src/java.base/share/classes/java/security/PEMDecoder.java line 82:

> 80:  * <p> For {@code PublicKey} and {@code PrivateKey} types, an algorithm-specific
> 81:  * subclass is returned if the algorithm is supported. For example an
> 82:  * ECPublicKey and ECPrivateKey for Elliptic Curve keys.

Put code font around ECPublicKey and ECPrivateKey.

src/java.base/share/classes/java/security/PEMDecoder.java line 90:

> 88:  * <p> The {@link #decode(String, Class)} and
> 89:  * {@link #decode(InputStream, Class)} methods take a class parameter
> 90:  * to specify the returned {@code DEREncodable} type. These

s/to specify/which specifies/

Also I think "type of {@code DEREncodable} that is returned" was more clear.

src/java.base/share/classes/java/security/PEMDecoder.java line 91:

> 89:  * {@link #decode(InputStream, Class)} methods take a class parameter
> 90:  * to specify the returned {@code DEREncodable} type. These
> 91:  * methods are useful when extracting or changing the return class.

"changing the return class" sounds confusing here. Is it better to say "These methods are useful to avoid casting the return type when the PEM type is known, or when extracting a specific type when there is more than one choice."

src/java.base/share/classes/java/security/PEMDecoder.java line 92:

> 90:  * to specify the returned {@code DEREncodable} type. These
> 91:  * methods are useful when extracting or changing the return class.
> 92:  * For example, if the PEM contains both a public and private keys, specifying

s/keys/key/

src/java.base/share/classes/java/security/PEMDecoder.java line 95:

> 93:  * {@code PrivateKey.class} returns only the private key.
> 94:  * If the class parameter specifies {@code X509EncodedKeySpec.class}, the
> 95:  * public key encoding is returned in that class.  Any type of PEM data can be

I think you could word "returned in that class"  differently - how about "returned in an instance of the `X509EncodedKeySpec` class"

src/java.base/share/classes/java/security/PEMDecoder.java line 103:

> 101:  * with {@link #withFactory(Provider)} and/or
> 102:  * {@link #withDecryption(char[])}. {@link #withFactory(Provider)}
> 103:  * restricts decoding to {@link KeyFactory} and

Wording suggestion: "The withFactory method uses the specified provider to produce cryptographic objects from `KeyFactory` and `CertificateFactory`"

src/java.base/share/classes/java/security/PEMDecoder.java line 107:

> 105:  * {@link #withDecryption(char[])} configures the decoder to process
> 106:  * encrypted private key PEM data using the given password.
> 107:  * If decryption fails, a {@link RuntimeException} is thrown.

Be more specific - `IllegalArgumentException` is specified by the decode methods.

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 112:

> 110:     /**
> 111:      * Constructor that takes both public and private encodings.  If
> 112:      * pubEncoding is null, a V1 PKCS8 encoding is created; otherwise, V2 is

s/pubEncoding/publicEncoding/

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 301:

> 299:      * PKCS8v2 DER-encoded byte[].
> 300:      *
> 301:      * @param pubKeyEncoded DER-encoded PublicKey, this maybe null.

s/maybe/may be/

test/jdk/java/security/PEM/PEMData.java line 290:

> 288:         """, RSAPrivateKey.class, "SunRsaSign");
> 289: 
> 290:     static final Entry ecsecp256ekpi = new Entry("ecsecp256epki",

Change variable name to match entry - `ecsecp256epki`

test/jdk/java/security/PEM/PEMEncoderTest.java line 124:

> 122:         kp = d.withDecryption(PEMData.ecsecp256ekpi.password()).decode(s, KeyPair.class);
> 123:         var newPriv = kp.getPrivate();
> 124:         if (Arrays.compare(origPriv.getEncoded(), newPriv.getEncoded()) != 0) {

Just use `Arrays.equals()`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356508260
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356511529
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356515723
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356517871
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356518400
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356524895
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356528661
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356530837
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356641222
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356651975
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356655793
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356448014
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356458951
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356386093
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2356400320