RFR: 8298420: PEM API: Implementation (Preview) [v12]
Sean Mullan
mullan at openjdk.org
Thu Apr 17 15:26:05 UTC 2025
On Thu, 12 Dec 2024 19:59:05 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 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
More information about the security-dev
mailing list