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/PEMDecoder.java line 1: > 1: /* Line 506: s/still/also/ 508: s/decrypt/decrypt the/ 510: s/PEMEncoder/PEMDecoder 511: put code around null 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 src/java.base/share/classes/java/security/PEMDecoder.java line 86: > 84: * <p> If the PEM type does not have a corresponding class, > 85: * {@code decode(String)} and {@code decode(InputStream)} will return a > 86: * {@link PEM}. s/PEM/PEM object/ src/java.base/share/classes/java/security/PEMDecoder.java line 103: > 101: * > 102: * <p> A new {@code PEMDecoder} instance is created when configured > 103: * with {@link #withFactory(Provider)} and/or I think you can just say "or" instead of "and/or". src/java.base/share/classes/java/security/PEMDecoder.java line 106: > 104: * {@link #withDecryption(char[])}. The {@link #withFactory(Provider)} > method > 105: * uses the specified provider to produce cryptographic objects from > 106: * {@link KeyFactory} and{@link CertificateFactory}. space after "and". src/java.base/share/classes/java/security/PEMDecoder.java line 107: > 105: * uses the specified provider to produce cryptographic objects from > 106: * {@link KeyFactory} and{@link CertificateFactory}. > 107: * {@link #withDecryption(char[])} configures the decoder to process Say "The withDecryption(char[]) method" so this is more visible as a new sentence. Also: s/process/decode and decrypt/ src/java.base/share/classes/java/security/PEMDecoder.java line 109: > 107: * {@link #withDecryption(char[])} configures the decoder to process > 108: * encrypted private key PEM data using the given password. > 109: * If decryption fails, a {@link IllegalArgumentException} is thrown. s/a/an/ 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. src/java.base/share/classes/java/security/PEMDecoder.java line 339: > 337: * a {@link PEM} is returned containing the > 338: * type identifier and Base64 encoding. Any non-PEM data preceding > 339: * the PEM header will be stored in {@code leadingData}. Other Most of my comments below on decode that takes an input stream also apply to this method. 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/ src/java.base/share/classes/java/security/PEMDecoder.java line 378: > 376: * > 377: * <p> If the class parameter is {@code PEM.class}, > 378: * a {@link PEM} is returned containing the s/PEM/PEM object/ src/java.base/share/classes/java/security/PEMDecoder.java line 379: > 377: * <p> If the class parameter is {@code PEM.class}, > 378: * a {@link PEM} is returned containing the > 379: * type identifier and Base64 encoding. Any non-PEM data preceding s/Base64 encoding/Base64-encoded data/ src/java.base/share/classes/java/security/PEMDecoder.java line 381: > 379: * type identifier and Base64 encoding. Any non-PEM data preceding > 380: * the PEM header will be stored in {@code leadingData}. Other > 381: * class parameters will not return preceding non-PEM data. Suggest rewording as: "For `DEREncodable` types other than `PEM`, leading data is ignored and not returned as part of the `DEREncodable` object." 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 386: "extends or implements" 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? 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" 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? src/java.base/share/classes/java/security/PEMDecoder.java line 395: > 393: * @throws EOFException at the end of the {@code InputStream} > 394: * @throws IllegalArgumentException on error with arguments or in > decoding > 395: * @throws ClassCastException if {@code tClass} is invalid for the > PEM type suggest: s/is invalid for/does not represent/ src/java.base/share/classes/java/security/PEMDecoder.java line 396: > 394: * @throws IllegalArgumentException on error with arguments or in > decoding > 395: * @throws ClassCastException if {@code tClass} is invalid for the > PEM type > 396: * @throws NullPointerException when any input values are null put code font around null 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." ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2402961418 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2402979334 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403075672 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403081459 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403083470 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403085853 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403092375 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403098282 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403066797 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403007926 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403008593 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403015740 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403025383 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403032724 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403063120 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403047428 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403043967 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403040845 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403038460 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2402952350
