729659 - HTTP MAC signing API

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Implement HTTP MAC APIs

The Firefox Sync client will need to support HTTP MAC authorization: https://tools.ietf.org/html/draft-ietf-oauth-v2-http-mac-01

I've implemented it generically on top of Sync's util.js module. A subsequent bug/patch will integrate this API with Sync's HTTP stack.

Requesting review from rnewman for Sync-land changes and dchan for security aspect.

Comment 1

[Gregory Szorc [:gps]]

I should also note that we may want to put this utility function elsewhere in the tree to facilitate re-use. I'll gladly do that if someone tells me where would be appropriate.

Comment 2

[JR Conlin [:jrconlin,:jconlin]]

Comment on attachment 599727 [details] [diff] [review]
Implement HTTP MAC APIs

Note: while probably not used heavily, the specification does include an 'ext' element (containing extra data that should be used when generating the signature base string. This data is also included in the passed Authorization header.

I see it's missing here. While it's probably not used by sync, it would be required for library reuse.

Comment 3

[David Chan [:dchan]]

Comment on attachment 599727 [details] [diff] [review]
Implement HTTP MAC APIs

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

Ben could comment more about the specific implementation details for the protocol.

::: services/sync/modules/util.js
@@ +782,5 @@
> +                                                  uri, extra) {
> +    let ts = extra && extra.ts ? extra.ts : Math.floor(Date.now() / 1000);
> +    // We are allowed to use more than the Base64 alphabet if we want.
> +    let nonce = extra && extra.nonce ? extra.nonce
> +                                     : btoa(Utils.generateRandomBytes(8));

btoa reduces the entropy in the nonce since not all acceptable characters can be generated
     nonce          = "nonce" "=" string-value
     string-value   = ( <"> plain-string <"> ) / plain-string
     plain-string   = 1*( %x20-21 / %x23-5B / %x5D-7E )
     ; Any printable ASCII character except for <"> and <\>


The length of nonce should also be configurable

@@ +799,5 @@
> +    let requestString = ts.toString(10) + "\n" +
> +                        nonce + "\n" +
> +                        method.toUpperCase() + "\n" +
> +                        uri.path + "\n" +
> +                        uri.host + "\n" +

I think this should be uri.asciiHost instead. There is no guarantee that uri.host is lowercase since it may contain internationalized domain names. There are also slight nuances since two different IDNs can canonicalize to the same punycode/asciihost representation. We may want to treat those as separate domains, though the underlying Firefox code won't

@@ +801,5 @@
> +                        method.toUpperCase() + "\n" +
> +                        uri.path + "\n" +
> +                        uri.host + "\n" +
> +                        port + "\n" +
> +                        "\n";

ditto to what JR said about ext

Comment 4

[Gregory Szorc [:gps]]

(In reply to David Chan [:dchan] from comment #3)
> ::: services/sync/modules/util.js
> @@ +782,5 @@
> > +                                                  uri, extra) {
> > +    let ts = extra && extra.ts ? extra.ts : Math.floor(Date.now() / 1000);
> > +    // We are allowed to use more than the Base64 alphabet if we want.
> > +    let nonce = extra && extra.nonce ? extra.nonce
> > +                                     : btoa(Utils.generateRandomBytes(8));
> 
> btoa reduces the entropy in the nonce since not all acceptable characters
> can be generated
>      nonce          = "nonce" "=" string-value
>      string-value   = ( <"> plain-string <"> ) / plain-string
>      plain-string   = 1*( %x20-21 / %x23-5B / %x5D-7E )
>      ; Any printable ASCII character except for <"> and <\>

I can see your argument, but just barely. Isn't the true security in the underlying randomly-generated value. If someone knows our nonces are Base64, they still have to crack a randomly generated 64 bit (or whatever) value.

Comment 5

[David Chan [:dchan]]

(In reply to Gregory Szorc [:gps] from comment #4)
> (In reply to David Chan [:dchan] from comment #3)
> I can see your argument, but just barely. Isn't the true security in the
> underlying randomly-generated value. If someone knows our nonces are Base64,
> they still have to crack a randomly generated 64 bit (or whatever) value.

You are completely correct, for some reason I looked at the representation rather than the underlying random data. The random data isn't from the base64 charset.

Thanks for correcting me.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 599727 [details] [diff] [review]
Implement HTTP MAC APIs

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

f+ until I can see the tests. Some tweaks and possible improvements. Good stuff, though, as always!

::: services/sync/modules/util.js
@@ +775,5 @@
> +   *         following keys:
> +   *           mac - (string) Raw HMAC digest bytes.
> +   *           header - (string) Authorization header that can be used.
> +   *           nonce - (string) Nonce value used.
> +   *           ts - (int) Integer seconds since UNIX epoch that was used.

These sentences need a little adjustment. For example, do you mean (integer seconds) since (UNIX epoch that was used)?

Teensy nits:

* "Unix". http://en.wikipedia.org/wiki/Unix#Branding
* s/int/number. JavaScript doesn't have ints. http://en.wikipedia.org/wiki/JavaScript_syntax#Number

@@ +798,5 @@
> +
> +    let requestString = ts.toString(10) + "\n" +
> +                        nonce + "\n" +
> +                        method.toUpperCase() + "\n" +
> +                        uri.path + "\n" +

We're lucky that nsIURI doesn't return out-of-range characters here!

@@ +799,5 @@
> +    let requestString = ts.toString(10) + "\n" +
> +                        nonce + "\n" +
> +                        method.toUpperCase() + "\n" +
> +                        uri.path + "\n" +
> +                        uri.host + "\n" +

As dchan pointed out, yes, uri.asciiHost.

It's also worth mentioning that we rely on nsIURI to downcase the host on our behalf, which it will for both host and asciiHost, but we can't rely on the remote server using the same definition of lowercase for IDNs.

(See also below, where I suggest returning the hostname used in the return value.)

@@ +812,5 @@
> +      mac: mac,
> +      nonce: nonce,
> +      ts: ts,
> +      header: Utils.getHTTPMACHeader(identifier, ts, nonce, mac)
> +    };

Three points. 

Zerothly, are you using *tabs* like a heathen?! :D

Firstly, return identifier in the object. It's nice to be symmetrical. You probably also want to return uri.asciiHost.

Secondly, if this is only ever used to compute the HTTP header, just return that. If you *still* want to return the components, consider:

  let header = new String("MAC id =...");
  header.mac = mac;
  ...
  return header;

which will return a string object with useful attributes. You can directly pass the output into whatever destination is expecting a string, but you can also get the intermediate values.

If callers will frequently just want to look at the calculated values, then don't return the header here; return the values.

Either the caller can chain those into the "make me a header" call, or you can stick a method into the return object to do it:

  function hmacHeader() {
    return 'MAC id="' + this.identifier + '", ' +
           'ts="'     + this.ts + '", ' +
           'nonce="'  + this.nonce + '", ' +
           'mac="'    + btoa(this.mac) + '"';
  }

...

  return {
    identifier: identifier,
    mac:        mac,
    nonce:      nonce,
    ts:         ts,
    getHeader:  hmacHeader
  };

That way callers that don't want the header don't have to compute it, and callers that do can call computeHMACSHA1(...).getHeader().

So: either return an object with all of the values, that can compute a header on demand, or return a string with attributes. I think you're in a middle ground here.

Feel free to disagree; you have more context for how this will be used than do I.

@@ +821,5 @@
> +   *
> +   * @param  identifier
> +   *         (string) MAC key identifier.
> +   * @param  ts
> +   *         (int) Seconds since UNIX epoch.

(number) Integer seconds since Unix epoch.

@@ +829,5 @@
> +   *         (string) Computed HMAC digest (raw bytes).
> +   * @returns
> +   *         (string) Value to put in Authorization header.
> +   */
> +  getHTTPMACHeader: function getHTTPMACSHA1Header(identifier, ts, nonce, mac) {

Nit: function and method name agreement.

::: services/sync/tests/unit/xpcshell.ini
@@ +114,5 @@
>  [test_utils_ensureOneOpen.js]
>  [test_utils_getErrorString.js]
>  [test_utils_getIcon.js]
>  [test_utils_hkdfExpand.js]
> +[test_utils_httpmac.js]

This file is missing from the patch.

Comment 7

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #6)
> It's also worth mentioning that we rely on nsIURI to downcase the host on
> our behalf, which it will for both host and asciiHost, but we can't rely on
> the remote server using the same definition of lowercase for IDNs.

In our case, nsIURI will control the Host HTTP request header eventually. The server should be looking at that value verbatim. So, I don't think we have to worry about client and server agreeing on the same methodology. Yes, intermediary agents could rewrite the header. But, I think ensuring consistency goes with the territory of deploying that functionality and thus is outside our concern.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Implement HTTP MAC APIs, v2

I think I addressed all points of feedback.

Comment 9

[JR Conlin [:jrconlin,:jconlin]]

"ext" handling looks good. Thanks!

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 601013 [details] [diff] [review]
Implement HTTP MAC APIs, v2

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

::: services/sync/modules/util.js
@@ +815,5 @@
> +    } else {
> +      throw new Error("Unsupported URI scheme: " + uri.scheme);
> +    }
> +
> +    let ext = extra && extra.ext ? extra.ext : "";

Nit: Parens for those who haven't got <https://developer.mozilla.org/en/JavaScript/Reference/Operators/Operator_Precedence> memorized :D

(Top hit in my awesomebar :D)

@@ +823,5 @@
> +                        usedMethod + "\n" +
> +                        uri.path + "\n" +
> +                        host + "\n" +
> +                        port + "\n" +
> +                        ext + "\n";

Did you intend to get two newlines at the end if extra is not provided? Perhaps define

  let ext = (extra && extra.ext) ? (extra.ext + "\n") : "";

@@ +842,5 @@
> +      ext:        ext,
> +      getHeader:  function() {
> +        return Utils.getHTTPMACSHA1Header(this.identifier, this.ts, this.nonce,
> +                                          this.mac, this.ext);
> +      }

I suggest defining this as a separate method, and doing

  getHeader: getHeaderHelper

rather than potentially creating a new closure here on each invocation… and also inline the logic from getHTTPMACSHA1Header, to avoid another function call. (Yes, duplication, but...)

(I don't know how smart the JS compiler is, but I'd guess "not very".)

@@ +870,5 @@
> +                'nonce="'  + nonce      + '", ' +
> +                'mac="'    + btoa(mac)  + '"';
> +
> +    if (ext) {
> +      header += ', ext="' + ext +'"';

if (!ext) {
  return header;
}
return header += ', ext="' + ext +'"';

Comment 11

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #10)
> 
> @@ +823,5 @@
> > +                        usedMethod + "\n" +
> > +                        uri.path + "\n" +
> > +                        host + "\n" +
> > +                        port + "\n" +
> > +                        ext + "\n";
> 
> Did you intend to get two newlines at the end if extra is not provided?

This is per the spec. An extra newline is included even if the "ext" param is not defined.

https://tools.ietf.org/html/draft-ietf-oauth-v2-http-mac-01 Sec 3.2.1

Comment 12

[Gregory Szorc [:gps]]

Attachment: Implement HTTP MAC APIs, v3

Implemented all feedback except the bit about duplicating the header generation logic: I refuse to go against DRY unless it is absolutely necessary. And, I don't think it is necessary in this case. We are talking about a local function call related to network I/O. Time will be dominated by network time and it's not like we will be making hundreds of network calls per second to generate enough GC pressure to concern us.

If you insist, I'd just rather delete the global Utils function and define it inside the signature generating function.

Comment 13

[David Chan [:dchan]]

Comment on attachment 601041 [details] [diff] [review]
Implement HTTP MAC APIs, v3

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

Looks good to me

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/54f2d811e79a

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/54f2d811e79a