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 src/java.base/share/classes/java/security/spec/EncodedKeySpec.java line 63: > 61: * This constructor extracts the algorithm name from the encoded > bytes, > 62: * which may be an OID if no standard algorithm name is defined. If > the > 63: * algorithm name cannot be extracted, it is set to null. Hmm, I think this is leaking details about DER encoding into this abstract class which does not make any assumptions about the type of encoding used. Have you considered only parsing the encoding in the `X509EncodedKeySpec` and `PKCS8EncodedKeySpec` subclasses which are DER specific? src/java.base/share/classes/java/security/spec/PKCS8EncodedKeySpec.java line 41: > 39: * privateKeyAlgorithm PrivateKeyAlgorithmIdentifier, > 40: * privateKey PrivateKey, > 41: * attributes [0] IMPLICIT Attributes OPTIONAL, IMPLICIT has been removed in RFC 5958. Also to be consistent with 5958, I think you should change `PrivateKeyInfo` to `OneAsymmetricKey` and add an addtional line with `PrivateKeyInfo ::= OneAsymmetricKey`. src/java.base/share/classes/sun/security/rsa/RSAKeyFactory.java line 345: > 343: } else { > 344: throw new InvalidKeySpecException("Only RSAPublicKeySpec " > 345: + "and X509EncodedKeySpec supported for RSA public > keys"); Update exception message to `keySpec.getClass().getName() + " not supported."` as you did in `EdDSAKeyFactory`. src/java.base/share/conf/security/java.security line 1557: > 1555: # > 1556: # This property defines default Public-based Encryption algorithm for > 1557: # java.security.PEMEncoder is configured for encryption with > `withEncryption()`. Suggested rewording: This property defines the default password-based encryption algorithm for java.security.PEMEncoder when configured for encryption with the withEncryption method. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2071707250 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2071753380 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2071746127 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2071660229
