On Fri, 2 May 2025 06:09:52 GMT, Anthony Scarpino <ascarp...@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
