Kerberos SPN generation for Negotiate challenges

Kerberos uses an SPN (Service Principal Name) to identify a server. This is typically in the form "HTTP/host:port", with the ":port" suffix being optional, and the "HTTP/" prefix is fixed regardless of whether the service is accessed over HTTP or HTTPS.

The issue this is fixing is that the URL host may be an incomplete domain name, a numerical address, or an alias for a canonical DNS name.

By default, Chrome will skip adding the optional port to the SPN, and will use the canonical DNS name for the server (which may be the original server name if it is an A or AAAA record). This matches IE and Firefox's default behavior.

Some intranets are set up so the original host name should be used rather than the canonical name. The canonical name resolution can be disabled with the --disable-spnego-cname-lookup command line flag.

Some intranets are also set up so the optional port should be specified when it is non-standard (non 80 or 443). Use the --enable-spnego-port command line flag.

BUG=29862
TEST=net_unittests.exe --gtest_filter="*CanonicalName*"

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

[cbentzel, 2010-04-02 20:53:11]

This is a rough skeleton of the code to generate a correct SPN for Kerberos (and
the Negotiate/SPNEGO auth scheme).

It's clearly in need of cleanup and unit tests, but I wanted you to take a quick
look to make sure that it's not completely out of line. I'd rather not waste
time polishing a turd.

Thanks,
Chris

[wtc, 2010-04-02 21:26:01]

cbentzel: this approach looks good.

The SPN resolution can be done by HttpNetworkTransaction
itself rather than the Negotiate auth handler.

Alternatively, we can make HttpAuthHandler::GenerateAuthToken
and GenerateDefaultAuthToken async, taking a completion
callback argument.  For Basic, Digest, and NTLM, these
methods would complete synchronously.  For Negotiate, they
would complete asynchronously.

http://codereview.chromium.org/1535019/diff/1/4
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/1/4#newcode72
net/http/http_auth_handler_negotiate_win.cc:72: HostResolver::RequestInfo
info(origin_.host(), port);
Add a TODO comment about handling origin_.host() that is an
IP address.  We'll need to do a reverse DNS lookup in that
case.

http://codereview.chromium.org/1535019/diff/1/5
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/1/5#newcode1129
net/http/http_network_transaction.cc:1129: int
HttpNetworkTransaction::DoResolveSpnComplete(int result) {
DoResolveSpnComplete needs to set next_state_.

http://codereview.chromium.org/1535019/diff/1/6
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/1535019/diff/1/6#newcode87
net/http/http_network_transaction.h:87: STATE_RESOLVE_SPN,
Between the two abbreviations, I think FQDN is more commonly
known than SPN.  So I'd use FQDN or CANONICAL_DNS_NAME
instead of SPN.

[cbentzel, 2010-04-09 15:40:24]

wtc: I addressed all of your issues.

I've made a number of other updates to this code as well. In particular, I'm
adding two command line flags to let users control how the SPNs are generated.
Read the comments in HttpAuthHandlerNegotiate::CreateSPN to see why, or
http://blog.michelbarneveld.nl/michel/archive/2009/11/14/the-reason-why-kb911...
is another great resource.

I've tested manually and am adding unit tests (in addition to a cleanup phase
once the unit tests are complete). 

I'd like to get this in before end-of-day today so it won't need to be cherry
picked into Mstone5, so any comments are appreciated.

[Paweł Hajdan Jr., 2010-04-09 16:53:47]

Drive-by with a test comment.

http://codereview.chromium.org/1535019/diff/11016/8034
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8034#newcode4682
net/http/http_network_transaction_unittest.cc:4682: #if 0
How about marking the test DISABLED instead of this hack?

[cbentzel, 2010-04-09 17:27:26]

http://codereview.chromium.org/1535019/diff/11016/8034
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8034#newcode4682
net/http/http_network_transaction_unittest.cc:4682: #if 0
On 2010/04/09 16:53:50, Paweł Hajdan Jr. wrote:
> How about marking the test DISABLED instead of this hack?

Sorry, the test was in a bad state and I was planning to fix it before sending
out for final review. 

If for some reason I don't get it fixed I will change from #if 0 to DISABLED.

[cbentzel, 2010-04-09 18:36:53]

One other point - this incorrectly changes how the SPNs are generated for the
NTLM scheme (although it doesn't appear to impact much in my
test setup). I need to return this behavior to the previous state.

[wtc, 2010-04-09 19:42:48]

LGTM.  I reviewed everything except for the new commented-out
http network transaction unit test.

We probably should do another round of review.

http://codereview.chromium.org/1535019/diff/11016/8018
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8018#newcode222
chrome/browser/io_thread.cc:222:
registry_factory->GetSchemeFactory("negotiate"));
Nit: this (the argument of static_cast) should also indent by 4.

http://codereview.chromium.org/1535019/diff/11016/8019
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8019#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias CNAME record for the
Kerberos SPN rather than trying to
Should this option be named auth-negotiate-use-alias?

