On Sun, 27 Apr 2025 22:11:57 GMT, Anthony Scarpino <ascarp...@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
