RFR: 8298420: PEM API: Implementation (Preview) [v20]
Weijun Wang
weijun at openjdk.org
Wed May 14 13:13:23 UTC 2025
On Wed, 14 May 2025 08:25:41 GMT, Anthony Scarpino <ascarpino at 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
More information about the security-dev
mailing list