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

[Anthony Scarpino]

On Wed, 7 May 2025 18:38:31 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> 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/PEMDecoder.java line 68:
> 
>> 66:  * class is specified.
>> 67:  *
>> 68:  * <p> A new immutable {@code PEMDecoder} instance is created when configured
> 
> no need to say "immutable" as first sentence says all PEMDecoders are immutable.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 71:
> 
>> 69:  * with {@linkplain #withFactory} and/or {@linkplain #withDecryption}.
>> 70:  * Configuring an instance for decryption does not prevent decoding with
>> 71:  * unencrypted PEM. Any encrypted PEM that does not use the configured password
> 
> I found the last 3 sentences a bit jumping too quickly into details. I suggest having 2 sentences after the first one, where each sentence explains what the `withDecryption` and `withFactory` methods do, respectively.

I split them and reworded it, HTH.

> src/java.base/share/classes/java/security/PEMDecoder.java line 92:
> 
>> 90:  * @spec https://www.rfc-editor.org/info/rfc7468
>> 91:  *       RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures
>> 92:  *
> 
> Might be useful to add an `@see PEMEncoder` (and vice-versa).

ok. I added a link

> src/java.base/share/classes/java/security/PEMDecoder.java line 228:
> 
>> 226:      * <p>This method will read the {@code InputStream} until PEM data is
>> 227:      * found or until the end of the stream.  Non-PEM data in the
>> 228:      * {@code InputStream} before the PEM header will be ignored by the decoder.
> 
> s/will be/is/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 230:
> 
>> 228:      * {@code InputStream} before the PEM header will be ignored by the decoder.
>> 229:      * If only non-PEM data is found a {@link PEMRecord} is returned with that
>> 230:      * data.
> 
> Why? Shouldn't this be an exception as it isn't PEM?

I think you're right.

> src/java.base/share/classes/java/security/PEMDecoder.java line 252:
> 
>> 250:      * <p>
>> 251:      * {@code tClass} can be used to change the return type instance:
>> 252:      * <ul>
> 
> This list feels more like programming advice. I would suggest either removing this, or putting it in the class description but not as a bulleted list, instead use complete sentences.

Rewriting sounds fine.

> src/java.base/share/classes/java/security/PEMDecoder.java line 399:
> 
>> 397: 
>> 398:     /**
>> 399:      * Returns a new {@code PEMDecoder} instance from the current instance
> 
> I find the words "a new instance from a current instance" a little unusual.
> 
> It might be simpler and clearer to say "Returns a copy of this PEMDecoder that will decrypt encrypted PEM data such as encrypted private keys with the specified password."
> 
> (I don't feel it is necessary to mention that it still also decodes unencrypted PEM - that seems implied to me).

The rewrite looks good, but I want to keep the comment about unencrypted PEM.  PEMEncoder only encrypts PrivateKey and rejects any other DEREncodable.  I think it's worth mentioning.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078839633
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078849405
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078855537
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078876102
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078948310
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080055983
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078969843