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

[Anthony Scarpino]

On Wed, 7 May 2025 16:37:00 GMT, Weijun Wang <weijun 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 60:
> 
>> 58:  * </pre>
>> 59:  *
>> 60:  * A specified return class must implement {@link DEREncodable} and be an
> 
> The `decode(data, Class)` method already requires the class be a `DEREncodable` otherwise it won't compile. So this is no longer a requirement for the caller and there is no need to say `must implement DEREncodable`.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 85:
> 
>> 83:  * <pre>
>> 84:  *     PEMDecoder pd = PEMDecoder.of();
>> 85:  *     PrivateKey priKey = pd.decode(priKeyPEM);
> 
> `decode` does not return a `PrivateKey`. Either cast or specify the class.

I think I originally had that as 'var'. That allows it to compile for the sake of the example.  I wanted to show a simple example without getting into specifying a class.

> 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.
> 
> This is not precise because you might put the leading data into a `PEMRecord`.
> 
> Also, I think it's very important to point out the pointer position of the input stream after this method is successfully called. Otherwise, the method cannot be reliably used in extracting multiple PEMs from a single stream.
> 
> Also, I'm not exactly sure what happens when there is nothing more to read.

There is an inconsistency with PEMRecord here.  These methods should not allow leadingData if it is not available elsewhere.  I will redo that.  leadingData can be reserved for the decode methods that take a class.

I tried not to mention the pointer position.  My opinion is these methods should not have to describe the InputStream functionality.  I already state it returns a value after it finds PEM and I think one can figure this out.

> src/java.base/share/classes/java/security/PEMDecoder.java line 384:
> 
>> 382:     /**
>> 383:      * Configures and returns a new {@code PEMDecoder} instance from the
>> 384:      * current instance that will use KeyFactory and CertificateFactory classes
> 
> Put `KeyFactory` and `CertificateFactory` into `<code>`.

probably should be a link, but yes.

> src/java.base/share/classes/java/security/PEMDecoder.java line 391:
> 
>> 389:      * the default provider configuration.
>> 390:      *
>> 391:      * @param provider the Factory provider.
> 
> s/Factory/factory/

yes

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078839155
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078854096
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078876015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078960107
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078960615