957863 - tolerate clock skew in HAWK requests

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Brian Warner [:warner :bwarner]]

The FxA login (and subsequent Sync protocol) has several servers, all of
which might have clocks that are offset from the client. The HAWK protocol
uses a timestamp to reduce replay attacks, and if the client-vs-server clock
skew is too large, the request will be rejected. Client clocks are frequently
really broken, and we need FxA/Sync to work anyways.

One piece of work is to create a stateful HAWK client object, one per server,
which remembers the most recent delta between the browser clock and the
server it talks to. (It's probably sufficient to subtract current browser
time from the HTTP response's "Date" header, when the response arrives.. no
need for more sophisticated NTP-like calculations). It should then use this
delta when computing the hashed timestamp of the next request. The object
should also automatically retry any request, at most once, when the request
is rejected due to a timestamp error.

The Persona signed assertion also has a client-generated timestamp, in the
"exp" (expires) field of the assertion object. This should be linked to the
fxa-auth-server's clock. (I think the Persona shim has code to do this
already). So the new HAWK client object needs an API to return the current
delta, the jwcrypto.jsm function which creates assertions needs to accept an
argument to set the desired expiration time, and the FxAccounts.jsm code
needs to fetch the delta (from the HAWK-authenticated /certificate/sign
request) and use it to compute the "exp" time.

I'm not sure if there's a clean way to expose the HAWK delta to
FxAccounts.jsm, as there's an intermediate "FxAccountsClient.jsm" object
between them. So we may have to do some refactoring.

We'll need one of these HAWK client objects for the fxa-auth-server. We might
also need one for the Sync token-server, depending upon what protocol it
uses. We probably need one for the Sync storage-server, since I think it
accepts HAWK-authenticated requests (using the tokenserver's token as the
HAWK key).

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I'm wondering whether the Persona assertion component of this shouldn't be a separate bug.  In any case, I'll keep the patches separate for now.

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate clock skew in hawk requests (WIP)

WIP:
- refactored hawk and xhr bits out of fxa client into new hawk.js
- implements clock skew correction
- added tests

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate clock skew in hawk requests (WIP)

Added test to verify that client calculates correct ts value in header
Added documentation

Brian, I now have to update the sync modules to use this - this is only a refactor of FxAccountsClient.jsm at this point, with the clock skew logic - but can you let me know if this looks like it's heading in the right direction for you?  Thanks!

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

I take back what I said in comment 1 - I think the persona bits do need to stay in this patch after all.

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

Added:
- FxAccounts learns clock skew and date (for mocking) from FxAccountsClient
- jwcrypto can be given skew and date for generating assertions
- tests for the above

I had to modify test_getAssertion in test_accounts.js slightly, and I'm not entirely certain I've done it right.  Brian, what do you think?

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

/cc spenrose, to keep him in the loop with modifications to fxa code

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 957863-hawk-tolerate-clock-skew.patch

Added:
- HAWK request retry once on 401 error
- Test for 401 then 200 responses
- Test for endless 401 responses
- Test for 401 then 500 responses

Trying hard to ensure we don't have any devious infinite loop conditions!

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

To qualify the Description slightly, the sync token server does not use hawk-authenticated requests.

Comment 9

[Richard Newman [:rnewman]]

Per Jed: this needs to coordinate with Bug 959222, so marking as See Also.

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

In order to get this patch functional and landed ahead of the Fx29 deadline, I think we should be able to get away with just checking our client's clock skew against the fxa-auth-server, and adjusting all hawk header and assertion timestamps accordingly.

We (Mozilla) run all the services that fxa and sync will be interacting with for Fx29.  I've confirmed with gene (IRC) that devops actively monitors the clocks on all our aws servers, because the amazon clocks tend to jump around from time to time, and fixes them faster than ntp can.  The examples of clock deviations on amazon I've seen are around 1 second.  Gene reports that keeping aws clocks withing the 60-second interval (what we require) is "totally doable".

With that, I think we should be able to:
- Establish clock skew from fxa-auth-server requests (retrying on 401 if necessary)
- Keep a record of fxa-auth-server skew where FxAccounts.jsm can reach it
- Know that fxa-auth-server will be hit each time the user cert expires (so regular calibration)
- Establish browserid expiration based on the fxAccounts skew
- Establish sync browserid_identity hawk timestamps based on fxAccounts skew

This plan works because contacting the fxa-auth-server is required before contacting the token and storage servers.  So we will always have a client-vs-Mozilla skew value to work with.

I hope this solution will be adequate for Fx29 (I can't see why it wouldn't be); future releases can be made more robust/elegant when more time is available.

Comment 11

