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

[Jamil Nimeh]

On Thu, 13 Mar 2025 01:15:03 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 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/sun/security/ec/ECKeyFactory.java line 232:

> 230:         } else if (keySpec instanceof PKCS8EncodedKeySpec p8) {
> 231:             PKCS8Key p8key = new ECPrivateKeyImpl(p8.getEncoded());
> 232:             if (p8key.hasPublicKey()) {

Shouldn't that be `if (!p8key.hasPublicKey()) {`, similar to XDHKeyFactory.generatePublicImpl() and others?

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 55:

> 53:  *        ...
> 54:  *      }
> 55:  * <p>

Actually this is for the next line, and just a nit: It appears that now we are at least parsing/saving the attributes and definitely attaching the public key when decoding from DER.  Should this line be removed?

src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 207:

> 205:             byte[] internal = rawKey.generateEncoding();
> 206:             PKCS8EncodedKeySpec pkcs8KeySpec =
> 207:                 new PKCS8EncodedKeySpec(internal);

Since you don't use `internal` beyond this point AFAICT, you may not need the reference as a local variable and just inline the call to `rawKey.generateEncoding()`.  But this is more of a nit/style thing so no big deal if you want to leave it as-is.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001457211
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001501575
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001518767