On Thu, 17 Apr 2025 15:51:09 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 two > additional commits since the last revision: > > - javadoc updates > - code review comments Some comments on internal classes. Since each `KeyFactory` now accepts PKCS8 in `generatePublicKey`, I suggest adding some tests on it. Also, add some tests to check if `new PKCS8EncodedKeySpec(data)` and `new X509EncodedKeySpec(data)` are able to detect algorithms from only data. src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 225: > 223: throws GeneralSecurityException { > 224: if (keySpec instanceof X509EncodedKeySpec) { > 225: return new > ECPublicKeyImpl(((X509EncodedKeySpec)keySpec).getEncoded()); We can also use `instanceof` pattern matching here. Same on line 246. src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 157: > 155: } > 156: > 157: public byte[] getArrayS() { Why remove `getArrayS0`? Not worth saving those bytes? src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 191: > 189: DerValue value = data.getDerValue(); > 190: if (value.isContextSpecific((byte) 0)) { > 191: attributes = value.getDataBytes(); // Save DER > sequence At [0], it's `ECDomainParameters`. I don't think it's the same as `attributes` inside `OneAsymmetricKey`. src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 195: > 193: return; > 194: } > 195: value = data.getDerValue(); The original code does not care about whether [0] comes before [1] or if there are duplicates. You modified code ensures only [1] can be after [0] but still allows duplicates (For example, [0], [1], [1]). The `while` on line 188 can be changed to `if` and we ensure `data.available() == 0` at the end. src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 201: > 199: BitArray bitArray = > bits.data.getUnalignedBitString(); > 200: pubKeyEncoded = new X509Key(algid, > 201: bitArray).getEncoded(); We could have 2 copies of the public key, one here and one inside `OneAsymmetricKey`. Do you want to compare them? src/java.base/share/classes/sun/security/util/DerValue.java line 195: > 193: > 194: /** > 195: * Returns true if the CONTEXT SPECIFIC bit is set in the type tag. `iff` was used to mean `if and only if`. It's more precise than a simple `if`. ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2793998438 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060169867 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060231627 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060222850 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060212787 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060201102 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2060236284