[Richard Newman [:rnewman]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #10)

> I think we should be able to get away with just checking our client's clock
> skew against the fxa-auth-server, and adjusting all hawk header and
> assertion timestamps accordingly.

This makes sense. For Android we will probably use an anchoring server timestamp and a measured elapsed realtime, which will be more precise and less error-prone, but I don't think desktop has access to anything more useful than the current calendar time. You're essentially proposing simulating a realtime clock by using Date.now.


> keeping aws clocks withing the 60-second interval (what we require) is
> "totally doable".

Sidenote: the Sync storage protocol itself depends on the storage server clock for two things:

1. Preventing a write from replacing an object that was just written by another client
2. Retrieving objects added since our last read.

For this we want adjustments to be very frequent and very small: ideally each adjustment should be smaller than 0.01 seconds, and as frequent as necessary to make that happen.

Clocks jumping around will result in incorrect race condition handling and data loss.

The alternative is that we should never adjust clocks on storage servers, which converges on a version-based protocol.

Comment 12

[Chris Karlof [:ckarlof]]

jedp, I'm fine with your proposal.

Comment 13

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thanks to :warner for emailing me some comments from the airport:

- in the HawkClient constructor, add some comments to explain the polarity of "skew", maybe something like "everywhere we use the word 'skew', positive numbers mean our client/browser clock is ahead of the server's, eg skew=120000 means that the server thinks it is 3:01pm but the browser thinks it is 3:03pm", or whatever.. Be overkill clear about sign and s-vs-ms

- deriveCredentials() is an FxA-auth-server convention, unrelated to hawk, so it probably wants to live up at the fxaclient lever, not in HawkClient. The HawkClient.request method should be passed keyid and key as arguments somehow

- not sure, but it might be better to have HawkClient.request take a full URL, instead of building one with host+prefix and passing a path into request. It would let someone violate the one-client-per-server rule, but would also make it easier to read the resulting request() calls. Not sure, it seems like a subtle judgement call.

- the jwcrypto.jsm getExpiration() function, that second argument that takes a default "Date.now()" .. does javascript do the same thing that Python does and only evaluates the default-argument value when the function definition is first evaluated?

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Use fxa auth clock skew in hawk, jwcrypto, and sync

- BrowserIDManager gets skew from FxAccounts for sync storage request headers
- Addresses issues from Comment 13 (except signature of request() method)

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Brian Warners feedback from comment #13)

Thanks again for your feedback, Brian; especially for finding time at the airport to get in touch!

> - in the HawkClient constructor, add some comments to explain the polarity
> of "skew" ...

Good point.  I've added verbose documentation at each of the points where skew is exposed on an object (hawk.js, FxAccounts.jsm, FxAccountsClient.jsm).

> - deriveCredentials() is an FxA-auth-server convention, unrelated to hawk,
> so it probably wants to live up at the fxaclient lever, not in HawkClient.
> The HawkClient.request method should be passed keyid and key as arguments
> somehow

Ah ok.  Moved back to fxaclient.

The HawkClient.request() method already gets the id and key in the credentials argument, doesn't it?  Or did I do this wrong?

> - not sure, but it might be better to have HawkClient.request take a full
> URL, instead of building one with host+prefix and passing a path into
> request. It would let someone violate the one-client-per-server rule, but
> would also make it easier to read the resulting request() calls. Not sure,
> it seems like a subtle judgement call.

It's true that you could end up building messy urls this way.  But I think that allowing the caller to specify a hostname would, as you say, make it easy for this to be misused.  I'm inclined to leave it as it is, but maybe add some checking for too many '/' characters etc.?

> - the jwcrypto.jsm getExpiration() function, that second argument that takes
> a default "Date.now()" .. does javascript do the same thing that Python does
> and only evaluates the default-argument value when the function definition
> is first evaluated?

Excellent catch.  It turns out that such default args are always evaluated dynamically in js, so this is ok.

Thanks, Brian
j

Comment 16

[Gene Wood [:gene]]

I've opened a separate issue related to :rnewman's sidenote above on Sync Storage protocol and clock skew : https://github.com/mozilla-services/server-syncstorage/issues/7

Comment 17

[Brian Warner [:warner :bwarner]]

Comment on attachment 8362283 [details] [diff] [review]
Use fxa auth clock skew in hawk, jwcrypto, and sync

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

Looks pretty good, most of my comments are just nits. r=warner with the following subset of changes:

* tolerate non-200 success codes
* test 500-error-first
* remove or justify "identity.fxaccounts.auth.prefix" pref

::: services/common/hawk.js
@@ +124,5 @@
> +  get skew() { 
> +    return this._skew;
> +  },
> +
> +  now: function() {

Might want to remove the dump() call. Also, this seems redundant with _currentDate.. is this function used? If not, it should get removed.

@@ +183,5 @@
> +
> +      if (xhr.status === 401 && this._retries === 0) {
> +        // Retry once if we were rejected due to a bad timestamp.
> +        // Clock skew is adjusted already in the top of this function.
> +        this._retries += 1;

This "_retries" is shared among all calls that use this HawkClient object, which is a bit weird. This probably behaves correctly if there are no overlapping requests, but if requests were to overlap, I think the all but the first would be falsely abandoned. OTOH, I think this approach safely avoids ever getting stuck: the first non-retried failure will reset the counter, which is great.

The ideal implementation might use an independent "allow_retry" boolean flag for each call, passed into request() that defaults to 'true', but which gets 'false' in the retrying path.

I'm neutral on whether we should make that change: it's probably fine to ship it this way, but we should clean it up sooner or later.

@@ +190,5 @@
> +            this.request(path, method, credentials, jsonPayload));
> +      }
> +
> +      this._retries = 0;
> +      if (xhr.status !== 200 || jsonResponse.error) {

There are some non-200 success codes (like "201 CREATED" or "202 ACCEPTED").. fxa-auth-server doesn't use any of them right now, but the Sync code might (I dunno). You might want to relax this to accept 200-299 instead of just 200.

@@ +198,5 @@
> +        return deferred.reject(this._constructError(xhr, "Request failed"));
> +      }
> +      // It's up to the caller to know how to decode the response.
> +      // We just return the raw text.
> +      deferred.resolve(xhr.responseText);

It's a little bit weird to have the success path return a string, but the error path return an object. But I guess that's the cleanest way to avoid imposing too much policy on the client's behavior (using JSON or otherwise in the response), so yeah, keep doing that.

@@ +222,5 @@
> +      let header = CryptoUtils.computeHAWK(uri, method, options);
> +      xhr.setRequestHeader("authorization", header.field);
> +    }
> +
> +    xhr.setRequestHeader("Content-Type", "application/json");

It might be good to add a docs note that this function can only be used for JSON request bodies. Since we *do* allow non-JSON responses, someone might conclude that they could use text/plain for the request-side too, and the hard-coded Content-Type might surprise them.

::: services/common/tests/unit/test_hawk.js
@@ +146,5 @@
> +
> +  do_check_eq(client.skew, 0);
> +
> +  let response = yield client.request("/foo", method);
> +  do_check_neq(client.skew, 0);

should we test that skew is about-equal oneHourInMS here?

