Don't do Negotiate with GSSAPI if default credentials are not allowed.

Don't do Negotiate with GSSAPI if default credentials are not allowed.

GSSAPI does not provide a mechanism for the user to specify username/password to obtain a TGT. If default credentials are not allowed for an end site, skip negotiate and use a different scheme.

Arguably in this case it may make sense to simply prompt the user whether they want to use their existing Kerberos credentials to authenticate to the server and use the existing TGT, but we'll need UI changes.

BUG=33033
TEST=net_unittests, try to authenticate to a Kerberized server which is not in the whitelist.

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

[cbentzel, 2010-07-15 21:59:11]

I think this all that is needed to bypass Negotiate authentication when the
server is not in the whitelist. I'm testing it out.

[ahendrickson, 2010-07-15 22:02:17]

LGTM

On 2010/07/15 21:59:11, cbentzel wrote:
> I think this all that is needed to bypass Negotiate authentication when the
> server is not in the whitelist. I'm testing it out.

[cbentzel, 2010-07-15 22:36:43]

Well, this isn't LG yet - it has some issues with unit tests as it won't create
a handler without a whitelist on POSIX (as I expected). I mainly want to
validate that this works fine against your server and then I'll make the unit
tests work.

[cbentzel, 2010-07-16 02:05:07]

OK, net_unittests pass and this works as I expected when I built Chrome.

[cbentzel, 2010-07-16 18:53:27]

I changed a few unit tests to have a permissive URLSecurityManager so this works
without reversing the logic. Please review again.

[ahendrickson, 2010-07-16 19:42:43]

LGTM, just a few nits.

http://codereview.chromium.org/3013003/diff/6001/7001
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/3013003/diff/6001/7001#newcode119
net/http/http_auth_handler_factory.cc:119: 
Why the extra space?

http://codereview.chromium.org/3013003/diff/6001/7002
File net/http/http_auth_handler_factory_unittest.cc (right):

http://codereview.chromium.org/3013003/diff/6001/7002#newcode40
net/http/http_auth_handler_factory_unittest.cc:40: virtual
~URLSecurityManagerAllow() {}
Do you need the destructor?

http://codereview.chromium.org/3013003/diff/6001/7004
File net/http/http_auth_unittest.cc (right):

http://codereview.chromium.org/3013003/diff/6001/7004#newcode26
net/http/http_auth_unittest.cc:26: virtual ~URLSecurityManagerAllow() {}
Same as above.

[cbentzel, 2010-07-16 21:19:17]

http://codereview.chromium.org/3013003/diff/6001/7001
File net/http/http_auth_handler_factory.cc (right):

http://codereview.chromium.org/3013003/diff/6001/7001#newcode119
net/http/http_auth_handler_factory.cc:119: 
On 2010/07/16 19:42:43, ahendrickson wrote:
> Why the extra space?

Thanks - I removed it locally but held off uploading the change so the trybot
results would show up.

[cbentzel, 2010-07-17 16:19:55]

Andy, I changed this CL a bit since the last LGTM, mainly because this broke on
the newly-enabled-on-POSIX negotiate authenticate unit tests.

I moved the URLSecurityManagerAllow into url_security_manager.h, and changed the
http_auth_handler_negotiate_unittest to use a factory to generate the handlers
instead of constructing them directly - this actually simplified things, despite
the amount of diff.

[ahendrickson, 2010-07-18 00:40:56]

LGTM, other than a few nits.

http://codereview.chromium.org/3013003/diff/28001/16005
File net/http/http_auth_handler_negotiate.cc (right):

http://codereview.chromium.org/3013003/diff/28001/16005#newcode28
net/http/http_auth_handler_negotiate.cc:28: #if defined(OS_POSIX)
#elif defined(OS_POSIX)

http://codereview.chromium.org/3013003/diff/28001/16005#newcode255
net/http/http_auth_handler_negotiate.cc:255: #if defined(OS_POSIX)
#elif defined(OS_POSIX)

http://codereview.chromium.org/3013003/diff/28001/16006
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/3013003/diff/28001/16006#newcode78
net/http/http_auth_handler_negotiate.h:78: // The caller is responsible for
managing the lifetime of |*library|,
|*auth_library|

http://codereview.chromium.org/3013003/diff/28001/16007
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/3013003/diff/28001/16007#newcode196
net/http/http_auth_handler_negotiate_unittest.cc:196:
static_cast<HttpAuthHandlerNegotiate*>(auth_handler.get());
Do you need this static_cast<>?

[cbentzel, 2010-07-18 11:44:18]

Thanks for the review. Now I need to understand why the trybots are having
difficulty patching in this CL.

http://codereview.chromium.org/3013003/diff/28001/16007
File net/http/http_auth_handler_negotiate_unittest.cc (right):

http://codereview.chromium.org/3013003/diff/28001/16007#newcode196
net/http/http_auth_handler_negotiate_unittest.cc:196:
static_cast<HttpAuthHandlerNegotiate*>(auth_handler.get());
On 2010/07/18 00:40:56, ahendrickson wrote:
> Do you need this static_cast<>?

Yes. I'm down-casting from HttpAuthHandler to HttpAuthHandlerNegotiate.
dynamic_cast would be the correct approach if we enabled RTTI - but we don't so
static_cast is the way to go.

[cbentzel, 2010-07-19 17:44:49]

This required a number of changes in http_auth_handler_negotiate_unittest.cc to
make the different platforms happy. Please take another look at that file (the
others have stayed consistent).

[ahendrickson, 2010-07-19 19:40:37]

LGTM.

On 2010/07/19 17:44:49, cbentzel wrote:
> This required a number of changes in http_auth_handler_negotiate_unittest.cc
to
> make the different platforms happy. Please take another look at that file (the
> others have stayed consistent).