RFR: 8298420: PEM API: Implementation (Preview) [v13]
Sean Mullan
mullan at openjdk.org
Thu Apr 17 15:26:02 UTC 2025
On Thu, 13 Mar 2025 01:15:03 GMT, Anthony Scarpino <ascarpino at 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
More information about the security-dev
mailing list