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

Thu May 15 01:31:21 UTC 2025

On Wed, 14 May 2025 13:16:17 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   comments
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 61:
> 
>> 59:  * </pre>
>> 60:  *
>> 61:  * <p>>If the PEM does not have a cryptographic object representation,
> 
> Extra `>`.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 86:
> 
>> 84:  * encrypted private key PEM data using the given password.
>> 85:  * Configuring an instance for decryption does not prevent decoding with
>> 86:  * unencrypted PEM. Any encrypted PEM that does not use the configured password
> 
> You mean when decryption fails? "Does not use the configured password" sounds strange to me,

I can tweak it

> src/java.base/share/classes/java/security/PEMDecoder.java line 87:
> 
>> 85:  * Configuring an instance for decryption does not prevent decoding with
>> 86:  * unencrypted PEM. Any encrypted PEM that does not use the configured password
>> 87:  * will throw a {@link RuntimeException}. When encrypted PEM is used with a
> 
> Add "an" before "encrypted PEM".

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 100:
> 
>> 98:  *
>> 99:  * <p> Here is an example that decryption with a factory provider:
>> 100:  * specified password:
> 
> "with a factory provider and a specified password".

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 237:
> 
>> 235:      * such as a {@code PrivateKey}, if the PEM type is supported.
>> 236:      * Any non-PEM data preceding the PEM header is ignored by the decoder.
>> 237:      * If no cryptographic object is found, a {@link PEMRecord} will be
> 
> Just say "otherwise". Same for other methods.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 290:
> 
>> 288:      * @throws EOFException at the end of the {@code InputStream}.
>> 289:      * @throws IllegalArgumentException on error in decoding
>> 290:      * @throws NullPointerException when {@code is} is null.
> 
> Some still have periods at the end when the description is not a complete sentence. Please go through the whole file.

ok

> src/java.base/share/classes/java/security/PEMDecoder.java line 456:
> 
>> 454: 
>> 455:     /**
>> 456:      * Returns a copy of this {@code PEMDecoder} instance that uses
> 
> Why use "a copy"? Do you mean the password is kept?

If this instance was configured with decryption, this method will return a new instance configured with decryption and the factory provider.  I use "a copy" so the user knows they are adding a new configuration and this instance is staying the same

> src/java.base/share/classes/java/security/PEMRecord.java line 45:
> 
>> 43:  * <p> {@code type} and {@code pem} may not be {@code null}.
>> 44:  * {@code leadingData} may be null if no non-PEM data preceded PEM header
>> 45:  * during decoding.  {@code leadingData} may be be useful for reading metadata
> 
> double "be".

ok

> src/java.base/share/classes/java/security/PEMRecord.java line 83:
> 
>> 81:     public PEMRecord(String type, String pem, byte[] leadingData) {
>> 82:         Objects.requireNonNull(type, "\"type\" cannot be null.");
>> 83:         Objects.requireNonNull(type, "\"pem\" cannot be null.");
> 
> Typo, should check `pem`.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 335:
> 
>> 333:      *
>> 334:      * @param key the {@code PrivateKey} to be encrypted
>> 335:      * @param password the password used during encryption.
> 
> In the other `encryptKey`, you mentioned password is cloned. Be consistent.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 337:
> 
>> 335:      * @param password the password used during encryption.
>> 336:      * @param algorithm the PBE encryption algorithm.  The default algorithm is
>> 337:      *                  will be used if {@code null}.  However, {@code null} is
> 
> Choose one of "is" and "will be".

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 342:
> 
>> 340:      *               encryption.  The provider default will be used if
>> 341:      *               {@code null}.
>> 342:      * @param provider the {@code Provider} is used for PBE
> 
> Change "is used" to "to be used" to be consistent with the one above.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 346:
> 
>> 344:      *                 encryption operations. The default provider list will be
>> 345:      *                 used if {@code null}.
>> 346:      * @return a {@code EncryptedPrivateKeyInfo}
> 
> s/a/an/

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 354:
> 
>> 352:      * IllegalBlockSizeException, BadPaddingException, or InvalidKeyException.
>> 353:      * @throws NullPointerException if the key or password are null. If
>> 354:      * {@code params} is non-null when {@code algorithm} is {@code null}.
> 
> A lot of names above should be in `{@code}`. Same with the other `encryptKey`s.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 357:
> 
>> 355:      *
>> 356:      * @implNote The encryption uses the algorithm set by
>> 357:      * `jdk.epkcs8.defaultAlgorithm` Security Property
> 
> Not markdown here, use `{@code}`. Same with the other `encryptKey`s.

yes

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 358:
> 
>> 356:      * @implNote The encryption uses the algorithm set by
>> 357:      * `jdk.epkcs8.defaultAlgorithm` Security Property
>> 358:      *  and default the {@code AlgorithmParameterSpec} of that provider.
> 
> You meant "the default"? In fact, since `params` is an argument, you can override the default. Maybe just remove "and...".

This one looks like an older version that what I've used in other places.  I'll replace this.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 464:
> 
>> 462:     @PreviewFeature(feature = PreviewFeature.Feature.PEM_API)
>> 463:     public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key, Key encKey,
>> 464:         String algorithm, AlgorithmParameterSpec params, Provider provider,
> 
> How useful is this method? Do users really want to create a PBE key instead of providing the password directly. Note that with Valerie's recent fix, there is no more PKCS11 PBE `SecretKeyFactory`s.

This is the advanced users method allows everything to be set.  It allows the `encKey` to be generated separately from the `provider` that does the encryption.  I think this is an important method so we don't need additional methods to cover the many parameters.  For example,  two `Provider` options for KeyFactory and Cipher.

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 521:
> 
>> 519: 
>> 520:     /**
>> 521:      * Returns a {@code PrivateKey} from the encrypted data in this instance.
> 
> Follow existing `get` methods, "Extract the enclosed PrivateKey object from the encrypted data and return it." Same with the other `getKey` method.

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 523:
> 
>> 521:      * Returns a {@code PrivateKey} from the encrypted data in this instance.
>> 522:      *
>> 523:      * @param password this array is cloned and used for PBE decryption.
> 
> Be consistent with other ones, "the password used in the PBE decryption.  This array is cloned before being used."

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 547:
> 
>> 545:      * using the given provider.
>> 546:      *
>> 547:      * @param decryptKey this is the decryption key and cannot be {@code null}.
> 
> Remove "this is".

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 548:
> 
>> 546:      *
>> 547:      * @param decryptKey this is the decryption key and cannot be {@code null}.
>> 548:      * @param provider the {@code Provider} is used for Cipher decryption and
> 
> Remove "is".

ok

> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 561:
> 
>> 559:     public PrivateKey getKey(Key decryptKey, Provider provider)
>> 560:         throws GeneralSecurityException {
>> 561:         Objects.requireNonNull("decryptKey cannot be null.");
> 
> Add `decryptKey` into `requireNonNull` call.

wow.. yeah

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089694492
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089385166
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089385510
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089387259
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089408885
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089409481
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089275573
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089270437
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089270150
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089423787
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089432207
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089445666
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089477795
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089478251
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089478503
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089482436
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089491575
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089680989
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089686768
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089688705
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089689794
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2089703680