On Tue, 13 May 2025 09:27:37 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: > > comments on the 11th src/java.base/share/classes/java/security/PEMDecoder.java line 54: > 52: * methods return an instance of a class that matches the data > 53: * type and implements {@link DEREncodable}. The > 54: * following types are decoded into Java Cryptographic Extensions (JCE) > object Lets make the wording consistent with PEMEncoder: "... into cryptographic objects that implement {@link DEREncodable}" src/java.base/share/classes/java/security/PEMDecoder.java line 133: > 131: * Returns an instance of {@code PEMDecoder}. > 132: * > 133: * @return new {@code PEMDecoder} instance s/new/a/ src/java.base/share/classes/java/security/PEMDecoder.java line 224: > 222: * > 223: * <p>This method reads the {@code String} until the first PEM data > is found > 224: * or the end the {@code String} is reached. Non-PEM data before > the PEM s/end/end of/ src/java.base/share/classes/java/security/PEMDecoder.java line 458: > 456: > 457: /** > 458: * Configures and returns a new {@code PEMDecoder} instance from the Change the wording to be consistent with `withDecryption`: "Returns a copy of this `PEMDecoder` that will use ..." src/java.base/share/classes/java/security/PEMDecoder.java line 459: > 457: /** > 458: * Configures and returns a new {@code PEMDecoder} instance from the > 459: * current instance that will use {@link KeyFactory} and s/will use/uses/ src/java.base/share/classes/java/security/PEMDecoder.java line 460: > 458: * Configures and returns a new {@code PEMDecoder} instance from the > 459: * current instance that will use {@link KeyFactory} and > 460: * {@link CertificateFactory} classes from the specified {@link > Provider}. Suggest saying what they are used for: s/from the specified {@link Provider}/from the specified {@link Provider} to produce cryptographic objects./ src/java.base/share/classes/java/security/PEMDecoder.java line 460: > 458: * Configures and returns a new {@code PEMDecoder} instance from the > 459: * current instance that will use {@link KeyFactory} and > 460: * {@link CertificateFactory} classes from the specified {@link > Provider}. "classes" should probably be "implementations". src/java.base/share/classes/java/security/PEMDecoder.java line 464: > 462: * > 463: * <p>If {@code provider} is {@code null}, a new instance is > returned with > 464: * the default provider configuration. IMHO, I think this is working around a bug in the calling code and an NPE should be thrown instead. src/java.base/share/classes/java/security/PEMDecoder.java line 467: > 465: * > 466: * @param provider the factory provider > 467: * @return new configured {@code PEMDecoder} instance Suggest: "a new `PEMDecoder` instance configured with the specified provider" src/java.base/share/classes/java/security/PEMDecoder.java line 474: > 472: > 473: /** > 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM > data Put code around PEMDecoder. s/will decrypt/decrypts/ src/java.base/share/classes/java/security/PEMDecoder.java line 475: > 473: /** > 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM > data > 475: * such as encrypted private keys with the specified password. "such as" sounds like it may support others but there is only private keys. Suggest being more specific: "Returns a copy of this `PEMDecoder` that decodes and decrypts encrypted private keys using the specified password." src/java.base/share/classes/java/security/PEMDecoder.java line 476: > 474: * Returns a copy of this PEMDecoder that will decrypt encrypted PEM > data > 475: * such as encrypted private keys with the specified password. > 476: * Non-encrypted PEM may still be decoded from this instance. "may" sounds like it is optional to support, suggest using "can". Maybe say "PEM types that are not encrypted can also be decoded from this instance." src/java.base/share/classes/java/security/PEMDecoder.java line 480: > 478: * @param password the password to decrypt encrypted PEM data. This > array > 479: * is cloned and stored in the new instance. > 480: * @return new configured {@code PEMDecoder} instance Suggest: "a new `PEMEncoder` instance configured for decryption" src/java.base/share/classes/java/security/PEMEncoder.java line 236: > 234: * > 235: * @param password sets the encryption password. The array is > cloned and > 236: * stored in the new instance. {@code null} is a > valid value. Why allow `null` here but not in `PEMDecoder.withDecryption`. I think we should probably disallow null. Even though PBEKeySpec allows it, I think it is for a corner case that might not be relevant here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087619323 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087506523 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087559801 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087512679 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087513847 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087515197 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087525022 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087518920 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087526437 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087529029 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087539558 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087543007 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087547542 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087551104
