Closed
Bug 1243311
Opened 8 years ago
Closed 8 years ago
Add structured cloning tests for CryptoKeys
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ttaubert, Assigned: ttaubert)
Details
Attachments
(1 file, 1 obsolete file)
11.58 KB,
patch
|
rbarnes
:
review+
|
Details | Diff | Splinter Review |
We should have tests to ensure structured cloning of CryptoKey objects works as intended.
Assignee | ||
Comment 1•8 years ago
|
||
DH, ECDH, and ECDSA tests included but commented out until we have PKCS8/SPKI format import/export for those.
Attachment #8712585 -
Flags: review?(rlb)
Comment 2•8 years ago
|
||
Comment on attachment 8712585 [details] [diff] [review] 0001-Bug-1243311-Add-structured-cloning-tests-for-CryptoK.patch Review of attachment 8712585 [details] [diff] [review]: ----------------------------------------------------------------- This is mostly OK, but it's not great to infer equality of keys from equality of results of a crypto operation. It seems like it would be cleaner and more correct to test an import/export round trip instead.) crypto.subtle.importKey(key) .then(clone) .then(exportKey) .then(util.ab2hex) .then(equals) You might even be able to wrap that all in one method, since the only thing changing is inputs to the WebCrypto methods.
Attachment #8712585 -
Flags: review?(rlb) → review-
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #2) > This is mostly OK, but it's not great to infer equality of keys from > equality of results of a crypto operation. It seems like it would be > cleaner and more correct to test an import/export round trip instead.) That definitely ensures that the key bits are the same, but I think it is also valuable to check whether we clone attributes correctly, and thus algorithm, usages, etc. allow an operation to succeed. I added util.cloneExportCompareKeys() to do what you suggested. It's in util so that all structured cloning tests can use it and that won't break the patch that makes tests run in workers. HKDF was left untouched because the spec doesn't define any export operation and we don't support any.
Attachment #8712585 -
Attachment is obsolete: true
Attachment #8728427 -
Flags: review?(rlb)
Comment 4•8 years ago
|
||
Comment on attachment 8728427 [details] [diff] [review] 0001-Bug-1243311-Add-structured-cloning-tests-for-CryptoK.patch, v2 Review of attachment 8728427 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/test_WebCrypto_Structured_Cloning.html @@ +185,5 @@ > + crypto.subtle.generateKey(alg, true, ["sign", "verify"]) > + .then(util.cloneExportCompareKeys) > + .then(x => { > + return crypto.subtle.sign(alg, x.privateKey, data) > + .then(sig => crypto.subtle.verify(alg, x.publicKey, sig, data)); Why is this not just part of the top-level .then() chain? Likewise elsewhere.
Attachment #8728427 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #4) > > + crypto.subtle.generateKey(alg, true, ["sign", "verify"]) > > + .then(util.cloneExportCompareKeys) > > + .then(x => { > > + return crypto.subtle.sign(alg, x.privateKey, data) > > + .then(sig => crypto.subtle.verify(alg, x.publicKey, sig, data)); > > Why is this not just part of the top-level .then() chain? Likewise > elsewhere. Only because we don't have a single key but a keypair. If we'd move those to the top-level chain then I'd have to introduce a helper function that sets function-global variables to keep the public and private keys.
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/55a7478c3343
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•