On Tue, 5 May 2026 23:44:16 GMT, Anthony Scarpino <[email protected]> wrote:
>> Please review the finalized PEM API at https://openjdk.org/jeps/8376991. The >> most significant changes from the second preview, JEP 524 >> (https://openjdk.org/jeps/524), include: >> >> - The `PEM` class is now an ordinary class rather than a record. It adds >> Binary-encoded content constructors and data is defensively copied. >> - The `DEREncodable` interface is renamed to `BinaryEncodable` to more >> accurately reflect the binary data stored in PEM text. >> - In `EncryptedPrivateKeyInfo`, the `encrypt` methods now accept >> `BinaryEncodable`, and the `getKey()` and `getKeyPair()` methods no longer >> include a `Provider` parameter. >> - A new `CryptoException` class indicates failures in cryptographic >> processing at runtime. >> >> thanks >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > comments and String/byte[] change src/java.base/share/classes/java/security/PEM.java line 79: > 77: private final String type; > 78: private final byte[] content; > 79: private byte[] leadingData; It's a little pity this is not `final`. src/java.base/share/classes/java/security/PEM.java line 139: > 137: leadingData, "leadingData cannot be null").clone(); > 138: final var l = this.leadingData; > 139: CleanerFactory.cleaner().register(this, () -> KeyUtil.clear(l)); Do we need to clear `l`? src/java.base/share/classes/java/security/PEM.java line 213: > 211: /** > 212: * Returns a PEM string representation of this object, using {@code > type} > 213: * for the header and footer lines and {@code content} for the > Base64 body. Do you want to mention again `leadingData` is not included in the output? src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 388: > 386: } finally { > 387: KeyUtil.destroySecretKeys(sk); > 388: KeyUtil.clear(passwd, encoding); Some day we should combine these two methods into one. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 618: > 616: } finally { > 617: KeyUtil.clear(data); > 618: } Hmm, cannot use `KeyUtil.clear(data, function)` because of checked exception. Wonder if there is way to cover it. src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 765: > 763: d.getClass().getName() + " not supported by this > method"); > 764: }; > 765: } catch (NullPointerException e) { Is this about private or public key in a key pair could be null? How about moving to `case KeyPair`? src/java.base/share/classes/sun/security/util/KeyUtil.java line 574: > 572: public interface ByteArrayOp<T> { > 573: T apply(byte[] bytes); > 574: } Why define a new interface? Isn't `Function<byte[],T>` enough? test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetKey.java line 83: > 81: } > 82: > 83: // Test getKey(key) provider null Instead of "provider null" you might want to say "no provider". test/jdk/javax/crypto/EncryptedPrivateKeyInfo/GetKey.java line 90: > 88: } > 89: > 90: // Test getKey(key, provider) with provider No provider. Just wonder: is the code below still necessary? I know it's about another key but since we have no provider now... ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204746692 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204731251 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204757913 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204613970 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204675984 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204655575 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204715092 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204709836 PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3204706025
