Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6901121:
MAC Cookies (patch 2 of N) (Closed)
|
Created:
9 years, 8 months ago by abarth-chromium Modified:
9 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMAC Cookies (patch 2 of N)
This CL contains the algorithmic guts of MAC cookies, including generating
the canonical represntation of the request and signing it using HMAC. This
CL does not include support for body_hash, which requires some more thought.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83600
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=83651
Patch Set 1 #
Total comments: 26
Patch Set 2 : '' #
Total comments: 1
Patch Set 3 : MAC Cookies (patch 2 of N) #
Total comments: 1
Messages
Total messages: 11 (0 generated)
Here are the "guts" of the algorithm. Note: This patch depends on http://codereview.chromium.org/6904118/, which adds RandString to base.
Looks pretty good, mostly nits or test requests. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc File net/http/http_mac_signature.cc (right): http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:22: size_t LengthForHMACAlgorithm(crypto::HMAC::HashAlgorithm algorithm) { I'm kind of surprised this isn't an HMAC method. Should it be? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:56: { Nit: { typically on same line as the last initializer list item. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:62: bool HttpMacSignature::AddStateInfo(const std::string& id, Should this be callable multiple times? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:71: if (mac_algorithm == kSHA1Name) Very tiny issue: maybe do input validity checking on mac_algorithm in the IsPlainString() section above, to be consistent with the other parameters? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:91: || port <= 0) Add a port > 65535 check as well. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:103: DCHECK(port_.empty()) << "Call AddHttpInfo first."; !port_.empty() http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:140: std::string HttpMacSignature::GenerateMAC(const std::string& timestamp, Should these be returning a bool in case hmac.Sign/Base64Encode fail? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:150: hmac.Sign(request, reinterpret_cast<unsigned char*>(buffer), length); You should check the result of hmac.Sign http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... File net/http/http_mac_signature_unittest.cc (right): http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:10: TEST(HttpMacSignatureTest, BogusAddStateInfo) { Add other tests, such as empty string. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:33: "/pAth?to=%22enlightenment%22&dest=magic", Nit: line these up http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:37: std::string timestame = "239034"; timestamp, not timestame http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:67: } Add a test for GenerateAuthorizationHeader. You'd unfortunately need to hack how timestamp is generated, though. The tests of GenerateNormalizedRequests/GenerateMAC would get covered by that as well, so you could always remove the FRIEND_TEST's as well.
Question: Should this code be in net/http or net/base? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc File net/http/http_mac_signature.cc (right): http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:22: size_t LengthForHMACAlgorithm(crypto::HMAC::HashAlgorithm algorithm) { On 2011/04/29 14:44:49, cbentzel wrote: > I'm kind of surprised this isn't an HMAC method. Should it be? Yes. I'll add it. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:56: { On 2011/04/29 14:44:49, cbentzel wrote: > Nit: { typically on same line as the last initializer list item. Fixed. (WebKit style leaking in.) http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:62: bool HttpMacSignature::AddStateInfo(const std::string& id, On 2011/04/29 14:44:49, cbentzel wrote: > Should this be callable multiple times? Not if it returns true. DCHECK added. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:71: if (mac_algorithm == kSHA1Name) On 2011/04/29 14:44:49, cbentzel wrote: > Very tiny issue: maybe do input validity checking on mac_algorithm in the > IsPlainString() section above, to be consistent with the other parameters? Do you mean checking for IsPlainString ? The mac_algorithm isn't actually required to be a plain string. (Now that I look at it, neither is mac_key.) I've added a check for empty() to the above block. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:91: || port <= 0) On 2011/04/29 14:44:49, cbentzel wrote: > Add a port > 65535 check as well. Done. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:103: DCHECK(port_.empty()) << "Call AddHttpInfo first."; On 2011/04/29 14:44:49, cbentzel wrote: > !port_.empty() I've changed this to used method_, actually, which is slightly more aesthetic. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:140: std::string HttpMacSignature::GenerateMAC(const std::string& timestamp, On 2011/04/29 14:44:49, cbentzel wrote: > Should these be returning a bool in case hmac.Sign/Base64Encode fail? Base64Encode can't fail in these cases because they only fail during overflow and we've controlled the size of all inputs, which rules out overflow. It's less clear to me whether Sign can fail. Do you think a DCHECK is sufficient, or do think think we should do something more drastic (e.g., CHECK) ? http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:150: hmac.Sign(request, reinterpret_cast<unsigned char*>(buffer), length); On 2011/04/29 14:44:49, cbentzel wrote: > You should check the result of hmac.Sign I've added a DCHECK. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... File net/http/http_mac_signature_unittest.cc (right): http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:10: TEST(HttpMacSignatureTest, BogusAddStateInfo) { On 2011/04/29 14:44:49, cbentzel wrote: > Add other tests, such as empty string. Done. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:33: "/pAth?to=%22enlightenment%22&dest=magic", On 2011/04/29 14:44:49, cbentzel wrote: > Nit: line these up Done. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:37: std::string timestame = "239034"; On 2011/04/29 14:44:49, cbentzel wrote: > timestamp, not timestame Done. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:67: } On 2011/04/29 14:44:49, cbentzel wrote: > Add a test for GenerateAuthorizationHeader. You'd unfortunately need to hack how > timestamp is generated, though. Yeah, that's why I did things this way. I can split off the string generation into its own test and have GenerateAuthorizationHeader just do the stamptime/nonce aspect. There will be more tests, of course, once this code is called elsewhere. > The tests of GenerateNormalizedRequests/GenerateMAC would get covered by that as > well, so you could always remove the FRIEND_TEST's as well. They're still useful is something goes wrong because they'll help you debug which layer is causing the problem.
I've uploaded a new version. I haven't moved the "byte count for signature algorithm" to the crypto component yet because I want to do that in a separate CL (which I'm happy to land either before or after this CL).
I don't see any directly cookie related changes as part of this CL, and I think Chris is a much better reviewer than I for authentication algorithm implementations. So I'm removing myself from the reviewer list. Feel free (either of you) to add me back on if you think there's some special expertise I have on this one that I'm missing.
LGTM net/http seems fine as I'm expecting net/http/http_auth_handler_mac.cc to be the main consumer of it. doing the hash-size-in-HMAC in a separate CL also makes sense. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc File net/http/http_mac_signature.cc (right): http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:71: if (mac_algorithm == kSHA1Name) On 2011/04/29 18:16:01, abarth wrote: > On 2011/04/29 14:44:49, cbentzel wrote: > > Very tiny issue: maybe do input validity checking on mac_algorithm in the > > IsPlainString() section above, to be consistent with the other parameters? > > Do you mean checking for IsPlainString ? The mac_algorithm isn't actually > required to be a plain string. (Now that I look at it, neither is mac_key.) > I've added a check for empty() to the above block. Sorry, I just meant moving this into if (!IsPlainString(id) || id.empty() ... || !(mac_algorithm == kSHA1Name || mac_algorithm == kSHA256Name)) return false; at the top of the function, rather than split input validation to two locations. http://codereview.chromium.org/6901121/diff/1/net/http/http_mac_signature.cc#... net/http/http_mac_signature.cc:140: std::string HttpMacSignature::GenerateMAC(const std::string& timestamp, On 2011/04/29 18:16:01, abarth wrote: > On 2011/04/29 14:44:49, cbentzel wrote: > > Should these be returning a bool in case hmac.Sign/Base64Encode fail? > > Base64Encode can't fail in these cases because they only fail during overflow > and we've controlled the size of all inputs, which rules out overflow. > > It's less clear to me whether Sign can fail. Do you think a DCHECK is > sufficient, or do think think we should do something more drastic (e.g., CHECK) > ? DCHECK seems fine, although I have not looked at what would cause hmac.Sign to fail. Another option is to change the signature to bool and propagate the errors along to the caller, but I think it's fine to contain to DCHECKs inside this function. http://codereview.chromium.org/6901121/diff/6/net/http/http_mac_signature_uni... File net/http/http_mac_signature_unittest.cc (right): http://codereview.chromium.org/6901121/diff/6/net/http/http_mac_signature_uni... net/http/http_mac_signature_unittest.cc:44: TEST(HttpMacSignatureTest, GenerateHeaderString) { This is a nice way to do it without going through torturous mock/depenency injection stuff for time.
> net/http/http_mac_signature.cc:71: if (mac_algorithm == kSHA1Name) > On 2011/04/29 18:16:01, abarth wrote: > > On 2011/04/29 14:44:49, cbentzel wrote: > > > Very tiny issue: maybe do input validity checking on mac_algorithm in the > > > IsPlainString() section above, to be consistent with the other parameters? > > > > Do you mean checking for IsPlainString ? The mac_algorithm isn't actually > > required to be a plain string. (Now that I look at it, neither is mac_key.) > > I've added a check for empty() to the above block. > > Sorry, I just meant moving this into > > if (!IsPlainString(id) || id.empty() ... > || !(mac_algorithm == kSHA1Name || mac_algorithm == kSHA256Name)) > return false; > > at the top of the function, rather than split input validation to two locations. The disadvantage of that approach we'd need to do the string comparison twice, but maybe the cleanliness of the code out-weighs performance cost? I'm happy to make that change if you'd prefer that. > > It's less clear to me whether Sign can fail. Do you think a DCHECK is > > sufficient, or do think think we should do something more drastic (e.g., > CHECK) > > ? > DCHECK seems fine, although I have not looked at what would cause hmac.Sign to > fail. I looked at the various implementations for different platforms. The function definitely can fail if we don't call init first (which is something we'd catch with the DCHECK). It can also fail if various low-level unlikely-to-happen events occur, such as failing to load the crypto DLL on windows. > Another option is to change the signature to bool and propagate the errors along > to the caller, but I think it's fine to contain to DCHECKs inside this function. Generally, our project-wide approach to this problem is to avoid propagating errors in this way, which leads me to believe we should keep this as a DCHECK. > This is a nice way to do it without going through torturous mock/depenency > injection stuff for time. Thanks.
LGTM, still On Fri, Apr 29, 2011 at 4:13 PM, <abarth@chromium.org> wrote: > net/http/http_mac_signature.cc:71: if (mac_algorithm == kSHA1Name) >> On 2011/04/29 18:16:01, abarth wrote: >> > On 2011/04/29 14:44:49, cbentzel wrote: >> > > Very tiny issue: maybe do input validity checking on mac_algorithm in >> the >> > > IsPlainString() section above, to be consistent with the other >> parameters? >> > > > >> > Do you mean checking for IsPlainString ? The mac_algorithm isn't >> actually >> > required to be a plain string. (Now that I look at it, neither is >> mac_key.) >> > > > I've added a check for empty() to the above block. >> > > Sorry, I just meant moving this into >> > > if (!IsPlainString(id) || id.empty() ... >> || !(mac_algorithm == kSHA1Name || mac_algorithm == kSHA256Name)) >> return false; >> > > at the top of the function, rather than split input validation to two >> > locations. > > The disadvantage of that approach we'd need to do the string comparison > twice, > but maybe the cleanliness of the code out-weighs performance cost? I'm > happy to > make that change if you'd prefer that. Good point. Current code is still pretty clean, and it doesn't do a partial state mutation if the algorithm string is invalid. I was going to suggest passing in an enum and realized that this value will be coming in from a cookie/oath2. > > > > It's less clear to me whether Sign can fail. Do you think a DCHECK is >> > sufficient, or do think think we should do something more drastic (e.g., >> CHECK) >> > ? >> DCHECK seems fine, although I have not looked at what would cause >> hmac.Sign to >> fail. >> > > I looked at the various implementations for different platforms. The > function > definitely can fail if we don't call init first (which is something we'd > catch > with the DCHECK). It can also fail if various low-level unlikely-to-happen > events occur, such as failing to load the crypto DLL on windows. I guess in that case we'd just provide an empty mac string to the server [which would reject it]. That's probably OK. We have histograms to track auth-failure-rates-per-scheme and this would get captured in there, if not the root cause.
Try job failure for 6901121-6 on win: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Can't process patch for file net/http/http_mac_signature_unittest.cc. A +
http://codereview.chromium.org/6901121/diff/4007/net/http/http_mac_signature.h File net/http/http_mac_signature.h (right): http://codereview.chromium.org/6901121/diff/4007/net/http/http_mac_signature.... net/http/http_mac_signature.h:17: class HttpMacSignature { Please document this class and the GenerateAuthorizationHeader method. Please add a reference to the specification of MAC Cookies. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
