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 Comments on the APIs. src/java.base/share/classes/java/security/PEM.java line 98: > 96: * formatted. > 97: * @throws NullPointerException if {@code type} and/or {@code > content} are > 98: * {@code null}. No need to add a period at the end if it's not a full sentence. There are several places in this file. src/java.base/share/classes/java/security/PEM.java line 117: > 115: * {@code content} data in String form. {@code leadingData} is set > to null. > 116: * > 117: * @param type the PEM type identifier In the other constructor, the words are "the type identifier". Please be consistent. src/java.base/share/classes/java/security/PEMDecoder.java line 76: > 74: * decryption)</li> > 75: * <li>ENCRYPTED PRIVATE KEY : {@code PKCS8EncodedKeySpec} (if > configured with > 76: * decryption)</li> Plus "and passed as a Class parameter". src/java.base/share/classes/java/security/PEMDecoder.java line 119: > 117: * {@snippet lang = java: > 118: * PEMDecoder pd = PEMDecoder.of(); > 119: * PrivateKey priKey = pd.decode(priKeyPEM, PrivateKey.class); In the 2nd example below, indent `withFactory(provider);` with 8 spaces. src/java.base/share/classes/java/security/PEMDecoder.java line 333: > 331: * > 332: * <p> This method reads the {@code String} until PEM data is found > 333: * or the end of the {@code String} is reached. If no PEM data is > found, Three lines above. Do we need to say "tClass must extend DEREncodable"? The `<S extends DEREncodable>` style means it has to be. 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 Three lines above. Is it better to say "until the end of the first PEM footer"? Same with the other decode-from-stream method. src/java.base/share/classes/java/security/PEMEncoder.java line 43: > 41: import java.util.Objects; > 42: import java.util.concurrent.locks.ReentrantLock; > 43: In the 2nd line below, you used "store and transfer security objects". Let's all use "cryptographic objects". Same in `PEMDecoder`. src/java.base/share/classes/java/security/PEMEncoder.java line 61: > 59: * which takes a password and returns a new {@code PEMEncoder} instance > 60: * configured to encrypt the key with that password. Alternatively, a > 61: * private key encrypted as an {@code EncryptedKeyInfo} object can be > encoded Typo, it's `EncryptedPrivateKeyInfo`. Maybe add a link. src/java.base/share/classes/java/security/PEMEncoder.java line 68: > 66: * contain both private and public keys. > 67: * {@link KeyPair} objects passed to the {@code encode} or > 68: * {@code encodeToString} methods are encoded as a Three lines above. Should "OneAsymmetricKey" be fixed-width `OneAsymmetricKey`? src/java.base/share/classes/java/security/PEMEncoder.java line 69: > 67: * {@link KeyPair} objects passed to the {@code encode} or > 68: * {@code encodeToString} methods are encoded as a > 69: * OneAsymmetricKey structure using the "PRIVATE KEY" type. Should "OneAsymmetricKey" be fixed-width `OneAsymmetricKey`? Also, 4 lines above here. src/java.base/share/classes/java/security/PEMEncoder.java line 95: > 93: * <li>{@code PKCS8EncodedKeySpec} (if configured with encryption) : > 94: * ENCRYPTED PRIVATE KEY</li> > 95: * <li>{@code PEM} : {@code PEM.type()}</li> `X509EncodedKeySpec` missing. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 330: > 328: > 329: /** > 330: * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a > given I know the line above is not new, but it sounds strange to me. You cannot encrypt something that is already named "encrypted". src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 368: > 366: */ > 367: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API) > 368: public static EncryptedPrivateKeyInfo encryptKey(DEREncodable de, Shall we name it `encryptKey` or simply `encrypt`? I'm asking because it can be something other than a key. The decrypt side has `getKey`, `getKeySpec`, and `getKayPair`. Since we have only one on the encrypt side, it needn't use the noun of one of them. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 459: > 457: public static EncryptedPrivateKeyInfo encryptKey(DEREncodable de, > 458: Key encryptKey, String algorithm, AlgorithmParameterSpec params, > 459: Provider provider, SecureRandom random) { Technically, must `encryptKey` be a PBE key? Can it be any key? I notice that PBE is not mentioned in `getKey`. ------------- PR Review: https://git.openjdk.org/jdk/pull/27147#pullrequestreview-3290505282 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395706082 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395700263 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395742704 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395761939 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395797969 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395782621 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395714807 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395720590 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395722689 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395724602 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395729138 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395505870 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395494777 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2395575014
