With Loop's current hawk client implementation we run the risk of hawk credentials being rejected due to clock skew. We should look at changing from using HAWKAuthenticatedRESTRequest to HawkClient. HawkClient automatically manages clock skew issues, it also has the nicer interface of using promises for the xhr requests.

This is what it takes to make HawkClient return all the response details for a request, and to make the deriveHawkCredentials functions into common code. I'll probably split this off to a separate bug for review/checkin. I'm running this and the next patch through try to make sure I've not broken anything obvious.

This makes Loop use HawkClient rather than HAWKAuthenticatedRESTRequest directly. This also simplifies the functions as we have the promise returned from HawkClient. The hawkRequest is a separate function so that we can easily add more calls for it later, which we want to do in bug 1020876. I've still got to fix up documentation on these two patches and do the final checks before asking review.

(In reply to Mark Banner (:standard8) from comment #2) > I've still got to fix up documentation on these two patches and do the final > checks before asking review. The patch here also probably vaguely depends on bug 1022689 (at least for bitrot purposes), so we'll get that landed first.

This is the same as the previous version, but I've decided to keep it on this bug. This is all the core changes necessary. I'll finish tidying up the other patch tomorrow.

Updated patch to fix the mobile code, and add a little extra information on deriveHawkCredentials about the context parameter.

This now works fine, and I've completed the documentation. There's some existing tests for the hawk parts already, so I don't think we need to extend those (they didn't need update). Both patches have been pushed to try server here: https://tbpl.mozilla.org/?tree=Try&rev=8db2ab799154

FWIW, we currently don't enforce timestamps and nonce reuse in the FxA and Sync servers. Here's some context on that: http://www.mail-archive.com/sync-dev@mozilla.org/msg00786.html

But it would still be a good idea to use a common Hawk client. :)

Comment on attachment 8441351 [details] [diff] [review] Part 1 - Make HawkClient return all the response details for a request, and make deriveHawkCredentials common code v2 Review of attachment 8441351 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, Mark. Changing the return value from response.body to the whole response seems right. Thanks for reorganizing the tests, too.

Comment on attachment 8441352 [details] [diff] [review] Part 2 - Change Loop to use HawkClient to reduce the possibility of clock skew issues with timestamps. Review of attachment 8441352 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/MozLoopService.jsm @@ +216,5 @@ > */ > registerWithLoopServer: function(pushUrl, noRetry) { > + this.hawkRequest("/registration", "POST", { simple_push_url: pushUrl}) > + .then((response) => { > + // If this failed, return early, as we got an invalid token. note it is possible response.headers will be null in the case where no request could actually be made (eg, a network issue) - so I think you need to handle this here. Also, in storeSessionToken failing and the non-headers case it looks like you want to reject the promise?

(In reply to Mark Hammond [:markh] from comment #10) > Comment on attachment 8441352 [details] [diff] [review] > Part 2 - Change Loop to use HawkClient to reduce the possibility of clock > skew issues with timestamps. > > Review of attachment 8441352 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/loop/MozLoopService.jsm > @@ +216,5 @@ > > */ > > registerWithLoopServer: function(pushUrl, noRetry) { > > + this.hawkRequest("/registration", "POST", { simple_push_url: pushUrl}) > > + .then((response) => { > > + // If this failed, return early, as we got an invalid token. > > note it is possible response.headers will be null in the case where no > request could actually be made (eg, a network issue) - so I think you need > to handle this here. As discussed on irc, this would be handled by the rejection block. > Also, in storeSessionToken failing and the non-headers case it looks like > you want to reject the promise? Discussed this on irc, and decided the best way to do this would be to add a comment at the storeSessionToken call site to explain that it resolves the rejection.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82d9d828e62a https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7732b638a3

https://hg.mozilla.org/mozilla-central/rev/82d9d828e62a https://hg.mozilla.org/mozilla-central/rev/cc7732b638a3

Comment on attachment 8441351 [details] [diff] [review] Part 1 - Make HawkClient return all the response details for a request, and make deriveHawkCredentials common code v2 Review of attachment 8441351 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/mobileid/MobileIdentityClient.jsm @@ +105,5 @@ > * } > */ > _deriveHawkCredentials: function(aSessionToken) { > + return deriveHawkCredentials(aSessionToken, CREDENTIALS_DERIVATION_INFO, > + CREDENTIALS_DERIVATION_SIZE); Note that we were doing a |byteAsHex| of the returned |key| parameter that we aren't doing anymore with |deriveHawkCredentials|:(

TEST-UNEXPECTED-FAIL | C:\slave\test\build\xpcshell\tests\services\common\tests\unit\test_hawkrequest.js | test failed (with xpcshell return code: 0), see following log: >>>>>>> TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | Source file C:/slave/test/build/xpcshell/tests/services/common/tests/unit/test_hawkrequest.js contains an error Diagnostic: NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]

Philip: please file new bugs for follow-up issues, especially when they don't affect mozilla-central apps.

bug 1027874 can serve as a follow-up. (In reply to Mark Banner (:standard8) from comment #16) > Philip: please file new bugs for follow-up issues, especially when they > don't affect mozilla-central apps.