Re: [PATCH] http(s): automatically try NTLM authentication first

From: Jeff King <peff@peff.net>
Date: Feb 22, 2017, 21:06 UTC
Message-ID: <20170222210636.k2ps3qhhpiyyv6cp@sigill.intra.peff.net>
In-Reply-To: <xmqqpoiaasgj.fsf@gitster.mtv.corp.google.com>

On Wed, Feb 22, 2017 at 12:19:56PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> > It would be one thing if cURL would not let the user specify credentials
> > interactively after attempting NTLM authentication (i.e. login
> > credentials), but that is not the case.
> >
> > It would be another thing if attempting NTLM authentication was not
> > usually what users need to do when trying to authenticate via https://.
> > But that is also not the case.
> 
> Some other possible worries we may have had I can think of are:
> 
>  - With this enabled unconditionally, would we leak some information?
> 
>  - With this enabled unconditionally, would we always incur an extra
>    roundtrip for people who are not running NTLM at all?
> 
> I do not think the former is the case, but what would I know (adding a
> few people involved in the original thread to CC: ;-)

I don't think it incurs an extra round-trip now, because of the way libcurl works. Though I think it _does_ make it harder for curl to later optimize out that extra round-trip.

The easiest way to see the difference is to run something like:

  GIT_CURL_VERBOSE=1 \
  git ls-remote https://example.com/repo-which-needs-auth.git 2>trace
  egrep '^>|^< HTTP|Authorization' trace

Before this patch, I get (this is against github.com, which only does Basic auth):

  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic <actual credentials>
  < HTTP/1.1 200 OK

And after I get:

  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic Og==
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic <actual credentials>
  < HTTP/1.1 200 OK

In the current trace, you can see that libcurl insists on making a second auth-less request after we've fed it credentials. I'm not sure how to get rid of this useless extra round-trip, but it would be nice to do so (IIRC, it is a probe request to find out the list of auth types that the server supports, which are not remembered from the previous request).

With http.emptyauth, the second round-trip _isn't_ useless. It's trying to send the empty credential.

So while curl isn't currently optimizing out the second call, I think http.emptyauth makes it harder to do the right thing. That's probably fixable if the logic ends up more like:

  - curl reports a 401 to us; actually look at the list of auth methods.

  - if there was gss-negotiate, then kick in the empty-auth magic
    automatically.

  - if empty-auth failed (or if we decided not to try it), ask for a
    credential and retry the request. Either way, tell curl that we want
    to use "Basic" so it doesn't have to do the probe request (and
    obviously if the server did not support Basic, then fail
    immediately).

I think that would keep it to 2 round-trips for the normal "Basic" case, as well as for the GSSNegotiate case. It would be 3 requests when the server offers GSSNegotiate but you can't use it (but you could set http.emptyauth=false to optimize that out).

That's all theoretical, though. I might not even be right about the reason for the second request, and I certainly haven't written any code (nor do I have a GSSNegotiate setup to test against).

-Peff