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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bzbarsky, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Assignee: rlb → ttaubert
Status: NEW → ASSIGNED
Attachment #8456502 - Flags: review?(rlb)
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 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-
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)
You at least want tests that check that uppercase, lowercase, and mixed-case input are all accepted, right?
Yeah, lowercase and mixed-case input are missing. Should add that indeed.
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 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+
> 1. Add ToUpperCase(mName) to the KeyAlgorithm ctor

And file a spec issue, right?
OS: Mac OS X → All
Hardware: x86 → All
(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
(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)
Oh, that's not addressed yet?  Yeah, in that case just mention the issue in there.
Also, I assume we'll be contributing tests to the spec's test suite, right?
(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).
(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)
Ok, so how about this?
Attachment #8462617 - Attachment is obsolete: true
Attachment #8465432 - Flags: review?(rlb)
Summary: WebCrypto algorithm names should be normalized to ASCII lowercase → WebCrypto algorithm names should be normalized as they are defined in the spec
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-
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+
https://hg.mozilla.org/mozilla-central/rev/93c52fb1e891
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: