[WebCrypto] Add WebCryptoAPI WPT for CryptoKey serialization [chromium/src : main]
David Benjamin (Gerrit)
David Benjamin added 12 comments![]( "Open in Gerrit")
Posting messages was simpler.)localStorage? Seems you could do...
```
let key = await ...;
// Save the key in localStorage.
localStorage.key = key;
// Get it back out of localStorage.
let key2 = localStorage.key;
// Check that key and key2 are the same.
...
```
opener.postMessage("loaded");Nit: Extra indent here
function run_test(algorithmNames, slowTest) {Unused parameter?
function testCryptoKeySerialization(Is this really a serialization test, or a `postMessage` test? I mean Chrome uses the same machinery for both anyway.
const promise =Similar comments as below regarding a mix of `await` and `then`.
window.addEventListener('message', resolve, {once: true});Do you actually need the popup to send you a `postMessage`? Does it work to just wait for the 'load' event on the popup?
t.done();Do you need to call `t.done()` if you're writing a `promise_test`? I thought the point of a promise test was that it used the promise to tell when the test was done.
The documentation suggests not taking a `t` parameter in the callback, etc.
const promise =Instead of a mix of `await` and `then`, why not just write this as:
```
function waitForEvent(obj, ev) {
return new Promise((resolve) => {
obj.addEventListener(ev, resolve, {once: true});
});
}
```
And then...
```
// Wait for the popup to load.
await waitForEvent(window, 'message');
// Pass keys through the popup and back.
popup.postMessage(...);
let evt = await waitForEvent(window, 'message');
const newPublicKeyExported = await crypto.subtle.exportKey(
publicExportFormat, evt.data.publicKey);
...
```
(Don't even need the helper function. You could just write `await new Promise(...)`, but since this is happening a few times here...)
if (algorithmNames && !Array.isArray(algorithmNames)) {Is this ever called with all algorithms? It seems you've split each one into its own test anyway.
Given that, just to reduce the number of files that have to be touched when we add a new algorith, why not just make `run_test` take `testVectors` as input and then, e.g., the ML-DSA-specific bits can live in the ML-DSA file.
(Or is this the pattern that WebCrypto prefers? It's a somewhat confusing pattern.)
var generateKeyAlgorithm = allAlgorithmSpecifiersFor(vector.name)[0];Is it worth iterating over these? Maybe we have a bug that only applies to some key sizes and not others.
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 12 comments![]( "Open in Gerrit")
localStorage? Seems you could do...
```
let key = await ...;
// Save the key in localStorage.
localStorage.key = key;
// Get it back out of localStorage.
let key2 = localStorage.key;
// Check that key and key2 are the same.
...
```
doesn't work because localstorage only takes strings.
canonical way to store CryptoKeys in localstore is to use exportKey, which won't test the serialization code.
opener.postMessage("loaded");Hubert ChaoNit: Extra indent here
fixed.
function run_test(algorithmNames, slowTest) {Hubert ChaoUnused parameter?
oops, removed.
function testCryptoKeySerialization(Is this really a serialization test, or a `postMessage` test? I mean Chrome uses the same machinery for both anyway.
Its a test of serialization but using `postMessage` as that's the easiest way I know of to test serialization. Is there a more direct way of testing this that is also simpler?
Similar comments as below regarding a mix of `await` and `then`.
Acknowledged
window.addEventListener('message', resolve, {once: true});Do you actually need the popup to send you a `postMessage`? Does it work to just wait for the 'load' event on the popup?
I tried this without waiting for an event from the popup, race conditions failed me.
What do you mean by `load` event on the popup? Isn't that equivalent to saying
```
window.addEventListener('load', () => opener.postMessage("loaded"), {once: true});
```
in the popup window? is there another way to detect from this side whether the popup has loaded?
Do you need to call `t.done()` if you're writing a `promise_test`? I thought the point of a promise test was that it used the promise to tell when the test was done.
The documentation suggests not taking a `t` parameter in the callback, etc.
removed.
const promise =Instead of a mix of `await` and `then`, why not just write this as:
```
function waitForEvent(obj, ev) {
return new Promise((resolve) => {
obj.addEventListener(ev, resolve, {once: true});
});
}
```And then...
```
// Wait for the popup to load.
await waitForEvent(window, 'message');
// Pass keys through the popup and back.
popup.postMessage(...);
let evt = await waitForEvent(window, 'message');
const newPublicKeyExported = await crypto.subtle.exportKey(
publicExportFormat, evt.data.publicKey);
...
```(Don't even need the helper function. You could just write `await new Promise(...)`, but since this is happening a few times here...)
done.
though I'm not sure I understand why these tests work now without returning a promise? (I think this needs to stay a promise test because `test()` would mean synchronous (which doesn't work with `await`) and we don't want `async_test` because of the event listeners on the window.
(i still don't fully grok promises in JS)
// Wait for window to loadHubert Chao(Ditto.)
Acknowledged
if (algorithmNames && !Array.isArray(algorithmNames)) {Is this ever called with all algorithms? It seems you've split each one into its own test anyway.
Given that, just to reduce the number of files that have to be touched when we add a new algorith, why not just make `run_test` take `testVectors` as input and then, e.g., the ML-DSA-specific bits can live in the ML-DSA file.
(Or is this the pattern that WebCrypto prefers? It's a somewhat confusing pattern.)
I see this in a few places in WebCryptoAPI WPTs:
https://github.com/web-platform-tests/wpt/blob/master/WebCryptoAPI/generateKey/successes.js
but I also see examples where tests in other files have the vectors in a different file, and have test files grouped by algorithm family (see https://github.com/web-platform-tests/wpt/tree/master/WebCryptoAPI/encrypt_decrypt as an example)
moved to the latter; having the extra vector files seemed a bit overkill though so didn't do that part. lemme know what you think?
var generateKeyAlgorithm = allAlgorithmSpecifiersFor(vector.name)[0];Is it worth iterating over these? Maybe we have a bug that only applies to some key sizes and not others.
how likely is that to happen? I figured it wasn't that likely and was just creating extra busy work so just grabbed the first one; if you think this is worth testing I can iterate over all of them easily enough.
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 7 comments![]( "Open in Gerrit")
vectors.forEach(async function(vector) {This function isn't `async`, is it?
function testCryptoKeySerialization(Hubert ChaoIs this really a serialization test, or a `postMessage` test? I mean Chrome uses the same machinery for both anyway.
Its a test of serialization but using `postMessage` as that's the easiest way I know of to test serialization. Is there a more direct way of testing this that is also simpler?
I thought there was with localStorage, but apparently not so ignore me. 😊
window.addEventListener('message', resolve, {once: true});Hubert ChaoDo you actually need the popup to send you a `postMessage`? Does it work to just wait for the 'load' event on the popup?
I tried this without waiting for an event from the popup, race conditions failed me.
What do you mean by `load` event on the popup? Isn't that equivalent to saying
```
window.addEventListener('load', () => opener.postMessage("loaded"), {once: true});
```in the popup window? is there another way to detect from this side whether the popup has loaded?
I meant that, even without the popup calling `opener.postMessage("loaded")`, the `popup` object already signals the `load` event when it has loaded. Or at least I think it does?
If that's right, you should be able to delete `opener.postMessage("loaded")` and then replace the first `await waitForEvent(window, 'message')` with `await waitForEvent(popup, "load")`.
if (algorithmNames && !Array.isArray(algorithmNames)) {Hubert ChaoIs this ever called with all algorithms? It seems you've split each one into its own test anyway.
Given that, just to reduce the number of files that have to be touched when we add a new algorith, why not just make `run_test` take `testVectors` as input and then, e.g., the ML-DSA-specific bits can live in the ML-DSA file.
(Or is this the pattern that WebCrypto prefers? It's a somewhat confusing pattern.)
I see this in a few places in WebCryptoAPI WPTs:
https://github.com/web-platform-tests/wpt/blob/master/WebCryptoAPI/generateKey/successes.js
but I also see examples where tests in other files have the vectors in a different file, and have test files grouped by algorithm family (see https://github.com/web-platform-tests/wpt/tree/master/WebCryptoAPI/encrypt_decrypt as an example)
moved to the latter; having the extra vector files seemed a bit overkill though so didn't do that part. lemme know what you think?
Agreed the separate files seems unnecessary.
var generateKeyAlgorithm = allAlgorithmSpecifiersFor(vector.name)[0];Hubert ChaoIs it worth iterating over these? Maybe we have a bug that only applies to some key sizes and not others.
how likely is that to happen? I figured it wasn't that likely and was just creating extra busy work so just grabbed the first one; if you think this is worth testing I can iterate over all of them easily enough.
I mean, it costs us nothing and seems prudent? That also avoids us accidentally losing test coverage if, say, the first option on the list is a curve we happen not to implement.
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 6 comments![]( "Open in Gerrit")
This function isn't `async`, is it?
oops leftover from when generateKey was outside of testSerialization
window.addEventListener('message', resolve, {once: true});Hubert ChaoDo you actually need the popup to send you a `postMessage`? Does it work to just wait for the 'load' event on the popup?
David BenjaminI tried this without waiting for an event from the popup, race conditions failed me.
What do you mean by `load` event on the popup? Isn't that equivalent to saying
```
window.addEventListener('load', () => opener.postMessage("loaded"), {once: true});
```in the popup window? is there another way to detect from this side whether the popup has loaded?
I meant that, even without the popup calling `opener.postMessage("loaded")`, the `popup` object already signals the `load` event when it has loaded. Or at least I think it does?
If that's right, you should be able to delete `opener.postMessage("loaded")` and then replace the first `await waitForEvent(window, 'message')` with `await waitForEvent(popup, "load")`.
oh i didn't think to register the listener from the main window.
yep that works.
const promise =Hubert ChaoInstead of a mix of `await` and `then`, why not just write this as:
```
function waitForEvent(obj, ev) {
return new Promise((resolve) => {
obj.addEventListener(ev, resolve, {once: true});
});
}
```And then...
```
// Wait for the popup to load.
await waitForEvent(window, 'message');
// Pass keys through the popup and back.
popup.postMessage(...);
let evt = await waitForEvent(window, 'message');
const newPublicKeyExported = await crypto.subtle.exportKey(
publicExportFormat, evt.data.publicKey);
...
```(Don't even need the helper function. You could just write `await new Promise(...)`, but since this is happening a few times here...)
done.
though I'm not sure I understand why these tests work now without returning a promise? (I think this needs to stay a promise test because `test()` would mean synchronous (which doesn't work with `await`) and we don't want `async_test` because of the event listeners on the window.
(i still don't fully grok promises in JS)
Acknowledged
var generateKeyAlgorithm = allAlgorithmSpecifiersFor(vector.name)[0];Hubert ChaoIs it worth iterating over these? Maybe we have a bug that only applies to some key sizes and not others.
David Benjaminhow likely is that to happen? I figured it wasn't that likely and was just creating extra busy work so just grabbed the first one; if you think this is worth testing I can iterate over all of them easily enough.
I mean, it costs us nothing and seems prudent? That also avoids us accidentally losing test coverage if, say, the first option on the list is a curve we happen not to implement.
That also avoids us accidentally losing test coverage if, say, the first option on the list is a curve we happen not to implement.
- David Benjamin
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 added 1 comment![]( "Open in Gerrit")
@dch...@chromium.org for third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
(note, change in that file only prevents the `DCHECK` from crashing chrome when we try to serialize ML-KEM/ML-DSA keys. actual fix to make the serialization work is in crrev.com/c/7846315)
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 1 comment![]( "Open in Gerrit")
Votes added by David Benjamin
| Code-Review | +1 |
1 comment
const promise =Hubert ChaoInstead of a mix of `await` and `then`, why not just write this as:
```
function waitForEvent(obj, ev) {
return new Promise((resolve) => {
obj.addEventListener(ev, resolve, {once: true});
});
}
```And then...
```
// Wait for the popup to load.
await waitForEvent(window, 'message');
// Pass keys through the popup and back.
popup.postMessage(...);
let evt = await waitForEvent(window, 'message');
const newPublicKeyExported = await crypto.subtle.exportKey(
publicExportFormat, evt.data.publicKey);
...
```(Don't even need the helper function. You could just write `await new Promise(...)`, but since this is happening a few times here...)
Hubert Chaodone.
though I'm not sure I understand why these tests work now without returning a promise? (I think this needs to stay a promise test because `test()` would mean synchronous (which doesn't work with `await`) and we don't want `async_test` because of the event listeners on the window.
(i still don't fully grok promises in JS)
Acknowledged
though I'm not sure I understand why these tests work now without returning a promise?
Every `async function` returns a promise. `async function` is a (very very involved) syntax sugar for promises, with `await` returning into a `then` call on the promise and all subsequent parts of the function going into the `then` call.
Related details
- Hubert Chao
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 1 comment![]( "Open in Gerrit")
WebCryptoAlgorithm::IsMlKem(algorithm.Id()) ||Copying this comment from @hc...@chromium.org from another thread, so it's more visible.
(note, change in that file only prevents the DCHECK from crashing chrome when we try to serialize ML-KEM/ML-DSA keys. actual fix to make the serialization work is in crrev.com/c/7846315)
- Daniel Cheng
- Hubert Chao
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
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59924.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Hubert Chao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar khattab (Al khattab) (Gerrit)
Omar khattab (Al khattab) added 1 comment![]( "Open in Gerrit")
nice
Related details
- Hubert Chao
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] Add WebCryptoAPI WPT for CryptoKey serialization
We added ML-DSA and ML-KEM with broken CryptoKey serialization, but
this wasn't exposed because there was no WPT test for it. Add a WPT test
for serialization by roundtripping a message with a CryptoKey and
comparing the exportKey results.
(The other main way to do CryptoKey serialization is to use an IndexedDB.
Posting messages was simpler.)
- M third_party/blink/renderer/bindings/modules/v8/serialization/v8_script_value_serializer_for_modules.cc
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-cbc.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-cbc.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-ctr.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-ctr.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-gcm.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-gcm.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-kw.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-kw.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-ocb.tentative.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/aes-ocb.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/chacha20-poly1305.tentative.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/chacha20-poly1305.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/ecdh.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/ecdsa.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/ed25519.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/ed448.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/ed448.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/hmac.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/kmac.tentative.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/kmac.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/mldsa.tentative.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/mldsa.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/mlkem.tentative.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/mlkem.tentative.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/resources/post-page.html
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/rsa-oaep.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/rsa-pss.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/rsassa-pkcs1-v1_5.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/serialization.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/x25519.https.window.js
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/x448.https.window-expected.txt
- A third_party/blink/web_tests/external/wpt/WebCryptoAPI/serialization/x448.https.window.js
- A third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/serialization/chacha20-poly1305.tentative.https.window-expected.txt
- A third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/serialization/mldsa.tentative.https.window-expected.txt
- A third_party/blink/web_tests/virtual/webcrypto-pqc/external/wpt/WebCryptoAPI/serialization/mlkem.tentative.https.window-expected.txt
Code-Review: +1 by Daniel Cheng, +1 by David Benjamin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omar khattab (Al khattab) (Gerrit)
Omar khattab (Al khattab) added 1 comment![]( "Open in Gerrit")
- Daniel Cheng
- David Benjamin
- Hubert Chao
- Kentaro Hara
- Omar khattab (Al khattab)
- Raphael Kubo da Costa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59924
Hubert Chao (Gerrit)
Hubert Chao removed Omar khattab (Al khattab) from this change![]( "Open in Gerrit")
- Daniel Cheng
- David Benjamin
- Hubert Chao
- Kentaro Hara
- Raphael Kubo da Costa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |