On Thu, 13 Mar 2025 01:15:03 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 61 commits: > > - merge with master > - better comment and remove commented out code > - Merge branch 'master' into pem > - Merge branch 'pem-merge' into pem > - merge > - Merge in PEMRecord as part of the API. > - Merge branch 'pem-record' into pem-merge > > # Conflicts: > # src/java.base/share/classes/java/security/PEMDecoder.java > # src/java.base/share/classes/java/security/PEMRecord.java > # src/java.base/share/classes/sun/security/util/Pem.java > # test/jdk/java/security/PEM/PEMData.java > # test/jdk/java/security/PEM/PEMDecoderTest.java > # test/jdk/java/security/PEM/PEMEncoderTest.java > - Merge branch 'master' into pem-record > > # Conflicts: > # src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java > - test fixes & cleanup > - Implement stream decoding > fix StringBuffer/Builder > X509C* changes > - ... and 51 more: https://git.openjdk.org/jdk/compare/a347ecde...106788ef src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 232: > 230: } else if (keySpec instanceof PKCS8EncodedKeySpec p8) { > 231: PKCS8Key p8key = new ECPrivateKeyImpl(p8.getEncoded()); > 232: if (p8key.hasPublicKey()) { Shouldn't that be `if (!p8key.hasPublicKey()) {`, similar to XDHKeyFactory.generatePublicImpl() and others? src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 55: > 53: * ... > 54: * } > 55: * <p> Actually this is for the next line, and just a nit: It appears that now we are at least parsing/saving the attributes and definitely attaching the public key when decoding from DER. Should this line be removed? src/java.base/share/classes/sun/security/pkcs/PKCS8Key.java line 207: > 205: byte[] internal = rawKey.generateEncoding(); > 206: PKCS8EncodedKeySpec pkcs8KeySpec = > 207: new PKCS8EncodedKeySpec(internal); Since you don't use `internal` beyond this point AFAICT, you may not need the reference as a local variable and just inline the call to `rawKey.generateEncoding()`. But this is more of a nit/style thing so no big deal if you want to leave it as-is. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001457211 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001501575 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2001518767
