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

Tue May 13 18:38:15 UTC 2025

On Tue, 13 May 2025 09:27:37 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 55:

> 53:  *
> 54:  * <p> Encoding may be performed on cryptographic objects that implement
> 55:  * {@link DEREncodable}.

Suggest adding another sentence after this: "The {@link encode(DEREncodable)} and {@link encodeToString(DEREncodable)} methods encode a `DEREncodable` into PEM and return the data in a byte array or String."

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

> 60:  * configured to encrypt the key with that password. Alternatively, a
> 61:  * private key encrypted as an {@code EncryptedKeyInfo} object can be encoded
> 62:  * directly to PEM by passing it to the encode method

s/encode method/`encode` or `encodeToString` methods./

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

> 64:  * <p> PKCS #8 2.0 defines the ASN.1 OneAsymmetricKey structure, which may
> 65:  * contain both private and public keys.
> 66:  * {@link KeyPair} objects passed to the {@code encode} methods are encoded as a

s/{@code encode}/{@code encode} or {@code encodeToString}/

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

> 68:  *
> 69:  * <p> When encoding a {@link PEMRecord}, the API surrounds the
> 70:  * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer

delete "a generated".

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

> 70:  * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer
> 71:  * from {@linkplain PEMRecord#type()}. {@linkplain PEMRecord#leadingData()} is
> 72:  * not included in the encoding.  {@code PEMRecord} will not preform

s/preform/perform/

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

> 82:  *
> 83:  * <p> Here is an example that encrypts and encodes a private key using the
> 84:  * specified password.

s/./:/

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

> 139:      * string.
> 140:      *
> 141:      * @param de the {@code DEREncodable} to be encoded.

Nit: no need for period.

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

> 140:      *
> 141:      * @param de the {@code DEREncodable} to be encoded.
> 142:      * @return a byte array containing the PEM encoded data

s/a byte array/a String/

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

> 141:      * @param de the {@code DEREncodable} to be encoded.
> 142:      * @return a byte array containing the PEM encoded data
> 143:      * @throws IllegalArgumentException If the DEREncodable cannot be encoded

s/If/if/

Put code around DEREncodable.

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

> 142:      * @return a byte array containing the PEM encoded data
> 143:      * @throws IllegalArgumentException If the DEREncodable cannot be encoded
> 144:      * @throws NullPointerException if {@code de} is {@code null}.

Nit: no need for period.

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

> 207:      * in a byte array.
> 208:      *
> 209:      * @param de the {@code DEREncodable} to be encoded.

Nit: no need for period.

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

> 209:      * @param de the {@code DEREncodable} to be encoded.
> 210:      * @return PEM encoded byte array
> 211:      * @throws IllegalArgumentException if the DEREncodable cannot be encoded

Put code around DEREncodable.

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

> 210:      * @return PEM encoded byte array
> 211:      * @throws IllegalArgumentException if the DEREncodable cannot be encoded
> 212:      * @throws NullPointerException if {@code de} is {@code null}.

Nit: no need for period.

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

> 214:      */
> 215:     public byte[] encode(DEREncodable de) {
> 216:         return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1);

Side comment, can look at this later. Since `Base64.Encoder.encodeToString()` first encodes to bytes and then converts it to a String, it might be more efficient to do the same here.

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

> 218: 
> 219:     /**
> 220:      * Returns a new {@code PEMEncoder} instance configured with the default

s/configured/configured for encryption/

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

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

Wording suggestion: "The default password-based encryption algorithm is defined by the `jdk.epkcs8.defaultAlgorithm` security property and uses the default encryption parameters of the provider that is selected."

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

> 233:      * returned object with {@link #encode(DEREncodable)}.
> 234:      *
> 235:      * @param password sets the encryption password.  The array is cloned and

No need to say "sets".

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

> 235:      * @param password sets the encryption password.  The array is cloned and
> 236:      *                stored in the new instance. {@code null} is a valid value.
> 237:      * @return new configured {@code PEMEncoder} instance

Suggest: "a new {@code PEMEncoder} instance configured for encryption"

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087411805
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087416237
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087417397
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087419002
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087419886
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087420308
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087358392
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087359029
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087360161
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087358550
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087345856
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087346460
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087346007
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087357208
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087398280
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087373659
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087389673
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087398501