RFR: 8298420: PEM API: Implementation (Preview) [v9]

[Weijun Wang]

On Mon, 21 Oct 2024 19:52:36 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:
> 
>   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