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/java/security/PEMDecoder.java line 43: > 41: > 42: /** > 43: * {@code PEMDecoder} is an immutable class for Privacy-Enhanced Mail > (PEM) s/for/for decoding/ src/java.base/share/classes/java/security/PEMDecoder.java line 46: > 44: * data. PEM is a textual encoding used to store and transfer security > 45: * objects, such as asymmetric keys, certificates, and certificate > revocation > 46: * lists (CRL). It is defined in RFC 1421 and RFC 7468. PEM consists of > a s/CRL/CRLs/ src/java.base/share/classes/java/security/PEMDecoder.java line 51: > 49: * > 50: * <p> Decoding methods return an instance of a class that matches the > data > 51: * type and implements {@link DEREncodable} unless specified. The > following "unless otherwise specified"? src/java.base/share/classes/java/security/PEMDecoder.java line 64: > 62: * <p> If the PEM does not have a corresponding JCE object, it returns a > 63: * {@link PEMRecord}. Any PEM can be decoded into a {@code PEMRecord} if > the > 64: * class is specified. Decoding a {@code PEMRecord} returns corresponding What do you mean by "Decoding a PEMRecord"? s/returns/returns a/ src/java.base/share/classes/java/security/PEMDecoder.java line 65: > 63: * {@link PEMRecord}. Any PEM can be decoded into a {@code PEMRecord} if > the > 64: * class is specified. Decoding a {@code PEMRecord} returns corresponding > 65: * JCE object or throws a {@link IllegalArgumentException} if no object is s/a/an/ 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 Suggest rewording as ... "A `PEMDecoder` can be configured with additional options by calling ..." src/java.base/share/classes/java/security/PEMDecoder.java line 71: > 69: * {@linkplain #withFactory} and/or {@linkplain #withDecryption}. > Configuring > 70: * an instance for decryption does not prevent decoding with unencrypted > PEM. > 71: * Any encrypted PEM that does not use the configured password will throw > an s/an/a/ src/java.base/share/classes/java/security/PEMDecoder.java line 74: > 72: * {@link SecurityException}. A decoder instance not configured with > decryption > 73: * returns an {@link EncryptedPrivateKeyInfo} with encrypted PEM. > 74: * EncryptedPrivateKeyInfo methods must be used to retrieve the Put code around EncryptedPrivateKeyInfo. src/java.base/share/classes/java/security/PEMDecoder.java line 197: > 195: }; > 196: } catch (GeneralSecurityException | IOException e) { > 197: throw new IllegalArgumentException(e); Should all of these decoding issues really be treated as `IllegalArgumentException`s? I would think that general decoding issues should be thrown as checked exceptions. Runtime exceptions are typically used for serious issues, like programming errors. src/java.base/share/classes/java/security/PEMDecoder.java line 202: > 200: > 201: /** > 202: * Decodes and returns {@link DEREncodable} from the given string. s/returns/returns a/ src/java.base/share/classes/java/security/PEMDecoder.java line 210: > 208: */ > 209: public DEREncodable decode(String str) { > 210: Objects.requireNonNull(str); Missing an `@throws NullPointerException` in the spec. src/java.base/share/classes/java/security/PEMDecoder.java line 236: > 234: */ > 235: public DEREncodable decode(InputStream is) throws IOException { > 236: Objects.requireNonNull(is); Missing an `@throws NullPointerException` in the spec. src/java.base/share/classes/java/security/PEMDecoder.java line 259: > 257: * @param <S> Class type parameter that extends {@code DEREncodable} > 258: * @param string the String containing PEM data. > 259: * @param tClass the returned object class that implementing s/implementing/implements/ src/java.base/share/classes/java/security/PEMDecoder.java line 261: > 259: * @param tClass the returned object class that implementing > 260: * {@code DEREncodable}. > 261: * @return A {@code DEREncodable} typecast to {@code tClass}. s/A/a/ src/java.base/share/classes/java/security/PEMDecoder.java line 266: > 264: */ > 265: public <S extends DEREncodable> S decode(String string, Class<S> > tClass) { > 266: Objects.requireNonNull(string); Missing an `@throws NullPointerException` in the spec. src/java.base/share/classes/java/security/PEMDecoder.java line 282: > 280: * @param <S> Class type parameter that extends {@code DEREncodable} > 281: * @param is an InputStream containing PEM data. > 282: * @param tClass the returned object class that implementing s/implementing/implements/ src/java.base/share/classes/java/security/PEMDecoder.java line 284: > 282: * @param tClass the returned object class that implementing > 283: * {@code DEREncodable}. > 284: * @return A {@code DEREncodable} typecast to {@code tClass} s/A/a/ src/java.base/share/classes/java/security/PEMDecoder.java line 294: > 292: public <S extends DEREncodable> S decode(InputStream is, Class<S> > tClass) > 293: throws IOException { > 294: Objects.requireNonNull(is); Missing an `@throws NullPointerException` in the spec. src/java.base/share/classes/java/security/PEMDecoder.java line 359: > 357: } > 358: return KeyFactory.getInstance(algorithm, factory); > 359: } catch(GeneralSecurityException e){ Nit, add space after `catch` and before `{`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049174941 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049175523 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049177250 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049181342 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049181631 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049187792 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049182807 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049183464 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049156935 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049122348 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049126766 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049134047 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049166538 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049167175 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049170008 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049168852 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049169282 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049170336 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049145705
