On Thu, 7 May 2026 21:13:34 GMT, Weijun Wang <[email protected]> wrote:
>> 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 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`?
`l` is pointing to the array. They are the same.
> src/java.base/share/classes/java/security/PEM.java line 169:
>
>> 167: this.type = type;
>> 168: final var c = content;
>> 169: CleanerFactory.cleaner().register(this, () -> KeyUtil.clear(c));
>
> Does it make sense to only clean PRIVATE KEY bytes?
Types that don't have cryptographic representations may have sensitive data
that should be cleaned. Doing `type` checking maybe more expensive than
cleaning everything.
> 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?
I think it's clear what is in the return value and doesn't not need to specify
what is not.
> src/java.base/share/classes/java/security/PEMDecoder.java line 199:
>
>> 197:
>> 198: try {
>> 199: p8key = new PKCS8Key(pem.decode());
>
> Should we clear `pem.decode()`?
Yes, I need to bring back a new form of `encoding`.
> 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.
I thought about having `clear()` call `destroySecretKeys()`, but didn't want to
inconsistency in other parts of the code.
> 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.
Wrapping the `Exception` from the `function` into a `RuntimeException` would
work. However that is neither what a general purpose cleaning method should
do, nor what we want in this case. Unwrapping to throw the `cause()`, would
look ugly code-wise
> 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`?
`PrivateKey.getEncoded()` can also return null.
> 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?
yes, that should work too.
> src/java.base/share/classes/sun/security/util/Pem.java line 472:
>
>> 470: * @param provider KeyFactory provider
>> 471: */
>> 472: public static BinaryEncodable toPKCS8Encodable(byte[] encoded,
>> boolean pair,
>
> Checked its usages, and `pair` seems always true.
ok
> 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".
I'll just remove the provider comment, it's not relevant
> 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...
I think you're correct that the whole test case isn't needed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209722909
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209752715
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209802254
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209834170
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209868676
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209922139
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209955338
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3209978235
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3210051218
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3220289615
PR Review Comment: https://git.openjdk.org/jdk/pull/29640#discussion_r3220344998
