On Thu, 17 Apr 2025 15:14:33 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> 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/ ok > 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/ ok > 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"? ok > 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/ I was mistakenly documenting an internal method > 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/ removed > 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 ..." I changed "calling" to "configured". Using your full text means the "new instance" part needs to be said elsewhere. > 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/ ok > 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. ok > 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. It has been the intent to avoid checked exceptions with these methods and these methods follow the same behavior and exception as `Base64.Decoder.decode()` > 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/ ok > 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. Added in an intermediate changeset > 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. Added in an intermediate changeset > 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/ ok > 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/ ok > 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. Added in an intermediate changeset > 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/ ok > 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/ ok > 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. Added in the recent update > 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 `{`. yes ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758001 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758108 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052758230 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052679240 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052679623 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052691444 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052692601 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052692794 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049729802 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052698263 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052699015 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052699478 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700392 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700498 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700709 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700833 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052700940 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2049721541 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2052756431
