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/java/security/PEM.java line 44:
> 42: * when the data type cannot be represented by a cryptographic object.
> 43: * If {@code PEM} is desired instead of a cryptographic object, the
> 44: * decoding methods {@link PEMDecoder#decode(String, Class)} or
Suggest rewording this, and reusing some text from the JEP as:
"If you need access to the leading data of a PEM text, or if you want to handle the text’s content yourself, use the decoding methods ... with `PEM.class` as an argument."
src/java.base/share/classes/java/security/PEM.java line 49:
> 47: *
> 48: * <p> A {@code PEM} object can be encoded back to its textual format by using
> 49: * encode methods in {@link PEMEncoder} or the {@link #toString()} method.
I'd probably say "calling" instead of "using" and switch the order, and a few other tweaks. Suggest:
"A {@code PEM} object can be encoded back to its textual format by calling the {@link #toString()} method or the encode methods of {@link PEMEncoder}."
src/java.base/share/classes/java/security/PEM.java line 55:
> 53: *
> 54: * <p>No validation is performed during instantiation to ensure that
> 55: * {@code type} conforms to {@code RFC 7468} or other legacy formats, that
I don't think RFC 7468 should be in code font.
src/java.base/share/classes/java/security/PEM.java line 61:
> 59: * Common {@code type} values include, but are not limited to:
> 60: * CERTIFICATE, CERTIFICATE REQUEST, ATTRIBUTE CERTIFICATE, X509 CRL, PKCS7,
> 61: * CMS, PRIVATE KEY, ENCRYPTED PRIVATE KEY, RSA PRIVATE KEY, or PUBLIC KEY.
s/RSA PRIVATE KEY/PRIVATE KEY/
src/java.base/share/classes/java/security/PEM.java line 63:
> 61: * CMS, PRIVATE KEY, ENCRYPTED PRIVATE KEY, RSA PRIVATE KEY, or PUBLIC KEY.
> 62: *
> 63: * <p> {@code leadingData} may be null if there was no data preceding the PEM
s/may be null/will be null/
s/was no/is no/
src/java.base/share/classes/java/security/PEM.java line 65:
> 63: * <p> {@code leadingData} may be null if there was no data preceding the PEM
> 64: * header during decoding. {@code leadingData} can be useful for reading
> 65: * metadata that accompanies PEM data. {@code leadingData} is not defensively
s/PEM/the PEM/
src/java.base/share/classes/java/security/PEM.java line 66:
> 64: * header during decoding. {@code leadingData} can be useful for reading
> 65: * metadata that accompanies PEM data. {@code leadingData} is not defensively
> 66: * copied and the {@link #leadingData()} method does not return a clone.
s/copied/copied by the constructor/
src/java.base/share/classes/java/security/PEM.java line 88:
> 86:
> 87: /**
> 88: * Creates a {@code PEM} instance with the given parameters.
s/the given/the specified/
In the other ctor, you say what the parameters are, so you should do the same here.
src/java.base/share/classes/java/security/PEM.java line 114:
> 112:
> 113: /**
> 114: * Creates a {@code PEM} instance with a given {@code type} and
s/a given/the specified/
src/java.base/share/classes/java/security/PEM.java line 115:
> 113: /**
> 114: * Creates a {@code PEM} instance with a given {@code type} and
> 115: * {@code content} data in String form. {@code leadingData} is set to null.
"in String form" is redundant since those are the types of the parameters. Suggest removing those words.
src/java.base/share/classes/java/security/PEM.java line 115:
> 113: /**
> 114: * Creates a {@code PEM} instance with a given {@code type} and
> 115: * {@code content} data in String form. {@code leadingData} is set to null.
Put null in code font.
src/java.base/share/classes/java/security/PEM.java line 130:
> 128:
> 129: /**
> 130: * Returns the type and Base64 encoding in PEM textual format.
s/Base64 encoding/Base64-encoded data/
src/java.base/share/classes/java/security/PEM.java line 130:
> 128:
> 129: /**
> 130: * Returns the type and Base64 encoding in PEM textual format.
Do we need to say "textual"? Suggest removing that word.
src/java.base/share/classes/java/security/PEM.java line 131:
> 129: /**
> 130: * Returns the type and Base64 encoding in PEM textual format.
> 131: * {@code leadingData} is not returned by this method.
Suggest rewording as "leadingData is not included in the PEM data returned by this method."
src/java.base/share/classes/java/security/PEM.java line 139:
> 137:
> 138: /**
> 139: * Returns a Base64 decoded byte array of {@code content}.
s/Base64 decoded/Base64-decoded/
src/java.base/share/classes/java/security/PEM.java line 139:
> 137:
> 138: /**
> 139: * Returns a Base64 decoded byte array of {@code content}.
Suggest stating that each invocation of this method returns a new byte array, as it isn't clear.
src/java.base/share/classes/java/security/PEM.java line 144:
> 142: * @throws IllegalArgumentException on a decoding error
> 143: *
> 144: * @see Base64#getMimeDecoder()
Suggest you also add a sentence that states this method uses the using the MIME type base64 decoding scheme, otherwise the `@see` doesn't have much context.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401784001
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401900160
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401801859
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401818161
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401824375
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401827152
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401882889
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401865919
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401861840
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401858716
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401860156
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401849924
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401851466
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401847055
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401831443
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401840888
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401839164