Don't use IDENT_SRC_URL for HttpAuth challenges. IE hasn't supported it for years, and at worst ...

Don't use IDENT_SRC_URL for HttpAuth challenges.  IE hasn't supported it for years, and at worst it represents a session fixation attack.

BUG=94578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120836

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

[Tom Sepez, 2012-02-03 21:45:19]

Hi, please review.

This is the follow-on work to remove the path that the histogram I added a few
months ago shows is rarely encountered.

[cbentzel, 2012-02-06 15:55:38]

Code mostly seems fine - just a few nits.

However - if you are positive that this is the correct change to make, why not
just remove all code related to embedded auth?

One potential reason _not_ to do that is that there are a few other consumers of
the networking library that are not content/chrome and the change in default
behavior may catch them unaware.

Also - how do you plan to balance site compatibility with security tradeoff? I
suppose if IE doesn't support it that this may not be a significant problem in
the wild, but I do wonder how you plan to measure the impact.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controller.h
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:39: CHALLENGE_OPTION_SEND_SERVER_AUTH = 1,
Perhaps 0x1 << 0, 0x1 << 2, etc. to better indicate that this a bitmask.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:41: CHALLENGE_OPTION_ESTABLISHING_TUNNEL = 4
"ESTABLISHING_TUNNEL" doesn't feel like much of an option - it's more of a
description of the current state. If you want to lump it in here to make callers
more obvious, I guess I'm OK but it feels like a stretch.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:68: int challenge_option_mask,
ChallengeOption should be fine here.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_proxy_utils.cc
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/9307093/diff/3002/net/http/http_proxy_utils.cc...
net/http/http_proxy_utils.cc:48:
HttpAuthController::CHALLENGE_OPTION_USE_EMBEDDED_AUTH |
Why is embedded auth OK here?

[Tom Sepez, 2012-02-06 17:58:25]

On 2012/02/06 15:55:38, cbentzel wrote:
> Code mostly seems fine - just a few nits.
> 
> However - if you are positive that this is the correct change to make, why not
> just remove all code related to embedded auth?
> 
> One potential reason _not_ to do that is that there are a few other consumers
of
> the networking library that are not content/chrome

Right, I didn't want to rule out the possibility that this path might be needed
either in the future or elsewhere.
 and the change in default
> behavior may catch them unaware.

How high up would you suggest passing the flags, then?

> 
> Also - how do you plan to balance site compatibility with security tradeoff? I
> suppose if IE doesn't support it that this may not be a significant problem in
> the wild, but I do wonder how you plan to measure the impact.
> 
> 

We've got a histogram showing that this is hitting fewer than 1k unique users
per day.

[Tom Sepez, 2012-02-06 18:05:30]

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controller.h
File net/http/http_auth_controller.h (right):

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:39: CHALLENGE_OPTION_SEND_SERVER_AUTH = 1,
On 2012/02/06 15:55:38, cbentzel wrote:
> Perhaps 0x1 << 0, 0x1 << 2, etc. to better indicate that this a bitmask.

Sure, will do.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:41: CHALLENGE_OPTION_ESTABLISHING_TUNNEL = 4
On 2012/02/06 15:55:38, cbentzel wrote:
> "ESTABLISHING_TUNNEL" doesn't feel like much of an option - it's more of a
> description of the current state. If you want to lump it in here to make
callers
> more obvious, I guess I'm OK but it feels like a stretch.

Understood.  I'd like to keep it here though for the sake of clarity.

http://codereview.chromium.org/9307093/diff/3002/net/http/http_auth_controlle...
net/http/http_auth_controller.h:68: int challenge_option_mask,
On 2012/02/06 15:55:38, cbentzel wrote:
> ChallengeOption should be fine here.
It's that idiotic way that c++ handles or's of enums, which result in type int
and can't be passed to the enum without a cast by the caller, which is nice to
avoid.  I'll change it,

http://codereview.chromium.org/9307093/diff/3002/net/http/http_proxy_utils.cc
File net/http/http_proxy_utils.cc (right):

http://codereview.chromium.org/9307093/diff/3002/net/http/http_proxy_utils.cc...
net/http/http_proxy_utils.cc:48:
HttpAuthController::CHALLENGE_OPTION_USE_EMBEDDED_AUTH |
On 2012/02/06 15:55:38, cbentzel wrote:
> Why is embedded auth OK here? 
Duplicating the existing logic in this path.  Was only concerned with changing
the other caller.

[cbentzel, 2012-02-06 18:33:47]

On Mon, Feb 6, 2012 at 12:58 PM, <tsepez@chromium.org> wrote:

> On 2012/02/06 15:55:38, cbentzel wrote:
>
>> Code mostly seems fine - just a few nits.
>>
>
>  However - if you are positive that this is the correct change to make,
>> why not
>> just remove all code related to embedded auth?
>>
>
>  One potential reason _not_ to do that is that there are a few other
>> consumers
>>
> of
>
>> the networking library that are not content/chrome
>>
>
> Right, I didn't want to rule out the possibility that this path might be
> needed
> either in the future or elsewhere.
>
>  and the change in default
>
>> behavior may catch them unaware.
>>
>
> How high up would you suggest passing the flags, then?


