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

Mon May 5 13:40:03 UTC 2025

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/PEMRecord.java line 46:

> 44:  * <p> {@code PEMRecord} may have a null {@code type} and {@code pem} when
> 45:  * {@code PEMDecoder.decode()} methods encounter only non-PEM data and has
> 46:  * reached the end of the stream. If there is PEM data, {@code type} and

Is this use case important? It seems unusual to call this a DEREncodable if it only contains non-PEM data. I think it might be better to simply ignore trailing non-PEM data at the end of a stream which is not attached to any subsequent PEM objects.

src/java.base/share/classes/java/security/PEMRecord.java line 53:

> 51:  * {@code type} or {@code pem}.
> 52:  *
> 53:  * @param type The type identifier in the PEM header.  For a public key,

parameter descriptions should not start with a capital letter. Same comment for the params in the ctors.

src/java.base/share/classes/java/security/PEMRecord.java line 67:

> 65: 
> 66:     /**
> 67:      * Return a PEMRecord instance with the given parameters.

s/Return/Creates/    (same comment for all ctors)
PEMRecord should be in code font.

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

> 69:      * <p> When {@code type} is given a properly formatted PEM header, only the
> 70:      * identifier will be set (ie: {@code PUBLIC KEY}).  Otherwise, {@code type}
> 71:      * will be set to what was passed in.

Put PUBLIC KEY in double quotes so it is clear this is a String.

I'm not sure I understand this paragraph - will the dashes be included if the type is not understood? If not, why do we need this paragraph at all?

src/java.base/share/classes/java/security/PEMRecord.java line 75:

> 73:      * <p> When {@code type} is given a correctly formatted PEM header, only the
> 74:      * identifier is set (for example, {@code PUBLIC KEY}). Otherwise,
> 75:      * {@code type} is set to the value that was passed in.

Duplicate paragaraph as first.

src/java.base/share/classes/java/security/PEMRecord.java line 78:

> 76:      *
> 77:      * @param type The type identifier in the PEM header and footer.
> 78:      *             If there is no PEM data, this value will be {@code null}.

Suggest rewording as "the type identifier in the PEM header and footer, or {@code null} if there is no PEM data." Same comment for other ctors.

src/java.base/share/classes/java/security/PEMRecord.java line 137:

> 135:     /**
> 136:      * Returns the binary encoding from the Base64 data contained in
> 137:      * {@code pem}.

Should say that it returns a new copy each time it is called.

src/java.base/share/classes/java/security/PEMRecord.java line 139:

> 137:      * {@code pem}.
> 138:      *
> 139:      * @throws IllegalArgumentException if {@code pem} could not be decoded.

s/could not/cannot/

src/java.base/share/classes/java/security/PEMRecord.java line 140:

> 138:      *
> 139:      * @throws IllegalArgumentException if {@code pem} could not be decoded.
> 140:      * @return binary encoding or null if {@code pem} is null.

s/binary/the binary/

src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 245:

> 243:                 throw new InvalidKeySpecException("Only ECPrivateKeySpec " +
> 244:                     "and PKCS8EncodedKeySpec supported for EC private keys. " +
> 245:                     keySpec.getClass().getName() + " provided.");

How about using the message "keySpec.getClass().getName() + " not supported." as on lines 223-224?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073424234
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073377746
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073380371
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073456846
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073441921
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073442784
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073386293
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073388519
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073387689
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2071975085