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

[Jamil Nimeh]

On Thu, 12 Dec 2024 19:59:05 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 58 commits:
> 
>  - 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
>  - Reorg tests data
>    minor fixes
>  - merge from pem branch
>  - Merge branch 'pem' into pem-record
>    
>    # Conflicts:
>    #	src/java.base/share/classes/java/security/PEMEncoder.java
>    #	src/java.base/share/classes/sun/security/provider/X509Factory.java
>    #	src/java.base/share/classes/sun/security/util/Pem.java
>    #	test/jdk/java/security/PEM/PEMDecoderTest.java
>    #	test/jdk/java/security/PEM/PEMEncoderTest.java
>  - ... and 48 more: https://git.openjdk.org/jdk/compare/22845a77...cc952c0b

src/java.base/share/classes/java/security/PEMDecoder.java line 58:

> 56:  * </pre>
> 57:  *
> 58:  * A specified return class must extend {@link DEREncodable} and be an

nit: extend -> implement

src/java.base/share/classes/java/security/PEMDecoder.java line 81:

> 79:  *
> 80:  * @apiNote
> 81:  * Here is an example of encoding a PrivateKey object:

Nit: encoding -> decoding

src/java.base/share/classes/java/security/PEMDecoder.java line 143:

> 141:                 case Pem.PRIVATE_KEY -> {
> 142:                     PKCS8Key p8key = new PKCS8Key(decoder.decode(pem.pem()));
> 143:                     KeyFactory kf = getKeyFactory(p8key.getAlgorithm());

nit: I see a lot of `p8key.getAlgorithm()` calls in here.  If it's always going to be the same value, perhaps it could be assigned to a String and just reuse it throughout (though it may not be usable for the `p8.getAlgorithm()` calls further down).

src/java.base/share/classes/java/security/PEMDecoder.java line 294:

> 292: 
> 293:         if (tClass.isAssignableFrom(PEMRecord.class)) {
> 294:         //if (PEMRecord.class.isInstance(tClass)) {

nit: dead code?

src/java.base/share/classes/java/security/PEMRecord.java line 48:

> 46:  * All values can never be null.
> 47:  *
> 48:  * During the instantiation of this record, there is no validation for the

These two sentences are redundant.  Either one is fine to keep though.

src/java.base/share/classes/java/security/PEMRecord.java line 53:

> 51:  * instantiation of this record.
> 52:  *
> 53:  * @param type The type identifier in the PEM header.  For a public key,

Since there are specific strings that will be needed in the `type` field, is there a pointer we could add so folks looking at this could see what values might be acceptable for this field.  I don't know if right here is the right place for it, but maybe a see-also or something?  Or is this left open-ended to support whatever could exist in a PEM header?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992070595
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992074350
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992118727
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992132131
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992257185
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1992263251