Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 10 years ago

http://www.w3.org/TR/WebCryptoAPI/#dfn-HmacKeyGenParams

"The length (in bits) of the key to generate. If unspecified, the recommended length will be used, which is the size of the associated hash function's block size."

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 10 years ago

Is there a better place or way to do this?

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

Comment on attachment 8434847 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch

The IDL interaction bits look good to me.  Richard, can you review the switch bit and tests?

	Richard Barnes [:rbarnes]
	Comment 3 • 10 years ago

Comment on attachment 8434847 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch

Review of attachment 8434847 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for catching this.  There might be a few other default cases that are missing, so your review is very helpful.

This is the right place to address the problem.

::: dom/crypto/WebCryptoTask.cpp
@@ +1032,5 @@
>  
> +      if (params.mLength.WasPassed()) {
> +        mLength = params.mLength.Value();
> +      } else {
> +        KeyAlgorithm hashAlg(global, hashName);

This seems duplicative of the HmacKeyAlgorithm construction below.

@@ +1039,5 @@
> +          case CKM_SHA224: mLength = 224; break;
> +          case CKM_SHA256: mLength = 256; break;
> +          case CKM_SHA384: mLength = 384; break;
> +          case CKM_SHA512: mLength = 512; break;
> +          default: mLength = 0; break;

I'm concerned about this default case.  If Mechanism() returns something unknown, we're in more serious trouble.  Could you check to see where we fail for an unknown hash algorithm?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 10 years ago

(In reply to Richard Barnes [:rbarnes] from comment #3)
> ::: dom/crypto/WebCryptoTask.cpp
> @@ +1032,5 @@
> >  
> > +      if (params.mLength.WasPassed()) {
> > +        mLength = params.mLength.Value();
> > +      } else {
> > +        KeyAlgorithm hashAlg(global, hashName);
> 
> This seems duplicative of the HmacKeyAlgorithm construction below.

I'm only doing this to let it parse the string into a Mechanism. HmacKeyAlgorithm wants the length given to the constructor and I can't change it afterwards. I could as well just do string comparisons like the KeyAlgorithm constructor does?

	Richard Barnes [:rbarnes]
	Comment 5 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #4)
> (In reply to Richard Barnes [:rbarnes] from comment #3)
> > ::: dom/crypto/WebCryptoTask.cpp
> > @@ +1032,5 @@
> > >  
> > > +      if (params.mLength.WasPassed()) {
> > > +        mLength = params.mLength.Value();
> > > +      } else {
> > > +        KeyAlgorithm hashAlg(global, hashName);
> > 
> > This seems duplicative of the HmacKeyAlgorithm construction below.
> 
> I'm only doing this to let it parse the string into a Mechanism.
> HmacKeyAlgorithm wants the length given to the constructor and I can't
> change it afterwards. I could as well just do string comparisons like the
> KeyAlgorithm constructor does?

Good point.  Let's just leave it.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 6 • 10 years ago

You're absolutely right, I missed that the spec says we should fail with a DataError when length is zero. Added two more test cases.

	Richard Barnes [:rbarnes]
	Comment 7 • 10 years ago

Comment on attachment 8435229 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2

Review of attachment 8435229 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

	Richard Barnes [:rbarnes]
	Comment 8 • 10 years ago

Comment on attachment 8435229 [details] [diff] [review]
0001-Bug-1020882-length-param-of-HmacKeyGenParams-should-.patch, v2

Review of attachment 8435229 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/65f1eeeb742c

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/65f1eeeb742c