On Sun, 11 May 2025 19:02: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: > > comments on the 11th src/java.base/share/classes/java/security/PEMEncoder.java line 47: > 45: > 46: /** > 47: * {@code PEMEncoder} is used for encoding Privacy-Enhanced Mail (PEM) > data. suggest changing "is used" to "implements an encoder" - then it will be consistent with PEMDecoder. src/java.base/share/classes/java/security/PEMEncoder.java line 59: > 57: * <p> Encrypted private key PEM data can be built by encoding with a > 58: * {@code PEMEncoder} instance returned by {@linkplain > #withEncryption(char[])} > 59: * or by encoding an {@link EncryptedPrivateKeyInfo} . Suggest rewording as: "Private keys can be encrypted and encoded by configuring a `PEMEncoder` with the `withEncryption` method, which takes a password and returns a new `PEMEncoder` instance configured to encrypt the key with that password. Alternatively, a private key encrypted as an `EncryptedKeyInfo` object can be encoded directly to PEM by passing it to the `encode` method." src/java.base/share/classes/java/security/PEMEncoder.java line 61: > 59: * or by encoding an {@link EncryptedPrivateKeyInfo} . > 60: * > 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain > both s/allows .../defines the ASN.1 OneAsymmetricKey structure/ src/java.base/share/classes/java/security/PEMEncoder.java line 62: > 60: * > 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain > both > 62: * private and public keys in the same PEM. This is supported by using the PKCS #8 doesn't define PEM. Suggest removing "in the same PEM". src/java.base/share/classes/java/security/PEMEncoder.java line 63: > 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain > both > 62: * private and public keys in the same PEM. This is supported by using the > 63: * {@link KeyPair} class with the encode methods. Suggest rewording 2nd sentence as: "`KeyPair` objects passed to the `encode` methods are encoded as a OneAsymmetricKey structure using the "PRIVATE KEY" type." src/java.base/share/classes/java/security/PEMEncoder.java line 70: > 68: * the data. > 69: * > 70: * <p>{@code String} values encoded use character set Why does this need to be mentioned if it is an RFC PEM requirement? src/java.base/share/classes/java/security/PEMEncoder.java line 81: > 79: * } > 80: * > 81: * <p>To make the {@code PEMEncoder} encrypt the above private key, only > the Suggest rewording as "Here is an example that encrypts and encodes a private key using the specified password." src/java.base/share/classes/java/security/PEMEncoder.java line 95: > 93: * RFC 1421: Privacy Enhancement for Internet Electronic Mail > 94: * @spec https://www.rfc-editor.org/info/rfc5958 > 95: * RFC 5958: Asymmetric Key Packages Do we really need to add RFC 5958 here? This class is just doing the PEM encoding, it doesn't implement RFC 5958. src/java.base/share/classes/java/security/PEMEncoder.java line 129: > 127: * Returns a new instance of {@code PEMEncoder}. > 128: * > 129: * @return new {@code PEMEncoder} instance "new" sounds like it is a new instance each time this method is called. Suggest just saying "Returns a `PEMDecoder`" src/java.base/share/classes/java/security/PEMEncoder.java line 136: > 134: > 135: /** > 136: * Encode the specified {@code DEREncodable} and return the PEM > encoding in a s/Encode/Encodes/ s/return/returns/ src/java.base/share/classes/java/security/PEMEncoder.java line 140: > 138: * > 139: * @param de the {@code DEREncodable} to be encoded. > 140: * @return PEM encoding in a string Suggest: "a PEM encoded string" src/java.base/share/classes/java/security/PEMEncoder.java line 141: > 139: * @param de the {@code DEREncodable} to be encoded. > 140: * @return PEM encoding in a string > 141: * @throws IllegalArgumentException when encoding the {@code > DEREncodable} Suggest: "if the `DEREncodable` cannot be encoded` src/java.base/share/classes/java/security/PEMEncoder.java line 205: > 203: > 204: /** > 205: * Encodes the specified {@code DEREncodable} into PEM. Make description consistent with other encode method: "Encodes the specified {@code DEREncodable} and returns the PEM encoding in a byte array." src/java.base/share/classes/java/security/PEMEncoder.java line 208: > 206: * > 207: * @param de the {@code DEREncodable} to be encoded. > 208: * @return PEM encoded byte array Suggest: "a byte array containing the PEM encoded data" src/java.base/share/classes/java/security/PEMEncoder.java line 209: > 207: * @param de the {@code DEREncodable} to be encoded. > 208: * @return PEM encoded byte array > 209: * @throws IllegalArgumentException when encoding the {@code > DEREncodable} "If the `DEREncodable` cannot be encoded" src/java.base/share/classes/java/security/PEMEncoder.java line 214: > 212: * @see #withEncryption(char[]) > 213: */ > 214: public byte[] encode(DEREncodable de) { Is this method that useful? Caller can just do what line 215 is doing. src/java.base/share/classes/java/security/PEMEncoder.java line 224: > 222: * <p> Only {@link PrivateKey} objects can be encrypted with this > newly > 223: * configured instance. Encoding other {@link DEREncodable} objects > will > 224: * throw an{@code IllegalArgumentException}. space after "an" src/java.base/share/classes/java/security/PEMEncoder.java line 228: > 226: * @implNote The default algorithm is defined by Security Property > {@code > 227: * jdk.epkcs8.defaultAlgorithm} using default password-based > encryption > 228: * parameters by the supporting provider. To configure all the > encryption Maybe reword as "If you need more control over the encryption algorithm and parameters, use the ..." ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085589104 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085605635 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085606515 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085610213 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085614212 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085617629 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085622259 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085623589 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085626881 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085627060 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085628121 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085628841 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085630670 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085631422 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085631103 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085632879 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085633317 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085634539
