On Tue, 6 May 2025 20:56:32 GMT, Weijun Wang <wei...@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 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 > > 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". The paragraph focus is about EKPI, not the encoding methods. I can just remove the references to the methods. > 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>`. Sure on the #8, but I don't think the enclosing is appropriate since it's not a class or field. It's just a format. Like I don't enclose "PEM" > 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. I think specifying the fields that are encoded makes it clear what is not in 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. It was a review comment back in early `24. I don't know who asked for the change > 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`. ok > 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? ok > 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`. yes > 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`. string it is > 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. yes > src/java.base/share/classes/java/security/PEMEncoder.java line 141: > >> 139: */ >> 140: public String encodeToString(DEREncodable de) { >> 141: Objects.requireNonNull(de); > > Do you need to check if `getFormat` of the key is "PKCS#8" or "X.509" before > passing the encoding to `buildKey`? For example, we actually allows RSA key > having "PKCS#1" format and ML-KEM/ML-DSA allows keys in "RAW" format. The format is not checked. > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 374: > >> 372: SecretKeyFactory factory; >> 373: if (provider == null) { >> 374: factory = SecretKeyFactory.getInstance(algorithm); > > `algorithm` might be null. yep > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 408: > >> 406: public static EncryptedPrivateKeyInfo encryptKey(PrivateKey key, >> 407: char[] password) { >> 408: char[] pass = password.clone(); > > No need to clone password here. It will be cloned into a `PBEKeySpec` later. ok > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 458: > >> 456: } >> 457: >> 458: if (Pem.DEFAULT_ALGO == null || Pem.DEFAULT_ALGO.length() == 0) >> { > > Is this required if `algorithm` is already specified? Yes it should > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 573: > >> 571: * {@code SecretKeyFactory}, and the {@code >> PrivateKey}, >> 572: * {@code KeyFactory}. A {@code null} value will >> use the >> 573: * default provider configuration. > > There is no `SecretKeyFactory` in this method, but there is `Cipher`. Yeah, that needs rewording. > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 577: > >> 575: * @throws InvalidKeyException if an error occurs during parsing of >> the >> 576: * encrypted data or creation of the key object. >> 577: * @throws NullPointerException if {@code key} is null. > > s/key/decryptKey/. ok > src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line > 582: > >> 580: */ >> 581: @PreviewFeature(feature = PreviewFeature.Feature.PEM_API) >> 582: public PrivateKey getKey(Key decryptKey, Provider provider) > > This method goes one step further than the existing `getKeySpec(dk, p)`. It > should only throw more types of exception than the existing one. Now you put > everything into a `InvalidKeyException`. Is that good? It's a questions of how many exceptions we really need to throw for a method that other than the provider, takes a Key object the user can't modify. I suspect I kept InvalidKeyException for consistency with the getKeySpec methods. The way I've been doing these newer APIs, I should throw an IllegalArgumentException as they are all a result of bad arguments. But my least favorite solution is throwing all 3 checked exceptions ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080475015 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080493004 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080501357 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080542968 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080607764 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2080607872 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081995021 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2081996525 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082000919 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082024183 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082569786 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082583428 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082590414 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082607937 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082612879 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2082720137
