[WebCrypto] fix ml-kem/ml-dsa CryptoKey serialization [chromium/src : main]
Hubert Chao (Gerrit)
Hubert Chao voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hubert Chao
| Commit-Queue | +1 |
2 comments
With the new tests, I don't think we get much from adding a serialization round trip for ML-KEM/ML-DSA to `third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc`, so I left it off. Lemme know if you think we should still add one.
WriteOneByte(kNoParamsAsymmetricKeyTag);
WriteUint32(AlgorithmIdForWireFormat(algorithm.Id()));
WriteUint32(AsymmetricKeyTypeForWireFormat(key.GetType()));
break;@davi...@chromium.org I think it is possible to change Ed25519/X25519 serialization to use the new tag `kNoParamsAsymmetricKeyTag`, but have deserialization code support both the old `kEd25519KeyTag/kX5519KeyTag` and `kNoParamsAsymmetricKeyTag`.
(we'd have to collapse `WebCryptoKeyAlgorithm::CreateEd25519/X25519` into `WebCryptoKeyAlgorithm::CreateWithoutParams`, but that's easy).
I'm not sure its worth it though, as it'd just make the code a bit more complex and we'd have to do work to get rid of the old tags, which doesn't seem like a good use of time.
thoughts?
Related details
- David Benjamin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Benjamin
| Code-Review | +1 |
2 comments
case kWebCryptoAlgorithmIdMlKem1024: {Optional: I think this new tag you added *also* works for symmetric, no-param algorithms. It's 4 more bytes than needed because `AsymmetricKeyTypeForWireFormat` is redundant but, if it's wrong, logic like this will flag it:
https://source.chromium.org/chromium/chromium/src/+/main:components/webcrypto/algorithms/chacha20_poly1305.cc;l=211
That means, if we want to reduce the number of lines we have to touch when we add a new algorithm, we *could* do something like:
- Rename this to `kNoParamsWithKeyTypeTag` or something.
- Special case the existing HKDF, PBKDF2, ChaCha20-Poly1305 symmetric algorithms to use `kNoParamsTag`
- Leave all these new ones to the `default` case
Then the story will be that *all* no paramless algorithms use this generic codepath and don't need new lines. The exceptions are Ed25519, X25519, HKDF, PBKDF, and ChaCha20-Poly1305, which use other tags for historical reasons.
I.e. we only special case the old stuff and leave the new stuff straightforward.
WDYT? I'm fine with either. Mentioning this in case you think this would be better. It means, e.g., X-Wing doesn't have to touch this function.
WriteOneByte(kNoParamsAsymmetricKeyTag);
WriteUint32(AlgorithmIdForWireFormat(algorithm.Id()));
WriteUint32(AsymmetricKeyTypeForWireFormat(key.GetType()));
break;@davi...@chromium.org I think it is possible to change Ed25519/X25519 serialization to use the new tag `kNoParamsAsymmetricKeyTag`, but have deserialization code support both the old `kEd25519KeyTag/kX5519KeyTag` and `kNoParamsAsymmetricKeyTag`.
(we'd have to collapse `WebCryptoKeyAlgorithm::CreateEd25519/X25519` into `WebCryptoKeyAlgorithm::CreateWithoutParams`, but that's easy).I'm not sure its worth it though, as it'd just make the code a bit more complex and we'd have to do work to get rid of the old tags, which doesn't seem like a good use of time.
thoughts?
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
With the new tests, I don't think we get much from adding a serialization round trip for ML-KEM/ML-DSA to `third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc`, so I left it off. Lemme know if you think we should still add one.
Yeah, I'm also inclined to say the WPT is good enough. Looking at RoundTripCryptoKeyX25519, ISTM the differences are:
- It checks we preserve the extractable bit
- It checks we preserve the usages bit
I guess we could add that to the WPTs if we're concerned?
Hubert Chao (Gerrit)
Hubert Chao added 2 comments![]( "Open in Gerrit")
David BenjaminWith the new tests, I don't think we get much from adding a serialization round trip for ML-KEM/ML-DSA to `third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules_test.cc`, so I left it off. Lemme know if you think we should still add one.
Yeah, I'm also inclined to say the WPT is good enough. Looking at RoundTripCryptoKeyX25519, ISTM the differences are:
- It checks we preserve the extractable bit
- It checks we preserve the usages bit
I guess we could add that to the WPTs if we're concerned?
I'm not super concerned about those; preserving the usages bit is something we could add without too much effort.
extractables bit might be harder; think I might have an idea but I can make that another CL (along with the usages bit)
Optional: I think this new tag you added *also* works for symmetric, no-param algorithms. It's 4 more bytes than needed because `AsymmetricKeyTypeForWireFormat` is redundant but, if it's wrong, logic like this will flag it:
https://source.chromium.org/chromium/chromium/src/+/main:components/webcrypto/algorithms/chacha20_poly1305.cc;l=211That means, if we want to reduce the number of lines we have to touch when we add a new algorithm, we *could* do something like:
- Rename this to `kNoParamsWithKeyTypeTag` or something.
- Special case the existing HKDF, PBKDF2, ChaCha20-Poly1305 symmetric algorithms to use `kNoParamsTag`
- Leave all these new ones to the `default` case
Then the story will be that *all* no paramless algorithms use this generic codepath and don't need new lines. The exceptions are Ed25519, X25519, HKDF, PBKDF, and ChaCha20-Poly1305, which use other tags for historical reasons.
I.e. we only special case the old stuff and leave the new stuff straightforward.
WDYT? I'm fine with either. Mentioning this in case you think this would be better. It means, e.g., X-Wing doesn't have to touch this function.
I like the idea of making future no-key-param webcrypto algorithms (hopefully _all_ future webcrypto algorithms) not have to touch this code.
Done; renamed a few things to make the naming make more sense.
(I don't think this should break anything because as you said, the checks in DeserializeKeyForClone should catch it).
oh also, because ChaCha is still not launched or even OTed, I changed ChaCha to use the new tag.
Related details
- David Benjamin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin voted and added 3 comments![]( "Open in Gerrit")
Votes added by David Benjamin
| Code-Review | +1 |
3 comments
NOTREACHED() << "Unknown asymmetric key type " << key_type;```suggestion
NOTREACHED() << "Unknown key type " << key_type;
```
DCHECK(WebCryptoAlgorithm::IsKdf(algorithm.Id()));Think this DCHECK is pretty useless now. 😊
kSecretKeyType = 3,Ohhhhh, *that's* why it worked that way. I missed that the key type encoding didn't even have an option for secret keys. Okay then!
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 2 comments![]( "Open in Gerrit")
NOTREACHED() << "Unknown asymmetric key type " << key_type;```suggestion
NOTREACHED() << "Unknown key type " << key_type;
```
missed that. done.
Think this DCHECK is pretty useless now. 😊
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
// params for the key should use the default case.I'm just curious: this says "fix the serialization"; was anything potentially serialized with the broken serialization, and do we break those things from deserializing successfully?
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
// params for the key should use the default case.I'm just curious: this says "fix the serialization"; was anything potentially serialized with the broken serialization, and do we break those things from deserializing successfully?
Nope. Any attempt to serialize a ML-DSA/ML-KEM key before this fix would have resulted in an unusable key.
You can see this from the WPT results on the previous CL (crrev.com/c/7842729)
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hubert Chao (Gerrit)
Hubert Chao voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[WebCrypto] fix ml-kem/ml-dsa CryptoKey serialization
Add a new tag for key serialization (kNoParamsWithKeyTypeKeyTag) so that
no code modification is needed for serialization of new algorithms with
no key parameters. Use this to fix ML-KEM and ML-DSA key
serialization/deserialization, and change ChaCha20-Poly1305
serialization to use the new tag as well.
It would be nice to get rid of the Ed25519/X25519 specific
CryptoKeySubTags, but removing those would break currently serialized
Ed25519/X25519 tags.
- M third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_deserializer_for_modules.cc
- M third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
- M third_party/blink/renderer/bindings/modules/v8/serialization/web_crypto_sub_tags.h
- M third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/serialization/mldsa.tentative.https.window-expected.txt
- M third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/serialization/mlkem.tentative.https.window-expected.txt
Code-Review: +1 by David Benjamin, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |