On Mon, 8 Sep 2025 15:42:13 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
> Hi > > Please review the [Second Preview](https://openjdk.org/jeps/8360563) for the > PEM API. The most significant changes from [JEP > 470](https://openjdk.org/jeps/470) are: > > - Renamed the name of `PEMRecord` class to `PEM`. > - Revised the new `encryptKey` methods of the `EncryptedPrivateKeyInfo` class > to accept `DEREncodable` objects rather than just `PrivateKey` objects so > that cryptographic objects with public keys, i.e., `KeyPair` and > `PKCS8EncodedKeySpec`, can also be encrypted. > - Enhanced the `PEMEncoder` and `PEMDecoder` classes to support the > encryption and decryption of `KeyPair` and `PKCS8EncodedKeySpec` objects. > > thanks > > Tony Some comments. src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 255: > 253: if (seq.data.available() != 0) { > 254: DerValue derValue = seq.data.getDerValue(); > 255: if (derValue.isContextSpecific((byte) 1)) { If any of these `if`s is false `null` is returned. Would you rather throw an IAE? src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 159: > 157: privKeyMaterial = val.data.getOctetString(); > 158: > 159: // Special check and parsing for ECDSA's SEC1v2 format So there are 2 ways to extract public key from PKCS8. Are you going to check whether they match? Also, can we add a test case for this format? src/java.base/share/classes/sun/security/util/Pem.java line 355: > 353: return pemEncoded(pem.type(), p); > 354: } > 355: Is it correct to put the 2 new methods here? Seems not closely related to PEM. src/java.base/share/classes/sun/security/util/Pem.java line 387: > 385: * return a PrivateKey > 386: * @param provider KeyFactory provider > 387: */ This method is very similar to `PKCS8Key::parseKey`. Maybe we should combine them. test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetKeyPair.java line 87: > 85: kp.getPublic().getEncoded())) { > 86: throw new AssertionError("PublicKey didn't match the > original."); > 87: } Maybe put these checks into a method and call it three times? Also, you can use `Asserts.assertEqualsByteArray`. ------------- PR Review: https://git.openjdk.org/jdk/pull/27147#pullrequestreview-3218055847 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345111099 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345047110 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345040355 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345041626 PR Review Comment: https://git.openjdk.org/jdk/pull/27147#discussion_r2345020741
