On Mon, 21 Oct 2024 19:52:36 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: > > apparently <p> can't be before a @implNote.. Who know. Some comments on `EncryptedPrivateKeyInfo`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 1: > 1: /* New encrypt and decrypt methods are all password-based and work on keys directly. Old methods uses a decryption key and works on key specs. For completeness; have you thought about more combinations? Maybe at least encryption with a key? I assume an `EncryptedPrivateKeyInfo` is not only encrypted with a password. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 84: > 82: * @param encoded the ASN.1 encoding to be parsed. > 83: * @throws NullPointerException if {@code encoded} is {@code null}. > 84: * @throws IOException if error occurs when parsing the ASN.1 > encoding. Why change the old spec? There seems to be no problem. Especially, why remove the "array are copied" sentence? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 339: > 337: * @throws IllegalArgumentException when an argument causes an > 338: * initialization error. > 339: * @throws SecurityException on a cryptographic errors. Oh, I didn't notice this before. Have we decided to repurpose `SecurityException` for this usage now that there will be no more Security Manager? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 393: > 391: > 392: /** > 393: * Creates and encrypts an `EncryptedPrivateKeyInfo` from a given > PrivateKey Oh, you should use `{@code}` here. Same in `@impNote` below. Also, there are some other class names that should be in enclosed in `{@code}`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 440: > 438: > 439: /** > 440: * Return a PrivateKey from the object's encrypted data with a > KeyFactory Should be `SecretKeyFactory`. Same in `@param provider` below. Also, "Return a key" is not clear. Existing `getKeySpec` methods use "Extract". You maybe also use "Recover". src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 444: > 442: * > 443: * @param password the password > 444: * @param provider the KeyFactory provider used to generate the key. Since you allow it to be null, mention it here. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 477: > 475: * @return the PKCS8EncodedKeySpec object. > 476: * @exception NullPointerException if {@code decryptKey} is {@code > null}. > 477: * @exception NoSuchAlgorithmException Cannot find appropriate > cipher to Use "if". ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2406198665 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823356547 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823326801 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823329427 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823333993 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823330725 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823341822 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823335846
