On Thu, 12 Dec 2024 19:59:05 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 58 commits: > > - 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 > - Reorg tests data > minor fixes > - merge from pem branch > - Merge branch 'pem' into pem-record > > # Conflicts: > # src/java.base/share/classes/java/security/PEMEncoder.java > # src/java.base/share/classes/sun/security/provider/X509Factory.java > # src/java.base/share/classes/sun/security/util/Pem.java > # test/jdk/java/security/PEM/PEMDecoderTest.java > # test/jdk/java/security/PEM/PEMEncoderTest.java > - ... and 48 more: https://git.openjdk.org/jdk/compare/22845a77...cc952c0b src/java.base/share/classes/java/security/PEMDecoder.java line 58: > 56: * </pre> > 57: * > 58: * A specified return class must extend {@link DEREncodable} and be an Suggest rewording as "Objects that are decoded and returned must extend ..." src/java.base/share/classes/java/security/PEMDecoder.java line 68: > 66: * available. > 67: * > 68: * <p> A new immutable {@code PEMDecoder} instance is created by using s/using/calling/ src/java.base/share/classes/java/security/PEMDecoder.java line 78: > 76: * > 77: * <p> {@code String} values returned by this class use character set > 78: * {@link java.nio.charset.StandardCharsets#ISO_8859_1 ISO-8859-1} Missing period at end of sentence. src/java.base/share/classes/java/security/PEMDecoder.java line 199: > 197: * Decodes and returns {@link DEREncodable} from the given string. > 198: * > 199: * @param str PEM data in a String. Remove the period at end. Same comment applies to other @param, @return and @throws descriptions. See https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@param for more details where it says "End the phrase with a period only if another phrase or sentence follows it." src/java.base/share/classes/java/security/PEMDecoder.java line 199: > 197: * Decodes and returns {@link DEREncodable} from the given string. > 198: * > 199: * @param str PEM data in a String. Suggest rewording as "a String containing PEM data". src/java.base/share/classes/java/security/PEMDecoder.java line 200: > 198: * > 199: * @param str PEM data in a String. > 200: * @return an {@code DEREncodable} generated from the PEM data. s/an/a/ src/java.base/share/classes/java/security/PEMDecoder.java line 218: > 216: * {@code InputStream}. > 217: * > 218: * <p>The method will read the {@code InputStream} until PEM data is s/The/This/ src/java.base/share/classes/java/security/PEMDecoder.java line 374: > 372: * Configures and returns a new {@code PEMDecoder} instance from the > 373: * current instance that will use KeyFactory and CertificateFactory > classes > 374: * from the specified {@link Provider}. Any errors using the What if `KeyFactory` and `CertificateFactory` are in different providers? Do we want to have a method that also takes two provider parameters? src/java.base/share/classes/java/security/PEMDecoder.java line 377: > 375: * {@code provider} will occur during decoding. > 376: * > 377: * <p>If {@code params} is {@code null}, a new instance is returned > with There is no variable named `params` - do you mean `provider`? Also, why not throw an NPE and not allow a `null` provider, since it would be the same as calling `of()`? src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 52: > 50: > 51: private final byte[] encodedKey; > 52: private String algorithmName; I think this can be marked `final` now. src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 200: > 198: DerValue bits = > value.withTag(DerValue.tag_BitString); > 199: //byte[] bytes = bits.getBitString(); > 200: //BitArray bitArray = new BitArray(bytes[0] * 8 - 2, > bytes, 3); Commented out code, remove? src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 207: > 205: pubKeyEncoded = new X509Key(algid, > 206: bits.getUnalignedBitString()).getEncoded(); > 207: */ Commented out code, remove? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951190132 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951191331 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951194543 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951463902 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951582549 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951587981 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951589447 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951505364 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1951507480 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1947197843 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1949565799 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1949565548
