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/PEMEncoder.java line 55: > 53: * > 54: * <p> Encoding may be performed on cryptographic objects that implement > 55: * {@link DEREncodable}. Suggest adding another sentence after this: "The {@link encode(DEREncodable)} and {@link encodeToString(DEREncodable)} methods encode a `DEREncodable` into PEM and return the data in a byte array or String." src/java.base/share/classes/java/security/PEMEncoder.java line 62: > 60: * configured to encrypt the key with that password. Alternatively, a > 61: * private key encrypted as an {@code EncryptedKeyInfo} object can be > encoded > 62: * directly to PEM by passing it to the encode method s/encode method/`encode` or `encodeToString` methods./ src/java.base/share/classes/java/security/PEMEncoder.java line 66: > 64: * <p> PKCS #8 2.0 defines the ASN.1 OneAsymmetricKey structure, which may > 65: * contain both private and public keys. > 66: * {@link KeyPair} objects passed to the {@code encode} methods are > encoded as a s/{@code encode}/{@code encode} or {@code encodeToString}/ src/java.base/share/classes/java/security/PEMEncoder.java line 70: > 68: * > 69: * <p> When encoding a {@link PEMRecord}, the API surrounds the > 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer delete "a generated". src/java.base/share/classes/java/security/PEMEncoder.java line 72: > 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer > 71: * from {@linkplain PEMRecord#type()}. {@linkplain > PEMRecord#leadingData()} is > 72: * not included in the encoding. {@code PEMRecord} will not preform s/preform/perform/ src/java.base/share/classes/java/security/PEMEncoder.java line 84: > 82: * > 83: * <p> Here is an example that encrypts and encodes a private key using > the > 84: * specified password. s/./:/ src/java.base/share/classes/java/security/PEMEncoder.java line 141: > 139: * string. > 140: * > 141: * @param de the {@code DEREncodable} to be encoded. Nit: no need for period. src/java.base/share/classes/java/security/PEMEncoder.java line 142: > 140: * > 141: * @param de the {@code DEREncodable} to be encoded. > 142: * @return a byte array containing the PEM encoded data s/a byte array/a String/ src/java.base/share/classes/java/security/PEMEncoder.java line 143: > 141: * @param de the {@code DEREncodable} to be encoded. > 142: * @return a byte array containing the PEM encoded data > 143: * @throws IllegalArgumentException If the DEREncodable cannot be > encoded s/If/if/ Put code around DEREncodable. src/java.base/share/classes/java/security/PEMEncoder.java line 144: > 142: * @return a byte array containing the PEM encoded data > 143: * @throws IllegalArgumentException If the DEREncodable cannot be > encoded > 144: * @throws NullPointerException if {@code de} is {@code null}. Nit: no need for period. src/java.base/share/classes/java/security/PEMEncoder.java line 209: > 207: * in a byte array. > 208: * > 209: * @param de the {@code DEREncodable} to be encoded. Nit: no need for period. src/java.base/share/classes/java/security/PEMEncoder.java line 211: > 209: * @param de the {@code DEREncodable} to be encoded. > 210: * @return PEM encoded byte array > 211: * @throws IllegalArgumentException if the DEREncodable cannot be > encoded Put code around DEREncodable. src/java.base/share/classes/java/security/PEMEncoder.java line 212: > 210: * @return PEM encoded byte array > 211: * @throws IllegalArgumentException if the DEREncodable cannot be > encoded > 212: * @throws NullPointerException if {@code de} is {@code null}. Nit: no need for period. src/java.base/share/classes/java/security/PEMEncoder.java line 216: > 214: */ > 215: public byte[] encode(DEREncodable de) { > 216: return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1); Side comment, can look at this later. Since `Base64.Encoder.encodeToString()` first encodes to bytes and then converts it to a String, it might be more efficient to do the same here. src/java.base/share/classes/java/security/PEMEncoder.java line 220: > 218: > 219: /** > 220: * Returns a new {@code PEMEncoder} instance configured with the > default s/configured/configured for encryption/ src/java.base/share/classes/java/security/PEMEncoder.java line 228: > 226: * > 227: * @implNote The default algorithm is defined by Security Property > {@code > 228: * jdk.epkcs8.defaultAlgorithm} using default password-based > encryption Wording suggestion: "The default password-based encryption algorithm is defined by the `jdk.epkcs8.defaultAlgorithm` security property and uses the default encryption parameters of the provider that is selected." src/java.base/share/classes/java/security/PEMEncoder.java line 235: > 233: * returned object with {@link #encode(DEREncodable)}. > 234: * > 235: * @param password sets the encryption password. The array is > cloned and No need to say "sets". src/java.base/share/classes/java/security/PEMEncoder.java line 237: > 235: * @param password sets the encryption password. The array is > cloned and > 236: * stored in the new instance. {@code null} is a > valid value. > 237: * @return new configured {@code PEMEncoder} instance Suggest: "a new {@code PEMEncoder} instance configured for encryption" ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087411805 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087416237 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087417397 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087419002 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087419886 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087420308 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087358392 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087359029 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087360161 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087358550 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087345856 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087346460 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087346007 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087357208 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087398280 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087373659 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087389673 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087398501
