MAC Cookies (patch 4 of N)

MAC Cookies (patch 4 of N)

Wire up the pieces of MAC cookies (behind the --enable-mac-cookies flag).
Also, update the syntax of the header now that issuer has been removed and the
timestamp and nonce are combined into one field.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85309

[abarth-chromium, 2011-05-12 21:11:10]

@cbentzel: This CL now contains the non-cookie monster parts of the MAC cookie
"wire up" patch.

[cbentzel, 2011-05-13 14:05:45]

http://codereview.chromium.org/6969050/diff/1/net/http/http_request_headers.cc
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/6969050/diff/1/net/http/http_request_headers.c...
net/http/http_request_headers.cc:19: const char
HttpRequestHeaders::kAuthorization[] = "Authoriziation";
This is also contained in net::HttpAuth::GetAuthorizationHeaderName()  - maybe
replace the string literal there?

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:64: return issuer.host() + ":" +
base::IntToString(issuer.IntPort());
You can just use GetHostAndPort from net_util.h here. It will also provide the
default port if unspecified.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:601: void
URLRequestHttpJob::AddAuthorizationHeader(
You could make a static/top-level function which passes in a HttpRequestInfo*,
which would be easier to unit test.

I guess you'd still run into issues with different nonces and timestamps,
however.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:607: int port = url.IntPort();
You'll want to use EffectiveIntPort here to capture default port.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:611: if (cookie_info.mac_key.empty() ||
cookie_info.mac_algorithm.empty())
I would remove the empty string tests here, since they are redundant with the
ones inside AddStateInfo.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:625: return;  // Only add the first
valid header.
I thought you were planning to support multiple Authorization headers?

[abarth-chromium, 2011-05-13 15:15:19]

> I thought you were planning to support multiple Authorization headers?

Turns out multiple Authorization headers are forbidden.  The spec now says to
pick the first cookie (in the cookie header) with appropriate attributes.

Thanks for the review.  I'll update the patch later today.

[cbentzel, 2011-05-13 15:26:11]

OK. I'll be on vacation next week, so I'll try to review the newest patch
before then.

On Fri, May 13, 2011 at 11:15 AM, <abarth@chromium.org> wrote:

> I thought you were planning to support multiple Authorization headers?
>>
>
> Turns out multiple Authorization headers are forbidden.  The spec now says
> to
> pick the first cookie (in the cookie header) with appropriate attributes.
>
> Thanks for the review.  I'll update the patch later today.
>
>
> http://codereview.chromium.org/6969050/
>

[abarth-chromium, 2011-05-13 17:36:23]

Patch updated.  PTAL!  Thanks.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:601: void
URLRequestHttpJob::AddAuthorizationHeader(
> I guess you'd still run into issues with different nonces and timestamps,
> however. 

Yeah, that makes unit testing at this layer slightly tricky.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:607: int port = url.IntPort();
On 2011/05/13 14:05:45, cbentzel wrote:
> You'll want to use EffectiveIntPort here to capture default port. 

Done.

http://codereview.chromium.org/6969050/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:611: if (cookie_info.mac_key.empty() ||
cookie_info.mac_algorithm.empty())
On 2011/05/13 14:05:45, cbentzel wrote:
> I would remove the empty string tests here, since they are redundant with the
> ones inside AddStateInfo.

Done.

[cbentzel, 2011-05-13 17:55:28]

http://codereview.chromium.org/6969050/diff/3003/net/http/http_mac_signature.cc
File net/http/http_mac_signature.cc (left):

http://codereview.chromium.org/6969050/diff/3003/net/http/http_mac_signature....
net/http/http_mac_signature.cc:120: "\", issuer=\"" + issuer_ +
uhh, why'd you get rid of the issuer? Is this a change to the spec? Are you
assuming only one key namespace per eTLD+1?

[abarth-chromium, 2011-05-13 18:02:14]

> uhh, why'd you get rid of the issuer? Is this a change to the spec? Are you
> assuming only one key namespace per eTLD+1?

It's a change to the spec.  The customers we talked to valued sending fewer
bytes on the wire over the increased security provided by explicitly sending the
issuer.  It's some sadness for me because I've wanted to solve that particular
problem with cookies for a while, but it seems more important to listen to what
the customers want.

The reason I merged it into his patch is because not needing the issuer lets us
avoid the code that generates it (which wasn't overly pretty).

[cbentzel, 2011-05-13 18:28:43]

LGTM in that case.

On Fri, May 13, 2011 at 2:02 PM, <abarth@chromium.org> wrote:

> uhh, why'd you get rid of the issuer? Is this a change to the spec? Are you
>> assuming only one key namespace per eTLD+1?
>>
>
> It's a change to the spec.  The customers we talked to valued sending fewer
> bytes on the wire over the increased security provided by explicitly
> sending the
> issuer.  It's some sadness for me because I've wanted to solve that
> particular
> problem with cookies for a while, but it seems more important to listen to
> what
> the customers want.
>
> The reason I merged it into his patch is because not needing the issuer
> lets us
> avoid the code that generates it (which wasn't overly pretty).
>
>
> http://codereview.chromium.org/6969050/
>

[abarth-chromium, 2011-05-13 18:31:18]

Thanks Chris.

On Fri, May 13, 2011 at 11:28 AM, Chris Bentzel <cbentzel@chromium.org> wrote:
> LGTM in that case.
>
> On Fri, May 13, 2011 at 2:02 PM, <abarth@chromium.org> wrote:
>>>
>>> uhh, why'd you get rid of the issuer? Is this a change to the spec? Are
>>> you
>>> assuming only one key namespace per eTLD+1?
>>
>> It's a change to the spec.  The customers we talked to valued sending
>> fewer
>> bytes on the wire over the increased security provided by explicitly
>> sending the
>> issuer.  It's some sadness for me because I've wanted to solve that
>> particular
>> problem with cookies for a while, but it seems more important to listen to
>> what
>> the customers want.
>>
>> The reason I merged it into his patch is because not needing the issuer
>> lets us
>> avoid the code that generates it (which wasn't overly pretty).
>>
>> http://codereview.chromium.org/6969050/
>
>

[commit-bot: I haz the power, 2011-05-13 20:06:49]

Change committed as 85309