Use SSPI for NTLM authentication on Windows....

Use SSPI for NTLM authentication on Windows.

Initial patch by Arindam.

Original review URL: http://codereview.chromium.org/159656

R=arindam,eroman
BUG=19, 18009
TEST=open a webpage that requests NTLM authentication
on Windows

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

[wtc, 2009-08-26 22:53:02]

[This message is intended for Arindam.  Everyone else may
skip it.]

Arindam,

Patch Set 1 is your patch.

Patch Set 2 has my quick-and-dirty changes to make
it work.  They are in
- the diffs between Patch Sets 1 and 2 in
  http_auth_handler_ntlm_win.cc, and
- the new changes to http_network_transaction.cc in
  Patch Set 2.

The NeedsIdentity change in http_auth_handler_ntlm_win.cc
is necessary because we need to pass username/password to
AcquireCredentialsHandle at the beginning of the auth
sequence.

In http_network_transaction.cc, the first change
allows us to remove the wrong username/password from
our auth cache only at the end of the (multi-round)
auth sequence.  I use a hack
  !auth_handler_[target]->NeedsIdentity()
for now.  It should be replaced by a new FinalRound()
method:
  auth_handler_[target]->FinalRound()

We also need to set auth_handler_[target] to NULL so
that the next auth sequence will start from scratch.
(The NTLM auth handler contains state, which should
not carry over to the next auth sequence.)

The second change prevents us from invalidating
auth_identity_[target] when NeedsIdentity() returns
false.  Notice that we set auth_identity_[target].invalid
to true above.  So all we need to do here is to set it
back to false.  All other members stay intact.

[Arindam, 2009-08-27 15:03:05]

Some minor nits I had not bothered to correct in my first patch.

http://codereview.chromium.org/173528/diff/22/1015
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/173528/diff/22/1015#newcode27
Line 27: #if defined(NTLM_SSPI)
This preprocessor macro block is redundant, since we disable the test definition
itself at compile time based on the implementation.

http://codereview.chromium.org/173528/diff/22/1015#newcode1514
Line 1514: #if defined(NTLM_PORTABLE)
We disable the definition of the tests here, this is needed since NTLM_SSPI
implementation does not have the HttpAuthHandlerNTLM::ScopedProcSetter defined
and so fails to compile on windows.

[wtc, 2009-08-28 02:54:10]

I cleaned up the CL and made the portable implementation
work.  Please review it.

[eroman, 2009-09-02 09:53:25]

A couple high level comments:

(1) http_auth_handler_ntlm_posix.cc is a weird name, since it has nothing to do
with posix. I would call this *_portable.cc instead.

(2) I am not a fan of the ifdefing; I had a hard time understanding the
commonality between the _posix.cc and _win.cc variants.

Why not give them separate class names and extract commonality to a base class?

Also, for the HttpNetworkTransaction NTLM unittests, it seems that rather than
disabling them on windows, we could choose the "portable" implementation when
running the unittests.