The LOAD_FLAG approach is correct if we want to give embedders the option
to allow. Otherwise I'm fine with removing the underlying code as well as
the LOAD_FLAG

>


>
>
>  Also - how do you plan to balance site compatibility with security
>> tradeoff? I
>> suppose if IE doesn't support it that this may not be a significant
>> problem in
>> the wild, but I do wonder how you plan to measure the impact.
>>
>
>
>
> We've got a histogram showing that this is hitting fewer than 1k unique
> users
> per day.
>

Thanks. Tradeoff seems worth it.


>
>
>
http://codereview.chromium.**org/9307093/<http://codereview.chromium.org/9307...
>

[cbentzel, 2012-02-06 18:40:12]

On Mon, Feb 6, 2012 at 1:33 PM, Chris Bentzel <cbentzel@chromium.org> wrote:

>
>
> On Mon, Feb 6, 2012 at 12:58 PM, <tsepez@chromium.org> wrote:
>
>> On 2012/02/06 15:55:38, cbentzel wrote:
>>
>>> Code mostly seems fine - just a few nits.
>>>
>>
>>  However - if you are positive that this is the correct change to make,
>>> why not
>>> just remove all code related to embedded auth?
>>>
>>
>>  One potential reason _not_ to do that is that there are a few other
>>> consumers
>>>
>> of
>>
>>> the networking library that are not content/chrome
>>>
>>
>> Right, I didn't want to rule out the possibility that this path might be
>> needed
>> either in the future or elsewhere.
>>
>>  and the change in default
>>
>>> behavior may catch them unaware.
>>>
>>
>> How high up would you suggest passing the flags, then?
>
>
> The LOAD_FLAG approach is correct if we want to give embedders the option
> to allow. Otherwise I'm fine with removing the underlying code as well as
> the LOAD_FLAG
>

Actually - this behavior can be done completely outside of the net stack,
so I'm in favor of stripping the behavior out instead of adding an optional
LOAD_FLAG.

It can be done by an URLRequest::Delegate at OnAuthRequired time, by
inspecting the URL and supplying the username/password there.

[Tom Sepez, 2012-02-06 18:45:23]

> The LOAD_FLAG approach is correct if we want to give embedders the option
> to allow. Otherwise I'm fine with removing the underlying code as well as
> the LOAD_FLAG
> 
I should note that I was also working on adding a unit_test for the LOAD_FLAG. 
To me, it feels  like the decision should be made above this layer, rather than
making net less flexible.

[Tom Sepez, 2012-02-06 19:54:52]

> It can be done by an URLRequest::Delegate at OnAuthRequired time, by
> inspecting the URL and supplying the username/password there.

Sure you want to put your callers through this burden?  No matter, I'll cobble
together a version that has no flag ...

[Tom Sepez, 2012-02-06 20:58:46]

Chris (cbentzel) please review.  Deleting code is easier anyways.

[cbentzel, 2012-02-06 21:18:58]

LGTM

http://codereview.chromium.org/9307093/diff/8004/net/http/http_auth_controlle...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/9307093/diff/8004/net/http/http_auth_controlle...
net/http/http_auth_controller.cc:460: if (target_ == HttpAuth::AUTH_SERVER &&
auth_url_.has_username() &&
Should this entire block be removed? Or do you want to keep that histogram?

[Tom Sepez, 2012-02-06 21:22:02]

http://codereview.chromium.org/9307093/diff/8004/net/http/http_auth_controlle...
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/9307093/diff/8004/net/http/http_auth_controlle...
net/http/http_auth_controller.cc:460: if (target_ == HttpAuth::AUTH_SERVER &&
auth_url_.has_username() &&
On 2012/02/06 21:18:58, cbentzel wrote:
> Should this entire block be removed? Or do you want to keep that histogram?
I wants to keep the histogram.

[commit-bot: I haz the power, 2012-02-06 22:27:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/9307093/8004

[commit-bot: I haz the power, 2012-02-07 10:18:27]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120752, job name
was 9307093-8004 (retry) (previous was lost) (previous was lost) (retry).

[commit-bot: I haz the power, 2012-02-07 19:55:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/9307093/8004

[commit-bot: I haz the power, 2012-02-07 21:52:55]

Try job failure for 9307093-8004 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[michaeln, 2012-02-07 23:40:03]

Looks like this removes content facing functionality (it caused a bunch of
layout tests to fail)? Are we ok with that?

[Tom Sepez, 2012-02-07 23:47:02]

Yes, we're OK with this, will need to update expectations

FYI, here is the (long) list of failures: 
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

[Tom Sepez, 2012-02-10 18:13:13]

Now that https://bugs.webkit.org/show_bug.cgi?id=78259 has landed, I can try to
land this again.

[commit-bot: I haz the power, 2012-02-10 18:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/9307093/8004

[commit-bot: I haz the power, 2012-02-10 20:19:44]

Change committed as 121506