On Thu, 15 May 2025 03:37:57 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> Hi all, >> >> I need a code review of the PEM API. Privacy-Enhanced Mail (PEM) is a >> format for encoding and decoding cryptographic keys and certificates. It >> will be integrated into JDK24 as a Preview Feature. Preview features does >> not permanently define the API and it is subject to change in future >> releases until it is finalized. >> >> Details about this change can be seen at [PEM API >> JEP](https://bugs.openjdk.org/browse/JDK-8300911). >> >> Thanks >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 76 commits: > > - merge in JEP 513 > - Merge branch 'master' into pem > - comments > - comments > - comments > - comments on the 11th > - comments on the 11th > - comments > - toString update > - non-sealed > Better X509 KeyPair parsing > - ... and 66 more: https://git.openjdk.org/jdk/compare/5e50a584...8bf36d6b src/java.base/share/classes/java/security/PEMDecoder.java line 55: > 53: * type and implements {@link DEREncodable}. > 54: * > 55: * The following lists the supported PEM types and the {@code > DEREncodable} The list seems too early before introducing the decode-with-class methods. src/java.base/share/classes/java/security/PEMDecoder.java line 64: > 62: * <li>PRIVATE KEY : {@code PrivateKey}</li> > 63: * <li>PRIVATE KEY : {@code PKCS8EncodedKeySpec} (Only supported when > passed as a {@code Class} parameter)</li> > 64: * <li>PRIVATE KEY : {@code KeyPair} (if the encoding also contains a > public key)</li> In a later paragraph you talk about reading `PublicKey` from a `PRIVATE KEY` if it is 2.0 and contains the public key. How about merging that info into this list? src/java.base/share/classes/java/security/PEMDecoder.java line 101: > 99: * Configuring an instance for decryption does not prevent decoding with > 100: * unencrypted PEM. Any encrypted PEM that fails decryption > 101: * will throw a {@link RuntimeException}. When an encrypted PEM is used > with a Let's be more clear here: `When an encrypted private key PEM is...`. src/java.base/share/classes/java/security/PEMDecoder.java line 119: > 117: * withFactory(provider); > 118: * byte[] pemData = pe.encode(privKey); > 119: * } The example is still encoder. src/java.base/share/classes/java/security/PEMDecoder.java line 121: > 119: * } > 120: * > 121: * @implNote An implementation may support other PEM types and > DEREncodables. Have you considered moving the whole decoding list above into this `@implNote`? Same question with the encoder side. src/java.base/share/classes/java/security/PEMDecoder.java line 290: > 288: * the read position in the stream may become inconsistent. > 289: * It is recommended to perform no further decoding operations > 290: * on the {@code InputStream}. I prefer the words in the previous commit. It was more positive -- the read position is here when there is no IOE. src/java.base/share/classes/java/security/PEMEncoder.java line 91: > 89: * <li>{@code KeyPair} : PRIVATE KEY</li> > 90: * <li>{@code EncryptedPrivateKeyInfo} : ENCRYPTED PRIVATE KEY</li> > 91: * <li>{@code PrivateKey} (if configured with encryption): ENCRYPTED > PRIVATE KEY</li> Move the two `PrivateKey` lines together. Maybe clearly add `if not configured with encryption` to the other one. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 349: > 347: * @throws IllegalArgumentException on initialization errors based > on the > 348: * arguments passed to the method > 349: * @throws RuntimeException on an encryption errors Remove `an` or change to `error`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 350: > 348: * arguments passed to the method > 349: * @throws RuntimeException on an encryption errors > 350: * @throws NullPointerException if the key or password are null. If `null` in `{@code}`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 401: > 399: * @throws IllegalArgumentException on initialization errors based > on the > 400: * arguments passed to the method > 401: * @throws RuntimeException on an encryption errors Remove `an` or change to `error`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 402: > 400: * arguments passed to the method > 401: * @throws RuntimeException on an encryption errors > 402: * @throws NullPointerException when the {code key} or {@code > password} You forgot the `@` after `{`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 438: > 436: * @throws IllegalArgumentException on initialization errors based > on the > 437: * arguments passed to the method > 438: * @throws RuntimeException on an encryption errors Remove `an` or change to `error`. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 440: > 438: * @throws RuntimeException on an encryption errors > 439: * @throws NullPointerException if the {@code key} or {@code encKey} > are > 440: * null. If {@code params} is non-null, {@code algorithm} cannot be Put `null` in `{@code}`. src/java.base/share/classes/sun/security/util/Pem.java line 63: > 61: DEFAULT_ALGO = (algo == null || algo.isBlank()) ? > 62: "PBEWithHmacSHA256AndAES_128" : algo; > 63: pbePattern = Pattern.compile("^PBEWith.*And.*"); Is the regex check in case-insensitive mode? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091394789 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091399606 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091410536 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091411461 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091413464 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091419078 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091387820 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091268965 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091270129 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091271961 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091273449 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091275819 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091276867 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2091282800
