RFR: 8360564: Implement JEP 524: PEM Encodings of Cryptographic Objects (Second Preview) [v4]
Anthony Scarpino
ascarpino at openjdk.org
Fri Oct 10 16:35:57 UTC 2025
On Fri, 3 Oct 2025 12:56:34 GMT, Sean Mullan <mullan at openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>>
>> missed some decoder comments
>
> src/java.base/share/classes/java/security/PEM.java line 61:
>
>> 59: * Common {@code type} values include, but are not limited to:
>> 60: * CERTIFICATE, CERTIFICATE REQUEST, ATTRIBUTE CERTIFICATE, X509 CRL, PKCS7,
>> 61: * CMS, PRIVATE KEY, ENCRYPTED PRIVATE KEY, RSA PRIVATE KEY, or PUBLIC KEY.
>
> s/RSA PRIVATE KEY/PRIVATE KEY/
"PRIVATE KEY" is earlier in the list and "RSA PRIVATE KEY" is a valid type.
> src/java.base/share/classes/java/security/PEM.java line 88:
>
>> 86:
>> 87: /**
>> 88: * Creates a {@code PEM} instance with the given parameters.
>
> s/the given/the specified/
>
> In the other ctor, you say what the parameters are, so you should do the same here.
I'll just remove the other constructor description as it's not necessary to repeat what `@params` already documents.
> src/java.base/share/classes/java/security/PEM.java line 131:
>
>> 129: /**
>> 130: * Returns the type and Base64 encoding in PEM textual format.
>> 131: * {@code leadingData} is not returned by this method.
>
> Suggest rewording as "leadingData is not included in the PEM data returned by this method."
I prefer the conciseness of the sentence as it is only clarifying the previous sentence.
> src/java.base/share/classes/java/security/PEM.java line 139:
>
>> 137:
>> 138: /**
>> 139: * Returns a Base64 decoded byte array of {@code content}.
>
> Suggest stating that each invocation of this method returns a new byte array, as it isn't clear.
I changed it to "a new decoded ..." as that should suggest it's a new byte array. Also this is a `record` so any value not defined in the class description has to be new.
> src/java.base/share/classes/java/security/PEM.java line 144:
>
>> 142: * @throws IllegalArgumentException on a decoding error
>> 143: *
>> 144: * @see Base64#getMimeDecoder()
>
> Suggest you also add a sentence that states this method uses the using the MIME type base64 decoding scheme, otherwise the `@see` doesn't have much context.
reworded
> src/java.base/share/classes/java/security/PEMDecoder.java line 1:
>
>> 1: /*
>
> Line 492: "Any errors using the {@code Provider} will occur during decoding."
>
> What does that mean? Does it mean they are thrown by this method? If so, it should be declared as an exception.
>
> Also: 495: s/to/with/
> 496: put code around null
They occur during decoding.
> src/java.base/share/classes/java/security/PEMDecoder.java line 135:
>
>> 133: * {@code X.509 CERTIFICATE}, {@code CRL}, and {@code RSA PRIVATE KEY}.
>> 134: *
>> 135: * @see PEMEncoder
>
> line 126, I think the "." should be on this line.
I don't understand this comment, but the paragraph was changed so it may not be applicable.
> src/java.base/share/classes/java/security/PEMDecoder.java line 374:
>
>> 372: * the PEM footer or the end of the stream. If an I/O error occurs,
>> 373: * the read position in the stream may become inconsistent.
>> 374: * It is recommended to perform no further decoding operations
>
> First sentence (line 367), change to: "Decodes and returns a DEREncodable of the specified class for the specified InputStream."
>
> 368: s/extend/extend or implement/
The second comment conflicts with a previous comment by Weijun. That part of the sentence was removed.
> src/java.base/share/classes/java/security/PEMDecoder.java line 391:
>
>> 389: * {@code DEREncodable}.
>> 390: * @return a {@code DEREncodable} typecast to {@code tClass}
>> 391: * @throws IOException on IO or PEM syntax error where the
>
> Why would bad PEM syntax be an IOE? Should this be an IAE, similar to a decoding error?
This was a @wangweij comment from the first preview that syntax error are not recoverable and should be IOE.
> src/java.base/share/classes/java/security/PEMDecoder.java line 393:
>
>> 391: * @throws IOException on IO or PEM syntax error where the
>> 392: * {@code InputStream} did not complete decoding.
>> 393: * @throws EOFException at the end of the {@code InputStream}
>
> How about: "if the end of the input stream has been reached unexpectedly"
I needed to rework the description anyway
> src/java.base/share/classes/java/security/PEMDecoder.java line 394:
>
>> 392: * {@code InputStream} did not complete decoding.
>> 393: * @throws EOFException at the end of the {@code InputStream}
>> 394: * @throws IllegalArgumentException on error with arguments or in decoding
>
> Can you be more specific here? What does "on error with arguments" mean? What type of decoding issues result in IAE?
I'm removing that change, it's unclear to me what I was trying to clarify.
> src/java.base/share/classes/java/security/PEMEncoder.java line 1:
>
>> 1: /*
>
> For withEncryption, suggest changing the first sentence to better match `PEMDecoder.withEncryption` which reads better to me. So suggest:
>
> "Returns a copy of this PEMEncoder that encrypts and encodes private keys using the specified password and default encryption algorithm."
Since PEMEncoder can encrypt 3 classes, it cannot follow your suggestion exactly. I think saying "... encrypts and encodes using the specified password ..." sets up the second paragraph that states what can be encrypted.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 398:
>
>> 396: *
>> 397: * @param de the {@code DEREncodable} to be encrypted. Usage
>> 398: * limited to {@link PrivateKey}, {@link KeyPair}, and
>
> "Usage limited" sounds odd. Maybe "The supported `DEREncodable types are ... An IllegalArgumentException is thrown for any other type."
I updated the supported types, but left the IAE to the @throws section.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 402:
>
>> 400: * @param password the password used in the PBE encryption. This array
>> 401: * will be cloned before being used.
>> 402: * @return an {@code EncryptedPrivateKeyInfo}
>
> Next line:
>
> "@throws IllegalArgumentException on initialization errors based on the
> arguments passed to the method"
>
> What does this mean - you need to be more specific about what are the error cases. Otherwise it will not be helpful for TCK test writers on how to test this.
I put more detail in IAE
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 565:
>
>> 563: */
>> 564: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API)
>> 565: public KeyPair getKeyPair(char[] password) throws GeneralSecurityException {
>
> This should specifically throw an `InvalidKeyException` if decryption fails. I think you want to specify the exact subclasses when it is clearly the right behavior. In this case, the method should always throw `InvalidKeyException` on decryption errors.
>
> Also why generalize it to throw `GeneralSecurityException`? This should throw the same exceptions as `getKeySpec(Key, Provider)` since the params are the same.
Sure, not specifying InvalidKeyException for non-KeyPair return cases in the javadoc is an oversight. Underneath the code, the `InvalidKeyException` and `NoSuchAlgorithmException` were always thrown, it's the method signature that is different. Documenting `getKeyPair()` that NSAE and IKE are thrown. Essentially, I'd replace `@throws GeneralSecurityException ...` with `@throws InvalidKeyException ...`.
I think it is user-friendly to have GSE on the method signature. Specifying `NoSuchAlgorithmException` in the method signature I don't think offers value and is purely informative. Since the algorithm is determined by the encoding and cannot be influenced by either getKeyPair method.
`getKeySpec()` has generalized its exception handling by wrapping `IllegalBlockSizeException` and `BadPaddingException` into `InvalidKeyException`. The `getKeySpecImpl()` method that all the`getKeySpec()` methods call, wrap `GeneralSecurityException` and `IOException` into `InvalidKeyException`.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 585:
>
>> 583: * @param decryptKey the decryption key and cannot be {@code null}
>> 584: * @param provider the {@code Provider} used for Cipher decryption and
>> 585: * key generation. A {@code null} value will
>
> Why do you allow null? This is inconsistent with the other methods that take Provider arguments. I think consistency is important.
This behavior is consistent with getKey() and encryptKey(). It allows the user to use a Key without specifying a provider. Otherwise, I would need to add a third method, getKeyPair(Key).
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2402621237
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2408968998
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2408981945
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2408995855
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2411543503
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2411550342
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2411981646
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412072575
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412222361
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412237603
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412400535
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2412457945
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2417744866
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2417890681
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2415573556
PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2418306533
More information about the security-dev
mailing list