On Mon, 12 May 2025 22:09:14 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> comments on the 11th
>
> src/java.base/share/classes/java/security/PEMEncoder.java line 61:
>
>> 59: * or by encoding an {@link EncryptedPrivateKeyInfo} .
>> 60: *
>> 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain
>> both
>
> s/allows .../defines the ASN.1 OneAsymmetricKey structure/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 62:
>
>> 60: *
>> 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain
>> both
>> 62: * private and public keys in the same PEM. This is supported by using
>> the
>
> PKCS #8 doesn't define PEM. Suggest removing "in the same PEM".
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 63:
>
>> 61: * <p> PKCS #8 2.0 allows OneAsymmetricKey encoding, which may contain
>> both
>> 62: * private and public keys in the same PEM. This is supported by using
>> the
>> 63: * {@link KeyPair} class with the encode methods.
>
> Suggest rewording 2nd sentence as: "`KeyPair` objects passed to the `encode`
> methods are encoded as a OneAsymmetricKey structure using the "PRIVATE KEY"
> type."
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 70:
>
>> 68: * the data.
>> 69: *
>> 70: * <p>{@code String} values encoded use character set
>
> Why does this need to be mentioned if it is an RFC PEM requirement?
It was documented with `PEMDecoder` I thought I should put it here as well, but
I can remove it.
> src/java.base/share/classes/java/security/PEMEncoder.java line 95:
>
>> 93: * RFC 1421: Privacy Enhancement for Internet Electronic Mail
>> 94: * @spec https://www.rfc-editor.org/info/rfc5958
>> 95: * RFC 5958: Asymmetric Key Packages
>
> Do we really need to add RFC 5958 here? This class is just doing the PEM
> encoding, it doesn't implement RFC 5958.
@wangweij asked for it. If you disagree I can remove it.
> src/java.base/share/classes/java/security/PEMEncoder.java line 129:
>
>> 127: * Returns a new instance of {@code PEMEncoder}.
>> 128: *
>> 129: * @return new {@code PEMEncoder} instance
>
> "new" sounds like it is a new instance each time this method is called.
> Suggest just saying "Returns a `PEMEncoder`"
Ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 136:
>
>> 134:
>> 135: /**
>> 136: * Encode the specified {@code DEREncodable} and return the PEM
>> encoding in a
>
> s/Encode/Encodes/
>
> s/return/returns/
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 140:
>
>> 138: *
>> 139: * @param de the {@code DEREncodable} to be encoded.
>> 140: * @return PEM encoding in a string
>
> Suggest: "a PEM encoded string"
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 141:
>
>> 139: * @param de the {@code DEREncodable} to be encoded.
>> 140: * @return PEM encoding in a string
>> 141: * @throws IllegalArgumentException when encoding the {@code
>> DEREncodable}
>
> Suggest: "if the `DEREncodable` cannot be encoded`
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 205:
>
>> 203:
>> 204: /**
>> 205: * Encodes the specified {@code DEREncodable} into PEM.
>
> Make description consistent with other encode method: "Encodes the specified
> {@code DEREncodable} and returns the PEM encoding in a byte array."
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 208:
>
>> 206: *
>> 207: * @param de the {@code DEREncodable} to be encoded.
>> 208: * @return PEM encoded byte array
>
> Suggest: "a byte array containing the PEM encoded data"
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 209:
>
>> 207: * @param de the {@code DEREncodable} to be encoded.
>> 208: * @return PEM encoded byte array
>> 209: * @throws IllegalArgumentException when encoding the {@code
>> DEREncodable}
>
> "If the `DEREncodable` cannot be encoded"
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 214:
>
>> 212: * @see #withEncryption(char[])
>> 213: */
>> 214: public byte[] encode(DEREncodable de) {
>
> Is this method that useful? Caller can just do what line 215 is doing.
I think it's user friendly to when writing to a byte[] or OutputStream.
Base64.Encoder has byte[], ByteBuffer, and String.
> src/java.base/share/classes/java/security/PEMEncoder.java line 224:
>
>> 222: * <p> Only {@link PrivateKey} objects can be encrypted with this
>> newly
>> 223: * configured instance. Encoding other {@link DEREncodable}
>> objects will
>> 224: * throw an{@code IllegalArgumentException}.
>
> space after "an"
ok
> src/java.base/share/classes/java/security/PEMEncoder.java line 228:
>
>> 226: * @implNote The default algorithm is defined by Security Property
>> {@code
>> 227: * jdk.epkcs8.defaultAlgorithm} using default password-based
>> encryption
>> 228: * parameters by the supporting provider. To configure all the
>> encryption
>
> Maybe reword as "If you need more control over the encryption algorithm and
> parameters, use the ..."
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 116:
>
>> 114: *
>> 115: * @param type the type identifier in the PEM header and footer, or
>> {@code null} if there is no PEM data.
>> 116: * @param pem The data between the PEM header and footer.
>
> s/The/the/
ok
> src/java.base/share/classes/java/security/PEMRecord.java line 142:
>
>> 140: *
>> 141: * @throws IllegalArgumentException if {@code pem} cannot be
>> decoded.
>> 142: * @return Returns a new array of the binary encoding each time this
>
> Remove "Returns".
ok
> src/java.base/share/classes/sun/security/rsa/RSAPrivateCrtKeyImpl.java line
> 72:
>
>> 70: private BigInteger coeff; // CRT coefficient
>> 71:
>> 72: // RSA or RSS-PSS KeyType
>
> Typo: s/RSS-PSS/RSA-PSS/
ok
> src/java.base/share/classes/sun/security/x509/X509Key.java line 147:
>
>> 145: * X509Key. Useful for PKCS8v2.
>> 146: */
>> 147: public static X509Key parse(byte[] encoded) throws IOException
>
> Isn't this the same as `X509Key.parse(byte[])`?
I'm assuming you mean `X509Key.decode()`? It looks like that can be used
instead of this method.
> src/java.base/share/classes/sun/security/x509/X509Key.java line 150:
>
>> 148: {
>> 149: DerValue in = new DerValue(encoded);
>> 150: AlgorithmId algorithm;
>
> Can move to line 155.
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085776593
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085777165
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085778467
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085803421
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085805970
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085806427
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085821485
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085822116
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085828381
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846397
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846463
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085846510
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2085850233
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086091768
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086100143
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086265099
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086270777
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086279737
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086340191
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2086340372
