On Thu, 24 Apr 2025 18:29:08 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - javadoc updates
>> - code review comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 60:
>
>> 58: * A specified return class must implement {@link DEREncodable} and be an
>> 59: * appropriate JCE object class for the PEM; otherwise an
>> 60: * {@link IllegalArgumentException} is thrown.
>
> Do we need to document somewhere what "appropriate" JCE classes are for each
> PEM type?
I view this as an advanced feature for experienced users. The list is large
and algorithm-dependent. For example an EC private key PEM could be
PrivateKey.class, ECPrivateKey.class, PEMRecord.class,
PKCS8EncodedKeySpec.class. I don't think it's realistic to list everything.
> src/java.base/share/classes/java/security/PEMDecoder.java line 398:
>
>> 396: * Returns a new {@code PEMDecoder} instance from the current
>> instance
>> 397: * configured to decrypt encrypted PEM data with given password.
>> 398: * Non-encrypted PEM may still be decoded from this instance.
>
> I assume that if users want to use a non-default cipher provider they should
> decode as `EncryptedPrivateKeyInfo` first and it has more sophisticated
> methods there. Do we need to document about this here?
I think that's would be too rare to specify in the javadoc.
> src/java.base/share/classes/java/security/PEMEncoder.java line 120:
>
>> 118:
>> 119: /**
>> 120: * Returns an instance of PEMEncoder.
>
> Do we need to say "simple" or "normal" or "raw" `PEMEncoder` here? Just to be
> clear that it does not do any encryption.
I'll just say "new"
> src/java.base/share/classes/java/security/PEMEncoder.java line 134:
>
>> 132: private String pemEncoded(PEMRecord pem) {
>> 133: StringBuilder sb = new StringBuilder(1024);
>> 134: sb.append("-----BEGIN ").append(pem.type()).append("-----");
>
> So you throw away the leading data? Shall we document this somewhere?
This is a private method that just converts the `pem` and `type` into a
properly formatted PEM. Leading data is not applicable here.
> src/java.base/share/classes/java/security/PEMEncoder.java line 175:
>
>> 173: throw new IllegalArgumentException("KeyPair does
>> not " +
>> 174: "contain PrivateKey.");
>> 175: }
>
> Do we really need to care about whether the private part or the public part
> is null?
Previous discussions determined that a null value in a KeyPair was undefined
and should not be done.
> src/java.base/share/classes/java/security/PEMEncoder.java line 235:
>
>> 233: */
>> 234: public byte[] encode(DEREncodable de) {
>> 235: return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1);
>
> Which operation is lighter? Have you considered letting `pemEncoded` to
> return a byte array and converting it into a string in `encodeToString()`?
It's easier to construct using StringBuilder. Either direction some data is
converting String to bytes or bytes to String.
> 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.
> src/java.base/share/classes/java/security/PEMEncoder.java line 287:
>
>> 285: keySpec = null;
>> 286: } catch (GeneralSecurityException e) {
>> 287: throw new SecurityException("Security property " +
>
> Let's discuss whether to use `SecurityException` here. I would use
> `ProviderException`.
This is catching any errors that may occur that may not a result of the
Provider. ProviderException is for errors/problems with the Provider.
> src/java.base/share/classes/java/security/PEMEncoder.java line 300:
>
>> 298: // If `key` is non-null, this is an encoder ready to encrypt.
>> 299: if (key != null) {
>> 300: if (privateBytes == null || publicBytes != null) {
>
> `publicBytes` cannot be null here. You rejected both being null at the
> beginning of this method.
The check at the top is AND, while this is OR.
> 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`.
> src/java.base/share/classes/java/security/PEMRecord.java line 79:
>
>> 77: * @param pem The data between the PEM header and footer.
>> 78: * @param leadingData Any non-PEM data read during the decoding
>> process
>> 79: * before the PEM header. This value maybe
>> {@code null}.
>
> Do we need to say if `leadingData` contains the final newline char?
I'm not sure what you mean by "final" as it contains whatever the stream
contains before the dashes. The point is not to parse the non-PEM data and
store it as is.
> src/java.base/share/classes/java/security/PEMRecord.java line 95:
>
>> 93: // including lowercase. The onus is on the caller.
>> 94: if (type != null && type.startsWith("-----")) {
>> 95: // Remove PEM headers syntax if present.
>
> I don't think we need to be so tolerant at the beginning. Just reject it.
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 135:
>
>> 133: /**
>> 134: * Returns the binary encoding from the Base64 data contained in
>> 135: * {@code pem}.
>
> The name does not sound correct to me. PEM encodes binary data to an ASCII
> string, so "encoding" is the ASCII string. How about just `getData`?
`PEMRecord` implements `DEREncodable` which all the classes return DER through
`getEncoded()`. I is consistent with the other implementing classes.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 81:
>
>> 79:
>> 80: /**
>> 81: * Constructs an {@code EncryptedPrivateKeyInfo} from a given
>> Encrypted
>
> Why is "E" capitalized? Is it a special term?
yep
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 83:
>
>> 81: * Constructs an {@code EncryptedPrivateKeyInfo} from a given
>> Encrypted
>> 82: * PKCS#8 ASN.1 encoding.
>> 83: * @param encoded the ASN.1 encoding which is cloned and then parsed.
>
> Somehow I prefer the original "The contents of the array...". We've used this
> style in a lot of places.
I didn't change the rest of them because I didn't want to make too many
unnecessary edits. I understand you like it, but it is not a concise
description.
The one word "cloned" is a lot simpler and means the same as:
"The contents of {@code encryptedData} are copied to protect against subsequent
modification when constructing this object."
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 324:
>
>> 322:
>> 323: /**
>> 324: * Creates and encrypts an {@code EncryptedPrivateKeyInfo} from a
>> given
>
> Can we say "encrypt" an "EncryptedPrivateKeyInfo"? It sounds like an already
> encrypted thing.
ok
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 338:
>
>> 336: *
>> 337: * @param key the PrivateKey object to encrypt.
>> 338: * @param password the password used for generating the PBE key.
>
> Do we need to mention the "PBE key"? It's just the password to encrypt the
> private key.
ok
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 341:
>
>> 339: * @param algorithm the PBE encryption algorithm.
>> 340: * @param params the parameters used with the PBE encryption.
>> 341: * @param provider the Provider that will perform the encryption.
>
> I prefer moving (if not copying) the nullable description of `params` and
> `provider` here.
>
> Also, in your implementation, the provider is used to choose both
> `SecretkeyFactory` and `Cipher`. I hope for each provider there always exists
> a pair of `SecretkeyFactory` and `Cipher` with a given algorithm name.
I'll move the descriptions
Yes this method needs both on the same provider. If they are on separate
providers, they can use the other method that takes the `Key` instead of a
`password`.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 351:
>
>> 349: * @implNote The encryption uses the algorithm set by
>> 350: * `jdk.epkcs8.defaultAlgorithm` Security Property
>> 351: * and default the {@code AlgorithmParameterSpec} of that provider.
>
> I think this `implNote` is not necessary here. You already required
> `algorithm` to be non-null.
Since I changed to allow null, it's needed now. (see below)
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 416:
>
>> 414: * {@link PrivateKey} using the {@code encKey} and given parameters.
>> 415: *
>> 416: * If {@code algorithm} is {@code null} the default algorithm will
>> be used.
>
> In the other `encryptKey` method using password, `algorithm` must be
> provided. Why the inconsistency?
>
> In fact, since you have `encKey`, doesn't it already have an algorithm name?
It maybe good for both `encryptKey` methods do not allow null algorithms when
APS is non-null. I think `PBEParameterSpec` is currently the only APS used,
but if in the future a new APS is used, a default algorithm with a specified
APS could lead to applications breaking.
There is no guarantee the key's algorithm will be properly formatted to use as
an algorithm name. I wouldn't trust it.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 422:
>
>> 420: *
>> 421: * @param key the {@code PrivateKey} object to encrypt.
>> 422: * @param encKey the encryption {@code Key}
>
> It will be more precise if we say `key` is the key to be encrypted and
> `encKey` is the key used to encrypt `key`.
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062695427
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062723080
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062725966
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062726982
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062727349
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062730381
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062733556
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062734449
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062734838
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061221714
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061224179
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062747907
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061217873
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061226526
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061225921
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062750755
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062750997
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062873270
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2061226193
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062846705
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2062876630
