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

[Weijun Wang]

On Thu, 17 Apr 2025 15:51:09 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 two additional commits since the last revision:
> 
>  - javadoc updates
>  - code review comments

Some comments on internal classes.

Since each `KeyFactory` now accepts PKCS8 in `generatePublicKey`, I suggest adding some tests on it. Also, add some tests to check if `new PKCS8EncodedKeySpec(data)` and `new X509EncodedKeySpec(data)` are able to detect algorithms from only data.

src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 225:

> 223:         throws GeneralSecurityException {
> 224:         if (keySpec instanceof X509EncodedKeySpec) {
> 225:             return new ECPublicKeyImpl(((X509EncodedKeySpec)keySpec).getEncoded());

We can also use `instanceof` pattern matching here. Same on line 246.

src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 157:

> 155:     }
> 156: 
> 157:     public byte[] getArrayS() {

Why remove `getArrayS0`? Not worth saving those bytes?

src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 191:

> 189:                 DerValue value = data.getDerValue();
> 190:                 if (value.isContextSpecific((byte) 0)) {
> 191:                     attributes = value.getDataBytes();  // Save DER sequence

At [0], it's `ECDomainParameters`. I don't think it's the same as `attributes` inside `OneAsymmetricKey`.

src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 195:

> 193:                         return;
> 194:                     }
> 195:                     value = data.getDerValue();

The original code does not care about whether [0] comes before [1] or if there are duplicates. You modified code ensures only [1] can be after [0] but still allows duplicates (For example, [0], [1], [1]). The `while` on line 188 can be changed to `if` and we ensure `data.available() == 0` at the end.

src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 201:

> 199:                     BitArray bitArray = bits.data.getUnalignedBitString();
> 200:                     pubKeyEncoded = new X509Key(algid,
> 201:                         bitArray).getEncoded();

We could have 2 copies of the public key, one here and one inside `OneAsymmetricKey`. Do you want to compare them?

src/java.base/share/classes/sun/security/util/DerValue.java line 195:

> 193: 
> 194:     /**
> 195:      * Returns true if the CONTEXT SPECIFIC bit is set in the type tag.

`iff` was used to mean `if and only if`. It's more precise than a simple `if`.

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

PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2793998438
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060169867
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060231627
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060222850
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060212787
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060201102
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060236284