[WebCrypto] ChaCha20-Poly1305 implementation [chromium/src : main]
Hubert Chao (Gerrit)
Hubert Chao added 5 comments![]( "Open in Gerrit")
David Benjamintesting: should i use the test cases in third_party/boringssl/src/crypto/cipher/test/chacha20_poly1305_tests.txt ?
Hubert ChaoThe number of layers in WebCrypto is unfortunate. We have some tests in components/webcrypto/algorithms/*_unittest.cc and we have end-to-end WPTs in third_party/blink/web_tests/external/wpt/WebCryptoAPI/
Since we need to test end-to-end stuff anyway, and this code only exists to be called from Blink (we should move it there), I would say test it there, unless there's really something you can't test from WPT. (But I doubt there is.)
This probably means doing the whole thing in one CL, but, per the CreateWithoutParams question, it seems how you do the Blink serialization impacts how you do this part anyway.
As for test vectors, RFC 8439 has test vectors that were generated independently from our implementation. If you need others, grabbing those is fine. But, for the most part, your goal is less to test edge cases of the BoringSSL implementation and more all the plumbing around it. (Serialization, unexpected algorithms, make sure bad tags are reported as bad tags and good inputs work, etc.)
just did the test in WPT using the RFC test vectors.
Hubert ChaoLink to bug and spec? (Otherwise I have to go searching for it in review. :P)
Done
if (algorithm.ParamsType() != blink::kWebCryptoKeyAlgorithmParamsTypeAes ||David Benjaminis this right? i'm not sure if it is, but I'm not 100% sure what I should be doing in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/platform/web_crypto_key_algorithm_params.h;l=58-64;drc=240825a2c96b5bd0ae6d124919de323054ac6080
(maybe creating a new subclass of WebCryptoKeyAlgorithmParams?)
Hubert ChaoThis does not look right. ChaCha20-Poly1305 is not AES.
AES has parameters because WebCrypto (foolishly) decided to express the AES variant as a key parameter. ChaCha20-Poly1305 does not seem to have parameters (good), so I assume you would use kWebCryptoKeyAlgorithmParamsTypeNone.
Done
kWebCryptoAlgorithmParamsTypeAesGcmParams, // EncryptDavid Benjaminthe parameters are exactly the same; I'm not sure if I should be reusing this, creating a new ParamsType, or reusing this and renaming it to `kWebCryptoAlgorithmParamsTypeAeadParams`?
Also, is this related in any way to work i might have to do in third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc (and the deserializer equivalent)?
Hubert Chaothe parameters are exactly the same; I'm not sure if I should be reusing this, creating a new ParamsType, or reusing this and renaming it to kWebCryptoAlgorithmParamsTypeAeadParams?
See the spec, which renames it:
https://wicg.github.io/webcrypto-modern-algos/#aead-paramsAlso, is this related in any way to work i might have to do in third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc (and the deserializer equivalent)?
That's about the algorithm parameters for the key, while this is about the algorithm parameters for the operation. (WebCrypto is a bad API and lifts too many things into funny "parameter" objects.)
parameters renamed in https://chromium-review.googlesource.com/c/chromium/src/+/7087752
// XXX why does CreateWithoutParams check for IsKdf?
WebCryptoKeyAlgorithm WebCryptoKeyAlgorithm::CreateChaCha20Poly1305(
WebCryptoAlgorithmId id) {
return WebCryptoKeyAlgorithm(id, nullptr);
}David Benjamin@davi...@chromium.org I would like to use CreateWithoutParams(), but i'm not sure why it checks for IsKdf(). Is there something better I can do here than to just create another Create function?
Hubert ChaoI think you actually should call CreateWithoutParams but modify it to accept ChaCha20. What's going on here is that a lot of symmetric key types that need no params get lumped into kNoParamsKeyTag, and I think it's checking for unexpected algorithms:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_deserializer_for_modules.cc;drc=9307c9e84c5288d820304085396016213f449fd8;l=370It happened that only KDFs fell under that case before:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc;drc=a416f377ead3d8c361b97126daf78381344937dd;l=562But there's no reason not to also make ChaCha20-Poly1305 one of those cases.
Done
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 added 3 comments![]( "Open in Gerrit")
(I haven't looked at any other files yet. Will take a look for real tomorrow.)
"XXX",Guessing you meant to change this value?
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")
Guessing you meant to change this value?
oops, yes, removed.
"chrome-...@google.com"Hubert ChaoThis too?
fixed (and the expiry date as well)
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 added 16 comments![]( "Open in Gerrit")
std::string expected_algorithm_name = "ChaCha20Poly1305";Hmm, where did this algorithm name come from? The spec seems to use C20P
https://wicg.github.io/webcrypto-modern-algos/#chacha20-poly1305-operations-import-key
Also, nit, no need to allocate a `std::string` to store this. We can just write `if (jwk_alg != "...")` and compare to a string literal.
I think that came from here, though it seems no one ever drove that spec to completion.
https://datatracker.ietf.org/doc/html/draft-amringer-jose-chacha-02#section-4.1
It is a bit unfortunate that we're putting a thing in the web platform, which cannot change, and there's no clear signal from the JOSE community that they're done with this. Can we file a bug with the WebCrypto spec asking whether C20P is actually ready for use? If not, maybe we hold on that.
std::string algorithm_name = "ChaCha20Poly1305";Ditto about both the wrong algorithm name, and not allocating a `std::string` for it. (The function takes a `std::string_view`.)
WriteSecretKeyJwk(raw_data, algorithm_name, key.Extractable(), key.Usages(),Not for this CL or anything, but if you feel inspired to do some follow-up cleanup... why doesn't this function just return a `std::vector` instead of taking an out-param?? (go/totw/176)
static Status ErrorImportChaCha20Poly1305KeyLength();Gotta say, it is kinda bizarre to have to lump all these one-off statuses in this one file, far from the code that triggers them. But I guess that's how this code is structured. Though just making the Status() constructor public and inlining the one-off ones would... not be wrong. Maybe some cleanup for later.
// TODO(crbug.com/450627018): Fix params for methodsStale TODO?
assert_unreached: importKey failed for ChaCha20-Poly1305 Reached unreachable codeIt's a little odd that the test expectations we're upstreaming say ChaCha20-Poly1305 *isn't* supported, but our local "webcrypto-pqc" ones say they've passed.
I would have expected it to be the other way around. Or is it because ChaCha20-Poly1305 isn't done yet?
var subtle = self.crypto.subtle; // Change to test prefixed implementationsProbably don't need this comment anymore.
});We have way more features in the platform now. Any reason not to just use `async function` for these? I'd also expect that, if the promise fails, we can just rely on WPT to catch it.
And then if we call `promise_test` early enough, it seems we also don't need the `all_promises` thing?
Seems all this could just be:
```
passingVectors.forEach(function(vector) {
promise_test(async function(test) {
vector = await importVectorKey(vector, ['encrypt', 'decrypt']);
let result = await subtle.encrypt(vector.algorithm, vector.key, vector.plaintext);
assert_true(equalBuffers(result, vector.result), 'Should return expected result');
});
});
```
Ditto for most of the others.
Edit: Although see comment below...
vector.key = key;This can also be an `async function`.
```
async function importVectorKey(vector, usages) {
if (vector.key !== null) {
return vector;
}
let key = await subtle.importKey(...);
vector.key = key;
return vector;
}
```
Although this is a little interesting. So, this caching behavior... doesn't exactly work. Because what will happen is we schedule all the imports in parallel, and then they all clobber each other.
There's this thing that maybe we should use instead? Though this does mean we skip all tests if a single test vector can't be imported.
https://web-platform-tests.org/writing-tests/testharness-api.html#setup
Alternatively, I don't think this caching behavior does anything useful. We could just remove it and make the importer just make a new key, and remove the caching:
```
function importVectorKey(vector, usages) {
return subtle.importKey(...);
}
```
This already returns a promise, so we don't need to do this:
```
async function importVectorKey(vector, usages) {
return await subtle.importKey(...);
}
```
Then just change the tests to:
```
passingVectors.forEach(function(vector) {
promise_test(async function(test) {
let key = await importVectorKey(vector, ['encrypt', 'decrypt']);
let result = await subtle.encrypt(vector.algorithm, key, vector.plaintext);
assert_true(equalBuffers(result, vector.result), 'Should return expected result');
});
});
```
That seems much more straightforward.
{name: 'CHACHA20-POLY1305', resultType: CryptoKey, usages: ['encrypt', 'decrypt', 'wrapKey', 'unwrapKey'], mandatoryUsages: []},Are algorithms case-insensitive? It's usually styled ChaCha20-Poly1305 and the spec matches: https://wicg.github.io/webcrypto-modern-algos/#chacha20-poly1305
{name: 'CHACHA20-POLY1305', resultType: CryptoKey, usages: ['encrypt', 'decrypt', 'wrapKey', 'unwrapKey'], mandatoryUsages: []},Ditto
{name: 'CHACHA20-POLY1305', legalUsages: ['encrypt', 'decrypt'], extractable: [true, false], formats: ['raw', 'jwk']},Ditto
"X448", "ChaCha20-Poly1305"]What does this generate? It sounds like it generates the generateKey tests, but in the generateKey tests, it's styled wrong? Or is this something else? Did you end up running the script? Is the script even still used? I can't find a test_successes_ file.
cryptParams = {name: "CHACHA20-POLY1305", iv: new Uint8Array(12)};I guess it's case-insensitive, but is this intentionally not styled in the conventional way?
"CHACHA": {What's this for? Where does the string CHACHA come in?
Edit: Oh, I see. It's because SYMMETRIC is 128-bit instead of 256-bit. Should we maybe rename SYMMETRIC to SYMMETRIC128 and call this SYMMETRIC256 or something?
algorithms, specifically:Feel like this warrants an explanatory comment somewhere that ChaCha20-Poly1305 is not actually PQC. (FWIW the spec is titled "modern" instead of "pqc".)
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 16 comments![]( "Open in Gerrit")
std::string expected_algorithm_name = "ChaCha20Poly1305";Hmm, where did this algorithm name come from? The spec seems to use C20P
https://wicg.github.io/webcrypto-modern-algos/#chacha20-poly1305-operations-import-keyAlso, nit, no need to allocate a `std::string` to store this. We can just write `if (jwk_alg != "...")` and compare to a string literal.
I think that came from here, though it seems no one ever drove that spec to completion.
https://datatracker.ietf.org/doc/html/draft-amringer-jose-chacha-02#section-4.1It is a bit unfortunate that we're putting a thing in the web platform, which cannot change, and there's no clear signal from the JOSE community that they're done with this. Can we file a bug with the WebCrypto spec asking whether C20P is actually ready for use? If not, maybe we hold on that.
I did a copy-paste of the import/export JWK code from the AES implementations; kinda assumed it would be an identical pattern and forgot to check the spec.
Change it to inlined "C20P", will discuss the C20P ready or not part offline.
Ditto about both the wrong algorithm name, and not allocating a `std::string` for it. (The function takes a `std::string_view`.)
Done
WriteSecretKeyJwk(raw_data, algorithm_name, key.Extractable(), key.Usages(),Not for this CL or anything, but if you feel inspired to do some follow-up cleanup... why doesn't this function just return a `std::vector` instead of taking an out-param?? (go/totw/176)
¯\_(ツ)_/¯
i'll maybe poke at it later.
static Status ErrorImportChaCha20Poly1305KeyLength();Gotta say, it is kinda bizarre to have to lump all these one-off statuses in this one file, far from the code that triggers them. But I guess that's how this code is structured. Though just making the Status() constructor public and inlining the one-off ones would... not be wrong. Maybe some cleanup for later.
yeah this file was weird; changing it would be another CL though. (I'd want a bit more experience in this area before doing big refactorings like this)
// TODO(crbug.com/450627018): Fix params for methodsHubert ChaoStale TODO?
ah yep removed.
assert_unreached: importKey failed for ChaCha20-Poly1305 Reached unreachable codeIt's a little odd that the test expectations we're upstreaming say ChaCha20-Poly1305 *isn't* supported, but our local "webcrypto-pqc" ones say they've passed.
I would have expected it to be the other way around. Or is it because ChaCha20-Poly1305 isn't done yet?
These expectation are because we're gating usage of these algorithms on the `WebCryptoPQC` feature flag, which is not on for tests by default (which is what this expectation file is saying).
the expectation files under `third_party/blink/web_tests/virtual/webcrypto-pqc/` (e.g.
`third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/encrypt_decrypt/chacha20_poly1305.https.any-expected.txt`
have the `WebCryptoPQC` feature flag on and so they pass.
var subtle = self.crypto.subtle; // Change to test prefixed implementationsProbably don't need this comment anymore.
removed.
We have way more features in the platform now. Any reason not to just use `async function` for these? I'd also expect that, if the promise fails, we can just rely on WPT to catch it.
And then if we call `promise_test` early enough, it seems we also don't need the `all_promises` thing?
Seems all this could just be:
```
passingVectors.forEach(function(vector) {
promise_test(async function(test) {
vector = await importVectorKey(vector, ['encrypt', 'decrypt']);
let result = await subtle.encrypt(vector.algorithm, vector.key, vector.plaintext);
assert_true(equalBuffers(result, vector.result), 'Should return expected result');
});
});
```Ditto for most of the others.
Edit: Although see comment below...
I copied this code directly from aes.js and modified it to work for chacha; I expect this code predated a lot of these extra features.
Would you be okay if we kept this code as is for now and then had a separate CL trying to simplify this code for both here and aes.js?
vector.key = key;ditto comment above.
{name: 'CHACHA20-POLY1305', resultType: CryptoKey, usages: ['encrypt', 'decrypt', 'wrapKey', 'unwrapKey'], mandatoryUsages: []},Are algorithms case-insensitive? It's usually styled ChaCha20-Poly1305 and the spec matches: https://wicg.github.io/webcrypto-modern-algos/#chacha20-poly1305
Done
{name: 'CHACHA20-POLY1305', resultType: CryptoKey, usages: ['encrypt', 'decrypt', 'wrapKey', 'unwrapKey'], mandatoryUsages: []},Hubert ChaoDitto
Done
{name: 'CHACHA20-POLY1305', legalUsages: ['encrypt', 'decrypt'], extractable: [true, false], formats: ['raw', 'jwk']},Hubert ChaoDitto
Done
"X448", "ChaCha20-Poly1305"]What does this generate? It sounds like it generates the generateKey tests, but in the generateKey tests, it's styled wrong? Or is this something else? Did you end up running the script? Is the script even still used? I can't find a test_successes_ file.
I have no idea; I made this change and then couldn't figure out what it was for. I'll leave this open for whatever third_party/blink reviewer sees this CL, see if they have an idea.
cryptParams = {name: "CHACHA20-POLY1305", iv: new Uint8Array(12)};I guess it's case-insensitive, but is this intentionally not styled in the conventional way?
for some reason I thought I saw in the spec that the algorithm name should be all caps. not sure why. Fixed.
What's this for? Where does the string CHACHA come in?
Edit: Oh, I see. It's because SYMMETRIC is 128-bit instead of 256-bit. Should we maybe rename SYMMETRIC to SYMMETRIC128 and call this SYMMETRIC256 or something?
sure. done.
Feel like this warrants an explanatory comment somewhere that ChaCha20-Poly1305 is not actually PQC. (FWIW the spec is titled "modern" instead of "pqc".)
Yeah, ChaCha20 was always second fiddle on this project but it became easier to start with when ML-DSA and ML-KEM weren't ready to implement yet.
Done.
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 added 4 comments![]( "Open in Gerrit")
When I go look at the upstream repo, it seems someone already added ChaCha20-Poly1305 WPTs, so much of this CL is probably moot. Except that we'll need to evaluate, for ourselves, whether the upstream tests have sufficient coverage. Can you look over that? https://github.com/web-platform-tests/wpt/pull/54417
We'll also need to make sure the tests for ML-KEM and ML-DSA use the seed form and not the "both" forms because we will not be implementing those. We can talk 1:1 about how to do that if you need help there.
assert_unreached: importKey failed for ChaCha20-Poly1305 Reached unreachable codeHubert ChaoIt's a little odd that the test expectations we're upstreaming say ChaCha20-Poly1305 *isn't* supported, but our local "webcrypto-pqc" ones say they've passed.
I would have expected it to be the other way around. Or is it because ChaCha20-Poly1305 isn't done yet?
These expectation are because we're gating usage of these algorithms on the `WebCryptoPQC` feature flag, which is not on for tests by default (which is what this expectation file is saying).
the expectation files under `third_party/blink/web_tests/virtual/webcrypto-pqc/` (e.g.
`third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/encrypt_decrypt/chacha20_poly1305.https.any-expected.txt`
have the `WebCryptoPQC` feature flag on and so they pass.
Right, I understand that _Chromium_ won't pass these tests by default and only when the feature flag is on. But WPT tests are synchronized automatically to upstream WPT for all other implementations. These tests are not just for Chromium.
It seems wrong to upstream test vectors that say you are supposed to not implement this. Rather, I would expect that the *default* expectations to be ones that work, and then Chromium to override the expectations when the flag is *off* with non-upstreamed ones that say they don't work.
I would also expect the tests to have to be named "tentative" given this is not a finished standard yet. See https://web-platform-tests.org/writing-tests/file-names.html
Separate CL's fine. Although it's really not that much code, and will save you an extra round of refreshing test expectations.
"X448", "ChaCha20-Poly1305"]Hubert ChaoWhat does this generate? It sounds like it generates the generateKey tests, but in the generateKey tests, it's styled wrong? Or is this something else? Did you end up running the script? Is the script even still used? I can't find a test_successes_ file.
I have no idea; I made this change and then couldn't figure out what it was for. I'll leave this open for whatever third_party/blink reviewer sees this CL, see if they have an idea.
I suspect the third_party/blink reviewer won't have any particular context here either. This is part of WPTs for a very obscure corner of the platform.
Perhaps poke around git history to find where this file was first added to upstream WPTs and what happened to the files surrounding it? Here's the upstream WPT repo that all this syncs to/from: https://github.com/web-platform-tests/wpt
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. |