@@ +190,5 @@
> +    return client.skew;
> +  }
> +
> +  client._currentDate = () => {
> +    return Date.now() - 1000 * 60 * 60 * 12;

how about defining SECOND=1000, MINUTE=60*SECOND, HOUR=60*MINUTE, and using 12*HOUR here?

@@ +375,5 @@
> +
> +  yield deferredStop(server);
> +});
> +
> +// End of tests.

Could you also add a test that returns a 500 error on the first request, and asserts that it only gets called once? 401 should be the only error that provokes a retry: everything else should give up without retrying.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +19,5 @@
>  try {
>    _host = Services.prefs.getCharPref("identity.fxaccounts.auth.uri");
>  } catch(keepDefault) {}
>  
> +// Default prefix can be changed with pref 'identity.fxaccounts.auth.prefix'

Did you find a need to modify this? Given how closely this HKDF-contextInfo string is tied to the protocol implementation in the rest of the file, I'm not sure how useful/safe it'd be to expose it in a pref.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +384,5 @@
>    // to the "we're ready" stage.
>    yield fxa.setSignedInUser(creds);
>  
>    _("== ready to go\n");
> +  // XXX i did not understand this - really set to May 16 1974 ??

I don't remember why either: feel free to just delete the old confused stuff.

@@ +451,3 @@
>    exp = Number(payload.exp);
> +  do_check_true(start + TWO_MINUTES_MS <= exp);
> +  // XXX is this right?

Hm, something does feel funny here. We're setting start/finish to real live times, but making everything else (via _now_is=) use fixed/simulated dates. But maybe the FxAccounts class isn't really mocking out *all* of the clocks: when we create a cert or assertion, do we set those internal validUntil values using the mocked now(), or with a raw "new Date()"?

It's possible that it is right, and furthermore the first check would be more correct as: start + ONE_DAY_MS + TWO_MINUTES_MS <= exp . I'm having problems getting my brain around it, though.

It might be easier to read if we rewrote the whole test to use "now" as the wristwatch of our imaginary narrator, so to speak, a variable that's updated as we simulate time progressing. At present "now" is really just a realistic-looking value captured when the test starts (and could be set to any other plausible value, like that 138M value that got deleted, without ill effect). If we replaced line 435 with something like "now += ONE_DAY_MS; fxa._now_is=now", then all the interval-membership tests could reference "now" and the normal offsets (TWO_MINUTES_MS for the expiration time), and *not* include the artificial offsets like ONE_DAY_MS.

::: services/sync/modules/browserid_identity.js
@@ +362,5 @@
>      method = method || httpObject.method;
> +
> +    // Get the local clock skew from the Firefox Accounts server.  This should
> +    // be close to the offset from the storage server.
> +    dump("get auth heaader: now, skew = " + this._now() + ", " + this._skew + "\n");

should we leave dump() calls in? I'm not sure what the coding guidelines say.

