On Fri, 2 May 2025 06:09:52 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 with a new target base due to a > merge or a rebase. The pull request now contains 66 commits: > > - major code review comments update > - Merge branch 'master' into pem > - Merge branch 'master' into pem > - javadoc updates > - code review comments > - merge with master > - better comment and remove commented out code > - Merge branch 'master' into pem > - Merge branch 'pem-merge' into pem > - merge > - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327 Comments on `EncryptedPrivateKeyInfo`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 374: > 372: SecretKeyFactory factory; > 373: if (provider == null) { > 374: factory = SecretKeyFactory.getInstance(algorithm); `algorithm` might be null. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 397: > 395: * arguments passed to the method. > 396: * @throws SecurityException on a encryption errors. > 397: * @throws NullPointerException when the password is null. and when `key` is `null`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 408: > 406: public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key, > 407: char[] password) { > 408: char[] pass = password.clone(); No need to clone password here. It will be cloned into a `PBEKeySpec` later. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 458: > 456: } > 457: > 458: if (Pem.DEFAULT_ALGO == null || Pem.DEFAULT_ALGO.length() == 0) { Is this required if `algorithm` is already specified? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 573: > 571: * {@code SecretKeyFactory}, and the {@code > PrivateKey}, > 572: * {@code KeyFactory}. A {@code null} value will > use the > 573: * default provider configuration. There is no `SecretKeyFactory` in this method, but there is `Cipher`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 577: > 575: * @throws InvalidKeyException if an error occurs during parsing of > the > 576: * encrypted data or creation of the key object. > 577: * @throws NullPointerException if {@code key} is null. s/key/decryptKey/. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 582: > 580: */ > 581: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API) > 582: public PrivateKey getKey(Key decryptKey, Provider provider) This method goes one step further than the existing `getKeySpec(dk, p)`. It should only throw more types of exception than the existing one. Now you put everything into a `InvalidKeyException`. Is that good? ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2825666685 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080069229 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080040299 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080032269 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080067182 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080114399 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080114811 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080127359
