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

[Sean Mullan]

On Thu, 8 May 2025 20:40:28 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 three additional commits since the last revision:
> 
>  - comments
>  - toString update
>  - non-sealed
>    Better X509 KeyPair parsing

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

> 89:  *
> 90:  * <p>This class is immutable and thread-safe.
> 91: 

Missing `*`.

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

> 129:      * Returns an instance of {@code PEMDecoder}.
> 130:      *
> 131:      * @return returns a {@code PEMDecoder}

you don't need to say "returns", just say "a `PEMDecoder`"

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

> 188:                         getKey(password.getPassword());
> 189:                 }
> 190:                 case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {

What about the "X.509 CERTIFICATE" header which is also mentioned in RFC 7468?

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

> 189:                 }
> 190:                 case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {
> 191:                     CertificateFactory cf = getCertFactory("X509");

Use "X.509". "X509" is an alias and may not be supported by other JDK implementations. Same comment on line 196.

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

> 198:                         new ByteArrayInputStream(decoder.decode(pem.pem())));
> 199:                 }
> 200:                 case Pem.RSA_PRIVATE_KEY -> {

Is it necessary to support this? It is not mentioned in RFC 7468.

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

> 218:      * the decoder.
> 219:      *
> 220:      * @param str a String containing PEM data.

General style comment throughout APIs: no period necessary at end when `@param`, `@return`, or `@throws` starts with a non-capital letter and no sentence follows.

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

> 221:      * @return a {@code DEREncodable} generated from the PEM data.
> 222:      * @throws IllegalArgumentException on error in decoding or if the PEM is
> 223:      * unsupported.

If the PEM is unsupported, you return a `PEMRecord` now, so you can remove those words. Same comment on lines 248-249.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082244005
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082246420
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082255980
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082251527
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082258814
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082307742
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082312862