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

Tue May 13 09:27:40 UTC 2025

On Mon, 12 May 2025 22:09:14 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   comments on the 11th
>
> src/java.base/share/classes/java/security/PEMEncoder.java line 61:
> 
>> 59:  * or by encoding an {@link EncryptedPrivateKeyInfo} .
>> 60:  *
>> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both
> 
> s/allows .../defines the ASN.1 OneAsymmetricKey structure/

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 62:
> 
>> 60:  *
>> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both
>> 62:  * private and public keys in the same PEM. This is supported by using the
> 
> PKCS #8 doesn't define PEM. Suggest  removing "in the same PEM".

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 63:
> 
>> 61:  * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain both
>> 62:  * private and public keys in the same PEM. This is supported by using the
>> 63:  * {@link KeyPair} class with the encode methods.
> 
> Suggest rewording 2nd sentence as: "`KeyPair` objects passed to the `encode` methods are encoded as a OneAsymmetricKey structure using the "PRIVATE KEY" type."

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 70:
> 
>> 68:  * the data.
>> 69:  *
>> 70:  * <p>{@code String} values encoded use character set
> 
> Why does this need to be mentioned if it is an RFC PEM requirement?

It was documented with `PEMDecoder` I thought I should put it here as well, but I can remove it.

> src/java.base/share/classes/java/security/PEMEncoder.java line 95:
> 
>> 93:  *       RFC 1421: Privacy Enhancement for Internet Electronic Mail
>> 94:  * @spec https://www.rfc-editor.org/info/rfc5958
>> 95:  *       RFC 5958: Asymmetric Key Packages
> 
> Do we really need to add RFC 5958 here? This class is just doing the PEM encoding, it doesn't implement RFC 5958.

@wangweij asked for it. If you disagree I can remove it.

> src/java.base/share/classes/java/security/PEMEncoder.java line 129:
> 
>> 127:      * Returns a new instance of {@code PEMEncoder}.
>> 128:      *
>> 129:      * @return new {@code PEMEncoder} instance
> 
> "new" sounds like it is a new instance each time this method is called. Suggest just saying "Returns a `PEMEncoder`"

Ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 136:
> 
>> 134: 
>> 135:     /**
>> 136:      * Encode the specified {@code DEREncodable} and return the PEM encoding in a
> 
> s/Encode/Encodes/
> 
> s/return/returns/

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 140:
> 
>> 138:      *
>> 139:      * @param de the {@code DEREncodable} to be encoded.
>> 140:      * @return PEM encoding in a string
> 
> Suggest: "a PEM encoded string"

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 141:
> 
>> 139:      * @param de the {@code DEREncodable} to be encoded.
>> 140:      * @return PEM encoding in a string
>> 141:      * @throws IllegalArgumentException when encoding the {@code DEREncodable}
> 
> Suggest: "if the `DEREncodable` cannot be encoded`

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 205:
> 
>> 203: 
>> 204:     /**
>> 205:      * Encodes the specified {@code DEREncodable} into PEM.
> 
> Make description consistent with other encode method: "Encodes the specified {@code DEREncodable} and returns the PEM encoding in a byte array."

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 208:
> 
>> 206:      *
>> 207:      * @param de the {@code DEREncodable} to be encoded.
>> 208:      * @return PEM encoded byte array
> 
> Suggest: "a byte array containing the PEM encoded data"

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 209:
> 
>> 207:      * @param de the {@code DEREncodable} to be encoded.
>> 208:      * @return PEM encoded byte array
>> 209:      * @throws IllegalArgumentException when encoding the {@code DEREncodable}
> 
> "If the `DEREncodable` cannot be encoded"

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 214:
> 
>> 212:      * @see #withEncryption(char[])
>> 213:      */
>> 214:     public byte[] encode(DEREncodable de) {
> 
> Is this method that useful? Caller can just do what line 215 is doing.

I think it's user friendly to when writing to a byte[] or OutputStream.  Base64.Encoder has byte[], ByteBuffer, and String.

> src/java.base/share/classes/java/security/PEMEncoder.java line 224:
> 
>> 222:      * <p> Only {@link PrivateKey} objects can be encrypted with this newly
>> 223:      * configured instance.  Encoding other {@link DEREncodable} objects will
>> 224:      * throw an{@code IllegalArgumentException}.
> 
> space after "an"

ok

> src/java.base/share/classes/java/security/PEMEncoder.java line 228:
> 
>> 226:      * @implNote The default algorithm is defined by Security Property {@code
>> 227:      * jdk.epkcs8.defaultAlgorithm} using default password-based encryption
>> 228:      * parameters by the supporting provider.  To configure all the encryption
> 
> Maybe reword as "If you need more control over the encryption algorithm and parameters, use the ..."

ok

> src/java.base/share/classes/java/security/PEMRecord.java line 116:
> 
>> 114:      *
>> 115:      * @param type the type identifier in the PEM header and footer, or {@code null} if there is no PEM data.
>> 116:      * @param pem The data between the PEM header and footer.
> 
> s/The/the/

ok

> src/java.base/share/classes/java/security/PEMRecord.java line 142:
> 
>> 140:      *
>> 141:      * @throws IllegalArgumentException if {@code pem} cannot be decoded.
>> 142:      * @return Returns a new array of the binary encoding each time this
> 
> Remove "Returns".

ok

> src/java.base/share/classes/sun/security/rsa/RSAPrivateCrtKeyImpl.java line 72:
> 
>> 70:     private BigInteger coeff;   // CRT coefficient
>> 71: 
>> 72:     // RSA or RSS-PSS KeyType
> 
> Typo: s/RSS-PSS/RSA-PSS/

ok

> src/java.base/share/classes/sun/security/x509/X509Key.java line 147:
> 
>> 145:      * X509Key.  Useful for PKCS8v2.
>> 146:      */
>> 147:     public static X509Key parse(byte[] encoded) throws IOException
> 
> Isn't this the same as `X509Key.parse(byte[])`?

I'm assuming you mean `X509Key.decode()`?   It looks like that can be used instead of this method.

> src/java.base/share/classes/sun/security/x509/X509Key.java line 150:
> 
>> 148:     {
>> 149:         DerValue in = new DerValue(encoded);
>> 150:         AlgorithmId algorithm;
> 
> Can move to line 155.

ok

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085776593
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085777165
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085778467
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085803421
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085805970
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085806427
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085821485
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085822116
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085828381
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846397
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846463
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846510
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085850233
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086091768
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086100143
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086265099
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086270777
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086279737
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086340191
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086340372