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

Sat Apr 26 07:26:07 UTC 2025

On Thu, 17 Apr 2025 15:14:33 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 61 commits:
>> 
>>  - merge with master
>>  - better comment and remove commented out code
>>  - Merge branch 'master' into pem
>>  - Merge branch 'pem-merge' into pem
>>  - merge
>>  - Merge in PEMRecord as part of the API.
>>  - Merge branch 'pem-record' into pem-merge
>>    
>>    # Conflicts:
>>    #	src/java.base/share/classes/java/security/PEMDecoder.java
>>    #	src/java.base/share/classes/java/security/PEMRecord.java
>>    #	src/java.base/share/classes/sun/security/util/Pem.java
>>    #	test/jdk/java/security/PEM/PEMData.java
>>    #	test/jdk/java/security/PEM/PEMDecoderTest.java
>>    #	test/jdk/java/security/PEM/PEMEncoderTest.java
>>  - Merge branch 'master' into pem-record
>>    
>>    # Conflicts:
>>    #	src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java
>>  - test fixes & cleanup
>>  - Implement stream decoding
>>    fix StringBuffer/Builder
>>    X509C* changes
>>  - ... and 51 more: https://git.openjdk.org/jdk/compare/a347ecde...106788ef
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 43:
> 
>> 41: 
>> 42: /**
>> 43:  * {@code PEMDecoder} is an immutable class for Privacy-Enhanced Mail (PEM)
> 
> s/for/for decoding/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 46:
> 
>> 44:  * data.  PEM is a textual encoding used to store and transfer security
>> 45:  * objects, such as asymmetric keys, certificates, and certificate revocation
>> 46:  * lists (CRL).  It is defined in RFC 1421 and RFC 7468.  PEM consists of a
> 
> s/CRL/CRLs/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 51:
> 
>> 49:  *
>> 50:  * <p> Decoding methods return an instance of a class that matches the data
>> 51:  * type and implements {@link DEREncodable} unless specified. The following
> 
> "unless otherwise specified"?

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 64:
> 
>> 62:  * <p> If the PEM does not have a corresponding JCE object, it returns a
>> 63:  * {@link PEMRecord}. Any PEM can be decoded into a {@code PEMRecord} if the
>> 64:  * class is specified. Decoding a {@code PEMRecord} returns corresponding
> 
> What do you mean by "Decoding a PEMRecord"? 
> s/returns/returns a/

I was mistakenly documenting an internal method

> src/java.base/share/classes/java/security/PEMDecoder.java line 65:
> 
>> 63:  * {@link PEMRecord}. Any PEM can be decoded into a {@code PEMRecord} if the
>> 64:  * class is specified. Decoding a {@code PEMRecord} returns corresponding
>> 65:  * JCE object or throws a {@link IllegalArgumentException} if no object is
> 
> s/a/an/

removed

> src/java.base/share/classes/java/security/PEMDecoder.java line 68:
> 
>> 66:  * available.
>> 67:  *
>> 68:  * <p> A new immutable {@code PEMDecoder} instance is created by using
> 
> Suggest rewording as ... "A `PEMDecoder` can be configured with additional options by calling ..."

I changed "calling" to "configured".  Using your full text means the "new instance" part needs to be said elsewhere.

> src/java.base/share/classes/java/security/PEMDecoder.java line 71:
> 
>> 69:  * {@linkplain #withFactory} and/or {@linkplain #withDecryption}.  Configuring
>> 70:  * an instance for decryption does not prevent decoding with unencrypted PEM.
>> 71:  * Any encrypted PEM that does not use the configured password will throw an
> 
> s/an/a/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 74:
> 
>> 72:  * {@link SecurityException}. A decoder instance not configured with decryption
>> 73:  * returns an {@link EncryptedPrivateKeyInfo} with encrypted PEM.
>> 74:  * EncryptedPrivateKeyInfo methods must be used to retrieve the
> 
> Put code around EncryptedPrivateKeyInfo.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 197:
> 
>> 195:             };
>> 196:         } catch (GeneralSecurityException | IOException e) {
>> 197:             throw new IllegalArgumentException(e);
> 
> Should all of these decoding issues really be treated as `IllegalArgumentException`s? I would think that general decoding issues should be thrown as checked exceptions. Runtime exceptions are typically used for serious issues, like programming errors.

It has been the intent to avoid checked exceptions with these methods and these methods follow the same behavior and exception as `Base64.Decoder.decode()`

> src/java.base/share/classes/java/security/PEMDecoder.java line 202:
> 
>> 200: 
>> 201:     /**
>> 202:      * Decodes and returns {@link DEREncodable} from the given string.
> 
> s/returns/returns a/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 210:
> 
>> 208:      */
>> 209:     public DEREncodable decode(String str) {
>> 210:         Objects.requireNonNull(str);
> 
> Missing an `@throws NullPointerException` in the spec.

Added in an intermediate changeset

> src/java.base/share/classes/java/security/PEMDecoder.java line 236:
> 
>> 234:      */
>> 235:     public DEREncodable decode(InputStream is) throws IOException {
>> 236:         Objects.requireNonNull(is);
> 
> Missing an `@throws NullPointerException` in the spec.

Added in an intermediate changeset

> src/java.base/share/classes/java/security/PEMDecoder.java line 259:
> 
>> 257:      * @param <S> Class type parameter that extends {@code DEREncodable}
>> 258:      * @param string the String containing PEM data.
>> 259:      * @param tClass the returned object class that implementing
> 
> s/implementing/implements/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 261:
> 
>> 259:      * @param tClass the returned object class that implementing
>> 260:      * {@code DEREncodable}.
>> 261:      * @return A {@code DEREncodable} typecast to {@code tClass}.
> 
> s/A/a/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 266:
> 
>> 264:      */
>> 265:     public <S extends DEREncodable> S decode(String string, Class<S> tClass) {
>> 266:         Objects.requireNonNull(string);
> 
> Missing an `@throws NullPointerException` in the spec.

Added in an intermediate changeset

> src/java.base/share/classes/java/security/PEMDecoder.java line 282:
> 
>> 280:      * @param <S> Class type parameter that extends {@code DEREncodable}
>> 281:      * @param is an InputStream containing PEM data.
>> 282:      * @param tClass the returned object class that implementing
> 
> s/implementing/implements/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 284:
> 
>> 282:      * @param tClass the returned object class that implementing
>> 283:      *   {@code DEREncodable}.
>> 284:      * @return A {@code DEREncodable} typecast to {@code tClass}
> 
> s/A/a/

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 294:
> 
>> 292:     public <S extends DEREncodable> S decode(InputStream is, Class<S> tClass)
>> 293:         throws IOException {
>> 294:         Objects.requireNonNull(is);
> 
> Missing an `@throws NullPointerException` in the spec.

Added in the recent update

> src/java.base/share/classes/java/security/PEMDecoder.java line 359:
> 
>> 357:             }
>> 358:             return KeyFactory.getInstance(algorithm, factory);
>> 359:         } catch(GeneralSecurityException e){
> 
> Nit, add space after `catch` and before `{`.

yes

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758001
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758108
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758230
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052679240
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052679623
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052691444
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052692601
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052692794
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049729802
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052698263
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052699015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052699478
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700392
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700498
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700709
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700833
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700940
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049721541
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052756431