::: services/sync/tests/unit/test_browserid_identity.js
@@ +94,5 @@
> +add_test(function test_resourceAuthenticatorSkew() {
> +  _("BrowserIDManager Resource Authenticator compensates for clock skew in Hawk header.");
> +
> +  // Clock is skewed 12 hours into the future
> +  let twelveHours = 1000 * 60 * 60 * 12;

would be nice to be consistent with the other tests, maybe 12*HOURS or something

@@ +97,5 @@
> +  // Clock is skewed 12 hours into the future
> +  let twelveHours = 1000 * 60 * 60 * 12;
> +  let now = Date.now() + twelveHours;
> +  let browseridManager = new BrowserIDManager();
> +  let hawkClient = new HawkClient("https://example.net", "/foo/v1/");

tiny nit, but I'd encourage putting the "v1" in the per-call path, and leaving it out of the static per-HAWKClient-object prefix, so that readers of later code can tell that the endpoint name is "v1/something". Obviously it doesn't matter in a test, and doesn't make a practical difference elsewhere, but it's an opportunity to educate future readers.

::: toolkit/identity/jwcrypto.jsm
@@ +127,5 @@
> +   *
> +   * @param aOptions (optional)
> +   *        Can include:
> +   *        {
> +   *          skew: <clock skew in milliseconds>,

nit: mention polarity briefly

Comment 18

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Brian Warner [:warner :bwarner] from comment #17)
> Comment on attachment 8362283 [details] [diff] [review]
> Use fxa auth clock skew in hawk, jwcrypto, and sync
> 
> Review of attachment 8362283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good, most of my comments are just nits. r=warner with the
> following subset of changes:

Thanks as always for your thorough review, Brian.

> * tolerate non-200 success codes

Done, with a test for "202" added.

> * test 500-error-first

Done

> * remove or justify "identity.fxaccounts.auth.prefix" pref

Done.  I've added the HW() function and a comment explaining the purpose of generating a unique, and unalterable, salt.

> ::: services/common/hawk.js
> @@ +124,5 @@
> > +  get skew() { 
> > +    return this._skew;
> > +  },
> > +
> > +  now: function() {
> 
> Might want to remove the dump() call. Also, this seems redundant with
> _currentDate.. is this function used? If not, it should get removed.

Good point.  Removed.

> @@ +183,5 @@
> > +
> > +      if (xhr.status === 401 && this._retries === 0) {
> > +        // Retry once if we were rejected due to a bad timestamp.
> > +        // Clock skew is adjusted already in the top of this function.
> > +        this._retries += 1;
> 
> This "_retries" is shared among all calls that use this HawkClient object,
> which is a bit weird. This probably behaves correctly if there are no
> overlapping requests, but if requests were to overlap, I think the all but
> the first would be falsely abandoned. OTOH, I think this approach safely
> avoids ever getting stuck: the first non-retried failure will reset the
> counter, which is great.
> 
> The ideal implementation might use an independent "allow_retry" boolean flag
> for each call, passed into request() that defaults to 'true', but which gets
> 'false' in the retrying path.
> 
> I'm neutral on whether we should make that change: it's probably fine to
> ship it this way, but we should clean it up sooner or later.

No, you're right.  That was an oversight.  I've modified the function to take an optional "retryOK" parameter.  It defaults to true; on retry, it is set to false.

> @@ +190,5 @@
> > +            this.request(path, method, credentials, jsonPayload));
> > +      }
> > +
> > +      this._retries = 0;
> > +      if (xhr.status !== 200 || jsonResponse.error) {
> 
> There are some non-200 success codes (like "201 CREATED" or "202
> ACCEPTED").. fxa-auth-server doesn't use any of them right now, but the Sync
> code might (I dunno). You might want to relax this to accept 200-299 instead
> of just 200.

Done.  We'll probably want a follow-up bug to pass more information to callers.  They may want access to the xhr object, for example, to know what the actual status code was.

> @@ +198,5 @@
> > +        return deferred.reject(this._constructError(xhr, "Request failed"));
> > +      }
> > +      // It's up to the caller to know how to decode the response.
> > +      // We just return the raw text.
> > +      deferred.resolve(xhr.responseText);
> 
> It's a little bit weird to have the success path return a string, but the
> error path return an object. But I guess that's the cleanest way to avoid
> imposing too much policy on the client's behavior (using JSON or otherwise
> in the response), so yeah, keep doing that.

I was torn by this, too, but couldn't think of a better way.  It seems like another reason to make a follow-up bug to make a fuller response object for us to return from request(), so that both responses and errors return an object, and the caller can be given more information.

> @@ +222,5 @@
> > +      let header = CryptoUtils.computeHAWK(uri, method, options);
> > +      xhr.setRequestHeader("authorization", header.field);
> > +    }
> > +
> > +    xhr.setRequestHeader("Content-Type", "application/json");
> 
> It might be good to add a docs note that this function can only be used for
> JSON request bodies. Since we *do* allow non-JSON responses, someone might
> conclude that they could use text/plain for the request-side too, and the
> hard-coded Content-Type might surprise them.

Good point.  Definitely easy to make an error here.

I've changed the name of the parameter to payloadObject.

I've added a check in the function to ensure that the thing really is an object, and a test to check this case.

On the error case, I've decided to throw rather than reject a promise, since it's an error on function invocation itself.  I hope that's correct.

> ::: services/common/tests/unit/test_hawk.js
> @@ +146,5 @@
> > +
> > +  do_check_eq(client.skew, 0);
> > +
> > +  let response = yield client.request("/foo", method);
> > +  do_check_neq(client.skew, 0);
> 
> should we test that skew is about-equal oneHourInMS here?

Good point.  I've updated it, and in one other test as well, to ensure that the delta is less than one second from the expected skew.  I hope this doesn't cause me some intermittent oranges down the road.

> @@ +190,5 @@
> > +    return client.skew;
> > +  }
> > +
> > +  client._currentDate = () => {
> > +    return Date.now() - 1000 * 60 * 60 * 12;
> 
> how about defining SECOND=1000, MINUTE=60*SECOND, HOUR=60*MINUTE, and using
> 12*HOUR here?

Much clearer.  Done.

> @@ +375,5 @@
> > +
> > +  yield deferredStop(server);
> > +});
> > +
> > +// End of tests.
> 
> Could you also add a test that returns a 500 error on the first request, and
> asserts that it only gets called once? 401 should be the only error that
> provokes a retry: everything else should give up without retrying.

Yes.  Done.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +19,5 @@
> >  try {
> >    _host = Services.prefs.getCharPref("identity.fxaccounts.auth.uri");
> >  } catch(keepDefault) {}
> >  
> > +// Default prefix can be changed with pref 'identity.fxaccounts.auth.prefix'
> 
> Did you find a need to modify this? Given how closely this HKDF-contextInfo
> string is tied to the protocol implementation in the rest of the file, I'm
> not sure how useful/safe it'd be to expose it in a pref.

Done, as described above.

> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +384,5 @@
> >    // to the "we're ready" stage.
> >    yield fxa.setSignedInUser(creds);
> >  
> >    _("== ready to go\n");
> > +  // XXX i did not understand this - really set to May 16 1974 ??
> 
> I don't remember why either: feel free to just delete the old confused stuff.

Note to posterity - Brian and I worked together to revise this test.

> @@ +451,3 @@
> >    exp = Number(payload.exp);
> > +  do_check_true(start + TWO_MINUTES_MS <= exp);
> > +  // XXX is this right?
> 
> Hm, something does feel funny here. We're setting start/finish to real live
> times, but making everything else (via _now_is=) use fixed/simulated dates.
> But maybe the FxAccounts class isn't really mocking out *all* of the clocks:
> when we create a cert or assertion, do we set those internal validUntil
> values using the mocked now(), or with a raw "new Date()"?
> 
> It's possible that it is right, and furthermore the first check would be
> more correct as: start + ONE_DAY_MS + TWO_MINUTES_MS <= exp . I'm having
> problems getting my brain around it, though.
> 
> It might be easier to read if we rewrote the whole test to use "now" as the
> wristwatch of our imaginary narrator, so to speak, a variable that's updated
> as we simulate time progressing. At present "now" is really just a
> realistic-looking value captured when the test starts (and could be set to
> any other plausible value, like that 138M value that got deleted, without
> ill effect). If we replaced line 435 with something like "now += ONE_DAY_MS;
> fxa._now_is=now", then all the interval-membership tests could reference
> "now" and the normal offsets (TWO_MINUTES_MS for the expiration time), and
> *not* include the artificial offsets like ONE_DAY_MS.

Second note to posterity - Brian and I worked on this bit as well.  I've commented the test to help clarify the intent.

> ::: services/sync/modules/browserid_identity.js
> @@ +362,5 @@
> >      method = method || httpObject.method;
> > +
> > +    // Get the local clock skew from the Firefox Accounts server.  This should
> > +    // be close to the offset from the storage server.
> > +    dump("get auth heaader: now, skew = " + this._now() + ", " + this._skew + "\n");
> 
> should we leave dump() calls in? I'm not sure what the coding guidelines say.

