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

[Sean Mullan]

On Tue, 13 May 2025 09:27:37 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 on the 11th

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

> 40:  * {@code DEREncodable} for the type.
> 41:  *
> 42:  * <p>{@code PEMRecord} may have a null {@code type} and {@code pem} when

I think this sentence can be removed now that non-PEM data is considered an error.

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

> 46:  *
> 47:  * <p> During the instantiation of this record, there is no validation for the
> 48:  * {@code type} or {@code pem}.

The ctors throw `IllegalArgumentException` though. I think you need to be more specific about what type of validation you mean here.

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

> 74:      *             {@code null} if there is no PEM data.
> 75:      * @param pem the data between the PEM header and footer.
> 76:      * @param leadingData Any non-PEM data read during the decoding process

s/Any/any/

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

> 75:      * @param pem the data between the PEM header and footer.
> 76:      * @param leadingData Any non-PEM data read during the decoding process
> 77:      *                    before the PEM header.  This value maybe {@code null}.

s/maybe/may be/

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

> 85:         if (type == null && pem != null || type != null && pem == null) {
> 86:             throw new IllegalArgumentException("\"type\" and \"pem\" must be" +
> 87:                 " both null or non-null");

If type and pem can be both null, and leadingData is also null (which is allowed per spec) then that should not be an error?

This check may need to be updated now that non-PEM data is an error.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087677034
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087681167
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087680087
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087680232
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087684345