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

Tue Nov 5 19:04:43 UTC 2024

On Wed, 30 Oct 2024 23:02:29 GMT, Weijun Wang <weijun at 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