Doesn't CNAME mean "canonical name"?  (I am not familiar with
DNS.)

http://codereview.chromium.org/1535019/diff/11016/8021
File net/base/address_list.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8021#newcode138
net/base/address_list.cc:138: if (!data_ || !data_->head ||
!data_->head->ai_canonname)
Nit: we can omit the null check for data_->head.

An AddressList::Data object should not have a null |head|
member.

http://codereview.chromium.org/1535019/diff/11016/8023
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/1535019/diff/11016/8023#newcode90
net/http/http_auth_handler.h:90: virtual bool NeedsFQDN() { return false; }
Should this method be renamed NeedsCanonicalName?

http://codereview.chromium.org/1535019/diff/11016/8023#newcode114
net/http/http_auth_handler.h:114: // Does an async lookup of the resolver.
Returns a net error code.
Nit: resolver => FQDN

http://codereview.chromium.org/1535019/diff/11016/8023#newcode143
net/http/http_auth_handler.h:143: // The {scheme, host, port} for the
authentication target.  Used by "ntlm"
Can you update this comment to say "ntlm" and "negotiate"? Thanks.

http://codereview.chromium.org/1535019/diff/11016/8025
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/1535019/diff/11016/8025#newcode23
net/http/http_auth_handler_negotiate.h:23: class HostResolver;
You don't need the forward declaraion of HostResolver because
you already include "net/base/host_resolver.h" above.

It seems that you can just rely on the forward declaration
of HostResolver in "net/http/http_auth_handler.h".

http://codereview.chromium.org/1535019/diff/11016/8025#newcode60
net/http/http_auth_handler_negotiate.h:60: bool use_cname_;
The use_cname_ and use_port_ members should be documented.
You can also just document their accessor methods above.

http://codereview.chromium.org/1535019/diff/11016/8025#newcode112
net/http/http_auth_handler_negotiate.h:112:
net::CompletionCallbackImpl<HttpAuthHandlerNegotiate> resolve_fqdn_callback_;
Don't need the net:: namespace prefix.

