On Wed, 30 Oct 2024 23:02:29 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> apparently <p> can't be before a @implNote.. Who know.
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 231:
>
>> 229: * algorithm-specific operations, or {@code X509EncodedKeySpec} if
>> the
>> 230: * X.509 binary encoding is desired instead of a Key object. An
>> IOException
>> 231: * will be thrown if the class is incorrect for the given PEM data.
>
> There is no IOE in this method.
Yep
> src/java.base/share/classes/java/security/PEMDecoder.java line 282:
>
>> 280: }
>> 281:
>> 282: DEREncodable so = decode(pem);
>
> The line above could throw IOE. Shall we wrap it in an IAE? I mean the same
> error in the other decode-from-string method is an IAE.
Yes, that would be consistent
> src/java.base/share/classes/java/security/PEMDecoder.java line 358:
>
>> 356:
>> 357: /**
>> 358: * Configures and returns a new {@code PEMDecoder} instance from the
>
> Are you going to be more specific on what kind of factories will be involved?
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 361:
>
>> 359: * current instance that will use Factory classes from the specified
>> 360: * {@link Provider}. Any errors using the {@code provider} will
>> occur
>> 361: * during decoding.
>
> Do you mean errors will happen during decoding? Do you want to be clear on
> what exceptions will be thrown?
Errors will be thrown during decoding, when `decode()` is called. This method
sets the provider in the PEMDecoder instance. This method throws no exceptions.
> src/java.base/share/classes/java/security/PEMDecoder.java line 367:
>
>> 365: *
>> 366: * @param provider the Factory provider.
>> 367: * @return a new PEM decoder instance.
>
> The return spec for this method and the next one should be using a consistent
> wording.
Yep
> src/java.base/share/classes/java/security/PEMEncoder.java line 63:
>
>> 61: * by passing an {@link EncryptedPrivateKeyInfo} object into the encode
>> methods.
>> 62: * <p>
>> 63: * PKCS8 v2.0 allows OneAsymmetric encoding, which is a private and
>> public
>
> Add a link to PKCS 8 2.0?
Add an external link? I don't believe that is allowed
> src/java.base/share/classes/java/security/PEMEncoder.java line 74:
>
>> 72: * @apiNote
>> 73: * Here is an example of encoding a PrivateKey object:
>> 74: * <pre>
>
> Change to code snippet.
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 143:
>
>> 141: * DEREncodable.
>> 142: * @return PEM encoding in a String
>> 143: * @throws IllegalArgumentException when the passed object returns
>> a null
>
> What does "returns a null binary encoding" mean? There is no other method
> talking about this. I think we can just say "if configured for encryption but
> object does not support" since this looks like the only reason.
>
> Also, how about `IllegalStateException`?
getEncoded() returns null.
I think your suggestion would still need me to explain why the object doesn't
support encryption.
I don't think `IllegalStateException` makes sense when object passed does not
provide the needed data.
> src/java.base/share/classes/java/security/PEMEncoder.java line 177:
>
>> 175: PEMRecord.ENCRYPTED_PRIVATE_KEY,
>> epki.getEncoded()));
>> 176: } catch (IOException e) {
>> 177: throw new SecurityException(e);
>
> Do you really want to use `SecurityException`? This only happens when the
> `AlgorithmParameters` inside EPKI is not initialized. I would say this is a
> very good candidate for an `IllegalStateException`.
For consistency, `InvalidArgumentException` is better than `SecurityException`
> src/java.base/share/classes/java/security/PEMEncoder.java line 240:
>
>> 238: *
>> 239: * @param password sets the encryption password. The array is
>> cloned and
>> 240: * stored in the new instance.
>
> Can password be empty? I vaguely remember some algorithms might not like it,
> or, is it just that `SecretKeySpec` does not like an empty key?
`PBEKeySpec` allows null and empty passwords and I hope the provider/algorithm
would throw an error if that was a problem.
I changed this to allow null. I realized `EKPI` allowed null, but PEM didn't.
It would be consistent to just allow what `PBEKeySpec` allows.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 1:
>
>> 1: /*
>
> New encrypt and decrypt methods are all password-based and work on keys
> directly. Old methods uses a decryption key and works on key specs. For
> completeness; have you thought about more combinations? Maybe at least
> encryption with a key? I assume an `EncryptedPrivateKeyInfo` is not only
> encrypted with a password.
I could add some `Key` related methods.
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 444:
>
>> 442: *
>> 443: * @param password the password
>> 444: * @param provider the KeyFactory provider used to generate the key.
>
> Since you allow it to be null, mention it here.
ok
> src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line
> 477:
>
>> 475: * @return the PKCS8EncodedKeySpec object.
>> 476: * @exception NullPointerException if {@code decryptKey} is {@code
>> null}.
>> 477: * @exception NoSuchAlgorithmException Cannot find appropriate
>> cipher to
>
> Use "if".
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879645
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879650
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879725
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879741
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879772
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879143
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879163
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879189
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1829753312
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879389
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1829756147
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879062
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1826879086
