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

Tue May 13 22:32:15 UTC 2025

On Tue, 13 May 2025 21:45:32 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

Quickly going through public APIs.

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

> 59:  * </pre>
> 60:  *
> 61:  * <p> If the PEM does not have a JCE object representation, it returns a

Not "it", but the two methods.

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

> 67:  * methods are useful when extracting or changing the return class.
> 68:  * The Class parameter can specify the returned
> 69:  * key object from a PEM containing a public and private key.  If only

Not always "key". If this is meant to be an example, prepend with "For example".

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

> 87:  * decoder not configured for decryption, an {@link EncryptedPrivateKeyInfo}
> 88:  * object is returned.  {@code EncryptedPrivateKeyInfo} methods must be used to
> 89:  * retrieve the {@link PrivateKey}.

Do we need this last sentence? "EncryptedPrivateKeyInfo methods must be used to...".

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

> 95:  *     PEMDecoder pd = PEMDecoder.of();
> 96:  *     PrivateKey priKey = pd.decode(priKeyPEM, PrivateKey.class);
> 97:  * }

Add examples on `withFactory` and `withDecryption`.

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

> 131:      * Returns an instance of {@code PEMDecoder}.
> 132:      *
> 133:      * @return a new {@code PEMDecoder} instance

Not always new.

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

> 232:      * {@code leadingData}.
> 233:      *
> 234:      * <p>If no PEM data is found, an {@code IllegalArgumentException} is

Duplicated sentence. Or do you mean to say the same exception is thrown if decoding fails?

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

> 279:      * @param is InputStream containing PEM data
> 280:      * @return a {@code DEREncodable}
> 281:      * @throws IOException on IO error with the {@code InputStream}

Since IOE is not purely IO error and contains format errors that are not recoverable. Do you want to say more?

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

> 281:      * @throws IOException on IO error with the {@code InputStream}
> 282:      * @throws EOFException the end of {@code InputStream} has been
> 283:      * unexpectedly reached.

Not really "unexpected". It looks like every program that tries to read multiple PEM blocks will encounter this.

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

> 283:      * unexpectedly reached.
> 284:      * @throws IllegalArgumentException on error in decoding or no PEM data
> 285:      * found

"no PEM data found". Isn't that EOFException?

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

> 310:      * {@link java.nio.charset.StandardCharsets#UTF_8 UTF-8}.
> 311:      *
> 312:      * <p> For all other class parameters, {@code IllegalArgumentException} is

"For all other class parameters", you mean not `PEMRecord`? There is a paragraph in between (UTF-8 thingy) and users have forgotten what "other" means.

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

> 323:      * @throws NullPointerException when any input values are null.
> 324:      *
> 325:      * @see PEMDecoder for how {@code tClass} can be used.

Since it's a `@see`, let's make the title a little more formal. Maybe add a header (`<h2>`) in the spec.

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

> 125: 
> 126:     /**
> 127:      * Returns a new instance of {@code PEMEncoder}.

Not always new.

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

PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2838231399
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087706506
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087707481
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087708948
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087709526
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087709760
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087711953
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087714476
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087713003
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087713801
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087716326
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087717780
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087702103