On Mon, 21 Oct 2024 19:52:36 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 incrementally with one > additional commit since the last revision: > > apparently <p> can't be before a @implNote.. Who know. For `PEMEncoder`. Also, do you want to update the `PKCS8EncodedKeySpec` class with a new ASN.1 grammar and a link to version 2.0? src/java.base/share/classes/java/security/PEMEncoder.java line 63: > 61: * by passing an {@link EncryptedPrivateKeyInfo} object into the encode > methods. > 62: * <p> > 63: * PKCS8 v2.0 allows OneAsymmetric encoding, which is a private and public Add a link to PKCS 8 2.0? src/java.base/share/classes/java/security/PEMEncoder.java line 74: > 72: * @apiNote > 73: * Here is an example of encoding a PrivateKey object: > 74: * <pre> Change to code snippet. src/java.base/share/classes/java/security/PEMEncoder.java line 143: > 141: * DEREncodable. > 142: * @return PEM encoding in a String > 143: * @throws IllegalArgumentException when the passed object returns a > null What does "returns a null binary encoding" mean? There is no other method talking about this. I think we can just say "if configured for encryption but object does not support" since this looks like the only reason. Also, how about `IllegalStateException`? src/java.base/share/classes/java/security/PEMEncoder.java line 177: > 175: PEMRecord.ENCRYPTED_PRIVATE_KEY, > epki.getEncoded())); > 176: } catch (IOException e) { > 177: throw new SecurityException(e); Do you really want to use `SecurityException`? This only happens when the `AlgorithmParameters` inside EPKI is not initialized. I would say this is a very good candidate for an `IllegalStateException`. src/java.base/share/classes/java/security/PEMEncoder.java line 240: > 238: * > 239: * @param password sets the encryption password. The array is > cloned and > 240: * stored in the new instance. Can password be empty? I vaguely remember some algorithms might not like it, or, is it just that `SecretKeySpec` does not like an empty key? ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2406520187 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823529997 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823530133 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823531557 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823532621 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r1823533634
