On Thu, 25 Sep 2025 23:03:11 GMT, Anthony Scarpino <ascarp...@openjdk.org> 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 src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 393: > 391: > 392: /** > 393: * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a > given s/a given/the specified/ src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 394: > 392: /** > 393: * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a > given > 394: * {@code DEREncodable} and password. Default algorithm and > parameters are Need to be more specific - what algorithm and what parameters. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 398: > 396: * > 397: * @param de the {@code DEREncodable} to be encrypted. Usage > 398: * limited to {@link PrivateKey}, {@link KeyPair}, and "Usage limited" sounds odd. Maybe "The supported `DEREncodable types are ... An IllegalArgumentException is thrown for any other type." src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 402: > 400: * @param password the password used in the PBE encryption. This > array > 401: * will be cloned before being used. > 402: * @return an {@code EncryptedPrivateKeyInfo} Next line: "@throws IllegalArgumentException on initialization errors based on the arguments passed to the method" What does this mean - you need to be more specific about what are the error cases. Otherwise it will not be helpful for TCK test writers on how to test this. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 413: > 411: * {@code AlgorithmParameterSpec} are the provider's algorithm > defaults. > 412: * > 413: * @since 25 Throwing a RuntimeException on encryption error is too general. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 535: > 533: * @param decryptKey the decryption key and cannot be {@code null} > 534: * @param provider the {@code Provider} used for Cipher decryption > and > 535: * {@code PrivateKey} generation. A {@code null} > value will Same comment about provider as below. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 540: > 538: * @throws GeneralSecurityException if an error occurs parsing, > 539: * decrypting the data, or producing the key object. > 540: * @throws NullPointerException if {@code decryptKey} is {@code null} Same comment about exceptions as below. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 565: > 563: */ > 564: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API) > 565: public KeyPair getKeyPair(char[] password) throws > GeneralSecurityException { This should specifically throw an `InvalidKeyException` if decryption fails. I think you want to specify the exact subclasses when it is clearly the right behavior. In this case, the method should always throw `InvalidKeyException` on decryption errors. Also why generalize it to throw `GeneralSecurityException`? This should throw the same exceptions as `getKeySpec(Key, Provider)` since the params are the same. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 585: > 583: * @param decryptKey the decryption key and cannot be {@code null} > 584: * @param provider the {@code Provider} used for Cipher decryption > and > 585: * key generation. A {@code null} value will Why do you allow null? This is inconsistent with the other methods that take Provider arguments. I think consistency is important. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 588: > 586: * use the default provider configuration. > 587: * @return a {@code KeyPair} extracted from the encrypted data > 588: * @throws GeneralSecurityException if an error occurs parsing, Same comment as above on the exceptions. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403250173 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403251624 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403255532 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403260574 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403264388 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403246374 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403245654 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403237211 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403243416 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2403241992
