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

[Jeff King <peff@peff.net>]

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

> > 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