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

Mon May 12 22:46:14 UTC 2025

On Sun, 11 May 2025 19:02:55 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 one additional commit since the last revision:
> 
>   comments on the 11th

src/java.base/share/classes/java/security/PEMEncoder.java line 47:

> 45: 
> 46: /**
> 47:  * {@code PEMEncoder} is used for encoding Privacy-Enhanced Mail (PEM) data.

suggest changing "is used" to "implements an encoder" - then it will be consistent with PEMDecoder.

src/java.base/share/classes/java/security/PEMEncoder.java line 59:

> 57:  * <p> Encrypted private key PEM data can be built by encoding with a
> 58:  * {@code PEMEncoder} instance returned by {@linkplain #withEncryption(char[])}
> 59:  * or by encoding an {@link EncryptedPrivateKeyInfo} .

Suggest rewording as: "Private keys can be encrypted and encoded by configuring a `PEMEncoder` with the `withEncryption` method, which takes a password and returns a new `PEMEncoder` instance configured to encrypt the key with that password. Alternatively, a private key encrypted as an `EncryptedKeyInfo` object can be encoded directly to PEM by passing it to the `encode` method."

src/java.base/share/classes/java/security/PEMEncoder.java line 61:

> 59:  * or by encoding an {@link EncryptedPrivateKeyInfo} .
> 60:  *
> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both

s/allows .../defines the ASN.1 OneAsymmetricKey structure/

src/java.base/share/classes/java/security/PEMEncoder.java line 62:

> 60:  *
> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both
> 62:  * private and public keys in the same PEM. This is supported by using the

PKCS #8 doesn't define PEM. Suggest  removing "in the same PEM".

src/java.base/share/classes/java/security/PEMEncoder.java line 63:

> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both
> 62:  * private and public keys in the same PEM. This is supported by using the
> 63:  * {@link KeyPair} class with the encode methods.

Suggest rewording 2nd sentence as: "`KeyPair` objects passed to the `encode` methods are encoded as a OneAsymmetricKey structure using the "PRIVATE KEY" type."

src/java.base/share/classes/java/security/PEMEncoder.java line 70:

> 68:  * the data.
> 69:  *
> 70:  * <p>{@code String} values encoded use character set

Why does this need to be mentioned if it is an RFC PEM requirement?

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

> 79:  * }
> 80:  *
> 81:  * <p>To make the {@code PEMEncoder} encrypt the above private key, only the

Suggest rewording as "Here is an example that encrypts and encodes a private key using the specified password."

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

> 93:  *       RFC 1421: Privacy Enhancement for Internet Electronic Mail
> 94:  * @spec https://www.rfc-editor.org/info/rfc5958
> 95:  *       RFC 5958: Asymmetric Key Packages

Do we really need to add RFC 5958 here? This class is just doing the PEM encoding, it doesn't implement RFC 5958.

src/java.base/share/classes/java/security/PEMEncoder.java line 129:

> 127:      * Returns a new instance of {@code PEMEncoder}.
> 128:      *
> 129:      * @return new {@code PEMEncoder} instance

"new" sounds like it is a new instance each time this method is called. Suggest just saying "Returns a `PEMDecoder`"

src/java.base/share/classes/java/security/PEMEncoder.java line 136:

> 134: 
> 135:     /**
> 136:      * Encode the specified {@code DEREncodable} and return the PEM encoding in a

s/Encode/Encodes/

s/return/returns/

src/java.base/share/classes/java/security/PEMEncoder.java line 140:

> 138:      *
> 139:      * @param de the {@code DEREncodable} to be encoded.
> 140:      * @return PEM encoding in a string

Suggest: "a PEM encoded string"

src/java.base/share/classes/java/security/PEMEncoder.java line 141:

> 139:      * @param de the {@code DEREncodable} to be encoded.
> 140:      * @return PEM encoding in a string
> 141:      * @throws IllegalArgumentException when encoding the {@code DEREncodable}

Suggest: "if the `DEREncodable` cannot be encoded`

src/java.base/share/classes/java/security/PEMEncoder.java line 205:

> 203: 
> 204:     /**
> 205:      * Encodes the specified {@code DEREncodable} into PEM.

Make description consistent with other encode method: "Encodes the specified {@code DEREncodable} and returns the PEM encoding in a byte array."

src/java.base/share/classes/java/security/PEMEncoder.java line 208:

> 206:      *
> 207:      * @param de the {@code DEREncodable} to be encoded.
> 208:      * @return PEM encoded byte array

Suggest: "a byte array containing the PEM encoded data"

src/java.base/share/classes/java/security/PEMEncoder.java line 209:

> 207:      * @param de the {@code DEREncodable} to be encoded.
> 208:      * @return PEM encoded byte array
> 209:      * @throws IllegalArgumentException when encoding the {@code DEREncodable}

"If the `DEREncodable` cannot be encoded"

src/java.base/share/classes/java/security/PEMEncoder.java line 214:

> 212:      * @see #withEncryption(char[])
> 213:      */
> 214:     public byte[] encode(DEREncodable de) {

Is this method that useful? Caller can just do what line 215 is doing.

src/java.base/share/classes/java/security/PEMEncoder.java line 224:

> 222:      * <p> Only {@link PrivateKey} objects can be encrypted with this newly
> 223:      * configured instance.  Encoding other {@link DEREncodable} objects will
> 224:      * throw an{@code IllegalArgumentException}.

space after "an"

src/java.base/share/classes/java/security/PEMEncoder.java line 228:

> 226:      * @implNote The default algorithm is defined by Security Property {@code
> 227:      * jdk.epkcs8.defaultAlgorithm} using default password-based encryption
> 228:      * parameters by the supporting provider.  To configure all the encryption

Maybe reword as "If you need more control over the encryption algorithm and parameters, use the ..."

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085589104
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085605635
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085606515
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085610213
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085614212
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085617629
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085622259
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085623589
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085626881
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085627060
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085628121
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085628841
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085630670
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085631422
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085631103
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085632879
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085633317
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085634539