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 for the `PEMDecoder` API. src/java.base/share/classes/java/security/PEMDecoder.java line 60: > 58: * </pre> > 59: * > 60: * A specified return class must implement {@link DEREncodable} and be an The `decode(data, Class)` method already requires the class be a `DEREncodable` otherwise it won't compile. So this is no longer a requirement for the caller and there is no need to say `must implement DEREncodable`. src/java.base/share/classes/java/security/PEMDecoder.java line 85: > 83: * <pre> > 84: * PEMDecoder pd = PEMDecoder.of(); > 85: * PrivateKey priKey = pd.decode(priKeyPEM); `decode` does not return a `PrivateKey`. Either cast or specify the class. src/java.base/share/classes/java/security/PEMDecoder.java line 118: > 116: /** > 117: * Returns an instance of {@code PEMDecoder}. This instance may be > repeatedly used > 118: * to decode different PEM text. Not only repeatable, but also thread safe I assume? Maybe we need to talk about this in the class spec. Immutable does not automatic implies thread-safe, if the immutability is only observed on the API level. src/java.base/share/classes/java/security/PEMDecoder.java line 228: > 226: * <p>This method will read the {@code InputStream} until PEM data is > 227: * found or until the end of the stream. Non-PEM data in the > 228: * {@code InputStream} before the PEM header will be ignored by the > decoder. 1. This is not precise because you might put the leading data into a `PEMRecord`. Also, I think it's very important to point out the pointer position of the input stream after this method is successfully called. Otherwise, the method cannot be reliably used in extracting multiple PEMs from a single stream. Also, I'm not exactly sure what happens when there is nothing more to read. src/java.base/share/classes/java/security/PEMDecoder.java line 384: > 382: /** > 383: * Configures and returns a new {@code PEMDecoder} instance from the > 384: * current instance that will use KeyFactory and CertificateFactory > classes Put `KeyFactory` and `CertificateFactory` into `<code>`. src/java.base/share/classes/java/security/PEMDecoder.java line 391: > 389: * the default provider configuration. > 390: * > 391: * @param provider the Factory provider. s/Factory/factory/ ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2822474346 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078047551 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078051030 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078055947 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078060563 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078066545 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2078067455
