On Wed, 14 May 2025 13:16:17 GMT, Weijun Wang <wei...@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
