Closed Bug 729659 Opened 8 years ago Closed 8 years ago

HTTP MAC signing API

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: gps, Assigned: gps)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch Implement HTTP MAC APIs (obsolete) — Splinter Review
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.
Attachment #599727 - Flags: review?(rnewman)
Attachment #599727 - Flags: review?(dchan+bugzilla)
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 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 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
Attachment #599727 - Flags: review?(dchan+bugzilla) → review-
Whiteboard: [qa-]
(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.
(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 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.
Attachment #599727 - Flags: review?(rnewman) → feedback+
(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.
Attached patch Implement HTTP MAC APIs, v2 (obsolete) — Splinter Review
I think I addressed all points of feedback.
Assignee: nobody → gps
Attachment #599727 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #601013 - Flags: review?(rnewman)
Attachment #601013 - Flags: review?(dchan+bugzilla)
"ext" handling looks good. Thanks!
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 +'"';
Attachment #601013 - Flags: review?(rnewman) → review+
(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
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.
Attachment #601013 - Attachment is obsolete: true
Attachment #601041 - Flags: review?(rnewman)
Attachment #601041 - Flags: review?(dchan+bugzilla)
Attachment #601013 - Flags: review?(dchan+bugzilla)
Comment on attachment 601041 [details] [diff] [review]
Implement HTTP MAC APIs, v3

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

Looks good to me
Attachment #601041 - Flags: review?(dchan+bugzilla) → review+
Attachment #601041 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/services/services-central/rev/54f2d811e79a
Whiteboard: [qa-] → [qa-][fixed-in-services]
Whiteboard: [qa-][fixed-in-services] → [qa-][fixed in services]
https://hg.mozilla.org/mozilla-central/rev/54f2d811e79a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa-][fixed in services] → [qa-]
Target Milestone: --- → mozilla13
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.