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.

For `PEMEncoder`.

Also, do you want to update the `PKCS8EncodedKeySpec` class with a new ASN.1 grammar and a link to version 2.0?

src/java.base/share/classes/java/security/PEMEncoder.java line 63:

> 61:  * by passing an {@link EncryptedPrivateKeyInfo} object into the encode methods.
> 62:  * <p>
> 63:  * PKCS8 v2.0 allows OneAsymmetric encoding, which is a private and public

Add a link to PKCS 8 2.0?

src/java.base/share/classes/java/security/PEMEncoder.java line 74:

> 72:  * @apiNote
> 73:  * Here is an example of encoding a PrivateKey object:
> 74:  * <pre>

Change to code snippet.

src/java.base/share/classes/java/security/PEMEncoder.java line 143:

> 141:      *           DEREncodable.
> 142:      * @return PEM encoding in a String
> 143:      * @throws IllegalArgumentException when the passed object returns a null

What does "returns a null binary encoding" mean? There is no other method talking about this. I think we can just say "if configured for encryption but object does not support" since this looks like the only reason.

Also, how about `IllegalStateException`?

src/java.base/share/classes/java/security/PEMEncoder.java line 177:

> 175:                         PEMRecord.ENCRYPTED_PRIVATE_KEY, epki.getEncoded()));
> 176:                 } catch (IOException e) {
> 177:                     throw new SecurityException(e);

Do you really want to use `SecurityException`? This only happens when the `AlgorithmParameters` inside EPKI is not initialized. I would say this is a very good candidate for an `IllegalStateException`.

src/java.base/share/classes/java/security/PEMEncoder.java line 240:

> 238:      *
> 239:      * @param password sets the encryption password.  The array is cloned and
> 240:      *                stored in the new instance.

Can password be empty? I vaguely remember some algorithms might not like it, or, is it just that `SecretKeySpec` does not like an empty key?

-------------

PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2406520187
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823529997
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823530133
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823531557
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823532621
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823533634