On Fri, 2 May 2025 06:09:52 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 66 commits: > > - major code review comments update > - Merge branch 'master' into pem > - Merge branch 'master' into pem > - javadoc updates > - code review comments > - merge with master > - better comment and remove commented out code > - Merge branch 'master' into pem > - Merge branch 'pem-merge' into pem > - merge > - ... and 56 more: https://git.openjdk.org/jdk/compare/e2ae50d8...0c540327 Comments on the `PEMEncoder` API. src/java.base/share/classes/java/security/PEMEncoder.java line 48: > 46: /** > 47: * PEMEncoder is an immutable class for Privacy-Enhanced Mail (PEM) > 48: * data. PEM is a textual encoding used to store and transfer security "An immutable class .. for ... data" sounds like it is the data itself. What about "for encoding data into...". The first word `PEMEncoder` should be in `<code>`. src/java.base/share/classes/java/security/PEMEncoder.java line 54: > 52: * and footer. > 53: * > 54: * <p> Encoding may be performed on Java Cryptographic Extension (JCE) > objects Is "JCE objects" a formal term? We used to say "JCA and JCE". How do we call them now? src/java.base/share/classes/java/security/PEMEncoder.java line 59: > 57: * {@linkplain X509EncodedKeySpec X509} formats. > 58: * > 59: * <p> Encrypted private key PEM data can be built by calling the encode > methods I understand "encode methods" here mean `encode` and `encodeToString`, but at this early place in the specification no one knows about those methods yet. Does it make sense to append several sentences at the end of the previous paragraph saying something similar to "call encode to encode, call encodeToString to encode to string". src/java.base/share/classes/java/security/PEMEncoder.java line 63: > 61: * by passing an {@link EncryptedPrivateKeyInfo} object into the encode > methods. > 62: * > 63: * <p> PKCS8 2.0 allows OneAsymmetricKey encoding, which may contain both > private It's "PKCS #8". Enclose `OneAsymmetricKey` in `<code>`. src/java.base/share/classes/java/security/PEMEncoder.java line 70: > 68: * {@linkplain PEMRecord#pem()} with a generated the PEM header and footer > 69: * from {@linkplain PEMRecord#type()}. It will not check the validity of > 70: * the data. Since you mention `PEMRecord` specifically, I'd see the clarification that the `leadingData` there will not be encoded. Otherwise, you cannot guarantee on the encoding. src/java.base/share/classes/java/security/PEMEncoder.java line 75: > 73: * {@link java.nio.charset.StandardCharsets#ISO_8859_1 ISO-8859-1}. > 74: * > 75: * @apiNote Why is this an `apiNote`. Can't we put an example directly into the spec. Also, please add an example on encrypting a private key. src/java.base/share/classes/java/security/PEMEncoder.java line 83: > 81: * > 82: * @see PKCS8EncodedKeySpec > 83: * @see X509EncodedKeySpec I cannot see how these 2 deserve this place. I'd rather link to `PEMRecord` and `PEMDecoder`. src/java.base/share/classes/java/security/PEMEncoder.java line 88: > 86: * RFC 1421: Privacy Enhancement for Internet Electronic Mail > 87: * @spec https://www.rfc-editor.org/info/rfc7468 > 88: * RFC 7468: Textual Encodings of PKIX, PKCS, and CMS Structures You mentioned PKCS #8 2.0. Is it worth adding it here? src/java.base/share/classes/java/security/PEMEncoder.java line 127: > 125: > 126: /** > 127: * Encoded a given {@code DEREncodable} and return the PEM encoding > in a Typo: `Encodes`. src/java.base/share/classes/java/security/PEMEncoder.java line 128: > 126: /** > 127: * Encoded a given {@code DEREncodable} and return the PEM encoding > in a > 128: * String Either `<code>String</code>` or `string`. src/java.base/share/classes/java/security/PEMEncoder.java line 130: > 128: * String > 129: * > 130: * @param de a cryptographic object to be PEM encoded that implements `@param de` for the 2 methods should be the same. ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2819604929 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076258346 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076262106 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076267314 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076270426 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076273132 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076281319 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076282886 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076284962 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076288529 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076291290 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2076299229
