RFR: 8298420: PEM API: Implementation (Preview) [v18]
Anthony Scarpino
ascarpino at openjdk.org
Wed May 14 08:25:45 UTC 2025
On Tue, 13 May 2025 17:15:39 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/DEREncodable.java line 37:
>
>> 35:
>> 36: /**
>> 37: * This is a top-level interface for security classes that contain cryptographic
>
> Wording suggestion (taken from JEP): "This interface is implemented by security API classes that contain binary-encodable key or certificate material."
ok
> src/java.base/share/classes/java/security/DEREncodable.java line 40:
>
>> 38: * data which may not be related or have a common class hierarchy. These
>> 39: * security objects provide standard binary encoding, like ASN.1, and possible
>> 40: * type formats, like X.509 and PKCS#8.
>
> Wording suggestion (taken from JEP): "These APIs or their subclasses typically provide methods to convert their instances to and from byte arrays in the [Distinguished Encoding Rules (DER)](https://en.wikipedia.org/wiki/X.690#DER_encoding) format."
I changed the text, I didn't add a link as the one you included was wikipedia, which I don't know if that's stable enough for javadoc. The other link I found was to a PDF, which I don't think is allowed either. What I can do is put the spec in there "ITU X.690"
> src/java.base/share/classes/java/security/PEMDecoder.java line 54:
>
>> 52: * methods return an instance of a class that matches the data
>> 53: * type and implements {@link DEREncodable}. The
>> 54: * following types are decoded into Java Cryptographic Extensions (JCE) object
>
> Lets make the wording consistent with PEMEncoder: "... into cryptographic objects that implement {@link DEREncodable}"
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 133:
>
>> 131: * Returns an instance of {@code PEMDecoder}.
>> 132: *
>> 133: * @return new {@code PEMDecoder} instance
>
> s/new/a/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 224:
>
>> 222: *
>> 223: * <p>This method reads the {@code String} until the first PEM data is found
>> 224: * or the end the {@code String} is reached. Non-PEM data before the PEM
>
> s/end/end of/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 458:
>
>> 456:
>> 457: /**
>> 458: * Configures and returns a new {@code PEMDecoder} instance from the
>
> Change the wording to be consistent with `withDecryption`: "Returns a copy of this `PEMDecoder` that will use ..."
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 459:
>
>> 457: /**
>> 458: * Configures and returns a new {@code PEMDecoder} instance from the
>> 459: * current instance that will use {@link KeyFactory} and
>
> s/will use/uses/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 460:
>
>> 458: * Configures and returns a new {@code PEMDecoder} instance from the
>> 459: * current instance that will use {@link KeyFactory} and
>> 460: * {@link CertificateFactory} classes from the specified {@link Provider}.
>
> Suggest saying what they are used for:
>
> s/from the specified {@link Provider}/from the specified {@link Provider} to produce cryptographic objects./
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 474:
>
>> 472:
>> 473: /**
>> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM data
>
> Put code around PEMDecoder.
>
> s/will decrypt/decrypts/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 475:
>
>> 473: /**
>> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM data
>> 475: * such as encrypted private keys with the specified password.
>
> "such as" sounds like it may support others but there is only private keys. Suggest being more specific: "Returns a copy of this `PEMDecoder` that decodes and decrypts encrypted private keys using the specified password."
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 476:
>
>> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM data
>> 475: * such as encrypted private keys with the specified password.
>> 476: * Non-encrypted PEM may still be decoded from this instance.
>
> "may" sounds like it is optional to support, suggest using "can". Maybe say "PEM types that are not encrypted can also be decoded from this instance."
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 66:
>
>> 64: * <p> PKCS #8 2.0 defines the ASN.1 OneAsymmetricKey structure, which may
>> 65: * contain both private and public keys.
>> 66: * {@link KeyPair} objects passed to the {@code encode} methods are encoded as a
>
> s/{@code encode}/{@code encode} or {@code encodeToString}/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 70:
>
>> 68: *
>> 69: * <p> When encoding a {@link PEMRecord}, the API surrounds the
>> 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer
>
> delete "a generated".
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 72:
>
>> 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer
>> 71: * from {@linkplain PEMRecord#type()}. {@linkplain PEMRecord#leadingData()} is
>> 72: * not included in the encoding. {@code PEMRecord} will not preform
>
> s/preform/perform/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 84:
>
>> 82: *
>> 83: * <p> Here is an example that encrypts and encodes a private key using the
>> 84: * specified password.
>
> s/./:/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 141:
>
>> 139: * string.
>> 140: *
>> 141: * @param de the {@code DEREncodable} to be encoded.
>
> Nit: no need for period.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 142:
>
>> 140: *
>> 141: * @param de the {@code DEREncodable} to be encoded.
>> 142: * @return a byte array containing the PEM encoded data
>
> s/a byte array/a String/
oops
> src/java.base/share/classes/java/security/PEMEncoder.java line 143:
>
>> 141: * @param de the {@code DEREncodable} to be encoded.
>> 142: * @return a byte array containing the PEM encoded data
>> 143: * @throws IllegalArgumentException If the DEREncodable cannot be encoded
>
> s/If/if/
>
> Put code around DEREncodable.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 144:
>
>> 142: * @return a byte array containing the PEM encoded data
>> 143: * @throws IllegalArgumentException If the DEREncodable cannot be encoded
>> 144: * @throws NullPointerException if {@code de} is {@code null}.
>
> Nit: no need for period.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 209:
>
>> 207: * in a byte array.
>> 208: *
>> 209: * @param de the {@code DEREncodable} to be encoded.
>
> Nit: no need for period.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 211:
>
>> 209: * @param de the {@code DEREncodable} to be encoded.
>> 210: * @return PEM encoded byte array
>> 211: * @throws IllegalArgumentException if the DEREncodable cannot be encoded
>
> Put code around DEREncodable.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 212:
>
>> 210: * @return PEM encoded byte array
>> 211: * @throws IllegalArgumentException if the DEREncodable cannot be encoded
>> 212: * @throws NullPointerException if {@code de} is {@code null}.
>
> Nit: no need for period.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 216:
>
>> 214: */
>> 215: public byte[] encode(DEREncodable de) {
>> 216: return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1);
>
> Side comment, can look at this later. Since `Base64.Encoder.encodeToString()` first encodes to bytes and then converts it to a String, it might be more efficient to do the same here.
noted
> src/java.base/share/classes/java/security/PEMEncoder.java line 220:
>
>> 218:
>> 219: /**
>> 220: * Returns a new {@code PEMEncoder} instance configured with the default
>
> s/configured/configured for encryption/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 228:
>
>> 226: *
>> 227: * @implNote The default algorithm is defined by Security Property {@code
>> 228: * jdk.epkcs8.defaultAlgorithm} using default password-based encryption
>
> Wording suggestion: "The default password-based encryption algorithm is defined by the `jdk.epkcs8.defaultAlgorithm` security property and uses the default encryption parameters of the provider that is selected."
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 235:
>
>> 233: * returned object with {@link #encode(DEREncodable)}.
>> 234: *
>> 235: * @param password sets the encryption password. The array is cloned and
>
> No need to say "sets".
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 236:
>
>> 234: *
>> 235: * @param password sets the encryption password. The array is cloned and
>> 236: * stored in the new instance. {@code null} is a valid value.
>
> Why allow `null` here but not in `PEMDecoder.withDecryption`. I think we should probably disallow null. Even though PBEKeySpec allows it, I think it is for a corner case that might not be relevant here.
I was supporting what PBEKeySpec allowed. Maybe with EPKI methods available to allow null, maybe this method could be more restrictive.
> src/java.base/share/classes/java/security/PEMRecord.java line 76:
>
>> 74: * {@code null} if there is no PEM data.
>> 75: * @param pem the data between the PEM header and footer.
>> 76: * @param leadingData Any non-PEM data read during the decoding process
>
> s/Any/any/
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 77:
>
>> 75: * @param pem the data between the PEM header and footer.
>> 76: * @param leadingData Any non-PEM data read during the decoding process
>> 77: * before the PEM header. This value maybe {@code null}.
>
> s/maybe/may be/
ok
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 228:
>
>> 226: ECOperations ops = ECOperations.forParameters(ecParams)
>> 227: .orElseThrow(ProviderException::new);
>> 228: MutablePoint pub = ops.multiply(ecParams.getGenerator(), arrayS);
>
> Can `arrayS` ever be `null` here?
Looks like it's possible
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087623623
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087624162
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087628211
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087641064
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087641806
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087726601
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087783466
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087783562
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087792527
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087794079
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087796063
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087800696
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087806171
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087807018
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087807683
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087815513
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087815610
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087817444
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087817932
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818027
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818069
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818128
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087821133
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087822205
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087826110
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087826224
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087827313
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087832770
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087833098
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088103430
More information about the security-dev
mailing list