http://codereview.chromium.org/1535019/diff/11016/8026
File net/http/http_auth_handler_negotiate_posix.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8026#newcode64
net/http/http_auth_handler_negotiate_posix.cc:64: bool
HttpAuthHandlerNegotiate::NeedsFQDN() {
This class can just use the default implementations from
the base class.

Did you implement these two methods here because we will
eventually need to implement them properly?

http://codereview.chromium.org/1535019/diff/11016/8027
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8027#newcode22
net/http/http_auth_handler_negotiate_win.cc:22: resolve_fqdn_callback_(
Nit: you can put this on the previous line.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode78
net/http/http_auth_handler_negotiate_win.cc:78: // Skip doing the canonical name
lookup if the user wants to just
This comment contradicts itself.  If use_cname_ is true, I'd
expect that we need to look up the canonical name.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode90
net/http/http_auth_handler_negotiate_win.cc:90: switch (rv) {
You can use an if-else statement so that there is only
one CreateSPN() call:

  if (rv == ERR_IO_PENDING) {
    user_callback_ = callback;
  } else {
    // Even in the error case, try to use the origin_.host
    // rather than failing.
    CreateSPN();
    rv = OK;
  }

http://codereview.chromium.org/1535019/diff/11016/8027#newcode109
net/http/http_auth_handler_negotiate_win.cc:109: // log the problem, but pretend
that everything is still OK.
Add the log message you referred to.

Should we also log |rv| in the synchronous completion case
at line 101 above if it's not OK?

http://codereview.chromium.org/1535019/diff/11016/8027#newcode114
net/http/http_auth_handler_negotiate_win.cc:114: if (user_callback_) {
Should we DCHECK user_callback_ is not NULL?
In async mode, we must have a user_callback_.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode156
net/http/http_auth_handler_negotiate_win.cc:156: if (port != 80 && port != 443
&& use_port_) {
The check for 80 and 443 could be more sophisticated and
also take origin_.scheme() into account.

http://codereview.chromium.org/1535019/diff/11016/8028
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8028#newcode25
net/http/http_auth_handler_ntlm.cc:25: ASCIIToWide(origin_.host()),
This needs to be in the SPN format.

http://codereview.chromium.org/1535019/diff/11016/8029
File net/http/http_auth_handler_ntlm_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8029#newcode50
net/http/http_auth_handler_ntlm_win.cc:50: ASCIIToWide(origin_.host()),
This needs to be in the SPN format.

http://codereview.chromium.org/1535019/diff/11016/8030
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8030#newcode357
net/http/http_auth_sspi_win.cc:357: #if 0
Remember to delete this block of code.

http://codereview.chromium.org/1535019/diff/11016/8032
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8032#newcode1134
net/http/http_network_transaction.cc:1134: next_state_ = STATE_NONE;
Remove this line.  DoLoop() sets next_state_ to STATE_NONE
before calling this method.

You can add a comment to explain that this state terminates
the Start sequence, which usually terminates in the
DoReadHeadersComplete state.  This fact is not obvious.

It seems that this method should return |result| instead of
OK.

http://codereview.chromium.org/1535019/diff/11016/8032#newcode1943
net/http/http_network_transaction.cc:1943: if
(auth_handler_[target]->NeedsFQDN()) {
Nit: no curly braces.

[cbentzel, 2010-04-09 19:54:45]

Thanks - I agree with the need for another round of review. I'm nearly done with
the unit tests, and will do a clean-up pass after that's done before pinging you
again.

Here's two quick responses to your comments:

http://codereview.chromium.org/1535019/diff/11016/8019
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8019#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias CNAME record for the
Kerberos SPN rather than trying to
On 2010/04/09 19:42:48, wtc wrote:
> Should this option be named auth-negotiate-use-alias?
> 
> Doesn't CNAME mean "canonical name"?  (I am not familiar with
> DNS.)

Yes. However, the IE options were all CNAME based, where a CNAME DNS record
indicates that the domain name being queried is an alias (and instead of being
an A or AAAA result, is a pointer to the canonical name).

I agree that it's a bit confusing.

http://codereview.chromium.org/1535019/diff/11016/8026
File net/http/http_auth_handler_negotiate_posix.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8026#newcode64
net/http/http_auth_handler_negotiate_posix.cc:64: bool
HttpAuthHandlerNegotiate::NeedsFQDN() {
On 2010/04/09 19:42:48, wtc wrote:
> This class can just use the default implementations from
> the base class.
> 
> Did you implement these two methods here because we will
> eventually need to implement them properly?

I believe the Windows implementation can be shared with the posix implementation
- but since I haven't tested that configuration I'd prefer to hold off.

[cbentzel, 2010-04-09 20:59:37]

http://codereview.chromium.org/1535019/diff/11016/8019
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8019#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias CNAME record for the
Kerberos SPN rather than trying to
On 2010/04/09 19:54:45, cbentzel wrote:
> On 2010/04/09 19:42:48, wtc wrote:
> > Should this option be named auth-negotiate-use-alias?

I will change this to "alias" rather than "cname" throughout to prevent
confusion. Thanks for the suggestion.

http://codereview.chromium.org/1535019/diff/11016/8021
File net/base/address_list.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8021#newcode138
net/base/address_list.cc:138: if (!data_ || !data_->head ||
!data_->head->ai_canonname)
On 2010/04/09 19:42:48, wtc wrote:
> Nit: we can omit the null check for data_->head.
> 
> An AddressList::Data object should not have a null |head|
> member.


For now, I will remove the if (!data_) check introduced with this CL and change
the caller site. This makes the implementation more consistent with the other
methods (such as GetPort) but ultimately I think this class needs to be cleaned
up a little bit.

I added the checks after segfault'ing on a call. The AddressList was constructed
but Adopt/Copy had not been called, so data_ was NULL. There's no DCHECK's on
Copy/Adopt to validate that the head is non-NULL. We should probably do a pass
on this code to either make it more paranoid on inputs or more defensive on get
methods. There is also no way for a caller to determine if data_ is non-NULL
(head() dereferences data_ for example).

[wtc, 2010-04-09 21:46:38]

http://codereview.chromium.org/1535019/diff/11016/8021
File net/base/address_list.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8021#newcode138
net/base/address_list.cc:138: if (!data_ || !data_->head ||
!data_->head->ai_canonname)
I agree with the !data_ check you added.  I was just pointing
out that the !data_->head check is not necessary.  I based that
on the AddressList::Data constructor, which always
initializes the |head| member.

[cbentzel, 2010-04-10 04:03:08]

Thanks again for the review. I think I'll try to get a unit test for the
HttpAuthHandlerNegotiate implementation of NeedsFQDN/ResolveFQDN as my current
test is only for HttpnetworkTransaction's handling of those cases.

http://codereview.chromium.org/1535019/diff/11016/8018
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8018#newcode222
chrome/browser/io_thread.cc:222:
registry_factory->GetSchemeFactory("negotiate"));
On 2010/04/09 19:42:48, wtc wrote:
> Nit: this (the argument of static_cast) should also indent by 4.

Done.

http://codereview.chromium.org/1535019/diff/11016/8019
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8019#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias CNAME record for the
Kerberos SPN rather than trying to
On 2010/04/09 20:59:37, cbentzel wrote:
> On 2010/04/09 19:54:45, cbentzel wrote:
> > On 2010/04/09 19:42:48, wtc wrote:
> > > Should this option be named auth-negotiate-use-alias?
> 
> I will change this to "alias" rather than "cname" throughout to prevent
> confusion. Thanks for the suggestion. 
> 
> 

Done.

http://codereview.chromium.org/1535019/diff/11016/8021
File net/base/address_list.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8021#newcode138
net/base/address_list.cc:138: if (!data_ || !data_->head ||
!data_->head->ai_canonname)
On 2010/04/09 21:46:40, wtc wrote:
> I agree with the !data_ check you added.  I was just pointing
> out that the !data_->head check is not necessary.  I based that
> on the AddressList::Data constructor, which always
> initializes the |head| member.

Sounds good. Done.

http://codereview.chromium.org/1535019/diff/11016/8023
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/1535019/diff/11016/8023#newcode90
net/http/http_auth_handler.h:90: virtual bool NeedsFQDN() { return false; }
On 2010/04/09 19:42:48, wtc wrote:
> Should this method be renamed NeedsCanonicalName?

Perhaps - I don't feel strongly either way.

http://codereview.chromium.org/1535019/diff/11016/8023#newcode114
net/http/http_auth_handler.h:114: // Does an async lookup of the resolver.
Returns a net error code.
On 2010/04/09 19:42:48, wtc wrote:
> Nit: resolver => FQDN

Done.

http://codereview.chromium.org/1535019/diff/11016/8023#newcode143
net/http/http_auth_handler.h:143: // The {scheme, host, port} for the
authentication target.  Used by "ntlm"
On 2010/04/09 19:42:48, wtc wrote:
> Can you update this comment to say "ntlm" and "negotiate"? Thanks.

Done.

http://codereview.chromium.org/1535019/diff/11016/8025
File net/http/http_auth_handler_negotiate.h (right):

http://codereview.chromium.org/1535019/diff/11016/8025#newcode23
net/http/http_auth_handler_negotiate.h:23: class HostResolver;
On 2010/04/09 19:42:48, wtc wrote:
> You don't need the forward declaraion of HostResolver because
> you already include "net/base/host_resolver.h" above.
> 
> It seems that you can just rely on the forward declaration
> of HostResolver in "net/http/http_auth_handler.h".

Done.

http://codereview.chromium.org/1535019/diff/11016/8025#newcode60
net/http/http_auth_handler_negotiate.h:60: bool use_cname_;
On 2010/04/09 19:42:48, wtc wrote:
> The use_cname_ and use_port_ members should be documented.
> You can also just document their accessor methods above.

Done.

http://codereview.chromium.org/1535019/diff/11016/8025#newcode112
net/http/http_auth_handler_negotiate.h:112:
net::CompletionCallbackImpl<HttpAuthHandlerNegotiate> resolve_fqdn_callback_;
On 2010/04/09 19:42:48, wtc wrote:
> Don't need the net:: namespace prefix.

Done.

http://codereview.chromium.org/1535019/diff/11016/8027
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8027#newcode22
net/http/http_auth_handler_negotiate_win.cc:22: resolve_fqdn_callback_(
On 2010/04/09 19:42:48, wtc wrote:
> Nit: you can put this on the previous line.

Done.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode78
net/http/http_auth_handler_negotiate_win.cc:78: // Skip doing the canonical name
lookup if the user wants to just
On 2010/04/09 19:42:48, wtc wrote:
> This comment contradicts itself.  If use_cname_ is true, I'd
> expect that we need to look up the canonical name.

You probably wrote this comment because of the confusing use_cname_ name. I've
changed it to use_alias_ after your suggestion, and fixed the contradiction.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode90
net/http/http_auth_handler_negotiate_win.cc:90: switch (rv) {
On 2010/04/09 19:42:48, wtc wrote:
> You can use an if-else statement so that there is only
> one CreateSPN() call:
> 
>   if (rv == ERR_IO_PENDING) {
>     user_callback_ = callback;
>   } else {
>     // Even in the error case, try to use the origin_.host
>     // rather than failing.
>     CreateSPN();
>     rv = OK;
>   }

I retained the switch statement so the default case logs what the problem was.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode109
net/http/http_auth_handler_negotiate_win.cc:109: // log the problem, but pretend
that everything is still OK.
On 2010/04/09 19:42:48, wtc wrote:
> Add the log message you referred to.
> 
> Should we also log |rv| in the synchronous completion case
> at line 101 above if it's not OK?

Yes. Added log statements in both locations.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode114
net/http/http_auth_handler_negotiate_win.cc:114: if (user_callback_) {
On 2010/04/09 19:42:48, wtc wrote:
> Should we DCHECK user_callback_ is not NULL?
> In async mode, we must have a user_callback_.

Good point. I DCHECK and skip the if (user_callback_) call, which seems more
consistent with other code.

http://codereview.chromium.org/1535019/diff/11016/8027#newcode156
net/http/http_auth_handler_negotiate_win.cc:156: if (port != 80 && port != 443
&& use_port_) {
On 2010/04/09 19:42:48, wtc wrote:
> The check for 80 and 443 could be more sophisticated and
> also take origin_.scheme() into account.

I'm not quite sure what the behavior should be in the very corner case of 

http://foo.bar.com:443 

or 

https://foo.bar.com:80

http://codereview.chromium.org/1535019/diff/11016/8028
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8028#newcode25
net/http/http_auth_handler_ntlm.cc:25: ASCIIToWide(origin_.host()),
On 2010/04/09 19:42:48, wtc wrote:
> This needs to be in the SPN format.

Yes, thanks for catching this.

http://codereview.chromium.org/1535019/diff/11016/8029
File net/http/http_auth_handler_ntlm_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8029#newcode50
net/http/http_auth_handler_ntlm_win.cc:50: ASCIIToWide(origin_.host()),
On 2010/04/09 19:42:48, wtc wrote:
> This needs to be in the SPN format.

Done.

http://codereview.chromium.org/1535019/diff/11016/8030
File net/http/http_auth_sspi_win.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8030#newcode357
net/http/http_auth_sspi_win.cc:357: #if 0
On 2010/04/09 19:42:48, wtc wrote:
> Remember to delete this block of code.

Thanks. Done.

http://codereview.chromium.org/1535019/diff/11016/8032
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/11016/8032#newcode1134
net/http/http_network_transaction.cc:1134: next_state_ = STATE_NONE;
On 2010/04/09 19:42:48, wtc wrote:
> Remove this line.  DoLoop() sets next_state_ to STATE_NONE
> before calling this method.
> 
> You can add a comment to explain that this state terminates
> the Start sequence, which usually terminates in the
> DoReadHeadersComplete state.  This fact is not obvious.

Good point - you actually mentioned that next_state_ needed to be set in the
first pass review a while ago, so this was not obvious.

> It seems that this method should return |result| instead  > of OK.

Fixed.

http://codereview.chromium.org/1535019/diff/11016/8032#newcode1943
net/http/http_network_transaction.cc:1943: if
(auth_handler_[target]->NeedsFQDN()) {
On 2010/04/09 19:42:48, wtc wrote:
> Nit: no curly braces.

It will take me a while to break this habit. Done.

[eroman, 2010-04-12 22:55:44]

WARNING: This is going to crash if the HttpNetworkTransaction is deleted while a
CNAME resolution is in progress.

(The auth handler starts a request on the global HostResolver but doesn't cancel
it after being deleted, so the callback could fire on a deleted object).

You can solve this by using a SingleRequestHostResolver to issue the resolve, it
will automatically cancel the outstanding request when it goes out of scope.


I haven't had a chance to actually read this CL full (just scanned for the
cancellation scenario), but my un-informed gut feeling is that the FQDN
resolving should not be a state of HttpNetworkTransaction, but rather hidden in
the auth handler. To accomplish this the auth handler API would need to be made
asynchronous.

[cbentzel, 2010-04-12 23:20:24]

Ay carumba - good catch!

On Apr 12, 2010 6:56 PM, <eroman@chromium.org> wrote:

WARNING: This is going to crash if the HttpNetworkTransaction is deleted
while a
CNAME resolution is in progress.

(The auth handler starts a request on the global HostResolver but doesn't
cancel
it after being deleted, so the callback could fire on a deleted object).

You can solve this by using a SingleRequestHostResolver to issue the
resolve, it
will automatically cancel the outstanding request when it goes out of scope.


I haven't had a chance to actually read this CL full (just scanned for the
cancellation scenario), but my un-informed gut feeling is that the FQDN
resolving should not be a state of HttpNetworkTransaction, but rather hidden
in
the auth handler. To accomplish this the auth handler API would need to be
made
asynchronous.



http://codereview.chromium.org/1535019/show

[wtc, 2010-04-12 23:21:16]

LGTM.

Please update the CL's description before you check this in.
In particular:
- the STATE_RESOLVE_SPN and STATE_RESOLVE_SPN_COMPLETE states
  have been renamed
- "TEST=Need to add some" is probably no longer true because
  you added one unit test.

I'd like to see the following two terminology issues eliminated.
- "FQDN" vs. "canonical name": I don't think FQDN implies it's
  a canonical name.
- "Alias" vs. "CNAME record": we add a CNAME record to
  create an alias, but if we look up a CNAME record, we
  want to know what the canonical name is.  I believe you're
  using "CNAME record" in the first sense, and this is
  confusing because HostResolver only *looks up* CNAME
  record.

http://codereview.chromium.org/1535019/diff/6002/37002
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37002#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias instead of trying to
determine the canonical name for a host
Nit: "Use an alias" is confusing because the host name in the
URL may actually be the canonical name.  We should say
something like "may use an alias" or "ok to use an alias".

http://codereview.chromium.org/1535019/diff/6002/37002#newcode51
chrome/common/chrome_switches.cc:51: // Use non-standard ports in the Kerberos
SPN. See
Nit: add "for the protocol" after "ports".

http://codereview.chromium.org/1535019/diff/6002/37005
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/1535019/diff/6002/37005#newcode114
net/http/http_auth_handler.h:114: // Resolves the canonical name for the
|origin_| host. The canonical
Nit: the comment should match the code.  The comment says
"canonical name", but the code says "FQDN", which may be a
DNS alias.

http://codereview.chromium.org/1535019/diff/6002/37009
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37009#newcode17
net/http/http_auth_handler_negotiate_win.cc:17: bool use_cname,
The use_cname argument should be renamed use_alias.

http://codereview.chromium.org/1535019/diff/6002/37009#newcode78
net/http/http_auth_handler_negotiate_win.cc:78: // use CNAME records.
Please clarify the comment "use CNAME records".

If I get a CNAME record, I know what the canonical name is.
So if the user wants to just use CNAME records, it implies
to me that the user wants to use the canonical name.  But
this is not what the code does.

http://codereview.chromium.org/1535019/diff/6002/37009#newcode85
net/http/http_auth_handler_negotiate_win.cc:85: int rv = resolver->Resolve(info,
&address_list_,
Nit: One way to eliminate the duplicate code between synchronous
and asynchronous completions is to call OnResolveFQDN for
synchronous completion as well.

This will require bringing back the null check for user_callback_ in
OnResolveFQDN (user_callback_ is NULL
in the synchronous completion case).

http://codereview.chromium.org/1535019/diff/6002/37009#newcode158
net/http/http_auth_handler_negotiate_win.cc:158: if (port != 80 && port != 443
&& use_port_) {
Nit: consider using url_canon::DefaultPortForScheme().

http://codereview.chromium.org/1535019/diff/6002/37010
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37010#newcode117
net/http/http_auth_handler_ntlm.cc:117: // TODO(cbentzel): Should this share
logic and command line flags
The IBM security researcher I worked with on the NTLM
reflection attack vulnerability told me the construction here
is what IE uses and is good enough for NTLM.

http://codereview.chromium.org/1535019/diff/6002/37014
File net/http/http_auth_sspi_win.h (right):

http://codereview.chromium.org/1535019/diff/6002/37014#newcode90
net/http/http_auth_sspi_win.h:90: // |target| is the target service that the
token is being generated for.
Since we use HttpAuthSSPI for NTLM and Negotiate only, the
comment should just say |target| is the SPN.  Otherwise a
reader would wonder what |target| should be for other schemes.

Note: I rememebr SSPI can also be used for Digest auth, but
we don't use SSPI for that.

http://codereview.chromium.org/1535019/diff/6002/37015
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37015#newcode1134
net/http/http_network_transaction.cc:1134: // The RESOLVE_FQDN_COMPLETE state
ends the Start sequence when the
Nit: RESOLVE_FQDN_COMPLETE => STATE_RESOLVE_FQDN_COMPLETE

http://codereview.chromium.org/1535019/diff/6002/37017
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37017#newcode4686
net/http/http_network_transaction_unittest.cc:4686: enum Resolve {
Nit: define this enum type and its values.

http://codereview.chromium.org/1535019/diff/6002/37017#newcode4716
net/http/http_network_transaction_unittest.cc:4716: EXPECT_TRUE(false);
Why don't you use NOTREACHED()?

http://codereview.chromium.org/1535019/diff/6002/37017#newcode4830
net/http/http_network_transaction_unittest.cc:4830: request.url =
GURL("http://www.google.com/");
It would be nice to see a URL of http://www/ and the short
host name "www" getting resolved to the FQDN "www.google.com".
The auth handler can write the host name (either as specified
in the URL or the resolved FQDN) into the generated auth token
for the unit test to check.

[wtc, 2010-04-12 23:36:13]

On 2010/04/12 22:55:44, eroman wrote:
> ... my un-informed gut feeling is that the FQDN
> resolving should not be a state of HttpNetworkTransaction, but rather hidden
in
> the auth handler. To accomplish this the auth handler API would need to be
made
> asynchronous.

This is actually the solution I came up with: do not add new
methods to the HttpAuthHandler interface; instead, just allow
GenerateAuthToken and GenerateDefaultAuthToken to work in
asynchronous mode.

Looking at Chris's code, I think this approach still requires us
to pass a HostResolver* to HttpAuthHandler.

[cbentzel, 2010-04-13 01:36:27]

Thanks again for the reviews.

I'll definitely fix the crashing problem Eric Roman pointed out.

Since both of you prefer moving the states into the HttpAuthHandler (and I
do as well) I'll take a stab at that change.

On Mon, Apr 12, 2010 at 7:36 PM, <wtc@chromium.org> wrote:

> On 2010/04/12 22:55:44, eroman wrote:
>
>> ... my un-informed gut feeling is that the FQDN
>>
>> resolving should not be a state of HttpNetworkTransaction, but rather
>> hidden
>>
> in
>
>> the auth handler. To accomplish this the auth handler API would need to be
>>
> made
>
>> asynchronous.
>>
>
> This is actually the solution I came up with: do not add new
> methods to the HttpAuthHandler interface; instead, just allow
> GenerateAuthToken and GenerateDefaultAuthToken to work in
> asynchronous mode.
>
> Looking at Chris's code, I think this approach still requires us
> to pass a HostResolver* to HttpAuthHandler.
>
>
> http://codereview.chromium.org/1535019/show
>

[wtc, 2010-04-13 01:41:38]

Chris,

To make code review easier, you can check in this CL first
(after cleaning it up).

But you may want to make all the requested changes to make
it easier to backport to the M5 branch.

[cbentzel, 2010-04-13 15:15:34]

On 2010/04/12 23:21:16, wtc wrote:

> I'd like to see the following two terminology issues eliminated.
> - "FQDN" vs. "canonical name": I don't think FQDN implies it's
>   a canonical name.

Agree, FQDN does not imply this. Canonical name is more precise. I am changing
the names.

> - "Alias" vs. "CNAME record": we add a CNAME record to
>   create an alias, but if we look up a CNAME record, we
>   want to know what the canonical name is.  I believe you're
>   using "CNAME record" in the first sense, and this is
>   confusing because HostResolver only *looks up* CNAME
>   record.

Rather than keep "use_alias" variables throughout the code base (and command
line flags) I will change this to "disable_cname_lookup". Then the alias
terminology is removed.

[cbentzel, 2010-04-13 20:09:30]

http://codereview.chromium.org/1535019/diff/6002/37002
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37002#newcode46
chrome/common/chrome_switches.cc:46: // Use an alias instead of trying to
determine the canonical name for a host
On 2010/04/12 23:21:17, wtc wrote:
> Nit: "Use an alias" is confusing because the host name in the
> URL may actually be the canonical name.  We should say
> something like "may use an alias" or "ok to use an alias".

I changed this to --disable-spnego-cname-lookup. This should be even clearer
about what the flag is for.

http://codereview.chromium.org/1535019/diff/6002/37002#newcode51
chrome/common/chrome_switches.cc:51: // Use non-standard ports in the Kerberos
SPN. See
On 2010/04/12 23:21:17, wtc wrote:
> Nit: add "for the protocol" after "ports".

I changed this to "Enable the inclusion of non-standard ports when generating
the Kerberos SPN in response to a SPNEGO challenge."

http://codereview.chromium.org/1535019/diff/6002/37005
File net/http/http_auth_handler.h (right):

http://codereview.chromium.org/1535019/diff/6002/37005#newcode114
net/http/http_auth_handler.h:114: // Resolves the canonical name for the
|origin_| host. The canonical
On 2010/04/12 23:21:17, wtc wrote:
> Nit: the comment should match the code.  The comment says
> "canonical name", but the code says "FQDN", which may be a
> DNS alias.

Done.

http://codereview.chromium.org/1535019/diff/6002/37009
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37009#newcode17
net/http/http_auth_handler_negotiate_win.cc:17: bool use_cname,
On 2010/04/12 23:21:17, wtc wrote:
> The use_cname argument should be renamed use_alias.

Changed everything to disable_cname_lookup

http://codereview.chromium.org/1535019/diff/6002/37009#newcode78
net/http/http_auth_handler_negotiate_win.cc:78: // use CNAME records.
On 2010/04/12 23:21:17, wtc wrote:
> Please clarify the comment "use CNAME records".
> 
> If I get a CNAME record, I know what the canonical name is.
> So if the user wants to just use CNAME records, it implies
> to me that the user wants to use the canonical name.  But
> this is not what the code does.

Done.

http://codereview.chromium.org/1535019/diff/6002/37009#newcode85
net/http/http_auth_handler_negotiate_win.cc:85: int rv = resolver->Resolve(info,
&address_list_,
On 2010/04/12 23:21:17, wtc wrote:
> Nit: One way to eliminate the duplicate code between synchronous
> and asynchronous completions is to call OnResolveFQDN for
> synchronous completion as well.
> 
> This will require bringing back the null check for user_callback_ in
> OnResolveFQDN (user_callback_ is NULL
> in the synchronous completion case).

Done.

http://codereview.chromium.org/1535019/diff/6002/37009#newcode158
net/http/http_auth_handler_negotiate_win.cc:158: if (port != 80 && port != 443
&& use_port_) {
On 2010/04/12 23:21:17, wtc wrote:
> Nit: consider using url_canon::DefaultPortForScheme().

I considered this but didn't know what the behavior should be if the following
was valid.

http://myserver:443

http://codereview.chromium.org/1535019/diff/6002/37010
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37010#newcode117
net/http/http_auth_handler_ntlm.cc:117: // TODO(cbentzel): Should this share
logic and command line flags
On 2010/04/12 23:21:17, wtc wrote:
> The IBM security researcher I worked with on the NTLM
> reflection attack vulnerability told me the construction here
> is what IE uses and is good enough for NTLM.

Removed the TODO.

http://codereview.chromium.org/1535019/diff/6002/37014
File net/http/http_auth_sspi_win.h (right):

http://codereview.chromium.org/1535019/diff/6002/37014#newcode90
net/http/http_auth_sspi_win.h:90: // |target| is the target service that the
token is being generated for.
On 2010/04/12 23:21:17, wtc wrote:
> Since we use HttpAuthSSPI for NTLM and Negotiate only, the
> comment should just say |target| is the SPN.  Otherwise a
> reader would wonder what |target| should be for other schemes.
> 
> Note: I rememebr SSPI can also be used for Digest auth, but
> we don't use SSPI for that.

There's also schannel. I just matched the name for the SSPI argument. I would
change the variable name to spn if we want to restrict to that.

http://codereview.chromium.org/1535019/diff/6002/37015
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37015#newcode1134
net/http/http_network_transaction.cc:1134: // The RESOLVE_FQDN_COMPLETE state
ends the Start sequence when the
On 2010/04/12 23:21:17, wtc wrote:
> Nit: RESOLVE_FQDN_COMPLETE => STATE_RESOLVE_FQDN_COMPLETE

Done.

http://codereview.chromium.org/1535019/diff/6002/37017
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1535019/diff/6002/37017#newcode4716
net/http/http_network_transaction_unittest.cc:4716: EXPECT_TRUE(false);
On 2010/04/12 23:21:17, wtc wrote:
> Why don't you use NOTREACHED()?

No good reason. Fixed.

http://codereview.chromium.org/1535019/diff/6002/37017#newcode4830
net/http/http_network_transaction_unittest.cc:4830: request.url =
GURL("http://www.google.com/");
On 2010/04/12 23:21:17, wtc wrote:
> It would be nice to see a URL of http://www/ and the short
> host name "www" getting resolved to the FQDN "www.google.com".
> The auth handler can write the host name (either as specified
> in the URL or the resolved FQDN) into the generated auth token
> for the unit test to check.

Done.

[cbentzel, 2010-04-13 21:07:16]

http://codereview.chromium.org/1535019/diff/50001/51015
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/50001/51015#newcode578
net/http/http_network_transaction.cc:578: rv = DoResolveCanonicalName();
I have a pending change which adds TRACE_EVENT_BEGIN/TRACE_EVENT_END and
net_log_.BeginEvent/EndEvent instrumentation here. Should I add this change as
well?

[wtc, 2010-04-13 23:31:37]

LGTM.

http://codereview.chromium.org/1535019/diff/13002/52002
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/1535019/diff/13002/52002#newcode175
chrome/common/chrome_switches.cc:175: extern const char
kDisableSpnegoCnameLookup[] = "disable-spnego-cname-lookup";
I would use "negotiate-auth" instead of "spnego" because we
have not used the SPNEGO term in our source code yet.

http://codereview.chromium.org/1535019/diff/13002/52008
File net/http/http_auth_handler_negotiate_posix.cc (right):

http://codereview.chromium.org/1535019/diff/13002/52008#newcode78
net/http/http_auth_handler_negotiate_posix.cc:78: use_port_(false) {
Nit: it is fine to list multiple member initiaizers on the
same line, even though the Style Guide doesn't show an example
of it.

http://codereview.chromium.org/1535019/diff/13002/52009
File net/http/http_auth_handler_negotiate_win.cc (right):

http://codereview.chromium.org/1535019/diff/13002/52009#newcode71
net/http/http_auth_handler_negotiate_win.cc:71: CreateSPN();
It is a little unexpected that a "predicate" function has a
side effect.

http://codereview.chromium.org/1535019/diff/13002/52010
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/1535019/diff/13002/52010#newcode117
net/http/http_auth_handler_ntlm.cc:117: std::wstring target(L"HTTP/");
Let's name this variable |spn| instead of |target|

http://codereview.chromium.org/1535019/diff/13002/52014
File net/http/http_auth_sspi_win.h (right):

http://codereview.chromium.org/1535019/diff/13002/52014#newcode90
net/http/http_auth_sspi_win.h:90: // |target| is the target service that the
token is being generated for.
Let's rename |target| to |spn| because our HTTP auth code also
has a Target type that indicates whether we're authenticating
to a proxy or a server.

Please rename the |target| parameter of GenerateAuthToken as
well and also update the .cc file with the new parameter
names.

http://codereview.chromium.org/1535019/diff/13002/52015
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/13002/52015#newcode1138
net/http/http_network_transaction.cc:1138: DCHECK(next_state_ == STATE_NONE);
This is guaranteed by DoLoop upon entering any DoState()
function.

It may be more useful to DCHECK_EQ(result, OK).

[wtc, 2010-04-13 23:34:52]

http://codereview.chromium.org/1535019/diff/50001/51015
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1535019/diff/50001/51015#newcode578
net/http/http_network_transaction.cc:578: rv = DoResolveCanonicalName();
On 2010/04/13 21:07:17, cbentzel wrote:
> I have a pending change which adds TRACE_EVENT_BEGIN/TRACE_EVENT_END and
> net_log_.BeginEvent/EndEvent instrumentation here. Should I add this change as
> well?

Let's check in this CL first.  It is tiring to review the
a large CL repeatedly.

[cbentzel, 2010-04-14 13:43:31]

The latest patchset resolves your last few concerns and naming suggestions.

I'll commit this (pending trybot success) and make the other changes in separate
CLs.