Also see: https://bugzilla.mozilla.org/show_bug.cgi?id=820643 This should be a minor change, since we already support MACAuth used in Oauth2.

Here's the first cut. Tests (with vectors copied from the node.js HAWK test suite) pass, but could use more coverage. I went with the node.js API to start, this may or may not fit into our style. (it's also my first m-c patch.. I need to learn both the functionality and the style of this strange new neighborhood, so I'm eager for any and all feedback you can give).

Comment on attachment 801865 [details] [diff] [review] initial patch Review of attachment 801865 [details] [diff] [review]: ----------------------------------------------------------------- I tried to be thorough to illustrate some style points. Looking good! ::: services/crypto/modules/utils.js @@ +55,5 @@ > + * Treat the given message as a bytes string and feed them into the given > + * hasher. Does not return a hash. This can be called multiple times with a > + * single hasher, but eventually you must extract the result yourself. > + */ > + updateBytes: function digestBytes(message, hasher) { Kill the function name; the debugger will pull it out of the object reference (and in any case, they disagree!). @@ +57,5 @@ > + * single hasher, but eventually you must extract the result yourself. > + */ > + updateBytes: function digestBytes(message, hasher) { > + let bytes = [b.charCodeAt() for each (b in message)]; > + hasher.update(bytes, bytes.length); This doesn't seem particularly safe in the presence of non-ASCII: [18:52:01.079] "fi".charCodeAt(0) [18:52:01.082] 64257 nor efficient. Fortunately, this same file provides the idiom: let data = this._utf8Converter.convertToByteArray(message, {}); hasher.update(data, data.length); @@ +353,5 @@ > return header += ', ext="' + ext +'"'; > }, > + > + /** > + * Compute the HAWK header for an HTTP request. This is really "HAWK header values", right? @@ +372,5 @@ > + * nonce - (string) pre-calculated nonce. Should only be defined > + * for testing as this function will generate a > + * cryptographically secure random one if not defined. > + * localtimeOffsetMsec - (number) local clock offset (vs server) > + * payload - (string) payload to include in hash. ENCODING? UTF-8. @@ +373,5 @@ > + * for testing as this function will generate a > + * cryptographically secure random one if not defined. > + * localtimeOffsetMsec - (number) local clock offset (vs server) > + * payload - (string) payload to include in hash. ENCODING? > + * contentType - (string) payload Content-Type Caller must include charset? @@ +389,5 @@ > + * port - (number) > + * hash - (string) payload hash > + * ext - (string) app-specific data > + */ > + computeHAWKHeader: function computeHAWKHeader(uri, method, options) { Again, kill function name. And probably computeHAWKHeaderValues 'cos you're not returning a string header, right? @@ +391,5 @@ > + * ext - (string) app-specific data > + */ > + computeHAWKHeader: function computeHAWKHeader(uri, method, options) { > + let credentials = options.credentials; > + let ts = options.ts || Math.floor((Date.now()+(options.localtimeOffsetMsec || 0)) / 1000); Nit: spaces around arithmetic operators. Another thing to consider: for testability, it's often convenient to be able to impose an instant in time. Think about whether you want to add , now=Date.now() to the function signature. @@ +409,5 @@ > + ts: ts, > + nonce: options.nonce || btoa(CryptoUtils.generateRandomBytes(8)), > + method: method.toUpperCase(), > + resource: uri.path, // TODO: include .search ? > + host: uri.asciiHost.toLowerCase(), Make sure this has test coverage. @@ +416,5 @@ > + ext: options.ext > + }; > + > + let algo; > + if (credentials.algorithm == "sha1") Move this block right to the front -- fail early without generating bytes etc. @@ +418,5 @@ > + > + let algo; > + if (credentials.algorithm == "sha1") > + algo = Ci.nsICryptoHMAC.SHA1; > + else if (credentials.algorithm == "sha256") Always { } else { } @@ +425,5 @@ > + throw new Error("Unsupported algorithm: " + credentials.algorithm); > + > + function hupdate(hasher, str) { > + hasher.update(str, str.length); > + } This function isn't used. @@ +429,5 @@ > + } > + > + let contentType = ""; > + if (options.contentType) > + contentType = options.contentType.split(";")[0].trim().toLowerCase(); .split allocates a new array and two (or more) string references. indexOf and substring are 3x faster (http://jsperf.com/strip-trailing-params). contentType = options.contentType || ""; if (options.contentType) { let i = contentType.indexOf(";"); contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase(); } If that's too messy, at least do: contentType.replace(/;.*$/, ""); @@ +433,5 @@ > + contentType = options.contentType.split(";")[0].trim().toLowerCase(); > + > + if (!artifacts.hash && options.hasOwnProperty("payload")) { > + let hasher = Cc["@mozilla.org/security/hash;1"] > + .createInstance(Ci.nsICryptoHash); Nit: align . with [ @@ +434,5 @@ > + > + if (!artifacts.hash && options.hasOwnProperty("payload")) { > + let hasher = Cc["@mozilla.org/security/hash;1"] > + .createInstance(Ci.nsICryptoHash); > + if (credentials.algorithm == "sha1") {} @@ +436,5 @@ > + let hasher = Cc["@mozilla.org/security/hash;1"] > + .createInstance(Ci.nsICryptoHash); > + if (credentials.algorithm == "sha1") > + hasher.init(hasher.SHA1); > + else if (credentials.algorithm == "sha256") Can we dispatch on algo instead? @@ +456,5 @@ > + artifacts.host + "\n" + > + artifacts.port + "\n" + > + artifacts.hash + "\n"); > + if (artifacts.ext) > + requestString += artifacts.ext.replace("\\", "\\\\").replace("\n", "\\n"); {} @@ +475,5 @@ > + (artifacts.ext ? ('ext="' + escape(artifacts.ext) + '", ') : '') + > + 'mac="' + mac_b64 + '"'); > + return { > + artifacts: artifacts, > + field: header Trailing comma. ::: services/crypto/tests/unit/test_utils_hawk.js @@ +11,5 @@ > + run_next_test(); > +} > + > +add_test(function test_hawk() { > + _("Ensure HAWK MAC generation works as expected."); Don't really need this; seeing the test output show "test_hawk" should be enough, I think. @@ +16,5 @@ > + > + let compute = CryptoUtils.computeHAWKHeader; > + > + // vectors copied from the HAWK (node.js) tests > + var credentials_sha1 = { s/var/let. @@ +19,5 @@ > + // vectors copied from the HAWK (node.js) tests > + var credentials_sha1 = { > + id: '123456', > + key: '2983d45yun89q', > + algorithm: 'sha1' General point: we usually include a trailing comma on the last item, because it's syntactically valid and it makes diffs much more pleasant. (Throughout.) @@ +35,5 @@ > + nonce: nonce, > + payload: "something to write about" > + }); > + > + // note HAWK spec uses non-urlsafe base64 for its output MAC string Nit: I tend to prefer full sentences (caps and period) for comments. (Throughout.) @@ +51,5 @@ > + // note: artifacts.hash is the *payload* hash, not the overall request MAC > + do_check_eq(result.artifacts.hash, "bsvY3IfUllw6V5rvk4tStEvpBhE="); > + do_check_eq(result.artifacts.ext, "Bazinga!"); > + > + var credentials_sha256 = { Let. @@ +58,5 @@ > + algorithm: 'sha256' > + }; > + > + let uri2 = CommonUtils.makeURI("https://example.net/somewhere/over/the/rainbow"); > + result = compute(uri2, method, { credentials: credentials_sha256, Be consistent with whitespace (cf line 32). @@ +102,5 @@ > + > +/* Leaving optional fields out should work, although of course then we can't > + * assert much about the resulting hashes. The resulting header should look > + * roughly like: > + * Hawk id="123456", ts="1378764955", nonce="QkynqsrS44M=", mac="/C5NsoAs2fVn+d/I5wMfwe2Gr1MZyAJ6pFyDHG4Gf9U=" */ Nit: */ on next line. @@ +108,5 @@ > + result = compute(uri2, method, { credentials: credentials_sha256 }); > + let fields = result.field.split(" "); > + do_check_eq(fields[0], 'Hawk'); > + do_check_eq(fields[1], 'id="123456",'); // from creds.id > + do_check_eq(fields[2].slice(0,4), 'ts="'); do_check_true(fields[2].startsWith("ts=")); @@ +113,5 @@ > + /* the HAWK spec calls for seconds-since-epoch, not ms-since-epoch. > + * Warning: this test will fail in the year 33658, and for time travellers > + * who journey earlier than 2001. Please plan accordingly. */ > + do_check_true(result.artifacts.ts > 1000*1000*1000); > + do_check_true(result.artifacts.ts < 1000*1000*1000*1000); Generally we always put spaces around arithmetic operators. For unit-bumping like this, I don't care too much, so up to you. @@ +114,5 @@ > + * Warning: this test will fail in the year 33658, and for time travellers > + * who journey earlier than 2001. Please plan accordingly. */ > + do_check_true(result.artifacts.ts > 1000*1000*1000); > + do_check_true(result.artifacts.ts < 1000*1000*1000*1000); > + do_check_eq(fields[3].slice(0,7), 'nonce="'); startsWith. ::: services/crypto/tests/unit/xpcshell.ini @@ +13,4 @@ > > [test_utils_hkdfExpand.js] > [test_utils_httpmac.js] > +[test_utils_hawk.js] Alphabetical order, plz.

Wow, thanks! This feedback is great. > Kill the function name; the debugger will pull it out of the object > reference (and in any case, they disagree!). Will do. Should I strip them from all the pre-existing functions too (consistency), or just from the ones I added (minimal patch)? > > + let bytes = [b.charCodeAt() for each (b in message)]; > > This doesn't seem particularly safe in the presence of non-ASCII: > > + * payload - (string) payload to include in hash. ENCODING? > > UTF-8. These touch on a deeper issue. The caller gets to chose an arbitrary payload, which can include arbitrary bytes (most higher-level protocols bend over backwards to avoid that, with base64/etc, but I can't enforce that at this layer). My sample case is a caller who sends a hash or some encrypted bytes in their HTTP payload. And JS doesn't provide a single obvious way to manage arbitrary bytes (I've seen array-of-small-numbers, String-of-code-points-0x00-to-0xff, and more recently TypedArray/ArrayBuffer objects). So we need a contract for how that payload is given to computeHAWK() and then hashed. I'm uncomfortable with having the caller pass down (unicode string, encoding), because then the data gets encoded twice (once by computeHAWK() before it feeds the hasher, once by the caller when they give it to the HTTP client code). That makes it possible to do it wrong (if the encoders differ, the MAC won't match, but only when you have non-ASCII in the payload, which delays discovery and makes the bug more expensive). So I prefer the API contract to call for computeHAWK(payload=bytes). Given that, what's the best hacked-up JS-bytestring workaround to use? I went with String-of-0x00-0xff-code-points, since the digestBytes() function already existed and suggested it wasn't too crazy. But utils.js only uses that for HKDF and PBKDF2, and doesn't seem to have any other functions that take HTTP payloads, so that guidance is weak at best. What does HTTP client code (XHR?) in this environment take? > > + * contentType - (string) payload Content-Type > > Caller must include charset? Nope, but it (and any other attributes) will be stripped off. The contentType is covered by the HMAC, but isn't used for anything else (in particular, it doesn't affect interpretation of the payload, see above). > And probably > > computeHAWKHeaderValues > > 'cos you're not returning a string header, right? True. I think I'll use "computeHAWK", to match "computeHTTPMACSHA1". > Another thing to consider: for testability, it's often convenient to be able > to impose an instant in time. Think about whether you want to add > > , now=Date.now() > > to the function signature. In addition to ts= ? Yeah, I guess that makes sense, to also test the offset computation. Will do. I still need to add tests for localtimeOffsetMsec. > > + ts: ts, > > + nonce: options.nonce || btoa(CryptoUtils.generateRandomBytes(8)), > > + method: method.toUpperCase(), > > + resource: uri.path, // TODO: include .search ? > > + host: uri.asciiHost.toLowerCase(), > > Make sure this has test coverage. Coverage added for .method and .host, and already there for .nonce . I'm still working on .search (the node.js library's tests didn't cover that, so I need to run some experiments to get the vectors). > > + let contentType = ""; > > + if (options.contentType) > > + contentType = options.contentType.split(";")[0].trim().toLowerCase(); > > .split allocates a new array and two (or more) string references. indexOf > and substring are 3x faster (http://jsperf.com/strip-trailing-params). > contentType = options.contentType || ""; > if (options.contentType) { > let i = contentType.indexOf(";"); > contentType = contentType.substring(0, (i >= 0) ? i : > undefined).trim().toLowerCase(); > } How about this?: let contentType = options.contentType || ""; let i = contentType.indexOf(";"); contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase(); (benefits: less messy, removes "if" statement from main path. problems: small-named "i" lives in a larger scope than really necessary, executes indexOf and substring even if options.contentType is undefined or "") > > + if (credentials.algorithm == "sha1") > > + hasher.init(hasher.SHA1); > > Can we dispatch on algo instead? Do you mean if (algo === Ci.nsICryptoHMAC.SHA1) ? I don't know if the nsICryptoHash instance's .SHA1 property is the same as the nsICryptoHMAC.SHA1 property. > ::: services/crypto/tests/unit/test_utils_hawk.js > General point: we usually include a trailing comma on the last item, > because it's syntactically valid and it makes diffs much more pleasant. > (Throughout.) Ah, that's nice. This is a mozilla-js -specific feature, right? I remember web-page JS (and/or JSON parsers) rejecting those. > do_check_true(fields[2].startsWith("ts=")); We have startsWith here? How civilized! :) > > + do_check_true(result.artifacts.ts < 1000*1000*1000*1000); > > Generally we always put spaces around arithmetic operators. For > unit-bumping like this, I don't care too much, so up to you. Yeah, for this particular case, I think I prefer the compact form. Inserting spaces would draw attention away from the comparison. > ::: services/crypto/tests/unit/xpcshell.ini > > +[test_utils_hawk.js] > > Alphabetical order, plz. Am I correct in assuming that the "skip-if" on line 12 is attached to the [test_crypto_random.js] that precedes it? So my hawk test will still run on android if I use?: [test_crypto_random.js] # Bug 676977: test hangs consistently on Android skip-if = os == "android" [test_utils_hawk.js] [test_utils_hkdfExpand.js]

This covers most (but not all) of rnewman's feedback. I'll post a third version later today with more improvements.

(In reply to Brian Warner [:warner :bwarner] from comment #3) > Will do. Should I strip them from all the pre-existing functions too > (consistency), or just from the ones I added (minimal patch)? Usually if I'm breaking nearby blame, I'll fix it, but generally err on the side of minimal. > These touch on a deeper issue. Yup, this is a sucky situation. If only we had HttpClient's Entity classes! > > So we need a contract for how that payload is given to computeHAWK() and then > hashed. I'm uncomfortable with having the caller pass down (unicode string, > encoding), because then the data gets encoded twice (once by computeHAWK() > before it feeds the hasher, once by the caller when they give it to the HTTP > client code). That makes it possible to do it wrong (if the encoders differ, > the MAC won't match, but only when you have non-ASCII in the payload, which > delays discovery and makes the bug more expensive). > > So I prefer the API contract to call for computeHAWK(payload=bytes). Given > that, what's the best hacked-up JS-bytestring workaround to use? I went with > String-of-0x00-0xff-code-points, since the digestBytes() function already > existed and suggested it wasn't too crazy. But utils.js only uses that for > HKDF and PBKDF2, and doesn't seem to have any other functions that take HTTP > payloads, so that guidance is weak at best. > > What does HTTP client code (XHR?) in this environment take? Concerns we have here: * Following existing code. * Perf. * Flexibility. I suspect that the common case here will be "I have a small JS string, probably a JSON serialization of a compact object", so (a) let's decide whether that's true, and (b) let's bear that in mind when we think about perf. Existing code here is twofold: there's the codebase you're in (which takes string input and does UTF-8 fiddling, as I mentioned in my feedback), and there's XHR, which goes for flexibility but is much more complicated: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#send%28%29 If we go the obvious ArrayBuffer approach, we need to not only have the caller do the right thing prepping and retaining that buffer, but also we must make sure that downstream code is prepared to send that over the wire. (I doubt it is already.) And doing that work when we'll only be hitting this code path for small JSON strings might be premature optimization. So I'd aim for getting this working with JSON strings first, and see where we go from there. > > > + host: uri.asciiHost.toLowerCase(), > > > > Make sure this has test coverage. > > Coverage added for .method and .host, and already there for .nonce . I meant the asciiHost part. Make your hostname 例子.測試 and make sure this does the right thing! (http://idn.icann.org/#Things_to_test) > (benefits: less messy, removes "if" statement from main path. problems: > small-named "i" lives in a larger scope than really necessary, executes > indexOf and substring even if options.contentType is undefined or "") *shrug* > Ah, that's nice. This is a mozilla-js -specific feature, right? I remember > web-page JS (and/or JSON parsers) rejecting those. Basically IE drools in the back of the bus. Current spec includes them. http://stackoverflow.com/questions/7246618/trailing-commas-in-javascript > > do_check_true(fields[2].startsWith("ts=")); > > We have startsWith here? How civilized! :) You wait until you get to maps, sets, and fat arrows! > Am I correct in assuming that the "skip-if" on line 12 is attached to the > [test_crypto_random.js] that precedes it? So my hawk test will still run on > android if I use?: skip-if applies to the previous entry. https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests#Adding_conditions_to_a_test

Relative to the previous patch, this adds documentation for the (lack of) payload encoding, tests and fixes payload=undefined, and adds tests for "now", "localtimeOffsetMsec", search/queryargs, payload hashing (with binary strings), and using "hash" to override the payload hash. It was written before rnewman's latest comment, so probably doesn't address anything therein.

bwarner: N.B., I'm waiting for an updated patch before I give this another run through, in the expectation that you omitted the flags for that reason :)

Ok, I think this should address everything.

found one more test I could add.. now it's ready for review.

Comment on attachment 803248 [details] [diff] [review] implement HAWK header creation Review of attachment 803248 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/crypto/modules/utils.js @@ +425,5 @@ > + } > + > + let port; > + if (uri.port != -1) { > + port = uri.port; This will be a `long`, not a string. Make the others numbers to match? @@ +447,5 @@ > + }; > + > + let contentType = options.contentType || ""; > + let i = contentType.indexOf(";"); > + contentType = contentType.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase(); On reflection, split this out as a standalone method ("stripParameters"?) so we can test it. @@ +483,5 @@ > + // The output MAC uses '+' and '/', and padded== . > + > + function escape(attribute) { > + // This is used for "x=y" attributes inside HTTP headers. > + return attribute.replace(/\\/g, '\\\\').replace(/\"/g, '\\"'); N.B., for future reference: we use double quotes for all strings, excepting those that contain lots of double quotes. ::: services/crypto/tests/unit/test_utils_hawk.js @@ +242,5 @@ > + > + run_next_test(); > +}); > + > +/* No commented-out tests, plz.

Incorporated latest feedback: double-quotes, integer portnum, split out stripHeaderAttributes() and added tests. One quirk: the node.js HAWK library (specifically node's URL parser) appears to leave the port as a string, but our parser gets an integer. So node.js will treat http://example.net:012345/ differently than http://example.net:12345/ , but we'll treat them identically. I think we can argue that HAWK should settle on integer ports.

Landed, in http://hg.mozilla.org/projects/elm/rev/3ff599ec9e3c . Thanks!

oops, closed this prematurely, it's only landed to the "elm" branch, not to m-c, and also awaits qa+.

Marking [qa-], 'cos there's nothing here that a human being can test…

Not even an automated human being? ;-) (fine with me. one less qa+)

(In reply to Richard Newman [:rnewman] from comment #14) > Marking [qa-], 'cos there's nothing here that a human being can test… How can we land this on m-c? This would be useful for FxAccts on Firefox OS.

:rnewman bump See Comment 16

(In reply to James Bonacci [:jbonacci] from comment #17) > :rnewman > bump > > See Comment 16 Hey, I was away for three days! :D (In reply to Zachary Carter [:zaach] from comment #16) > How can we land this on m-c? This would be useful for FxAccts on Firefox OS. If you're happy with the level of security oversight that this patch has received -- i.e., does it need a brief sec code review? -- then just make sure the tests are passing on Elm, and land it. I don't know if you need to land on another branch for FxOS; one of those folks will need to chime in for that.

small extra patch to tolerate options={payload:undefined}

This patch adds a little test to :warner 's previous enabling of present-but-empty payload field in the computeHawk's options parameter. We would like to land these three patches in mozilla-central.

Comment on attachment 824918 [details] [diff] [review] 911384-test-empty-payload.patch Review of attachment 824918 [details] [diff] [review]: ----------------------------------------------------------------- Repetitive, but I doubt these'll be changing much. Don't forget to set a commit message!

Comment on attachment 824786 [details] [diff] [review] undefined-payload.diff Review of attachment 824786 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/crypto/modules/utils.js @@ +489,5 @@ > > let contentType = CryptoUtils.stripHeaderAttributes(options.contentType); > > + if (!artifacts.hash && options.hasOwnProperty("payload") > + && options.payload) { if (!... && ... && ...) {

Add test for empty options.payload parameter to computeHawk; this time with commit message and whitespace cleanup.

Whitespace tweak to "824786: undefined-payload.diff" per rnewman feedback.

Will check these in as soon as trees reopen. (Bug 932781.)

Follow-up in Elm: https://hg.mozilla.org/projects/elm/rev/ac4db1c22698

https://hg.mozilla.org/integration/fx-team/rev/30ad8d901361 https://hg.mozilla.org/integration/fx-team/rev/b7bfd927abc0

https://hg.mozilla.org/mozilla-central/rev/30ad8d901361 https://hg.mozilla.org/mozilla-central/rev/b7bfd927abc0

https://hg.mozilla.org/mozilla-central/rev/3ff599ec9e3c https://hg.mozilla.org/mozilla-central/rev/ac4db1c22698

Cleaning up Resolved/Fixed bugs from December's first release. Verified that we now have a working first-release of FxA to Desktop/Android Nightly. Re-open as needed.