On Thu, 25 Sep 2025 23:03:11 GMT, Anthony Scarpino <[email protected]> wrote:
>> Hi >> >> Please review the [Second Preview](https://openjdk.org/jeps/8360563) for the >> PEM API. The most significant changes from [JEP >> 470](https://openjdk.org/jeps/470) are: >> >> - Renamed the name of `PEMRecord` class to `PEM`. >> - Revised the new `encryptKey` methods of the `EncryptedPrivateKeyInfo` >> class to accept `DEREncodable` objects rather than just `PrivateKey` objects >> so that cryptographic objects with public keys, i.e., `KeyPair` and >> `PKCS8EncodedKeySpec`, can also be encrypted. >> - Enhanced the `PEMEncoder` and `PEMDecoder` classes to support the >> encryption and decryption of `KeyPair` and `PKCS8EncodedKeySpec` objects. >> >> thanks >> >> Tony > > 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 44: > 42: * when the data type cannot be represented by a cryptographic object. > 43: * If {@code PEM} is desired instead of a cryptographic object, the > 44: * decoding methods {@link PEMDecoder#decode(String, Class)} or Suggest rewording this, and reusing some text from the JEP as: "If you need access to the leading data of a PEM text, or if you want to handle the text’s content yourself, use the decoding methods ... with `PEM.class` as an argument." src/java.base/share/classes/java/security/PEM.java line 49: > 47: * > 48: * <p> A {@code PEM} object can be encoded back to its textual format by > using > 49: * encode methods in {@link PEMEncoder} or the {@link #toString()} method. I'd probably say "calling" instead of "using" and switch the order, and a few other tweaks. Suggest: "A {@code PEM} object can be encoded back to its textual format by calling the {@link #toString()} method or the encode methods of {@link PEMEncoder}." src/java.base/share/classes/java/security/PEM.java line 55: > 53: * > 54: * <p>No validation is performed during instantiation to ensure that > 55: * {@code type} conforms to {@code RFC 7468} or other legacy formats, that I don't think RFC 7468 should be in code font. 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/ src/java.base/share/classes/java/security/PEM.java line 63: > 61: * CMS, PRIVATE KEY, ENCRYPTED PRIVATE KEY, RSA PRIVATE KEY, or PUBLIC > KEY. > 62: * > 63: * <p> {@code leadingData} may be null if there was no data preceding the > PEM s/may be null/will be null/ s/was no/is no/ src/java.base/share/classes/java/security/PEM.java line 65: > 63: * <p> {@code leadingData} may be null if there was no data preceding the > PEM > 64: * header during decoding. {@code leadingData} can be useful for reading > 65: * metadata that accompanies PEM data. {@code leadingData} is not > defensively s/PEM/the PEM/ src/java.base/share/classes/java/security/PEM.java line 66: > 64: * header during decoding. {@code leadingData} can be useful for reading > 65: * metadata that accompanies PEM data. {@code leadingData} is not > defensively > 66: * copied and the {@link #leadingData()} method does not return a clone. s/copied/copied by the constructor/ 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. src/java.base/share/classes/java/security/PEM.java line 114: > 112: > 113: /** > 114: * Creates a {@code PEM} instance with a given {@code type} and s/a given/the specified/ src/java.base/share/classes/java/security/PEM.java line 115: > 113: /** > 114: * Creates a {@code PEM} instance with a given {@code type} and > 115: * {@code content} data in String form. {@code leadingData} is set > to null. "in String form" is redundant since those are the types of the parameters. Suggest removing those words. src/java.base/share/classes/java/security/PEM.java line 115: > 113: /** > 114: * Creates a {@code PEM} instance with a given {@code type} and > 115: * {@code content} data in String form. {@code leadingData} is set > to null. Put null in code font. src/java.base/share/classes/java/security/PEM.java line 130: > 128: > 129: /** > 130: * Returns the type and Base64 encoding in PEM textual format. s/Base64 encoding/Base64-encoded data/ src/java.base/share/classes/java/security/PEM.java line 130: > 128: > 129: /** > 130: * Returns the type and Base64 encoding in PEM textual format. Do we need to say "textual"? Suggest removing that word. 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." src/java.base/share/classes/java/security/PEM.java line 139: > 137: > 138: /** > 139: * Returns a Base64 decoded byte array of {@code content}. s/Base64 decoded/Base64-decoded/ 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. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401784001 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401900160 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401801859 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401818161 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401824375 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401827152 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401882889 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401865919 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401861840 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401858716 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401860156 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401849924 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401851466 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401847055 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401831443 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401840888 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2401839164
