On Tue, 13 May 2025 17:15:39 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/DEREncodable.java line 37: > >> 35: >> 36: /** >> 37: * This is a top-level interface for security classes that contain >> cryptographic > > Wording suggestion (taken from JEP): "This interface is implemented by > security API classes that contain binary-encodable key or certificate > material." ok > src/java.base/share/classes/java/security/DEREncodable.java line 40: > >> 38: * data which may not be related or have a common class hierarchy. These >> 39: * security objects provide standard binary encoding, like ASN.1, and >> possible >> 40: * type formats, like X.509 and PKCS#8. > > Wording suggestion (taken from JEP): "These APIs or their subclasses > typically provide methods to convert their instances to and from byte arrays > in the [Distinguished Encoding Rules > (DER)](https://en.wikipedia.org/wiki/X.690#DER_encoding) format." I changed the text, I didn't add a link as the one you included was wikipedia, which I don't know if that's stable enough for javadoc. The other link I found was to a PDF, which I don't think is allowed either. What I can do is put the spec in there "ITU X.690" > src/java.base/share/classes/java/security/PEMDecoder.java line 54: > >> 52: * methods return an instance of a class that matches the data >> 53: * type and implements {@link DEREncodable}. The >> 54: * following types are decoded into Java Cryptographic Extensions (JCE) >> object > > Lets make the wording consistent with PEMEncoder: "... into cryptographic > objects that implement {@link DEREncodable}" ok > src/java.base/share/classes/java/security/PEMDecoder.java line 133: > >> 131: * Returns an instance of {@code PEMDecoder}. >> 132: * >> 133: * @return new {@code PEMDecoder} instance > > s/new/a/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 224: > >> 222: * >> 223: * <p>This method reads the {@code String} until the first PEM data >> is found >> 224: * or the end the {@code String} is reached. Non-PEM data before >> the PEM > > s/end/end of/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 458: > >> 456: >> 457: /** >> 458: * Configures and returns a new {@code PEMDecoder} instance from the > > Change the wording to be consistent with `withDecryption`: "Returns a copy of > this `PEMDecoder` that will use ..." ok > src/java.base/share/classes/java/security/PEMDecoder.java line 459: > >> 457: /** >> 458: * Configures and returns a new {@code PEMDecoder} instance from the >> 459: * current instance that will use {@link KeyFactory} and > > s/will use/uses/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 460: > >> 458: * Configures and returns a new {@code PEMDecoder} instance from the >> 459: * current instance that will use {@link KeyFactory} and >> 460: * {@link CertificateFactory} classes from the specified {@link >> Provider}. > > Suggest saying what they are used for: > > s/from the specified {@link Provider}/from the specified {@link Provider} to > produce cryptographic objects./ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 474: > >> 472: >> 473: /** >> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted >> PEM data > > Put code around PEMDecoder. > > s/will decrypt/decrypts/ ok > src/java.base/share/classes/java/security/PEMDecoder.java line 475: > >> 473: /** >> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted >> PEM data >> 475: * such as encrypted private keys with the specified password. > > "such as" sounds like it may support others but there is only private keys. > Suggest being more specific: "Returns a copy of this `PEMDecoder` that > decodes and decrypts encrypted private keys using the specified password." ok > src/java.base/share/classes/java/security/PEMDecoder.java line 476: > >> 474: * Returns a copy of this PEMDecoder that will decrypt encrypted >> PEM data >> 475: * such as encrypted private keys with the specified password. >> 476: * Non-encrypted PEM may still be decoded from this instance. > > "may" sounds like it is optional to support, suggest using "can". Maybe say > "PEM types that are not encrypted can also be decoded from this instance." ok > src/java.base/share/classes/java/security/PEMEncoder.java line 66: > >> 64: * <p> PKCS #8 2.0 defines the ASN.1 OneAsymmetricKey structure, which >> may >> 65: * contain both private and public keys. >> 66: * {@link KeyPair} objects passed to the {@code encode} methods are >> encoded as a > > s/{@code encode}/{@code encode} or {@code encodeToString}/ ok > src/java.base/share/classes/java/security/PEMEncoder.java line 70: > >> 68: * >> 69: * <p> When encoding a {@link PEMRecord}, the API surrounds the >> 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and >> footer > > delete "a generated". ok > src/java.base/share/classes/java/security/PEMEncoder.java line 72: > >> 70: * {@linkplain PEMRecord#pem()} with a generated the PEM header and >> footer >> 71: * from {@linkplain PEMRecord#type()}. {@linkplain >> PEMRecord#leadingData()} is >> 72: * not included in the encoding. {@code PEMRecord} will not preform > > s/preform/perform/ ok > src/java.base/share/classes/java/security/PEMEncoder.java line 84: > >> 82: * >> 83: * <p> Here is an example that encrypts and encodes a private key using >> the >> 84: * specified password. > > s/./:/ ok > src/java.base/share/classes/java/security/PEMEncoder.java line 141: > >> 139: * string. >> 140: * >> 141: * @param de the {@code DEREncodable} to be encoded. > > Nit: no need for period. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 142: > >> 140: * >> 141: * @param de the {@code DEREncodable} to be encoded. >> 142: * @return a byte array containing the PEM encoded data > > s/a byte array/a String/ oops > src/java.base/share/classes/java/security/PEMEncoder.java line 143: > >> 141: * @param de the {@code DEREncodable} to be encoded. >> 142: * @return a byte array containing the PEM encoded data >> 143: * @throws IllegalArgumentException If the DEREncodable cannot be >> encoded > > s/If/if/ > > Put code around DEREncodable. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 144: > >> 142: * @return a byte array containing the PEM encoded data >> 143: * @throws IllegalArgumentException If the DEREncodable cannot be >> encoded >> 144: * @throws NullPointerException if {@code de} is {@code null}. > > Nit: no need for period. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 209: > >> 207: * in a byte array. >> 208: * >> 209: * @param de the {@code DEREncodable} to be encoded. > > Nit: no need for period. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 211: > >> 209: * @param de the {@code DEREncodable} to be encoded. >> 210: * @return PEM encoded byte array >> 211: * @throws IllegalArgumentException if the DEREncodable cannot be >> encoded > > Put code around DEREncodable. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 212: > >> 210: * @return PEM encoded byte array >> 211: * @throws IllegalArgumentException if the DEREncodable cannot be >> encoded >> 212: * @throws NullPointerException if {@code de} is {@code null}. > > Nit: no need for period. ok > src/java.base/share/classes/java/security/PEMEncoder.java line 216: > >> 214: */ >> 215: public byte[] encode(DEREncodable de) { >> 216: return encodeToString(de).getBytes(StandardCharsets.ISO_8859_1); > > Side comment, can look at this later. Since `Base64.Encoder.encodeToString()` > first encodes to bytes and then converts it to a String, it might be more > efficient to do the same here. noted > src/java.base/share/classes/java/security/PEMEncoder.java line 220: > >> 218: >> 219: /** >> 220: * Returns a new {@code PEMEncoder} instance configured with the >> default > > s/configured/configured for encryption/ ok > src/java.base/share/classes/java/security/PEMEncoder.java line 228: > >> 226: * >> 227: * @implNote The default algorithm is defined by Security Property >> {@code >> 228: * jdk.epkcs8.defaultAlgorithm} using default password-based >> encryption > > Wording suggestion: "The default password-based encryption algorithm is > defined by the `jdk.epkcs8.defaultAlgorithm` security property and uses the > default encryption parameters of the provider that is selected." ok > src/java.base/share/classes/java/security/PEMEncoder.java line 235: > >> 233: * returned object with {@link #encode(DEREncodable)}. >> 234: * >> 235: * @param password sets the encryption password. The array is >> cloned and > > No need to say "sets". ok > src/java.base/share/classes/java/security/PEMEncoder.java line 236: > >> 234: * >> 235: * @param password sets the encryption password. The array is >> cloned and >> 236: * stored in the new instance. {@code null} is a >> valid value. > > Why allow `null` here but not in `PEMDecoder.withDecryption`. I think we > should probably disallow null. Even though PBEKeySpec allows it, I think it > is for a corner case that might not be relevant here. I was supporting what PBEKeySpec allowed. Maybe with EPKI methods available to allow null, maybe this method could be more restrictive. > src/java.base/share/classes/java/security/PEMRecord.java line 76: > >> 74: * {@code null} if there is no PEM data. >> 75: * @param pem the data between the PEM header and footer. >> 76: * @param leadingData Any non-PEM data read during the decoding >> process > > s/Any/any/ ok > src/java.base/share/classes/java/security/PEMRecord.java line 77: > >> 75: * @param pem the data between the PEM header and footer. >> 76: * @param leadingData Any non-PEM data read during the decoding >> process >> 77: * before the PEM header. This value maybe >> {@code null}. > > s/maybe/may be/ ok > src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 228: > >> 226: ECOperations ops = ECOperations.forParameters(ecParams) >> 227: .orElseThrow(ProviderException::new); >> 228: MutablePoint pub = ops.multiply(ecParams.getGenerator(), >> arrayS); > > Can `arrayS` ever be `null` here? Looks like it's possible ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087623623 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087624162 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087628211 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087641064 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087641806 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087726601 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087783466 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087783562 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087792527 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087794079 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087796063 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087800696 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087806171 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087807018 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087807683 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087815513 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087815610 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087817444 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087817932 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818027 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818069 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087818128 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087821133 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087822205 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087826110 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087826224 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087827313 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087832770 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087833098 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2088103430
