On Wed, 30 Jul 2025 15:45:50 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> The private key encoding formats of ML-KEM and ML-DSA are updated to match >> the latest IETF drafts at: >> https://datatracker.ietf.org/doc/html/draft-ietf-lamps-dilithium-certificates-11 >> and >> https://datatracker.ietf.org/doc/html/draft-ietf-lamps-kyber-certificates-10. >> New security/system properties are introduced to determine which CHOICE a >> private key is encoded when a new key pair is generated or when >> `KeyFactory::translateKey` is called. >> >> By default, the choice is "seed". >> >> Both the encoding and the expanded format are stored inside a >> `NamedPKCS8Key` now. When loading from a PKCS #8 key, the expanded format is >> calculated from the input if it's seed only. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > combine security properties description; remove one test If my understanding is correct, we have now up to three byte[] in the NamedPKCS8Key class: * the encoded format * the raw expanded format * the raw seed With `getRawBytes()` either returning the encoded or the expanded format, depending on whether the encoded format and the expanded key are the same. `getEncoded()` on the other hand always wraps the raw key in its ASN.1 structure. Shouldn't `engineTranslateKey()` rely on the latter, then? src/java.base/share/classes/com/sun/crypto/provider/ML_KEM_Impls.java line 132: > 130: if (nk instanceof NamedPKCS8Key npk) { > 131: var type = KeyChoices.getPreferred("mlkem"); > 132: if (KeyChoices.typeOfChoice(npk.getRawBytes()) != type) { > ``` > /// 1. If only `privKeyMaterial` is present, it's also the expanded format. > /// 2. If both `privKeyMaterial` and `expanded` are available, > `privKeyMaterial` > /// is the encoding format, and `expanded` is the expanded format. > ``` This in mind, the result of `getRawBytes()` differs. In the first case, it is in fact raw bytes, as the method name suggests. (See also usage of `privKeyMaterial` in `PKCS8Key.generateEncoding()`). Therefore, the ASN.1 envelope may be missing, causing java.security.InvalidKeyException: Wrong tag: -39 at java.base/sun.security.util.KeyChoices.typeOfChoice(KeyChoices.java:144) at java.base/com.sun.crypto.provider.ML_KEM_Impls$KF.engineTranslateKey(ML_KEM_Impls.java:132) at java.base/java.security.KeyFactory.translateKey(KeyFactory.java:475) Isn't `getEncoded()` the safer bet? Suggestion: if (KeyChoices.typeOfChoice(npk.getEncoded()) != type) { src/java.base/share/classes/sun/security/provider/ML_DSA_Impls.java line 157: > 155: if (nk instanceof NamedPKCS8Key npk) { > 156: var type = KeyChoices.getPreferred("mldsa"); > 157: if (KeyChoices.typeOfChoice(npk.getRawBytes()) != type) { See corresponding comment above for ML-KEM ------------- Changes requested by overheadhun...@github.com (no known OpenJDK username). PR Review: https://git.openjdk.org/jdk/pull/24969#pullrequestreview-3083718545 PR Review Comment: https://git.openjdk.org/jdk/pull/24969#discussion_r2251165104 PR Review Comment: https://git.openjdk.org/jdk/pull/24969#discussion_r2251166383
