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

Wed May 14 08:25:51 UTC 2025

On Tue, 13 May 2025 22:13:13 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 61:
> 
>> 59:  * </pre>
>> 60:  *
>> 61:  * <p> If the PEM does not have a JCE object representation, it returns a
> 
> Not "it", but the two methods.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 69:
> 
>> 67:  * methods are useful when extracting or changing the return class.
>> 68:  * The Class parameter can specify the returned
>> 69:  * key object from a PEM containing a public and private key.  If only
> 
> Not always "key". If this is meant to be an example, prepend with "For example".

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 89:
> 
>> 87:  * decoder not configured for decryption, an {@link EncryptedPrivateKeyInfo}
>> 88:  * object is returned.  {@code EncryptedPrivateKeyInfo} methods must be used to
>> 89:  * retrieve the {@link PrivateKey}.
> 
> Do we need this last sentence? "EncryptedPrivateKeyInfo methods must be used to...".

maybe not

> src/java.base/share/classes/java/security/PEMDecoder.java line 97:
> 
>> 95:  *     PEMDecoder pd = PEMDecoder.of();
>> 96:  *     PrivateKey priKey = pd.decode(priKeyPEM, PrivateKey.class);
>> 97:  * }
> 
> Add examples on `withFactory` and `withDecryption`.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 133:
> 
>> 131:      * Returns an instance of {@code PEMDecoder}.
>> 132:      *
>> 133:      * @return a new {@code PEMDecoder} instance
> 
> Not always new.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 234:
> 
>> 232:      * {@code leadingData}.
>> 233:      *
>> 234:      * <p>If no PEM data is found, an {@code IllegalArgumentException} is
> 
> Duplicated sentence. Or do you mean to say the same exception is thrown if decoding fails?

no

> src/java.base/share/classes/java/security/PEMDecoder.java line 281:
> 
>> 279:      * @param is InputStream containing PEM data
>> 280:      * @return a {@code DEREncodable}
>> 281:      * @throws IOException on IO error with the {@code InputStream}
> 
> Since IOE is not purely IO error and contains format errors that are not recoverable. Do you want to say more?

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 283:
> 
>> 281:      * @throws IOException on IO error with the {@code InputStream}
>> 282:      * @throws EOFException the end of {@code InputStream} has been
>> 283:      * unexpectedly reached.
> 
> Not really "unexpected". It looks like every program that tries to read multiple PEM blocks will encounter this.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 285:
> 
>> 283:      * unexpectedly reached.
>> 284:      * @throws IllegalArgumentException on error in decoding or no PEM data
>> 285:      * found
> 
> "no PEM data found". Isn't that EOFException?

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 312:
> 
>> 310:      * {@link java.nio.charset.StandardCharsets#UTF_8 UTF-8}.
>> 311:      *
>> 312:      * <p> For all other class parameters, {@code IllegalArgumentException} is
> 
> "For all other class parameters", you mean not `PEMRecord`? There is a paragraph in between (UTF-8 thingy) and users have forgotten what "other" means.

paragraph is unnecessary

> src/java.base/share/classes/java/security/PEMDecoder.java line 325:
> 
>> 323:      * @throws NullPointerException when any input values are null.
>> 324:      *
>> 325:      * @see PEMDecoder for how {@code tClass} can be used.
> 
> Since it's a `@see`, let's make the title a little more formal. Maybe add a header (`<h2>`) in the spec.

actually I don't think this like is necessary

> src/java.base/share/classes/java/security/PEMEncoder.java line 127:
> 
>> 125: 
>> 126:     /**
>> 127:      * Returns a new instance of {@code PEMEncoder}.
> 
> Not always new.

ok

> src/java.base/share/classes/java/security/PEMRecord.java line 50:
> 
>> 48:  * <p> During the instantiation of this record, there is no validation
>> 49:  * for the {@code type} or {@code pem}. {@code leadingData} is not
>> 50:  * defensively copied.
> 
> Not only instantiation, but the data is also not defensively copied when someone calls `leadingData`. Do we need to mention this as well?

ok

> src/java.base/share/classes/java/security/PEMRecord.java line 76:
> 
>> 74:      *                    before the PEM header.  This value maybe {@code null}.
>> 75:      * @throws IllegalArgumentException if the {@code type} is incorrectly
>> 76:      * formatted.
> 
> Is there a place defining what is a correctly formatted `type`?

The class definition for `type` given an example.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088189751
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088192299
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088202146
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088358703
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088209096
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088209735
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088232122
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088251848
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088281449
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088296864
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088290091
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088299845
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087829601
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087834756