(3) I don't have a good answer to the unit-testing problem... but at the very
least I think it would be valuable to add some tests which simply exercise the
code (don't have to do much in way of checking the result).

The benefit to doing this, is if we have coverage, purify can check for
un-initialized memory reads, leaks, etc. And we also have some very basic
crash-regression capability.

http://codereview.chromium.org/173528/diff/2001/2002
File net/http/http_auth_handler_ntlm_win.cc (right):

http://codereview.chromium.org/173528/diff/2001/2002#newcode21
Line 21: :  max_token_len_(0) {
indent by 4 i believe.

http://codereview.chromium.org/173528/diff/2001/2002#newcode38
Line 38: SEC_WCHAR *package = L"NTLM";
asterisk left.

http://codereview.chromium.org/173528/diff/2001/2002#newcode56
Line 56: reinterpret_cast<USHORT*>(const_cast<wchar_t*>(domain_.data()));
it wasn't clear to me from MSDN whether Domain and Password need to be NULL
terminated. The way it describes DomainLength it sounds like it assumes
NULL-termination.

In which case, can you use .c_str() to ensure it is NULL terminated?

http://codereview.chromium.org/173528/diff/2001/2002#newcode59
Line 59: reinterpret_cast<USHORT*>(const_cast<wchar_t*>(username_.data()));
same comment as above.

http://codereview.chromium.org/173528/diff/2001/2002#newcode62
Line 62: reinterpret_cast<USHORT*>(const_cast<wchar_t*>(password_.data()));
same comment as above.

http://codereview.chromium.org/173528/diff/2001/2002#newcode85
Line 85: uint32 in_token_len,
indentation

http://codereview.chromium.org/173528/diff/2001/2002#newcode92
Line 92: CtxtHandle *ctx_in;
asterisk left?

http://codereview.chromium.org/173528/diff/2001/2002#newcode94
Line 94: SecBuffer ib, ob;
I am guessing "i" means input and "o"  output, and "b" buffer and "bd"
BufferDesc. I strongly recommend choosing longer names that are a bit more
descriptive (like "in" / "out").

http://codereview.chromium.org/173528/diff/2001/2002#newcode127
Line 127: SEC_WCHAR *sn = NULL;  // Package is NTLM.
asterisk left?

http://codereview.chromium.org/173528/diff/2001/2002#newcode144
Line 144: free(ob.pvBuffer);
indentation

[wtc, 2009-09-03 01:54:27]

eroman: I made all of your suggested changes except those
on class hierarchy and unit tests.  I will work on those
tomorrow.  Re: class hierarchy I'd like to discuss with
you first.

Please review the new Patch Set.  I found some sample code
on MSDN, so I changed our code to match the sample code
closer.  I also use more descriptive variable names.

[eroman, 2009-09-03 19:31:59]

lgtm.

My main concern is the lack of tests. Can you file a bug for making sure we test
the SSPI implementation?

Some ideas:

(a) test in HttpNetworkTransaction, by injecting a dynamic mock response rather
than stack response; so we would write the server-side NTLM logic as part of the
unit-test to verify the response

(b) test from the url_request_unittest, by adding NTLM support to testserver.py

(c) Add unittests for HttpAuthHandlerNTLM. Although this would only work "in
addition to" something like (a) or (b), since it wouldn't give any coverage of
the integration into HttpNetworkTransaction

http://codereview.chromium.org/173528/diff/6001/7007
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/173528/diff/6001/7007#newcode74
Line 74: virtual bool FinalRound() { return true; }
nit: How about IsFinalRound() ?

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

http://codereview.chromium.org/173528/diff/6001/7001#newcode119
Line 119: LOG(ERROR) <<"Cannot restart authentication sequence";
indentation.

also add a space after the "<<".

http://codereview.chromium.org/173528/diff/6001/7006
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/173528/diff/6001/7006#newcode257
Line 257: // TODO(wtc): For NTLM_SSPI, we add the same auth entry to the cache
in
Can you link to  http://crbug.com/21015 ?

(I filed a bug about refactoring the auth stuff so NTLM isn't a special case,
would be good if we annotate all the places that pop up, so when we tackle this
it is easier to get up to speed; I am interested in doing that change).

http://codereview.chromium.org/173528/diff/6001/7006#newcode288
Line 288: // TODO(wtc): has_auth_identity is not the right condition.  We should
Can you link to http://crbug.com/21015

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

http://codereview.chromium.org/173528/diff/6001/7002#newcode1506
Line 1506: // authorization headers.
Can you add a bug about adding tests for SSPI NTLM?

[wtc, 2009-09-04 17:26:53]

eroman: the CL was at a good checkpoint, so I checked it in
when the tree was open.  Please review the diffs between
the latest Patch Set and Patch Set 5.

(It looks like I may have to revert it because the layout
test failed-auth.html failed on Mac.

I will address the remaining issues in a follow-up CL.
1. Class reorganization.
2. File a bug and add a TODO about unit tests.