On Wed, 14 May 2025 08:25:41 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 More comments on API. src/java.base/share/classes/java/security/PEMRecord.java line 45: > 43: * <p> {@code type} and {@code pem} may not be {@code null}. > 44: * {@code leadingData} may be null if no non-PEM data preceded PEM header > 45: * during decoding. {@code leadingData} may be be useful for reading > metadata double "be". src/java.base/share/classes/java/security/PEMRecord.java line 119: > 117: > 118: * @param type the PEM type identifier > 119: * @param pem the Base64-encoded data encapsulated by the PEM header > and Since internal `pem` is a string, you need to mention the charset of `pem` argument here. Does it make sense to choose another argument name? Like `pemBytes`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 335: > 333: * > 334: * @param key the {@code PrivateKey} to be encrypted > 335: * @param password the password used during encryption. In the other `encryptKey`, you mentioned password is cloned. Be consistent. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 337: > 335: * @param password the password used during encryption. > 336: * @param algorithm the PBE encryption algorithm. The default > algorithm is > 337: * will be used if {@code null}. However, {@code > null} is Choose one of "is" and "will be". src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 342: > 340: * encryption. The provider default will be used if > 341: * {@code null}. > 342: * @param provider the {@code Provider} is used for PBE Change "is used" to "to be used" to be consistent with the one above. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 346: > 344: * encryption operations. The default provider list > will be > 345: * used if {@code null}. > 346: * @return a {@code EncryptedPrivateKeyInfo} s/a/an/ src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 354: > 352: * IllegalBlockSizeException, BadPaddingException, or > InvalidKeyException. > 353: * @throws NullPointerException if the key or password are null. If > 354: * {@code params} is non-null when {@code algorithm} is {@code null}. A lot of names above should be in `{@code}`. Same with the other `encryptKey`s. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 357: > 355: * > 356: * @implNote The encryption uses the algorithm set by > 357: * `jdk.epkcs8.defaultAlgorithm` Security Property Not markdown here, use `{@code}`. Same with the other `encryptKey`s. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 358: > 356: * @implNote The encryption uses the algorithm set by > 357: * `jdk.epkcs8.defaultAlgorithm` Security Property > 358: * and default the {@code AlgorithmParameterSpec} of that provider. You meant "the default"? In fact, since `params` is an argument, you can override the default. Maybe just remove "and...". src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 464: > 462: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API) > 463: public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key, Key > encKey, > 464: String algorithm, AlgorithmParameterSpec params, Provider > provider, How useful is this method? Do users really want to create a PBE key instead of providing the password directly. Note that with Valerie's recent fix, there is no more PKCS11 PBE `SecretKeyFactory`s. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 521: > 519: > 520: /** > 521: * Returns a {@code PrivateKey} from the encrypted data in this > instance. Follow existing `get` methods, "Extract the enclosed PrivateKey object from the encrypted data and return it." Same with the other `getKey` method. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 523: > 521: * Returns a {@code PrivateKey} from the encrypted data in this > instance. > 522: * > 523: * @param password this array is cloned and used for PBE decryption. Be consistent with other ones, "the password used in the PBE decryption. This array is cloned before being used." src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 547: > 545: * using the given provider. > 546: * > 547: * @param decryptKey this is the decryption key and cannot be {@code > null}. Remove "this is". src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 548: > 546: * > 547: * @param decryptKey this is the decryption key and cannot be {@code > null}. > 548: * @param provider the {@code Provider} is used for Cipher > decryption and Remove "is". ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2839835945 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088728318 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088731294 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088752934 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088745307 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088746291 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088746740 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088750354 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088738810 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088740381 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088757629 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088903324 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088899916 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088905109 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088905519
