On Fri, 9 May 2025 15:13:29 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - comments
>> - toString update
>> - non-sealed
>> Better X509 KeyPair parsing
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 64:
>
>> 62: * class is specified.
>> 63: *
>> 64: * For decode methods that accept a {@code Class<S> tClass} as input,
>> they can
>
> Start a new paragraph. Suggest rewording the beginning as "The `{@linkplain
> #decode(String, Class<S>) }` and `{@linkplain #decode(InputStream,
> Class<S>)}` methods take a `Class` parameter which determines the type of
> `DerEncodable` that is returned. These methods are useful when ..."
>
> Then you can insert the sentence above somewhere towards the end of this
> paragraph: "Any type of PEM data can be decoded into a `{@code PEMRecord}` by
> specifying `{@code PEMRecord.class}` as a parameter."
Ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 70:
>
>> 68: * the returned key object from a PEM containing a public and private
>> key.
>> 69: * If only the private key is required, {@code PrivateKey.class} can be
>> used.
>> 70: * {@class PEMRecord.class} is used for returning PEM text. If {@code
>> tClass}
>
> This causes a javadoc error, I think you meant `@code` instead of `@class`.
yeah, saw that after the push
> src/java.base/share/classes/java/security/PEMDecoder.java line 79:
>
>> 77: * Configuring an instance for decryption does not prevent decoding with
>> 78: * unencrypted PEM. Any encrypted PEM that does not use the configured
>> password
>> 79: * will throw an {@link RuntimeException}.
>
> s/an/a/
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 91:
>
>> 89: *
>> 90: * <p>This class is immutable and thread-safe.
>> 91:
>
> Missing `*`.
yeah, cleaned that up
> src/java.base/share/classes/java/security/PEMDecoder.java line 93:
>
>> 91:
>> 92: * @apiNote
>> 93: * Here is an example of decoding a PrivateKey object:
>
> Put `@code` around PrivateKey.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 131:
>
>> 129: * Returns an instance of {@code PEMDecoder}.
>> 130: *
>> 131: * @return returns a {@code PEMDecoder}
>
> you don't need to say "returns", just say "a `PEMDecoder`"
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 190:
>
>> 188: getKey(password.getPassword());
>> 189: }
>> 190: case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {
>
> What about the "X.509 CERTIFICATE" header which is also mentioned in RFC 7468?
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 191:
>
>> 189: }
>> 190: case Pem.CERTIFICATE, Pem.X509_CERTIFICATE -> {
>> 191: CertificateFactory cf = getCertFactory("X509");
>
> Use "X.509". "X509" is an alias and may not be supported by other JDK
> implementations. Same comment on line 196.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 200:
>
>> 198: new
>> ByteArrayInputStream(decoder.decode(pem.pem())));
>> 199: }
>> 200: case Pem.RSA_PRIVATE_KEY -> {
>
> Is it necessary to support this? It is not mentioned in RFC 7468.
Very much so, it's has been seen elsewhere than the PKCS#1 format is still
used. Our RSA has supported it for a long time.
> src/java.base/share/classes/java/security/PEMDecoder.java line 220:
>
>> 218: * the decoder.
>> 219: *
>> 220: * @param str a String containing PEM data.
>
> General style comment throughout APIs: no period necessary at end when
> `@param`, `@return`, or `@throws` starts with a non-capital letter and no
> sentence follows.
ok
> src/java.base/share/classes/java/security/PEMDecoder.java line 223:
>
>> 221: * @return a {@code DEREncodable} generated from the PEM data.
>> 222: * @throws IllegalArgumentException on error in decoding or if the
>> PEM is
>> 223: * unsupported.
>
> If the PEM is unsupported, you return a `PEMRecord` now, so you can remove
> those words. Same comment on lines 248-249.
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083274773
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083274961
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275316
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275184
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275285
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083275393
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276015
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276024
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083276160
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083595650
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083278879