Thanks for catching this.  No, we don't want dump() calls in shipping code.

> ::: services/sync/tests/unit/test_browserid_identity.js
> @@ +94,5 @@
> > +add_test(function test_resourceAuthenticatorSkew() {
> > +  _("BrowserIDManager Resource Authenticator compensates for clock skew in Hawk header.");
> > +
> > +  // Clock is skewed 12 hours into the future
> > +  let twelveHours = 1000 * 60 * 60 * 12;
> 
> would be nice to be consistent with the other tests, maybe 12*HOURS or
> something

Agreed.  Fixed.

> @@ +97,5 @@
> > +  // Clock is skewed 12 hours into the future
> > +  let twelveHours = 1000 * 60 * 60 * 12;
> > +  let now = Date.now() + twelveHours;
> > +  let browseridManager = new BrowserIDManager();
> > +  let hawkClient = new HawkClient("https://example.net", "/foo/v1/");
> 
> tiny nit, but I'd encourage putting the "v1" in the per-call path, and
> leaving it out of the static per-HAWKClient-object prefix, so that readers
> of later code can tell that the endpoint name is "v1/something". Obviously
> it doesn't matter in a test, and doesn't make a practical difference
> elsewhere, but it's an opportunity to educate future readers.

Thank you.  Fixed.

> ::: toolkit/identity/jwcrypto.jsm
> @@ +127,5 @@
> > +   *
> > +   * @param aOptions (optional)
> > +   *        Can include:
> > +   *        {
> > +   *          skew: <clock skew in milliseconds>,
> 
> nit: mention polarity briefly

Done.

Comment 19

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

r=warner

I'd like to get rnewman's review before landing this in services

Thanks!
j

Comment 20

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

Rebased

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 8364016 [details] [diff] [review]
Tolerate skew in HAWK requests and jwcrypto assertions

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

General question (which isn't specific to this patch): I'm not sure why we're mixing in another HTTP client library here, rather than using RESTRequest with an authenticator (which browserid_identity.js already does).

(I wonder if the use of XHR plus spinning here results in the hangs that Mark spotted?)

Regardless of whether it's introducing novel and exciting bugs, it's duplicating a lot of well-tested service code, and probably omitting stuff. E.g., I see that RESTRequest specifies charset, handles timeouts, aborting, logging, cert handling, redirects, etc. etc. (See TokenAuthenticatedRESTRequest for an example of extending RESTRequest to use more sophisticated authentication.)

I would rather see hawk.js implemented as an authenticator with a hook for date handling -- just as we do for x-weave-backoff, which is a similar process.

This will likely make it simpler to add in the necessary backoff handling for Hawk-authenticated resources, and to centralize per-host skew measurements (see <https://hg.mozilla.org/mozilla-central/rev/07195f5c3eb3> for how we did this in Android Sync).

::: services/common/hawk.js
@@ +1,1 @@
> +"use strict";

License block.

@@ +200,5 @@
> +    };
> +
> +    xhr.open(method, URI);
> +    xhr.channel.loadFlags = Ci.nsIChannel.LOAD_BYPASS_CACHE |
> +                            Ci.nsIChannel.INHIBIT_CACHING;

Resource.DEFAULT_LOAD_FLAGS. You don't want to be sending cookies along with these requests.

::: toolkit/identity/jwcrypto.jsm
@@ +99,5 @@
> +   *        Current date in milliseconds.  Useful for mocking clock
> +   *        skew in testing.
> +   */
> +  getExpiration: function(skew=0, now=Date.now()) {
> +    return now - skew + ASSERTION_LIFETIME_MS;

Is there a reason why the assertion lifetime itself isn't a parameter, with that value being DEFAULT_ASSERTION_LIFETIME_MILLIS?

@@ +128,5 @@
> +   * @param aOptions (optional)
> +   *        Can include:
> +   *        {
> +   *          skew: <clock skew in milliseconds>,
> +   *          now: <current date in milliseconds>

duration: <validity duration for this assertion in milliseconds>

Comment 22

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions (WIP)

- rebased over patch for bug 962849
- added lifetime as param for jwcrypto getAssertion
- addressing rnewman's comments (WIP - not done)

Comment 23

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Richard Newman [:rnewman] from comment #21)
> Comment on attachment 8364016 [details] [diff] [review]
> Tolerate skew in HAWK requests and jwcrypto assertions

Thanks for your review, Richard.

> Review of attachment 8364016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General question (which isn't specific to this patch): I'm not sure why
> we're mixing in another HTTP client library here, rather than using
> RESTRequest with an authenticator (which browserid_identity.js already does).
> 
> (I wonder if the use of XHR plus spinning here results in the hangs that
> Mark spotted?)
> 
> Regardless of whether it's introducing novel and exciting bugs, it's
> duplicating a lot of well-tested service code, and probably omitting stuff.
> E.g., I see that RESTRequest specifies charset, handles timeouts, aborting,
> logging, cert handling, redirects, etc. etc. (See
> TokenAuthenticatedRESTRequest for an example of extending RESTRequest to use
> more sophisticated authentication.)
> 
> I would rather see hawk.js implemented as an authenticator with a hook for
> date handling -- just as we do for x-weave-backoff, which is a similar
> process.
> 
> This will likely make it simpler to add in the necessary backoff handling
> for Hawk-authenticated resources, and to centralize per-host skew
> measurements (see <https://hg.mozilla.org/mozilla-central/rev/07195f5c3eb3>
> for how we did this in Android Sync).

Thanks for the references and the suggestion.  I certainly don't want to re-invent any wheels.  I can work on this, but it will take me a little while to sort it all out and re-write the tests.

> ::: services/common/hawk.js
> @@ +1,1 @@
> > +"use strict";
> 
> License block.

Thank you - fixed.

> @@ +200,5 @@
> > +    };
> > +
> > +    xhr.open(method, URI);
> > +    xhr.channel.loadFlags = Ci.nsIChannel.LOAD_BYPASS_CACHE |
> > +                            Ci.nsIChannel.INHIBIT_CACHING;
> 
> Resource.DEFAULT_LOAD_FLAGS. You don't want to be sending cookies along with
> these requests.

Got it.

> ::: toolkit/identity/jwcrypto.jsm
> @@ +99,5 @@
> > +   *        Current date in milliseconds.  Useful for mocking clock
> > +   *        skew in testing.
> > +   */
> > +  getExpiration: function(skew=0, now=Date.now()) {
> > +    return now - skew + ASSERTION_LIFETIME_MS;
> 
> Is there a reason why the assertion lifetime itself isn't a parameter, with
> that value being DEFAULT_ASSERTION_LIFETIME_MILLIS?

Good idea.  I've added it provisionally, but we'll need to sync this up with the main jwcrypto library if it's to be a new feature.  I'll consult with François.

> @@ +128,5 @@
> > +   * @param aOptions (optional)
> > +   *        Can include:
> > +   *        {
> > +   *          skew: <clock skew in milliseconds>,
> > +   *          now: <current date in milliseconds>
> 
> duration: <validity duration for this assertion in milliseconds>

Yes.  And test added.

Thanks, Richard,

As I say, It'll take me a little longer to address all your concerns.  This is only a start.

Cheers
j

Comment 24

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Hmm - and my patch appears to have caused a regression in the test_client.js test.  In line 135, client.signOut() is blowing up with 'JavaScript component does not have a method named: "available"'.  I'm guessing this has something to do with available bytes in an input stream of some sort.  I'll find it and fix it.

Comment 25

[Richard Newman [:rnewman]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #23)

> Thanks for the references and the suggestion.  I certainly don't want to
> re-invent any wheels.  I can work on this, but it will take me a little
> while to sort it all out and re-write the tests.

Let me know if you need any input, and do feel free to push back if you do some digging and decide it's not achievable.

Comment 26

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

The token server can respond with a 401 and the message "invalid-timestamp", meaning that authentication failed because the included timestamp differed too greatly from the server's current time.  This would signal the value that we should adjust skew to.

Comment 27

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

- replace xhr with new hawk-authenticated RESTRequest
- changed 'skew' to 'localtimeOffsetMsec' everywhere to match crypto usage
- rewrote tests

Comment 28

[Richard Newman [:rnewman]]

Is this ready to review, Jed?

Comment 29

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Richard, almost!  I just want 20 minutes to do one last edit of comments etc.

I will post the final revision and ask you for review.  thanks!

Comment 30

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

I'm hoping this successfully addresses the comments made in Comment 21

On a high level, the changes are:

- Now uses a RESTRequest instead of XHR.  (New HAWKAuthenticatedRESTRequest)
- More flexible args to jwcrypto getAssertion
- Changed skew to localtimeOffsetMsec, to match crypto utils
- Updated all existing tests
- Added tests for new HAWKAuthenteticatedRESTRequest

Thanks for your review, richard,
j

Comment 31

[Richard Newman [:rnewman]]

Comment on attachment 8368262 [details] [diff] [review]
Tolerate skew in HAWK requests and jwcrypto assertions

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

*phew*

Looks good, I think. Just nits.

Thanks for writing so many tests!

::: services/common/hawk.js
@@ +28,5 @@
> +
> +const {interfaces: Ci, utils: Cu} = Components;
> +
> +const XMLHttpRequest =
> +  Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1");

This can be removed.

@@ +71,5 @@
> +    return {
> +      error: errorString,
> +      message: restResponse.statusText,
> +      code: restResponse.status,
> +      errno: restResponse.status

Preserve the old comment:

// When things really blow up, reconstruct an error object that follows the general format		
// of the server on error responses.

@@ +77,5 @@
> +  },
> +
> +  /*
> +   *
> +   * Update clock offset by determining difference from date give in the (RFC

s/give/given

@@ +92,5 @@
> +   * https://github.com/hueniverse/hawk#replay-protection
> +   */
> +  _updateClockOffset: function(dateString) {
> +    try {
> +      let serverDateMsec = (new Date(dateString)).valueOf();

= Date.parse(dateString);

::: services/common/rest.js
@@ +634,5 @@
> +   */
> +  get statusText() {
> +    let statusText;
> +    try {
> +      let channel = this.request.channel.QueryInterface(Ci.nsIHttpChannel);

I know this method is aping `status()`, but the channel is already QIed to nsIHttpChannel, so you can just use this.request.channel. Feel free to fix status(), too.

@@ +771,5 @@
> +        now: this.now,
> +        localtimeOffsetMsec: this.localtimeOffsetMsec,
> +        credentials: this.credentials,
> +        payload: data && JSON.stringify(data) || "",
> +        contentType: "application/json",

charset?

@@ +773,5 @@
> +        credentials: this.credentials,
> +        payload: data && JSON.stringify(data) || "",
> +        contentType: "application/json",
> +      };
> +      let header = CryptoUtils.computeHAWK(this.uri, method, options);

Can this fail?

::: services/common/tests/unit/test_hawk.js
@@ +1,1 @@
> +Cu.import("resource://gre/modules/Promise.jsm");

License block at top of file.

@@ +15,5 @@
> +
> +add_task(function test_now() {
> +  let client = new HawkClient("https://example.com");
> +
> +  do_check_true(client.now() - (new Date()) < SECOND_MS);

Date.now() instead of (new Date())

@@ +127,5 @@
> +  yield deferredStop(server);
> +});
> +
> +add_task(function test_server_error_json() {
> +  let message = JSON.stringify({error: "Cannot get ye flask."});

+1

::: services/common/tests/unit/test_restrequest.js
@@ +832,5 @@
>    });
>  });
> +
> +add_test(function test_hawk_authenticated_request() {
> +  do_test_pending();

You're mixing do_test_pending...

@@ +878,5 @@
> +    do_check_eq(200, this.response.status);
> +    do_check_eq(this.response.body, "yay");
> +    do_check_true(onProgressCalled);
> +    do_test_finished();
> +    server.stop(run_next_test);

... with run_next_test. That seems weird, but it's late so I'll take your word for it -- but make sure this is correct!

::: services/fxaccounts/FxAccountsClient.jsm
@@ +78,5 @@
> +   * Not used by this module, but made available to the FxAccounts.jsm
> +   * that uses this client.
> +   */
> +  now: function() {
> +    dump("fxaclient now: " + this.hawk.now() + "\n");

Does this need to stay?

@@ +262,3 @@
>            // the account doesn't exist
>            if (err.errno === 102) {
> +            log.debug("returning false for errno 102\n\n");

If this is the usual logger, you don't need to append newlines. You only need that for dump().

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +413,5 @@
>    let header = JSON.parse(atob(p2[0]));
>    _("HEADER: "+JSON.stringify(header)+"\n");
>    do_check_eq(header.alg, "DS128");
>    let payload = JSON.parse(atob(p2[1]));
>    _("PAYLOAD: "+JSON.stringify(payload)+"\n");

All of this code would be way easier to read with some spaces around operators :P

@@ +417,5 @@
>    _("PAYLOAD: "+JSON.stringify(payload)+"\n");
>    do_check_eq(payload.aud, "audience.example.com");
> +  do_check_eq(fxa.internal.keyPair.validUntil, start + KEY_LIFETIME);
> +  do_check_eq(fxa.internal.cert.validUntil, start + CERT_LIFETIME);
> +  _("delta: "+(new Date(payload.exp) - start)+"\n");

Date.parse?

@@ +425,5 @@
>  
>    // Reset for next call.
>    fxa._d_signCertificate = Promise.defer();
>  
> +  // Getting a new assertion "soon" (i.e., w/o incrementing "now"), even for

Thank you for adding that comma. One of my pet peeves.

::: services/sync/modules/browserid_identity.js
@@ +136,5 @@
>          throw err;
>        });
>        // and we are done - the fetch continues on in the background...
>      }).then(null, err => {
> +      this._log.error("Processing logged in account: "+err.message);

Spaces around operators.

@@ +480,5 @@
> +      now: this._now(),
> +      localtimeOffsetMsec: this._localtimeOffsetMsec,
> +      credentials: credentials,
> +    };
> +    dump("*** options: " + JSON.stringify(options) +"\n");

Remove this?

::: toolkit/identity/tests/unit/test_jwcrypto.js
@@ +130,5 @@
> +
> +
> +  // Use an arbitrary date in the past to ensure we don't accidentally pass
> +  // this test with current dates, missing offsets, etc.
> +  let serverMsec = (new Date("Tue Oct 31 2000 00:00:00 GMT-0800")).valueOf();

Date.parse.

@@ +211,5 @@
> +  if (typeof(signedObject) != 'string') {
> +    throw new Error("malformed signature " + typeof(signedObject));
> +  }
> +
> +  var parts = signedObject.split(".");

`let`. (Throughout.)

@@ +221,5 @@
> +  var payloadSegment = parts[1];
> +  var cryptoSegment = parts[2];
> +
> +  // we verify based on the actual string
> +  // FIXME: we should validate that the header contains only proper fields

File a bug?

@@ +225,5 @@
> +  // FIXME: we should validate that the header contains only proper fields
> +  var header = JSON.parse(CryptoService.base64UrlDecode(headerSegment));
> +  var payload = JSON.parse(CryptoService.base64UrlDecode(payloadSegment));
> +
> +  // XXX signature = b64urltohex(cryptoSecment);

Remove.

Comment 32

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Richard Newman [:rnewman] from comment #31)
> Comment on attachment 8368262 [details] [diff] [review]
> Tolerate skew in HAWK requests and jwcrypto assertions
> 
> Review of attachment 8368262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *phew*
> 
> Looks good, I think. Just nits.
> 
> Thanks for writing so many tests!

And thank you for such a prompt and thorough review of such a large patch!

> ::: services/common/hawk.js
> @@ +28,5 @@
> > +
> > +const {interfaces: Ci, utils: Cu} = Components;
> > +
> > +const XMLHttpRequest =
> > +  Components.Constructor("@mozilla.org/xmlextras/xmlhttprequest;1");
> 
> This can be removed.

Oops.  Yes.

> @@ +71,5 @@
> > +    return {
> > +      error: errorString,
> > +      message: restResponse.statusText,
> > +      code: restResponse.status,
> > +      errno: restResponse.status
> 
> Preserve the old comment:
> 
> // When things really blow up, reconstruct an error object that follows the
> general format		
> // of the server on error responses.

Restored

> @@ +77,5 @@
> > +  },
> > +
> > +  /*
> > +   *
> > +   * Update clock offset by determining difference from date give in the (RFC
> 
> s/give/given

Fixed

> @@ +92,5 @@
> > +   * https://github.com/hueniverse/hawk#replay-protection
> > +   */
> > +  _updateClockOffset: function(dateString) {
> > +    try {
> > +      let serverDateMsec = (new Date(dateString)).valueOf();
> 
> = Date.parse(dateString);

Indeed.  Thanks.

> ::: services/common/rest.js
> @@ +634,5 @@
> > +   */
> > +  get statusText() {
> > +    let statusText;
> > +    try {
> > +      let channel = this.request.channel.QueryInterface(Ci.nsIHttpChannel);
> 
> I know this method is aping `status()`, but the channel is already QIed to
> nsIHttpChannel, so you can just use this.request.channel. Feel free to fix
> status(), too.

I see.  Fixed, along with status() and success() as well.

> @@ +771,5 @@
> > +        now: this.now,
> > +        localtimeOffsetMsec: this.localtimeOffsetMsec,
> > +        credentials: this.credentials,
> > +        payload: data && JSON.stringify(data) || "",
> > +        contentType: "application/json",
> 
> charset?

Ooh - thank you.  That's bitten us before.  Fixed.

> @@ +773,5 @@
> > +        credentials: this.credentials,
> > +        payload: data && JSON.stringify(data) || "",
> > +        contentType: "application/json",
> > +      };
> > +      let header = CryptoUtils.computeHAWK(this.uri, method, options);
> 
> Can this fail?

I'm not sure what the behavior should be if it does.  It doesn't appear to have been a concern in browserid_identity.js, the other module that calls it.  I've flied Bug 966353 to investigate the failure modes and see what those modules need to do in case of an error.

> ::: services/common/tests/unit/test_hawk.js
> @@ +1,1 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> 
> License block at top of file.

Thank you

> @@ +15,5 @@
> > +
> > +add_task(function test_now() {
> > +  let client = new HawkClient("https://example.com");
> > +
> > +  do_check_true(client.now() - (new Date()) < SECOND_MS);
> 
> Date.now() instead of (new Date())

Oops.  Thanks.

> @@ +127,5 @@
> > +  yield deferredStop(server);
> > +});
> > +
> > +add_task(function test_server_error_json() {
> > +  let message = JSON.stringify({error: "Cannot get ye flask."});
> 
> +1

:)

> ::: services/common/tests/unit/test_restrequest.js
> @@ +832,5 @@
> >    });
> >  });
> > +
> > +add_test(function test_hawk_authenticated_request() {
> > +  do_test_pending();
> 
> You're mixing do_test_pending...
> 
> @@ +878,5 @@
> > +    do_check_eq(200, this.response.status);
> > +    do_check_eq(this.response.body, "yay");
> > +    do_check_true(onProgressCalled);
> > +    do_test_finished();
> > +    server.stop(run_next_test);
> 
> ... with run_next_test. That seems weird, but it's late so I'll take your
> word for it -- but make sure this is correct!

Yeah ... that doesn't look right.  I've fixed it.  Thanks for catching it.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +78,5 @@
> > +   * Not used by this module, but made available to the FxAccounts.jsm
> > +   * that uses this client.
> > +   */
> > +  now: function() {
> > +    dump("fxaclient now: " + this.hawk.now() + "\n");
> 
> Does this need to stay?

Nope.

> @@ +262,3 @@
> >            // the account doesn't exist
> >            if (err.errno === 102) {
> > +            log.debug("returning false for errno 102\n\n");
> 
> If this is the usual logger, you don't need to append newlines. You only
> need that for dump().

Thanks.  I've fixed this and some other occurrences in this and another file.

> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +413,5 @@
> >    let header = JSON.parse(atob(p2[0]));
> >    _("HEADER: "+JSON.stringify(header)+"\n");
> >    do_check_eq(header.alg, "DS128");
> >    let payload = JSON.parse(atob(p2[1]));
> >    _("PAYLOAD: "+JSON.stringify(payload)+"\n");
> 
> All of this code would be way easier to read with some spaces around
> operators :P

Agreed.  I've fixed the spacing.  (I had initially left it alone because I didn't want to confuse the blame history on a pre-existing test.  But they are just printf statements, after all.)

> @@ +417,5 @@
> >    _("PAYLOAD: "+JSON.stringify(payload)+"\n");
> >    do_check_eq(payload.aud, "audience.example.com");
> > +  do_check_eq(fxa.internal.keyPair.validUntil, start + KEY_LIFETIME);
> > +  do_check_eq(fxa.internal.cert.validUntil, start + CERT_LIFETIME);
> > +  _("delta: "+(new Date(payload.exp) - start)+"\n");
> 
> Date.parse?

is becoming my new best friend.

> @@ +425,5 @@
> >  
> >    // Reset for next call.
> >    fxa._d_signCertificate = Promise.defer();
> >  
> > +  // Getting a new assertion "soon" (i.e., w/o incrementing "now"), even for
> 
> Thank you for adding that comma. One of my pet peeves.

:)

> ::: services/sync/modules/browserid_identity.js
> @@ +136,5 @@
> >          throw err;
> >        });
> >        // and we are done - the fetch continues on in the background...
> >      }).then(null, err => {
> > +      this._log.error("Processing logged in account: "+err.message);
> 
> Spaces around operators.

Fixed

> @@ +480,5 @@
> > +      now: this._now(),
> > +      localtimeOffsetMsec: this._localtimeOffsetMsec,
> > +      credentials: credentials,
> > +    };
> > +    dump("*** options: " + JSON.stringify(options) +"\n");
> 
> Remove this?

Removed

> ::: toolkit/identity/tests/unit/test_jwcrypto.js
> @@ +130,5 @@
> > +
> > +
> > +  // Use an arbitrary date in the past to ensure we don't accidentally pass
> > +  // this test with current dates, missing offsets, etc.
> > +  let serverMsec = (new Date("Tue Oct 31 2000 00:00:00 GMT-0800")).valueOf();
> 
> Date.parse.

Loving it

> @@ +211,5 @@
> > +  if (typeof(signedObject) != 'string') {
> > +    throw new Error("malformed signature " + typeof(signedObject));
> > +  }
> > +
> > +  var parts = signedObject.split(".");
> 
> `let`. (Throughout.)

Fixed

> @@ +221,5 @@
> > +  var payloadSegment = parts[1];
> > +  var cryptoSegment = parts[2];
> > +
> > +  // we verify based on the actual string
> > +  // FIXME: we should validate that the header contains only proper fields
> 
> File a bug?

It wasn't a big deal to add the tests to verify.  I've done so.

> @@ +225,5 @@
> > +  // FIXME: we should validate that the header contains only proper fields
> > +  var header = JSON.parse(CryptoService.base64UrlDecode(headerSegment));
> > +  var payload = JSON.parse(CryptoService.base64UrlDecode(payloadSegment));
> > +
> > +  // XXX signature = b64urltohex(cryptoSecment);
> 
> Remove.

Done.

Thank you again, Richard.  Cheers, j

Comment 33

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Tolerate skew in HAWK requests and jwcrypto assertions

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ebb67b1952de

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebb67b1952de