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

Thu May 15 15:13:19 UTC 2025

On Thu, 15 May 2025 03:37:57 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 76 commits:
> 
>  - merge in JEP 513
>  - Merge branch 'master' into pem
>  - comments
>  - comments
>  - comments
>  - comments on the 11th
>  - comments on the 11th
>  - comments
>  - toString update
>  - non-sealed
>    Better X509 KeyPair parsing
>  - ... and 66 more: https://git.openjdk.org/jdk/compare/5e50a584...8bf36d6b

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

> 53:  * type and implements {@link DEREncodable}.
> 54:  *
> 55:  * The following lists the supported PEM types and the {@code DEREncodable}

The list seems too early before introducing the decode-with-class methods.

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

> 62:  *  <li>PRIVATE KEY : {@code PrivateKey}</li>
> 63:  *  <li>PRIVATE KEY : {@code PKCS8EncodedKeySpec} (Only supported when passed as a {@code Class} parameter)</li>
> 64:  *  <li>PRIVATE KEY : {@code KeyPair} (if the encoding also contains a public key)</li>

In a later paragraph you talk about reading `PublicKey` from a `PRIVATE KEY` if it is 2.0 and contains the public key. How about merging that info into this list?

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

> 99:  * Configuring an instance for decryption does not prevent decoding with
> 100:  * unencrypted PEM. Any encrypted PEM that fails decryption
> 101:  * will throw a {@link RuntimeException}. When an encrypted PEM is used with a

Let's be more clear here: `When an encrypted private key PEM is...`.

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

> 117:  *         withFactory(provider);
> 118:  *     byte[] pemData = pe.encode(privKey);
> 119:  * }

The example is still encoder.

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

> 119:  * }
> 120:  *
> 121:  * @implNote An implementation may support other PEM types and DEREncodables.

Have you considered moving the whole decoding list above into this `@implNote`? Same question with the encoder side.

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

> 288:      * the read position in the stream may become inconsistent.
> 289:      * It is recommended to perform no further decoding operations
> 290:      * on the {@code InputStream}.

I prefer the words in the previous commit. It was more positive -- the read position is here when there is no IOE.

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

> 89:  *  <li>{@code KeyPair} : PRIVATE KEY</li>
> 90:  *  <li>{@code EncryptedPrivateKeyInfo} : ENCRYPTED PRIVATE KEY</li>
> 91:  *  <li>{@code PrivateKey} (if configured with encryption): ENCRYPTED PRIVATE KEY</li>

Move the two `PrivateKey` lines together. Maybe clearly add `if not configured with encryption` to the other one.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 349:

> 347:      * @throws IllegalArgumentException on initialization errors based on the
> 348:      * arguments passed to the method
> 349:      * @throws RuntimeException on an encryption errors

Remove `an` or change to `error`.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 350:

> 348:      * arguments passed to the method
> 349:      * @throws RuntimeException on an encryption errors
> 350:      * @throws NullPointerException if the key or password are null. If

`null` in `{@code}`.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 401:

> 399:      * @throws IllegalArgumentException on initialization errors based on the
> 400:      * arguments passed to the method
> 401:      * @throws RuntimeException on an encryption errors

Remove `an` or change to `error`.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 402:

> 400:      * arguments passed to the method
> 401:      * @throws RuntimeException on an encryption errors
> 402:      * @throws NullPointerException when the {code key} or {@code password}

You forgot the `@` after `{`.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 438:

> 436:      * @throws IllegalArgumentException on initialization errors based on the
> 437:      * arguments passed to the method
> 438:      * @throws RuntimeException on an encryption errors

Remove `an` or change to `error`.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 440:

> 438:      * @throws RuntimeException on an encryption errors
> 439:      * @throws NullPointerException if the {@code key} or {@code encKey} are
> 440:      * null. If {@code params} is non-null, {@code algorithm} cannot be

Put `null` in `{@code}`.

src/java.base/share/classes/sun/security/util/Pem.java line 63:

> 61:         DEFAULT_ALGO = (algo == null || algo.isBlank()) ?
> 62:             "PBEWithHmacSHA256AndAES_128" : algo;
> 63:         pbePattern = Pattern.compile("^PBEWith.*And.*");

Is the regex check in case-insensitive mode?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091394789
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091399606
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091410536
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091411461
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091413464
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091419078
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091387820
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091268965
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091270129
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091271961
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091273449
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091275819
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091276867
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091282800