On Thu, 17 Apr 2025 15:51:09 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 two > additional commits since the last revision: > > - javadoc updates > - code review comments Some comments on the new `EncryptedPrivateKeyInfo` APIs. Many trailing periods in `@params` tags can be removed if the descriptions are not full sentences. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 81: > 79: > 80: /** > 81: * Constructs an {@code EncryptedPrivateKeyInfo} from a given > Encrypted Why is "E" capitalized? Is it a special term? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 83: > 81: * Constructs an {@code EncryptedPrivateKeyInfo} from a given > Encrypted > 82: * PKCS#8 ASN.1 encoding. > 83: * @param encoded the ASN.1 encoding which is cloned and then parsed. Somehow I prefer the original "The contents of the array...". We've used this style in a lot of places. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 324: > 322: > 323: /** > 324: * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a > given Can we say "encrypt" an "EncryptedPrivateKeyInfo"? It sounds like an already encrypted thing. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 338: > 336: * > 337: * @param key the PrivateKey object to encrypt. > 338: * @param password the password used for generating the PBE key. Do we need to mention the "PBE key"? It's just the password to encrypt the private key. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 341: > 339: * @param algorithm the PBE encryption algorithm. > 340: * @param params the parameters used with the PBE encryption. > 341: * @param provider the Provider that will perform the encryption. I prefer moving (if not copying) the nullable description of `params` and `provider` here. Also, in your implementation, the provider is used to choose both `SecretkeyFactory` and `Cipher`. I hope for each provider there always exists a pair of `SecretkeyFactory` and `Cipher` with a given algorithm name. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 351: > 349: * @implNote The encryption uses the algorithm set by > 350: * `jdk.epkcs8.defaultAlgorithm` Security Property > 351: * and default the {@code AlgorithmParameterSpec} of that provider. I think this `implNote` is not necessary here. You already required `algorithm` to be non-null. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 416: > 414: * {@link PrivateKey} using the {@code encKey} and given parameters. > 415: * > 416: * If {@code algorithm} is {@code null} the default algorithm will > be used. In the other `encryptKey` method using password, `algorithm` must be provided. Why the inconsistency? In fact, since you have `encKey`, doesn't it already have an algorithm name? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 422: > 420: * > 421: * @param key the {@code PrivateKey} object to encrypt. > 422: * @param encKey the encryption {@code Key} It will be more precise if we say `key` is the key to be encrypted and `encKey` is the key used to encrypt `key`. ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2788731077 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056901657 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056902408 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056903380 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056906894 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056908199 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056911191 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056949605 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2056952122
