On Wed, 25 Sep 2024 18:50:55 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 incrementally with one > additional commit since the last revision: > > fix decoding non-encrypted types src/java.base/share/classes/java/security/PEMDecoder.java line 52: > 50: * Decoding methods return a class that matches the data type and > implements > 51: * {@link DEREncodable}. > 52: * If a return class is specified, an IllegalAlgorithmException is thrown > if Should `IllegalAlgorithmException` be surrounded by `{@code}`? src/java.base/share/classes/java/security/PEMDecoder.java line 56: > 54: * <p> > 55: * When passing input data into {@code decode}, the application is > responsible > 56: * for processing input data non-PEM text. All data before the PEM Maybe I'm not reading it correctly, but there may be a type-o/phrasing issue with this sentence: "When passing input data into {@code decode}, the application is responsible for processing input data non-PEM text." src/java.base/share/classes/java/security/PEMDecoder.java line 91: > 89: > 90: /** > 91: * Creates a immutable instance with a specific KeyFactory and/or > password. I suggest: "Creates an immutable instance with a specific {@code KeyFactory} provider and/or password." src/java.base/share/classes/java/security/PEMDecoder.java line 92: > 90: /** > 91: * Creates a immutable instance with a specific KeyFactory and/or > password. > 92: * @param withFactory KeyFactory provider Should `KeyFactory` be surrounded by `{@code}`? src/java.base/share/classes/java/security/PEMDecoder.java line 93: > 91: * Creates a immutable instance with a specific KeyFactory and/or > password. > 92: * @param withFactory KeyFactory provider > 93: * @param withPassword char[] password for EncryptedPrivateKeyInfo Should `EncryptedPrivateKeyInfo` be surrounded by `{@code}`? src/java.base/share/classes/java/security/PEMDecoder.java line 102: > 100: > 101: /** > 102: * Returns an instance of PEMDecoder. This instance may be > repeatedly used Should `PEMDecoder` be surrounded by `{@code}`? Maybe I'll just suggest sweeping the javadocs for similar instances of this issue. src/java.base/share/classes/java/security/PEMDecoder.java line 197: > 195: * Decodes and returns a {@link DEREncodable} from the given > 196: * {@code InputStream}. > 197: * The method will read the {@code InputStream} until PEM data is nit: not required, but did you mean to add a `<p>` tag here? There's a newline, so I wondered. src/java.base/share/classes/java/security/PEMDecoder.java line 356: > 354: > 355: /** > 356: * Configures and return a new PEMDecoder instance from the current > instance nit: "return" -> "returns" src/java.base/share/classes/java/security/PEMDecoder.java line 371: > 369: * Non-encrypted PEM may still be decoded from this instance. > 370: * > 371: * @param password the password to decrypt encrypted PEM data. I suggest something to the effect of "the char array is cloned to prevent subsequent modification" src/java.base/share/classes/java/security/PEMEncoder.java line 200: > 198: > 199: /** > 200: * Encoded a given {@code DEREncodable} into PEM. nit: "Encoded" -> "Encode(s)" src/java.base/share/classes/java/security/PEMEncoder.java line 234: > 232: */ > 233: public PEMEncoder withEncryption(char[] password) { > 234: // PBEKeySpec clones the password May consider moving this comment to the `@param`. src/java.base/share/classes/java/security/cert/X509CRL.java line 1: > 1: /* Update copyright year. src/java.base/share/classes/java/security/cert/X509Certificate.java line 1: > 1: /* Update copyright year, if needed. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 397: > 395: * > 396: * @param key The PrivateKey object to encrypt. > 397: * @param password the password used in the PBE encryption. Consider indicating that a clone happens. src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 85: > 83: MODULE_IMPORTS, > 84: //XXX Number will change when assigned > 85: @JEP(number=999, title="PEM API", status="Preview") No JEP # assigned yet? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794023285 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794027533 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794036646 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794038536 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794040067 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794046026 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794058810 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794065015 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794067651 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794072975 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794077342 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794088602 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794089436 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794096332 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1794100097
