Add support for delegated Kerberos tickets during Negotiate authentication.

Add support for delegated kerberos tickets to Negotiate authentication.

This is controlled by the --auth-negotiate-delegate-whitelist command line. By default no servers are delegated to.

BUG=50076
TEST=net_unittests, go to an IIS server specified in --auth-negotiate-delegate-whitelist and see if this works correctly.


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

[cbentzel, 2010-08-26 20:03:43]

Wan-Teh, Andy is on vacation this week so I sent this to you.

The main concern is whether ISC_REQ_MUTUAL needs to be set in the SSPI case ->
MSDN claims it needs to be set, but Firefox does not appear to set it. I will
test without it. 

A good chunk of this CL is just adding const.

[wtc, 2010-08-26 21:30:06]

LGTM.

http://codereview.chromium.org/3155046/diff/22001/1006
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1006#newcode252
chrome/browser/io_thread.cc:252: if
(command_line.HasSwitch(switches::kAuthDefaultCredentialsWhitelist))
Nit: add curly braces around these multi-line "if" bodies even though they are
technically single statements.

http://codereview.chromium.org/3155046/diff/22001/1007
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1007#newcode71
chrome/common/chrome_switches.cc:71: // Whitelist of servers that Negotiate will
allow kerberos delegation with.
Nit: kerberos => Kerberos

http://codereview.chromium.org/3155046/diff/22001/1008
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/3155046/diff/22001/1008#oldcode37
chrome/common/chrome_switches.h:37: extern const char kAuthServerWhitelist[];
It seems bad to remove a command-line option.  I remember these options
are documented...

http://codereview.chromium.org/3155046/diff/22001/1010
File net/http/http_auth_filter.h (right):

http://codereview.chromium.org/3155046/diff/22001/1010#newcode45
net/http/http_auth_filter.h:45: void SetWhitelist(const std::string&
server_whitelist);
Now that the constructor takes a server_whitelist argument, do we still need
the SetWhitelist method?

http://codereview.chromium.org/3155046/diff/22001/1013
File net/http/http_auth_gssapi_posix.h (right):

http://codereview.chromium.org/3155046/diff/22001/1013#newcode243
net/http/http_auth_gssapi_posix.h:243: void AllowDelegation();
Please document this.  It's not obvious that this method is a "setter".

http://codereview.chromium.org/3155046/diff/22001/1014
File net/http/http_auth_handler_negotiate.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1014#newcode93
net/http/http_auth_handler_negotiate.cc:93: if (AllowsDelegation())
It's easy to miss the difference between AllowsDelegation and AllowDelegation.

Perhaps AllowsDelegation can be renamed CanDelegate?  See line 124 below.

http://codereview.chromium.org/3155046/diff/22001/1016
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1016#newcode274
net/http/http_auth_sspi_win.cc:274: // Firefox only sets ISC_REQ_DELEATE, but
MSDN documentation indicates that
Typo: ISC_REQ_DELEATE => ISC_REQ_DELEGATE

http://codereview.chromium.org/3155046/diff/22001/1017
File net/http/http_auth_sspi_win.h (right):

http://codereview.chromium.org/3155046/diff/22001/1017#newcode99
net/http/http_auth_sspi_win.h:99: void AllowDelegation();
Same here: please document this method, as it's not obvious this is a "setter".

http://codereview.chromium.org/3155046/diff/22001/1019
File net/http/url_security_manager.h (right):

http://codereview.chromium.org/3155046/diff/22001/1019#newcode26
net/http/url_security_manager.h:26: // The URLSecurityManager takes ownership of
the HttpAuthFilter.
This comment needs updating as there are two HttpAuthFilters now.

http://codereview.chromium.org/3155046/diff/22001/1022
File net/http/url_security_manager_win.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1022#newcode38
net/http/url_security_manager_win.cc:38: mutable
ScopedComPtr<IInternetSecurityManager> security_manager_;
Why does this member need to be mutable?  Do we need to propagage 'const'
more?

http://codereview.chromium.org/3155046/diff/22001/1022#newcode122
net/http/url_security_manager_win.cc:122: // If we have a whitelist, just use
that.
This comment needs updating as there are two whitelists now.

[cbentzel, 2010-08-27 17:10:53]

http://codereview.chromium.org/3155046/diff/22001/1008
File chrome/common/chrome_switches.h (left):

http://codereview.chromium.org/3155046/diff/22001/1008#oldcode37
chrome/common/chrome_switches.h:37: extern const char kAuthServerWhitelist[];
On 2010/08/26 21:30:06, wtc wrote:
> It seems bad to remove a command-line option.  I remember these options
> are documented...

I wrote the documentation, but it also exists in some bugs. I'll move this back
to the same string for users. I don't want to have to tell users different
command lines for different versions.

http://codereview.chromium.org/3155046/diff/22001/1014
File net/http/http_auth_handler_negotiate.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1014#newcode93
net/http/http_auth_handler_negotiate.cc:93: if (AllowsDelegation())
On 2010/08/26 21:30:06, wtc wrote:
> It's easy to miss the difference between AllowsDelegation and AllowDelegation.
> 
> Perhaps AllowsDelegation can be renamed CanDelegate?  See line 124 below.

Good name suggestion. I went with CanDelegate for the query and Delegate for the
action.

http://codereview.chromium.org/3155046/diff/22001/1022
File net/http/url_security_manager_win.cc (right):

http://codereview.chromium.org/3155046/diff/22001/1022#newcode38
net/http/url_security_manager_win.cc:38: mutable
ScopedComPtr<IInternetSecurityManager> security_manager_;
On 2010/08/26 21:30:06, wtc wrote:
> Why does this member need to be mutable?  Do we need to propagage 'const'
> more?

CanUseDefaultCredentials calls CreateInstance for an IInternetSecurityManager if
security_manager_ does not already have a COM pointer. This was done lazilly
because there was a startup performance regression when done early.

I moved this logic into a separate non-const method and did a const_cast in
CanUseDefaultCredentials instead.