On Wed, 13 May 2026 18:38:01 GMT, Anthony Scarpino <[email protected]> wrote:
>> Please review the finalized PEM API at https://openjdk.org/jeps/8376991. The >> most significant changes from the second preview, JEP 524 >> (https://openjdk.org/jeps/524), include: >> >> - The `PEM` class is now an ordinary class rather than a record. It adds >> Binary-encoded content constructors and data is defensively copied. >> - The `DEREncodable` interface is renamed to `BinaryEncodable` to more >> accurately reflect the binary data stored in PEM text. >> - In `EncryptedPrivateKeyInfo`, the `encrypt` methods now accept >> `BinaryEncodable`, and the `getKey()` and `getKeyPair()` methods no longer >> include a `Provider` parameter. >> - A new `CryptoException` class indicates failures in cryptographic >> processing at runtime. >> >> thanks >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > zero IS (Hopefully) my last comments on APIs. src/java.base/share/classes/java/security/PEM.java line 40: > 38: * A {@link BinaryEncodable} representing a Privacy-Enhanced Mail (PEM) > structure > 39: * composed of a type identifier, Base64-encoded content, and optional > 40: * leading data that precedes the PEM header during decoding. This "during decoding" seems unnecessary, src/java.base/share/classes/java/security/PEM.java line 121: > 119: /** > 120: * Creates a {@code PEM} instance with the specified type and > Base64-encoded > 121: * byte array content. You should mention ", and leading data" here. src/java.base/share/classes/java/security/PEM.java line 204: > 202: * {@link Base64#getMimeDecoder()}. > 203: * > 204: * @return a Base64-decoded byte array I know we now have base64 content inside so this return value is always a copy. However, I still think we should say "a copy of a Base64-decoded byte array" to not reveal this implementation detail. src/java.base/share/classes/java/security/PEMDecoder.java line 109: > 107: * for decryption, an {@link EncryptedPrivateKeyInfo} is returned. > 108: * A {@code PEMDecoder} configured for decryption can also decode > unencrypted PEM. > 109: * Add a `<p>` here. src/java.base/share/classes/java/security/PEMDecoder.java line 125: > 123: * <p> Example: configure decryption and a factory provider: > 124: * {@snippet lang = java: > 125: * PEMDecoder pd = PEMDecoder.of().withDecryption(password). We usually put the `.` on the new line when a line break is inserted. src/java.base/share/classes/java/security/PEMDecoder.java line 314: > 312: * a PEM footer or the end of the stream. If an I/O error occurs, > 313: * the stream position may become inconsistent. Further decoding > 314: * operations on the same {@code InputStream} are not recommended. The last sentence above is slightly different from the sentence used in `decode(stream, class)`. Suggest using identical wording. src/java.base/share/classes/java/security/PEMDecoder.java line 327: > 325: * @return a {@code BinaryEncodable} > 326: * @throws IOException if an I/O error occurs or PEM syntax is > invalid > 327: * before decoding completes Just curious, is this before decoding "completes" or "starts"? :-) Also make it consistent with `decode(stream, class)`. src/java.base/share/classes/java/security/PEMDecoder.java line 330: > 328: * @throws EOFException if no PEM data is found or the stream ends > unexpectedly > 329: * @throws IllegalArgumentException if decoding fails > 330: * @throws NullPointerException when {@code is} is {@code null} The 2 consecutive `is` sound awkward. In fact, I've looked at other APIs that take `InputStream` as an argument and most of them name it `in`. src/java.base/share/classes/java/security/PEMDecoder.java line 352: > 350: * containing the type identifier, Base64-encoded data, and any > leading data > 351: * preceding the PEM header. For {@code BinaryEncodable} types other > than > 352: * {@code PEM}, leading data is ignored. The paragraph above is slightly different from `decode(string,class)`. Suggest using identical wording. src/java.base/share/classes/java/security/PEMDecoder.java line 382: > 380: > 381: /** > 382: * Decodes and returns a {@code BinaryEncodable} of the specified > class for Typo, `for` -> `from`. src/java.base/share/classes/java/security/PEMDecoder.java line 384: > 382: * Decodes and returns a {@code BinaryEncodable} of the specified > class for > 383: * the given {@code InputStream}. > 384: * Add a `<p>` here. src/java.base/share/classes/java/security/PEMEncoder.java line 48: > 46: * objects, such as asymmetric keys, certificates, and certificate > revocation > 47: * lists (CRLs). It is defined in RFC 1421 and RFC 7468. PEM consists of > a > 48: * Base64-encoded binary encoding enclosed by a type-identifying header Instead of "binary encoding" I prefer "content" to be consistent with the `PEM` spec. Same comment for `PEMDecoder`. src/java.base/share/classes/java/security/PEMEncoder.java line 89: > 87: * <li>{@link PrivateKey}: ENCRYPTED PRIVATE KEY</li> > 88: * <li>{@link KeyPair}: ENCRYPTED PRIVATE KEY</li> > 89: * <li>{@link PKCS8EncodedKeySpec}: ENCRYPTED PRIVATE KEY</li> Show we list them with the same type, or we should say all three classes encode to one single type? ------------- PR Review: https://git.openjdk.org/jdk/pull/29640#pullrequestreview-4285331764 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237427846 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237442654 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237463241 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237522028 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237531983 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237588352 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237559486 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237574719 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237597919 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237537180 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237578594 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237472072 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3237483136
