-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't let AES-KW to wrap/unwrap JWK #6102
Don't let AES-KW to wrap/unwrap JWK #6102
Conversation
Yes, it appears that this requirement was lost in the spec at some point. It is currently open as w3c/crypto#187. In the event that the document is changed then this should be fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an error that was lost in the document. Orignally everything was returned that way.
*This report has been truncated because the total content is 129172 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Firefox (nightly)Testing web-platform-tests at revision ecb20b2 Unstable results
All results2 tests ran/WebCryptoAPI/wrapKey_unwrapKey/test_wrapKey_unwrapKey.https.html
|
Sauce (safari)Testing web-platform-tests at revision ecb20b2 All results2 tests ran/WebCryptoAPI/wrapKey_unwrapKey/test_wrapKey_unwrapKey.https.html
/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKey.worker.html
|
Chrome (unstable)Testing web-platform-tests at revision ecb20b2 |
Sauce (MicrosoftEdge)Testing web-platform-tests at revision ecb20b2 |
Thanks @jimsch for approving my PR. I am quite confused of what the check fail is. Do you know how should I proceed and fix that? Sorry for troubling you with this as this is my first PR to web-platform-tests. |
You need to find out what failed in the firefox build. It may be that it always failed in the past but it could also be your changes. |
Firefox fails as usual. Is that enough to merge this PR? |
What is this waiting on? |
@sideshowbarker I am unsure of what the correct procedure should be for this. It is still failing a test, but it is claimed that this is what it does before the changes. Given my position as that of an interested bystander who does not do anything at all with browsers, it is not clear to me that I really should be the one to merge such a change into the test suite. |
@alanwaketan can you rebase this to rerun the bots? |
@alanwaketan if you rebase this and it then passes all the CI, I can merge it. Or you can, I've added you to the reviewers team so you have write access. Please see https://github.com/orgs/web-platform-tests/teams/reviewers/discussions for things you may have to do to avoid excessive GitHub email. |
I rebased this in #21149 to see what would happen. Safari overall improves in % of tests passed (https://wpt.fyi/results/WebCryptoAPI/wrapKey_unwrapKey?diff&filter=ADC&run_id=389460003&run_id=406580004), but there are three tests in test_wrapKey_unwrapKey.https.html that go from PASS -> FAIL. There are no (easily extractable) Chrome/Firefox results because it fails the stability checks on both: Chrome:
Firefox:
I don't know if those are pre-existing flakes or not. |
@alanwaketan if you disagree, please feel free to re-open this PR to discuss :) |
Reopening per #6102 (comment) |
Also, from the spec: For implementations that don't want to add padding to address the constraints of AES-KW. The operations may well fail. Therefore, excluding AES-KW in the test seems better. |
I would agree that there is no guaranteed order of how the fields of a JWK are serialized. I don't understand why you believe that this matters. The output from the encryption process is hopefully going to be different in most cases for symmetric keys because of different IVs. It is going to be different for an RSA key because of the random bytes that are put into the padding. |
@jimsch Sorry for leaving misleading comments. See my last comment, that's my reason to not include AES-KW in the wrapping/unwrapping tests. |
But the test already has a check for the case of the exported key not being a multiple of 8 already. |
I believe that check is for other binary output formats. Also, JWK doesn't have the field "byteLength". |
so, instead adding a check for "JSON.stringify(exportedKey).length % 8 == 0" would be the same. The only issue would be if stringify is not used. |
I'm fine with that as WebKit uses stringify. |
Go ahead and mke the change and I will approve that. |
2c7cc34
to
7306f19
Compare
@sideshowbarker - I don't know how to check if the failures are because of this change. The only thing that should have changed is that a set of tests are no longer run. The specification allows for the removed tests to be successful if a custom JSON to string function is used, but I do not know of any way to check this. |
I'll check and then post a comment after I've looked into it |
OK, looking at the Taskcluster log, it seems there’s an actual problem in the test that was touched in this change (rather than, say, some unrelated failures elsewhere). Specifically, I see the failure seems to be due to a test that has accurately been identified as flaky — with both Firefox and Chrome: Chrome
Chrome
Now, I don’t know whether that test was already flaky before the change in this PR — but, regardless, I think the flakiness needs to be investigated and resolved before this can be merged. |
When I look at the actual patch, I see what was added is this: wpt/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKey.js Lines 249 to 251 in 7306f19
…so when you say, “The only thing that should have changed is that a set of tests are no longer run”, I guess you mean that change adds an early return that has the effect of causing some tests to be run that would be run otherwise if that early return weren’t there? |
I’ve zero specific domain knowledge about the underlying functionality here, but looking at this: wpt/WebCryptoAPI/wrapKey_unwrapKey/wrapKey_unwrapKey.js Lines 249 to 251 in 7306f19
…it doesn’t at all look to me at least that part would be a potential cause for flakiness, so I suspect the test was already flaky prior to the change in this PR… |
The test is flaky without my change. I thought my change will improve its stability. Anyway, I think we should land my change and not wait another 3 years. Then if we have time, we should make another attempt to improve the test. |
OK, merging per #6102 (comment) |
Thank you all! @jimsch @sideshowbarker |
No description provided.