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

[Weijun Wang]

On Sun, 27 Apr 2025 22:11:57 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> src/java.base/share/classes/java/security/PEMEncoder.java line 279:
>> 
>>> 277:         if (keySpec != null) {
>>> 278:             // For thread safety
>>> 279:             lock.lock();
>> 
>> How much does this lock buy? If someone provides a password, I assume they will use it anyway.
>
> key generation was delayed because we don't want `withEncryption()` to throw an exception as it's a config/builder method.  The lock was to prevent multiple versions of the same key being generated in a threaded situation.

OK, throwing an exception in the builder sounds a bad idea.

>> src/java.base/share/classes/java/security/PEMRecord.java line 61:
>> 
>>> 59:  *       RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures
>>> 60:  */
>>> 61: public record PEMRecord(String type, String pem, byte[] leadingData)
>> 
>> How about using the raw byte data instead of the `pem` string? This would automatically reject all format problems, like extra newline at the end, too wide string, and invalid Base64 chars. Also, two `PEMRecord` should equal to each other even if they are encoded differently (for example, different width).
>> 
>> Also, how about forcing all fields to be non null? If there is no leading bytes, use an empty array. I cannot imagine how data could be empty.
>
> This record was built for two reasons:
> 1) For PEM that we do not have a cryptographic representation for (ECPrivateKey, X509Certificate, etc).
> 2) An early comment about reading a private key in PEM from a file, but not creating a PrivateKey object with it.
> 
>  The classes are only supposeed to include Base64.  In fact, I found an inconsistency with `PEMEncoder` using `PEMRecord` with binary data that I've fixed but yet to push. 
> 
> It checking two records are equal, then overriding the `equals()` makes more sense than changing how the data is stored.  Also, storing the PEM solves the simplest consistency test where  PEM read from a file may be different when Base64 decoded and encoded. 
> 
> Some of the values have to be null because of `leadingData`.  You were a proponent of storing the non-PEM data before the PEM data was found when reading from an `InputStream`.  The null situation happens if at the end of the stream there is only non-PEM data.  Throwing an exception or ignoring the data was inconsistent.  So PEMRecord has to allow for `leadingData` with no `type` and `pem`.

Ah, I see. I didn't realize there can be a `PEMRecord` after all PEM records. I should have read the spec carefully.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2064125957
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2064126319