RFR: 8298420: PEM API: Implementation (Preview) [v15]
Sean Mullan
mullan at openjdk.org
Mon May 5 15:14:07 UTC 2025
On Fri, 2 May 2025 06:09:52 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 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 57:
> 55: * that implement {@link DEREncodable} and support
> 56: * {@linkplain PKCS8EncodedKeySpec PKCS#8} or
> 57: * {@linkplain X509EncodedKeySpec X509} formats.
The "and" in "and support {@linkplain PKCS8EncodedKeySpec PKCS#8} or {@linkplain X509EncodedKeySpec X509} formats" is too strong, ex - X509Certificate & X509CRL do not support those keyspecs. I would suggest deleting this part about "and support" as it is doesn't seem necessary.
src/java.base/share/classes/java/security/PEMEncoder.java line 64:
> 62: *
> 63: * <p> PKCS8 2.0 allows OneAsymmetricKey encoding, which may contain both private
> 64: * and public keys in the same PEM.This is supported by using the
Add space before "This".
src/java.base/share/classes/java/security/PEMEncoder.java line 118:
> 116:
> 117: /**
> 118: * Returns a new instance of PEMEncoder.
put code around PEMEncoder.
src/java.base/share/classes/java/security/PEMEncoder.java line 118:
> 116:
> 117: /**
> 118: * Returns a new instance of PEMEncoder.
This could be interpreted as a new instance is returned every time. Suggest removing the word "new".
src/java.base/share/classes/java/security/PEMEncoder.java line 120:
> 118: * Returns a new instance of PEMEncoder.
> 119: *
> 120: * @return PEMEncoder instance
s/PEMEncoder/a PEMEncoder/
Use code font for PEMEncoder, also in method description.
src/java.base/share/classes/java/security/PEMEncoder.java line 122:
> 120: * @return PEMEncoder instance
> 121: */
> 122: static public PEMEncoder of() {
"public" should be before "static".
src/java.base/share/classes/java/security/PEMEncoder.java line 133:
> 131: * {@code DEREncodable}.
> 132: * @return PEM encoding in a String
> 133: * @throws IllegalArgumentException when the passed object returns a null
When would a DEREncodable return a null encoding? I think it should never return this under normal circumstances. I think we should not specifically mention this case, and be more general like "if the object cannot be encoded for some reason".
src/java.base/share/classes/java/security/PEMEncoder.java line 199:
> 197:
> 198: /**
> 199: * Encodes a given {@code DEREncodable} into PEM.
Suggest:
s/a given/the specified/
src/java.base/share/classes/java/security/PEMEncoder.java line 207:
> 205: * configured for encryption while encoding a {@code DEREncodable} that does
> 206: * not support encryption.
> 207: * @throws NullPointerException when object passed is null.
Wording suggestion: "if {@code de} is {@code null}"
src/java.base/share/classes/java/security/PEMEncoder.java line 215:
> 213:
> 214: /**
> 215: * Returns a new immutable PEMEncoder instance configured to the default
I don't think you need to say "immutable" as the first sentence of the class description already says PEMEncoder objects are immutable.
s/configured to/configured with/
src/java.base/share/classes/java/security/PEMEncoder.java line 216:
> 214: /**
> 215: * Returns a new immutable PEMEncoder instance configured to the default
> 216: * encryption algorithm and a given password.
s/a given/the specified/
src/java.base/share/classes/java/security/PEMEncoder.java line 218:
> 216: * encryption algorithm and a given password.
> 217: *
> 218: * <p> Only {@link PrivateKey} will be encrypted with this newly configured
s/will/objects will/
Also suggest saying "can be" instead of "will be".
src/java.base/share/classes/java/security/PEMEncoder.java line 220:
> 218: * <p> Only {@link PrivateKey} will be encrypted with this newly configured
> 219: * instance. Other {@link DEREncodable} classes that do not support
> 220: * encrypted PEM will cause encode() to throw an IllegalArgumentException.
This sentence sounds like it is possible for classes other than PrivateKey to support encrypted PEM. Is that true? If not, I would be more specific and just say objects other than PrivateKey.
Put code font around IllegalArgumentException.
src/java.base/share/classes/java/security/PEMEncoder.java line 222:
> 220: * encrypted PEM will cause encode() to throw an IllegalArgumentException.
> 221: *
> 222: * @implNote Default algorithm defined by Security Property {@code
First sentence is incomplete.
src/java.base/share/classes/java/security/PEMEncoder.java line 229:
> 227: *
> 228: * @param password sets the encryption password. The array is cloned and
> 229: * stored in the new instance. {@code null} is a valid entry.
s/entry/value/
src/java.base/share/classes/java/security/PEMEncoder.java line 230:
> 228: * @param password sets the encryption password. The array is cloned and
> 229: * stored in the new instance. {@code null} is a valid entry.
> 230: * @return a new PEMEncoder
Put code around PEMEncoder.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073567927
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073570896
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073574066
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073639928
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073579896
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073574516
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073625588
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073620495
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073620596
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073583887
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073585281
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073587422
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073597828
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073598651
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073604425
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2073605917
More information about the security-dev
mailing list