Fix multi-round authentication....

Fix multi-round authentication.

In the case of Negotiate, authentication can look like

C: GET
S: 401, WWW-Authenticate: Negotiate

C: GET, WWW-Authorization: Negotiate <client_token_1>
S: 401, WWW-Authenticate: Negotiate <server_token_1>

C: GET, WWW-Authorization: Negotiate <client_token_2>
S: 401, WWW-Authenticate: Negotiate <server_token_2>

on that third challenge, the handler was reported as being in "the final round" and this was treated as a rejection of the authentication attempt. After that, the new challenge token was used by a new auth handler that hadn't established a security context, and an ERR_INVALID_HANDLE would be returned.

This CL also does some prep work to correctly handle the "stale=true" value for Digest authentication, but I decided to defer the HttpAuthCache changes needed for that to a separate CL since this was large enough.

BUG=53282
TEST=net_unittests. Unfortunately, I haven't been able to set up a proxy/server to do more than two auth challenges, but this does happen in the wild.


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

[cbentzel, 2010-09-10 02:20:59]

I think this may fix the cause of some of the user's issues with Negotiate and
NTLM.

There's a few things I need to do before I'm ready to check this in:
  - A few more unit tests.
  - More manual testing.
  - Get SocketStream to work with this change (it should move over to using
HttpAuthController, but I'd like to hold off on that one).

http://codereview.chromium.org/3360017/diff/5004/6028
File net/http/mock_sspi_library_win.cc (right):

http://codereview.chromium.org/3360017/diff/5004/6028#newcode56
net/http/mock_sspi_library_win.cc:56: // See what actually happens.
Validate this with an actual run.

[ahendrickson, 2010-09-10 16:15:16]

LGTM, with minor nits.

I only skimmed the unit tests, though.

http://codereview.chromium.org/3360017/diff/30003/71005
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71005#newcode161
net/http/http_auth_controller.cc:161: // challenge was rejected (either
explicitly or via lack of
Finish the comment?

http://codereview.chromium.org/3360017/diff/30003/71007
File net/http/http_auth_gssapi_posix.cc (left):

http://codereview.chromium.org/3360017/diff/30003/71007#oldcode722
net/http/http_auth_gssapi_posix.cc:722: LOG(ERROR) << "Base64 decoding of auth
token failed.";
Can we keep this as a WARNING or INFO?

http://codereview.chromium.org/3360017/diff/30003/71009
File net/http/http_auth_gssapi_posix_unittest.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71009#newcode51
net/http/http_auth_gssapi_posix_unittest.cc:51: void
EstablishInitialContext(test::MockGSSAPILibrary* library) {
Nit: Personally, I'd find this more readable with a space between the
declaration and the function.

http://codereview.chromium.org/3360017/diff/30003/71028
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71028#newcode6684
net/http/http_network_transaction_unittest.cc:6684: // Second round
Could 2nd thru 4th rounds be in a loop?  With the RestartWithAuth() strings in
an array?

[cbentzel, 2010-09-10 20:00:18]

Thanks for the review. I've added a few more unit tests. I still need to solve
the SocketStream issue, although I'm tempted not to spend much time on it since
I don't think that many people are using it with proxy authentication and this
is fixing a known bug.

http://codereview.chromium.org/3360017/diff/30003/71005
File net/http/http_auth_controller.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71005#newcode161
net/http/http_auth_controller.cc:161: // challenge was rejected (either
explicitly or via lack of
On 2010/09/10 16:15:17, ahendrickson wrote:
> Finish the comment?

The comment below is better, I removed this one.

http://codereview.chromium.org/3360017/diff/30003/71007
File net/http/http_auth_gssapi_posix.cc (left):

http://codereview.chromium.org/3360017/diff/30003/71007#oldcode722
net/http/http_auth_gssapi_posix.cc:722: LOG(ERROR) << "Base64 decoding of auth
token failed.";
On 2010/09/10 16:15:17, ahendrickson wrote:
> Can we keep this as a WARNING or INFO?

Possibly INFO. It's more likely a sign of a problem with the server/proxy than
it is with the base64 decode function.

http://codereview.chromium.org/3360017/diff/30003/71009
File net/http/http_auth_gssapi_posix_unittest.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71009#newcode51
net/http/http_auth_gssapi_posix_unittest.cc:51: void
EstablishInitialContext(test::MockGSSAPILibrary* library) {
On 2010/09/10 16:15:17, ahendrickson wrote:
> Nit: Personally, I'd find this more readable with a space between the
> declaration and the function.
> 

Done.

http://codereview.chromium.org/3360017/diff/30003/71028
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3360017/diff/30003/71028#newcode6684
net/http/http_network_transaction_unittest.cc:6684: // Second round
On 2010/09/10 16:15:17, ahendrickson wrote:
> Could 2nd thru 4th rounds be in a loop?  With the RestartWithAuth() strings in
> an array?
> 

I prefer straight-line code in unit tests where reasonable because any
EXPECT/ASSERT failures will point directly to the line that is having issues.
Otherwise you need to annotate with the current iteration.

[cbentzel, 2010-09-10 20:22:50]

On 2010/09/10 20:00:18, cbentzel wrote:
> Thanks for the review. I've added a few more unit tests. I still need to solve
> the SocketStream issue, although I'm tempted not to spend much time on it
since
> I don't think that many people are using it with proxy authentication and this
> is fixing a known bug.

It turns out the SocketStream fix was pretty easy - the only authentication type
currently supported is Basic, so any follow-up challenges should be treated as a
rejection of the previous attempt.