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

Sat May 10 02:15:15 UTC 2025

On Tue, 6 May 2025 20:56:32 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/PEMEncoder.java line 59:
> 
>> 57:  * {@linkplain X509EncodedKeySpec X509} formats.
>> 58:  *
>> 59:  * <p> Encrypted private key PEM data can be built by calling the encode methods
> 
> I understand "encode methods" here mean `encode` and `encodeToString`, but at this early place in the specification no one knows about those methods yet. Does it make sense to append several sentences at the end of the previous paragraph saying something similar to "call encode to encode, call encodeToString to encode to string".

The paragraph focus is about EKPI, not the encoding methods. I can just remove the references to the methods.

> src/java.base/share/classes/java/security/PEMEncoder.java line 63:
> 
>> 61:  * by passing an {@link EncryptedPrivateKeyInfo} object into the encode methods.
>> 62:  *
>> 63:  * <p> PKCS8 2.0 allows OneAsymmetricKey encoding, which may contain both private
> 
> It's "PKCS #8". Enclose `OneAsymmetricKey` in `<code>`.

Sure on the #8, but I don't think the enclosing is appropriate since it's not a class or field.  It's just a format.  Like I don't enclose "PEM"

> src/java.base/share/classes/java/security/PEMEncoder.java line 70:
> 
>> 68:  * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer
>> 69:  * from {@linkplain PEMRecord#type()}.  It will not check the validity of
>> 70:  * the data.
> 
> Since you mention `PEMRecord` specifically, I'd see the clarification that the `leadingData` there will not be encoded. Otherwise, you cannot guarantee on the encoding.

I think specifying the fields that are encoded makes it clear what is not in the encoding.

> src/java.base/share/classes/java/security/PEMEncoder.java line 75:
> 
>> 73:  * {@link java.nio.charset.StandardCharsets#ISO_8859_1 ISO-8859-1}.
>> 74:  *
>> 75:  * @apiNote
> 
> Why is this an `apiNote`. Can't we put an example directly into the spec. Also, please add an example on encrypting a private key.

It was a review comment back in early `24.  I don't know who asked for the change

> src/java.base/share/classes/java/security/PEMEncoder.java line 83:
> 
>> 81:  *
>> 82:  * @see PKCS8EncodedKeySpec
>> 83:  * @see X509EncodedKeySpec
> 
> I cannot see how these 2 deserve this place. I'd rather link to `PEMRecord` and `PEMDecoder`.

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 88:
> 
>> 86:  *       RFC 1421: Privacy Enhancement for Internet Electronic Mail
>> 87:  * @spec https://www.rfc-editor.org/info/rfc7468
>> 88:  *       RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures
> 
> You mentioned PKCS #8 2.0. Is it worth adding it here?

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 127:
> 
>> 125: 
>> 126:     /**
>> 127:      * Encoded a given {@code DEREncodable} and return the PEM encoding in a
> 
> Typo: `Encodes`.

yes

> src/java.base/share/classes/java/security/PEMEncoder.java line 128:
> 
>> 126:     /**
>> 127:      * Encoded a given {@code DEREncodable} and return the PEM encoding in a
>> 128:      * String
> 
> Either `<code>String</code>` or `string`.

string it is

> src/java.base/share/classes/java/security/PEMEncoder.java line 130:
> 
>> 128:      * String
>> 129:      *
>> 130:      * @param de a cryptographic object to be PEM encoded that implements
> 
> `@param de` for the 2 methods should be the same.

yes

> src/java.base/share/classes/java/security/PEMEncoder.java line 141:
> 
>> 139:      */
>> 140:     public String encodeToString(DEREncodable de) {
>> 141:         Objects.requireNonNull(de);
> 
> Do you need to check if `getFormat` of the key is "PKCS#8" or "X.509" before passing the encoding to `buildKey`? For example, we actually allows RSA key having "PKCS#1" format and ML-KEM/ML-DSA allows keys in "RAW" format.

The format is not checked.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 374:
> 
>> 372:             SecretKeyFactory factory;
>> 373:             if (provider == null) {
>> 374:                 factory = SecretKeyFactory.getInstance(algorithm);
> 
> `algorithm` might be null.

yep

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 408:
> 
>> 406:     public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key,
>> 407:         char[] password) {
>> 408:         char[] pass = password.clone();
> 
> No need to clone password here. It will be cloned into a `PBEKeySpec` later.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 458:
> 
>> 456:         }
>> 457: 
>> 458:         if (Pem.DEFAULT_ALGO == null || Pem.DEFAULT_ALGO.length() == 0) {
> 
> Is this required if `algorithm` is already specified?

Yes it should

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 573:
> 
>> 571:      *                 {@code SecretKeyFactory}, and the {@code PrivateKey},
>> 572:      *                 {@code KeyFactory}.  A {@code null} value will use the
>> 573:      *                 default provider configuration.
> 
> There is no `SecretKeyFactory` in this method, but there is `Cipher`.

Yeah, that needs rewording.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 577:
> 
>> 575:      * @throws InvalidKeyException if an error occurs during parsing of the
>> 576:      * encrypted data or creation of the key object.
>> 577:      * @throws NullPointerException if {@code key} is null.
> 
> s/key/decryptKey/.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 582:
> 
>> 580:      */
>> 581:     @PreviewFeature(feature = PreviewFeature.Feature.PEM_API)
>> 582:     public PrivateKey getKey(Key decryptKey, Provider provider)
> 
> This method goes one step further than the existing `getKeySpec(dk, p)`. It should only throw more types of exception than the existing one. Now you put everything into a `InvalidKeyException`. Is that good?

It's a questions of how many exceptions we really need to throw for a method that other than the provider, takes a Key object the user can't modify.
I suspect I kept InvalidKeyException for consistency with the getKeySpec methods.  The way I've been doing these newer APIs, I should throw an IllegalArgumentException as they are all a result of bad arguments.
But my least favorite solution is throwing all 3 checked exceptions

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080475015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080493004
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080501357
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080542968
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080607764
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080607872
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081995021
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081996525
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082000919
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082024183
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082569786
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082583428
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082590414
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082607937
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082612879
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082720137