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

[Sean Mullan]

On Fri, 2 May 2025 06:09:52 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 with a new target base due to a merge or a rebase. The pull request now contains 66 commits:
> 
>  - major code review comments update
>  - Merge branch 'master' into pem
>  - Merge branch 'master' into pem
>  - javadoc updates
>  - code review comments
>  - merge with master
>  - better comment and remove commented out code
>  - Merge branch 'master' into pem
>  - Merge branch 'pem-merge' into pem
>  - merge
>  - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327

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

> 66:  * class is specified.
> 67:  *
> 68:  * <p> A new immutable {@code PEMDecoder} instance is created when configured

no need to say "immutable" as first sentence says all PEMDecoders are immutable.

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

> 69:  * with {@linkplain #withFactory} and/or {@linkplain #withDecryption}.
> 70:  * Configuring an instance for decryption does not prevent decoding with
> 71:  * unencrypted PEM. Any encrypted PEM that does not use the configured password

I found the last 3 sentences a bit jumping too quickly into details. I suggest having 2 sentences after the first one, where each sentence explains what the `withDecryption` and `withFactory` methods do, respectively.

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

> 90:  * @spec https://www.rfc-editor.org/info/rfc7468
> 91:  *       RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures
> 92:  *

Might be useful to add an `@see PEMEncoder` (and vice-versa).

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

> 224:      * {@code InputStream}.
> 225:      *
> 226:      * <p>This method will read the {@code InputStream} until PEM data is

Use present tense:

s/will read/reads/

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

> 226:      * <p>This method will read the {@code InputStream} until PEM data is
> 227:      * found or until the end of the stream.  Non-PEM data in the
> 228:      * {@code InputStream} before the PEM header will be ignored by the decoder.

s/will be/is/

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

> 228:      * {@code InputStream} before the PEM header will be ignored by the decoder.
> 229:      * If only non-PEM data is found a {@link PEMRecord} is returned with that
> 230:      * data.

Why? Shouldn't this be an exception as it isn't PEM?

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

> 250:      * <p>
> 251:      * {@code tClass} can be used to change the return type instance:
> 252:      * <ul>

This list feels more like programming advice. I would suggest either removing this, or putting it in the class description but not as a bulleted list, instead use complete sentences.

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

> 397: 
> 398:     /**
> 399:      * Returns a new {@code PEMDecoder} instance from the current instance

I find the words "a new instance from a current instance" a little unusual.

It might be simpler and clearer to say "Returns a copy of this PEMDecoder that will decrypt encrypted PEM data such as encrypted private keys with the specified password."

(I don't feel it is necessary to mention that it still also decodes unencrypted PEM - that seems implied to me).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078271620
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078305808
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078267302
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078316191
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078316388
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078318044
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078332711
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078358673