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

Wed Oct 9 19:18:27 UTC 2024

On Wed, 25 Sep 2024 18:50:55 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:
> 
>   fix decoding non-encrypted types

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

> 50:  * Decoding methods return a class that matches the data type and implements
> 51:  * {@link DEREncodable}.
> 52:  * If a return class is specified, an IllegalAlgorithmException is thrown if

Should `IllegalAlgorithmException` be surrounded by `{@code}`?

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

> 54:  * <p>
> 55:  * When passing input data into {@code decode}, the application is responsible
> 56:  * for processing input data non-PEM text. All data before the PEM

Maybe I'm not reading it correctly, but there may be a type-o/phrasing issue with this sentence: "When passing input data into {@code decode}, the application is responsible for processing input data non-PEM text."

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

> 89: 
> 90:     /**
> 91:      * Creates a immutable instance with a specific KeyFactory and/or password.

I suggest: "Creates an immutable instance with a specific {@code KeyFactory} provider and/or password."

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

> 90:     /**
> 91:      * Creates a immutable instance with a specific KeyFactory and/or password.
> 92:      * @param withFactory KeyFactory provider

Should `KeyFactory` be surrounded by `{@code}`?

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

> 91:      * Creates a immutable instance with a specific KeyFactory and/or password.
> 92:      * @param withFactory KeyFactory provider
> 93:      * @param withPassword char[] password for EncryptedPrivateKeyInfo

Should `EncryptedPrivateKeyInfo` be surrounded by `{@code}`?

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

> 100: 
> 101:     /**
> 102:      * Returns an instance of PEMDecoder.  This instance may be repeatedly used

Should `PEMDecoder` be surrounded by `{@code}`? Maybe I'll just suggest sweeping the javadocs for similar instances of this issue.

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

> 195:      * Decodes and returns a {@link DEREncodable} from the given
> 196:      * {@code InputStream}.
> 197:      * The method will read the {@code InputStream} until PEM data is

nit: not required, but did you mean to add a `<p>` tag here? There's a newline, so I wondered.

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

> 354: 
> 355:     /**
> 356:      * Configures and return a new PEMDecoder instance from the current instance

nit: "return" -> "returns"

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

> 369:      * Non-encrypted PEM may still be decoded from this instance.
> 370:      *
> 371:      * @param password the password to decrypt encrypted PEM data.

I suggest something to the effect of "the char array is cloned to prevent subsequent modification"

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

> 198: 
> 199:     /**
> 200:      * Encoded a given {@code DEREncodable} into PEM.

nit: "Encoded" -> "Encode(s)"

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

> 232:      */
> 233:     public PEMEncoder withEncryption(char[] password) {
> 234:         // PBEKeySpec clones the password

May consider moving this comment to the `@param`.

src/java.base/share/classes/java/security/cert/X509CRL.java line 1:

> 1: /*

Update copyright year.

src/java.base/share/classes/java/security/cert/X509Certificate.java line 1:

> 1: /*

Update copyright year, if needed.

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

> 395:      *
> 396:      * @param key The PrivateKey object to encrypt.
> 397:      * @param password the password used in the PBE encryption.

Consider indicating that a clone happens.

src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 85:

> 83:         MODULE_IMPORTS,
> 84:         //XXX Number will change when assigned
> 85:         @JEP(number=999, title="PEM API", status="Preview")

No JEP # assigned yet?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794023285
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794027533
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794036646
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794038536
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794040067
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794046026
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794058810
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794065015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794067651
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794072975
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794077342
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794088602
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794089436
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794096332
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794100097