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

[Anthony Scarpino]

On Fri, 9 May 2025 15:13:29 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Anthony Scarpino has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - comments
>>  - toString update
>>  - non-sealed
>>    Better X509 KeyPair parsing
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 64:
> 
>> 62:  * class is specified.
>> 63:  *
>> 64:  * For decode methods that accept a {@code Class<S> tClass} as input, they can
> 
> Start a new paragraph. Suggest rewording the beginning as "The `{@linkplain #decode(String, Class<S>) }` and `{@linkplain #decode(InputStream, Class<S>)}` methods take a `Class` parameter which determines the type of `DerEncodable` that is returned. These methods are useful when ..."
> 
> Then you can insert the sentence above somewhere towards the end of this paragraph: "Any type of PEM data can be decoded into a `{@code PEMRecord}` by specifying `{@code PEMRecord.class}` as a parameter."

Ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 70:
> 
>> 68:  * the returned key object from a PEM containing a public and private key.
>> 69:  * If only the private key is required, {@code PrivateKey.class} can be used.
>> 70:  * {@class PEMRecord.class} is used for returning PEM text. If {@code tClass}
> 
> This causes a javadoc error, I think you meant `@code` instead of `@class`.

yeah, saw that after the push

> src/java.base/share/classes/java/security/PEMDecoder.java line 79:
> 
>> 77:  * Configuring an instance for decryption does not prevent decoding with
>> 78:  * unencrypted PEM. Any encrypted PEM that does not use the configured password
>> 79:  * will throw an {@link RuntimeException}.
> 
> s/an/a/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 91:
> 
>> 89:  *
>> 90:  * <p>This class is immutable and thread-safe.
>> 91: 
> 
> Missing `*`.

yeah, cleaned that up

> src/java.base/share/classes/java/security/PEMDecoder.java line 93:
> 
>> 91: 
>> 92:  * @apiNote
>> 93:  * Here is an example of decoding a PrivateKey object:
> 
> Put `@code` around PrivateKey.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 131:
> 
>> 129:      * Returns an instance of {@code PEMDecoder}.
>> 130:      *
>> 131:      * @return returns a {@code PEMDecoder}
> 
> you don't need to say "returns", just say "a `PEMDecoder`"

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 190:
> 
>> 188:                         getKey(password.getPassword());
>> 189:                 }
>> 190:                 case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {
> 
> What about the "X.509 CERTIFICATE" header which is also mentioned in RFC 7468?

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 191:
> 
>> 189:                 }
>> 190:                 case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {
>> 191:                     CertificateFactory cf = getCertFactory("X509");
> 
> Use "X.509". "X509" is an alias and may not be supported by other JDK implementations. Same comment on line 196.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 200:
> 
>> 198:                         new ByteArrayInputStream(decoder.decode(pem.pem())));
>> 199:                 }
>> 200:                 case Pem.RSA_PRIVATE_KEY -> {
> 
> Is it necessary to support this? It is not mentioned in RFC 7468.

Very much so, it's has been seen elsewhere than the PKCS#1 format is still used.  Our RSA has supported it for a long time.

> src/java.base/share/classes/java/security/PEMDecoder.java line 220:
> 
>> 218:      * the decoder.
>> 219:      *
>> 220:      * @param str a String containing PEM data.
> 
> General style comment throughout APIs: no period necessary at end when `@param`, `@return`, or `@throws` starts with a non-capital letter and no sentence follows.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 223:
> 
>> 221:      * @return a {@code DEREncodable} generated from the PEM data.
>> 222:      * @throws IllegalArgumentException on error in decoding or if the PEM is
>> 223:      * unsupported.
> 
> If the PEM is unsupported, you return a `PEMRecord` now, so you can remove those words. Same comment on lines 248-249.

ok

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083274773
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083274961
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275316
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275184
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275285
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275393
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276024
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276160
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083595650
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083278879