Closed
Bug 1037501
Opened 10 years ago
Closed 10 years ago
WebCrypto algorithm names should be normalized as they are defined in the spec
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bzbarsky, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
rbarnes
:
review+
|
Details | Diff | Splinter Review |
Updated•10 years ago
|
Blocks: web-crypto
Assignee | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Note that you probably need to merge with the patches of Richard's I was reviewing today, since they add more EqualsLiteral/EqualsAscii bits.
Comment 3•10 years ago
|
||
Comment on attachment 8456502 [details] [diff] [review] 0001-Bug-1037501-WebCrypto-algorithm-names-should-be-norm.patch Review of attachment 8456502 [details] [diff] [review]: ----------------------------------------------------------------- I think this patch goes about things a little backwards. For the normalization, rather than using LowerCaseEqualsLiteral all the time, I would think that you would normalize the input to lower case (ToLowerCase in appropriate places), then just use EqualsLiteral. For the tests, there's no need to convert all the old tests to lower case, only to add a test with a lower-case or mixed-case identifier.
Attachment #8456502 -
Flags: review?(rlb) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Do we need a separate test for this given that I had to convert some of the checks to use .toLowerCase()?
Attachment #8456502 -
Attachment is obsolete: true
Attachment #8462603 -
Flags: review?(rlb)
Reporter | ||
Comment 5•10 years ago
|
||
You at least want tests that check that uppercase, lowercase, and mixed-case input are all accepted, right?
Assignee | ||
Comment 6•10 years ago
|
||
Yeah, lowercase and mixed-case input are missing. Should add that indeed.
Assignee | ||
Comment 7•10 years ago
|
||
Added some tests for lowercase and mixed-case algorithm names. In theory we had to check all the places (every crypto task) that take algorithm names but that's a little excessive. We're fine if we use GetAlgorithmName() everywhere - which this patch also corrects for HMAC's hash param.
Attachment #8462603 -
Attachment is obsolete: true
Attachment #8462603 -
Flags: review?(rlb)
Attachment #8462617 -
Flags: review?(rlb)
Comment 8•10 years ago
|
||
Comment on attachment 8462617 [details] [diff] [review] 0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v3 Review of attachment 8462617 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/tests.js @@ +114,5 @@ > function doExport(x) { > if (!hasKeyFields(x)) { > window.result = x; > throw "Invalid key; missing field(s)"; > + } else if ((x.algorithm.name != alg.toLowerCase()) || Sigh. Every time we go through this, it makes me sad in a different way. MAYBE WE SHOULD GO BACK TO ONLY HAVING ONE CASE, THE WAY THE ROMANS DID IT. The sadness here is that the algorithm names in the spec are all upper-case, so it seems like it would surprise developers to have lower-case things showing up here. I would propose that we: 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor 2. Revert these tests to use == r=me with that change
Attachment #8462617 -
Flags: review?(rlb) → review+
Reporter | ||
Comment 9•10 years ago
|
||
> 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor
And file a spec issue, right?
Assignee | ||
Updated•10 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #9) > > 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor > > And file a spec issue, right? Should we rather morph the issue you filed already? https://www.w3.org/Bugs/Public/show_bug.cgi?id=26311
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #8) > I would propose that we: > 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor > 2. Revert these tests to use == Yeah, I like that a lot more. One thing though is "RSAES-PKCS1-v1_5" which we would need to turn into "RSAES-PKCS1-V1_5" for equality testing. The spec does of course list the algorithm with a lowercase "v"... That's the only algorithm where we run into this :| How should we handle this?
Flags: needinfo?(rlb)
Reporter | ||
Comment 12•10 years ago
|
||
Oh, that's not addressed yet? Yeah, in that case just mention the issue in there.
Reporter | ||
Comment 13•10 years ago
|
||
Also, I assume we'll be contributing tests to the spec's test suite, right?
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #12) > Oh, that's not addressed yet? Yeah, in that case just mention the issue in > there. Ok. (In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #13) > Also, I assume we'll be contributing tests to the spec's test suite, right? Yeah (not sure how that works though).
Comment 15•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #11) > (In reply to Richard Barnes [:rbarnes] from comment #8) > > I would propose that we: > > 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor > > 2. Revert these tests to use == > > Yeah, I like that a lot more. One thing though is "RSAES-PKCS1-v1_5" which > we would need to turn into "RSAES-PKCS1-V1_5" for equality testing. The spec > does of course list the algorithm with a lowercase "v"... That's the only > algorithm where we run into this :| > > How should we handle this? Well, I'm not too bothered about RSAES-PKCS1-v1_5 in particular, since that's been removed from the spec. (We just haven't removed it from the code yet. One component of Bug 1037892.) But it's still a problem for RSASSA-PKCS1-v1_5. Let's review requirements/goals here: 1. Avoid the need for lots of LowerCaseEqualsLiteral 2. Have the #define'd constants be the ones in the spec (including mixed case) 3. Have the KeyAlgorithm name values be the ones in the spec 3. Accept algorithm names per the spec (Of course, (4) is hard to evaluate, because this requirement hasn't yet been re-added to the spec.) How about if we add some normalization to GetAlgorithmName? Something like the following: > nsString tempName; > // Extract name to tempName from aAlgorithm or SYNTAX_ERR > if (tempName.LowerCaseEqualsLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1)) { > aName.Assign(WEBCRYPTO_ALG_RSASSA_PKCS1); > } else if (/* ... */) { > // etc. > } else { > return NS_ERROR_DOM_NOT_SUPPORTED_ERR; > } > return NS_OK;
Flags: needinfo?(rlb)
Assignee | ||
Comment 16•10 years ago
|
||
Ok, so how about this?
Attachment #8462617 -
Attachment is obsolete: true
Attachment #8465432 -
Flags: review?(rlb)
Assignee | ||
Updated•10 years ago
|
Summary: WebCrypto algorithm names should be normalized to ASCII lowercase → WebCrypto algorithm names should be normalized as they are defined in the spec
Comment 17•10 years ago
|
||
Comment on attachment 8465432 [details] [diff] [review] 0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v4 Review of attachment 8465432 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/WebCryptoTask.cpp @@ +139,5 @@ > + if (aName.EqualsLiteral("RSAES-PKCS1-V1_5")) { > + aName.AssignLiteral(WEBCRYPTO_ALG_RSAES_PKCS1); > + } else if (aName.EqualsLiteral("RSASSA-PKCS1-V1_5")) { > + aName.AssignLiteral(WEBCRYPTO_ALG_RSASSA_PKCS1); > + } Rather than special-casing the mixed-case ones (since then we have to do something special for each algorithm), I would rather we just delete the ToUpperCase() and have the if-statement cover all the cases.
Attachment #8465432 -
Flags: review?(rlb) → review-
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8465432 -
Attachment is obsolete: true
Attachment #8465510 -
Flags: review?(rlb)
Comment 19•10 years ago
|
||
Comment on attachment 8465510 [details] [diff] [review] 0001-Bug-1037501-Normalize-WebCrypto-algorithm-names-to-A.patch, v5 Review of attachment 8465510 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks for persisting through all the back-and-forth.
Attachment #8465510 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/93c52fb1e891
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93c52fb1e891
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•