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

Jeff King [off-list ref] writes:

On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:

quoted

Thanks for your thoughts.  I'd think that we should take this change
and leave the optimization for later, then.  It's not like the
change of the default is making the normal situation any worse, it
seems.

I'm not excited that it will start making known bogus-username requests
by default to servers which do not even support Negotiate. I guess that
is really the server-operators problem, but it feels pretty hacky.

I guess that's another valid concern.  The servers used to be able
to say "Ah, this repository needs auth and this request does not, so
reject it without asking the auth-db".  Now it must say "Ah, this
repository needs auth and this request does have one, but it is
empty so let's not even bother the auth-db" in order to reject a
useless "empty-auth" request with the same efficiency.

After the first request without auth (that fails), do we learn
anything useful from the server side (like "it knows Negotiate")
that we can use to flip the "empty-auth" bit to give a better
default to people from both worlds, I wonder...?

Yes, that's exactly what I was trying to say in my first message.

I see.  I am still inclined to take this as-is for now to cook in
'next', though.  

A solution along your line would help Negotiate users OOB experience
without hurting the servers that do not offer Negotiate, but until
that materializes, users can set the lazier http.emptyAuth on
(without selectively setting http.<host>.emptyAuth off for sites
without Negotiate) and hurt the servers by throwing an empty auth
anyway regardless of the default, so the flipping of the default is
not fundamentally adding more harm in that sense.

On Wed, Feb 22, 2017 at 02:35:11PM -0800, Junio C Hamano wrote:

A solution along your line would help Negotiate users OOB experience
without hurting the servers that do not offer Negotiate, but until
that materializes, users can set the lazier http.emptyAuth on
(without selectively setting http.<host>.emptyAuth off for sites
without Negotiate) and hurt the servers by throwing an empty auth
anyway regardless of the default, so the flipping of the default is
not fundamentally adding more harm in that sense.

I was hoping to materialize it today. :)

Here's what I came up with. I have a lot of questions about the second
patch which I'll outline there. But I think it may be a good start.

  [1/2]: http: restrict auth methods to what the server advertises
  [2/2]: http: add an "auto" mode for http.emptyauth

 http.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

-Peff

This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.

However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.

Instead, let's introduce an automatic mode, that works like
this:

  1. We won't try to send the bogus credential on the first
     request. We'll wait to get an HTTP 401, as usual.

  2. After seeing an HTTP 401, the empty-auth hack will kick
     in only when we know there is an auth method beyond
     "Basic" to be tried.

That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.

This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).

The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.

Signed-off-by: Jeff King <redacted>
---
My open questions are:

  - I don't have anything but a Basic server to test against. So it's
    entirely possible that this doesn't actually work in the NTLM case.

  - what does a request log look like for somebody actually using NTLM?
    It's possible if the initial request sends a restrict auth_avail,
    that curl could get away without the extra probe request, and we'd
    end up with the same number of requests for "auto" mode versus
    http.emptyauth=true.

  - the whole "don't use this on the initial request" flag feels really
    hacky. It's a side effect of how emptyauth tries to kick in even
    before we have sent any requests. Probably it should have been
    handled in the 401 code path originally, but I'm hesitant to change
    it now. I suspect it is eliminating a round-trip in practice when it
    is enabled.

  - I didn't test a server that advertises Basic and something else, but
    really only takes Basic. So I'm just assuming that it incurs the
    extra round-trip (actually probably two, one for curl's method
    probe).

  - When your curl is too old to do CURLAUTH_ANY, I just left the
    default to disable emptyauth. But it could easily be "1" if people
    care.

 http.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index a05609766..ea70ec1ee 100644
--- a/http.c
+++ b/http.c

@@ -109,7 +109,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 

@@ -125,6 +125,7 @@ static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
 #endif
 
 static struct curl_slist *pragma_header;

@@ -382,10 +383,37 @@ static int http_options(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+static int curl_empty_auth_enabled(void)
+{
+	if (curl_empty_auth < 0) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+		/*
+		 * In the automatic case, kick in the empty-auth
+		 * hack as long as we would potentially try some
+		 * method more exotic than "Basic".
+		 *
+		 * But only do so when this is _not_ our initial
+		 * request, as we would not then yet know what
+		 * methods are available.
+		 */
+		return http_auth_methods_restricted &&
+		       http_auth_methods != CURLAUTH_BASIC;
+#else
+		/*
+		 * Our libcurl is too old to do AUTH_ANY in the first place;
+		 * just default to turning the feature off.
+		 */
+		return 0;
+#endif
+	}
+
+	return curl_empty_auth;
+}
+
 static void init_curl_http_auth(CURL *result)
 {
 	if (!http_auth.username || !*http_auth.username) {
-		if (curl_empty_auth)
+		if (curl_empty_auth_enabled())
 			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
 		return;
 	}

@@ -1079,7 +1107,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-	if (http_auth.password || curl_empty_auth)
+	if (http_auth.password || curl_empty_auth_enabled())
 		init_curl_http_auth(slot->curl);
 
 	return slot;

@@ -1347,8 +1375,10 @@ static int handle_curl_result(struct slot_results *results)
 		} else {
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-			if (results->auth_avail)
+			if (results->auth_avail) {
 				http_auth_methods &= results->auth_avail;
+				http_auth_methods_restricted = 1;
+			}
 #endif
 			return HTTP_REAUTH;
 		}

-- 
2.12.0.rc2.597.g959f68882

By default, we tell curl to use CURLAUTH_ANY, which does not
limit its set of auth methods. However, this results in an
extra round-trip to the server when authentication is
required. After we've fed the credential to curl, it wants
to probe the server to find its list of available methods
before sending an Authorization header.

We can shortcut this by limiting our http_auth_methods by
what the server told us it supports. In some cases (such as
when the server only supports Basic), that lets curl skip
the extra probe request.

The end result should look the same to the user, but you can
use GIT_TRACE_CURL to verify the sequence of requests:

  GIT_TRACE_CURL=1 \
  git ls-remote https://example.com/repo.git \
  2>&1 >/dev/null |
  egrep '(Send|Recv) header: (GET|HTTP|Auth)'

Before this patch, hitting a Basic-only server like
github.com results in:

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

And after:

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

The possible downsides are:

  - This only helps for a Basic-only server; for a server
    with multiple auth options, curl may still send a probe
    request to see which ones are available (IOW, there's no
    way to say "don't probe, I already know what the server
    will say").

  - The http_auth_methods variable is global, so this will
    apply to all further requests. That's acceptable for
    Git's usage of curl, though, which also treats the
    credentials as global. I.e., in any given program
    invocation we hit only one conceptual server (we may be
    redirected at the outset, but in that case that's whose
    auth_avail field we'd see).

Signed-off-by: Jeff King <redacted>
---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http.c b/http.c
index 90a1c0f11..a05609766 100644
--- a/http.c
+++ b/http.c

@@ -1347,6 +1347,8 @@ static int handle_curl_result(struct slot_results *results)
 		} else {
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+			if (results->auth_avail)
+				http_auth_methods &= results->auth_avail;
 #endif
 			return HTTP_REAUTH;
 		}

-- 
2.12.0.rc2.597.g959f68882

I don't know enough about how libcurl handles authentication to know whether 
these patches are a good idea, but I have a minor comment anyway.

-----Original Message-----
From: Jeff King [mailto:peff@peff.net]
+static int curl_empty_auth_enabled(void) {
+	if (curl_empty_auth < 0) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+		/*
+		 * In the automatic case, kick in the empty-auth
+		 * hack as long as we would potentially try some
+		 * method more exotic than "Basic".
+		 *
+		 * But only do so when this is _not_ our initial
+		 * request, as we would not then yet know what
+		 * methods are available.
+		 */

Eliminate double-negative:

"But only do this when this is our second or subsequent request, 
as by then we know what methods are available."

On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:

I don't know enough about how libcurl handles authentication to know whether 
these patches are a good idea, but I have a minor comment anyway.

As somebody who is using non-Basic auth, can you apply these patches and
show us the output of:

   GIT_TRACE_CURL=1 \
   git ls-remote https://your-server 2>&1 >/dev/null |
   egrep '(Send|Recv) header: (GET|HTTP|Auth)'

(without http.emptyauth turned on, obviously).

quoted

+		 * But only do so when this is _not_ our initial
+		 * request, as we would not then yet know what
+		 * methods are available.
+		 */

Eliminate double-negative:

"But only do this when this is our second or subsequent request, 
as by then we know what methods are available."

Yeah, that is clearer.

-Peff

-----Original Message-----
From: Jeff King [mailto:peff@peff.net]
Sent: Wednesday, February 22, 2017 8:38 PM
To: David Turner <redacted>
Cc: Junio C Hamano <redacted>; git@vger.kernel.org;
sandals@crustytoothpaste.net; Johannes Schindelin
[off-list ref]; Eric Sunshine [off-list ref]
Subject: Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:

quoted

I don't know enough about how libcurl handles authentication to know
whether these patches are a good idea, but I have a minor comment

anyway.

As somebody who is using non-Basic auth, can you apply these patches and
show us the output of:

   GIT_TRACE_CURL=1 \
   git ls-remote https://your-server 2>&1 >/dev/null |
   egrep '(Send|Recv) header: (GET|HTTP|Auth)'

(without http.emptyauth turned on, obviously).

The results appear to be identical with and without
the patch.  With http.emptyauth turned off,
16:27:28.208924 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
16:27:28.212872 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required
Username for 'http://git': [I just pressed enter]
Password for 'http://git': [ditto]
16:27:29.928872 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
16:27:29.929787 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required

(if someone else wants to replicate this, delete >/dev/null bit 
from Jeff's shell snippet)

On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:

quoted

As somebody who is using non-Basic auth, can you apply these patches and
show us the output of:

   GIT_TRACE_CURL=1 \
   git ls-remote https://your-server 2>&1 >/dev/null |
   egrep '(Send|Recv) header: (GET|HTTP|Auth)'

(without http.emptyauth turned on, obviously).

The results appear to be identical with and without
the patch.  With http.emptyauth turned off,
16:27:28.208924 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
16:27:28.212872 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required
Username for 'http://git': [I just pressed enter]
Password for 'http://git': [ditto]
16:27:29.928872 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
16:27:29.929787 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required

Just to be sure: did you remove http.emptyauth config completely from
your config files, or did you turn it to "false"? Because the new
behavior only kicks in when it isn't configured at all (probably we
should respect "auto" as a user-provided name).

(if someone else wants to replicate this, delete >/dev/null bit 
from Jeff's shell snippet)

Hrm, you shouldn't need to. The stderr redirection comes first, so it
should become the new stdout.

-Peff

-----Original Message-----
From: Jeff King [mailto:peff@peff.net]
Sent: Thursday, February 23, 2017 2:44 PM
To: David Turner <redacted>
Cc: Junio C Hamano <redacted>; git@vger.kernel.org;
sandals@crustytoothpaste.net; Johannes Schindelin
[off-list ref]; Eric Sunshine [off-list ref]
Subject: Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

On Thu, Feb 23, 2017 at 04:31:13PM +0000, David Turner wrote:

quoted

quoted

As somebody who is using non-Basic auth, can you apply these patches
and show us the output of:

   GIT_TRACE_CURL=1 \
   git ls-remote https://your-server 2>&1 >/dev/null |
   egrep '(Send|Recv) header: (GET|HTTP|Auth)'

(without http.emptyauth turned on, obviously).

The results appear to be identical with and without the patch.  With
http.emptyauth turned off,
16:27:28.208924 http.c:524              => Send header: GET

/info/refs?service=git-upload-pack HTTP/1.1

quoted

16:27:28.212872 http.c:524              <= Recv header: HTTP/1.1 401

Authorization Required

quoted

Username for 'http://git': [I just pressed enter] Password for
'http://git': [ditto]
16:27:29.928872 http.c:524              => Send header: GET

/info/refs?service=git-upload-pack HTTP/1.1

quoted

16:27:29.929787 http.c:524              <= Recv header: HTTP/1.1 401

Authorization Required

Just to be sure: did you remove http.emptyauth config completely from your
config files, or did you turn it to "false"? Because the new behavior only kicks
in when it isn't configured at all (probably we should respect "auto" as a user-
provided name).

I turned it to false. With it completely removed, I get this, both times:

20:03:49.896797 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.900776 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required
20:03:49.900929 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.904754 http.c:524              <= Recv header: HTTP/1.1 401 Authorization Required
20:03:49.906649 http.c:524              => Send header: GET /info/refs?service=git-upload-pack HTTP/1.1
20:03:49.906654 http.c:524              => Send header: Authorization: Negotiate <redacted>
20:03:49.956753 http.c:524              <= Recv header: HTTP/1.1 200 OK - $gitservername

quoted

(if someone else wants to replicate this, delete >/dev/null bit from
Jeff's shell snippet)

Hrm, you shouldn't need to. The stderr redirection comes first, so it should
become the new stdout.

Weird.  It didn't appear work earlier, but I must have screwed something up.
And I learned something about shell redirection.

Hi,

On Wed, 22 Feb 2017, Jeff King wrote:

[two beautiful patches]

I applied them and verified that the reported issue is fixed. Thank you!

Hopefully you do not mind that I cherry-picked them in preparation for
Git for Windows v2.12.0?

I added a small fixup (https://github.com/dscho/git/commit/44ae0bcae5):

-- snip --
Subject: [PATCH] fixup! http: add an "auto" mode for http.emptyauth

Note: we keep a "black list" of authentication methods for which we do
not want to enable http.emptyAuth automatically. A white list would be
nicer, but less robust, as we want to support linking to several cURL
versions and the list of authentication methods (as well as their names)
changed over time.

[jes: actually added the "auto" handling, excluded Digest, too]

This fixes https://github.com/git-for-windows/git/issues/1034

Signed-off-by: Johannes Schindelin <redacted>
---
 http.c | 55 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 22 deletions(-)

diff --git a/http.c b/http.c
index f8eb0f23d6c..fb94c444c80 100644
--- a/http.c
+++ b/http.c

@@ -334,7 +334,10 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&user_agent, var, value);
 
 	if (!strcmp("http.emptyauth", var)) {
-		curl_empty_auth = git_config_bool(var, value);
+		if (value && !strcmp("auto", value))
+			curl_empty_auth = -1;
+		else
+			curl_empty_auth = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -385,29 +388,37 @@ static int http_options(const char *var, const char *value, void *cb)
 
 static int curl_empty_auth_enabled(void)
 {
-	if (curl_empty_auth < 0) {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-		/*
-		 * In the automatic case, kick in the empty-auth
-		 * hack as long as we would potentially try some
-		 * method more exotic than "Basic".
-		 *
-		 * But only do so when this is _not_ our initial
-		 * request, as we would not then yet know what
-		 * methods are available.
-		 */
-		return http_auth_methods_restricted &&
-		       http_auth_methods != CURLAUTH_BASIC;
+	if (curl_empty_auth >= 0)
+		return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+	/*
+	 * Our libcurl is too old to do AUTH_ANY in the first place;
+	 * just default to turning the feature off.
+	 */
 #else
-		/*
-		 * Our libcurl is too old to do AUTH_ANY in the first place;
-		 * just default to turning the feature off.
-		 */
-		return 0;
+	/*
+	 * In the automatic case, kick in the empty-auth
+	 * hack as long as we would potentially try some
+	 * method more exotic than "Basic".
+	 *
+	 * But only do this when this is our second or
+	 * subsequent * request, as by then we know what
+	 * methods are available.
+	 */
+	if (http_auth_methods_restricted)
+		switch (http_auth_methods) {
+		case CURLAUTH_BASIC:
+		case CURLAUTH_DIGEST:
+#ifdef CURLAUTH_DIGEST_IE
+		case CURLAUTH_DIGEST_IE:
 #endif
-	}
-
-	return curl_empty_auth;
+			return 0;
+		default:
+			return 1;
+		}
+#endif
+	return 0;
 }
 
 static void init_curl_http_auth(CURL *result)
-- snap --

As you can see, I actually implemented the handling for
http.emptyauth=auto, and I was more comfortable with handling the "easy"
cases first in the curl_empty_auth_enabled function.

I also took Dave's suggestion:

On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:

quoted

quoted

+		 * But only do so when this is _not_ our initial
+		 * request, as we would not then yet know what
+		 * methods are available.
+		 */

Eliminate double-negative:

"But only do this when this is our second or subsequent request, 
as by then we know what methods are available."

Yeah, that is clearer.

Thank you all!

Now, how to get this into upstream Git, too? Jeff, do you want to submit a
v2? In that case, would you please consider the fixup! I mentioned above?
Otherwise I'd be happy to take it from here.

Ciao,
Dscho

This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.

However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.

Instead, let's introduce an automatic mode, that works like
this:

  1. We won't try to send the bogus credential on the first
     request. We'll wait to get an HTTP 401, as usual.

  2. After seeing an HTTP 401, the empty-auth hack will kick
     in only when we know there is an auth method available
     that might make use of it (i.e., something besides
     "Basic" or "Digest").

That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.

This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).

The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.

Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Jeff King <redacted>
---
And here's the full patch. It is meant to go on top of the
already-queued 1/2, though I suspect it could apply separately.

Test reports welcome from people who actually have NTLM or Kerberos
servers. The changes from the previous are fairly minimal, but this kind
of bit-mangling is exactly the kind of thing where I tend to
accidentally invert the logic. ;)

 http.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index a05609766..dd637d031 100644
--- a/http.c
+++ b/http.c

@@ -109,7 +109,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 

@@ -125,6 +125,14 @@ static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+	CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+	| CURLAUTH_DIGEST_IE
+#endif
+	| CURLAUTH_DIGEST;
 #endif
 
 static struct curl_slist *pragma_header;

@@ -333,7 +341,10 @@ static int http_options(const char *var, const char *value, void *cb)
 		return git_config_string(&user_agent, var, value);
 
 	if (!strcmp("http.emptyauth", var)) {
-		curl_empty_auth = git_config_bool(var, value);
+		if (value && !strcmp("auto", value))
+			curl_empty_auth = -1;
+		else
+			curl_empty_auth = git_config_bool(var, value);
 		return 0;
 	}
 

@@ -382,10 +393,37 @@ static int http_options(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+static int curl_empty_auth_enabled(void)
+{
+	if (curl_empty_auth >= 0)
+		return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+	/*
+	 * Our libcurl is too old to do AUTH_ANY in the first place;
+	 * just default to turning the feature off.
+	 */
+#else
+	/*
+	 * In the automatic case, kick in the empty-auth
+	 * hack as long as we would potentially try some
+	 * method more exotic than "Basic" or "Digest".
+	 *
+	 * But only do this when this is our second or
+	 * subsequent * request, as by then we know what
+	 * methods are available.
+	 */
+	if (http_auth_methods_restricted &&
+	    (http_auth_methods & ~empty_auth_useless))
+		return 1;
+#endif
+	return 0;
+}
+
 static void init_curl_http_auth(CURL *result)
 {
 	if (!http_auth.username || !*http_auth.username) {
-		if (curl_empty_auth)
+		if (curl_empty_auth_enabled())
 			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
 		return;
 	}

@@ -1079,7 +1117,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-	if (http_auth.password || curl_empty_auth)
+	if (http_auth.password || curl_empty_auth_enabled())
 		init_curl_http_auth(slot->curl);
 
 	return slot;

@@ -1347,8 +1385,10 @@ static int handle_curl_result(struct slot_results *results)
 		} else {
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-			if (results->auth_avail)
+			if (results->auth_avail) {
 				http_auth_methods &= results->auth_avail;
+				http_auth_methods_restricted = 1;
+			}
 #endif
 			return HTTP_REAUTH;
 		}

-- 
2.12.0.616.g5f622f3b1

On Sat, Feb 25, 2017 at 12:48:54PM +0100, Johannes Schindelin wrote:

Hi,

On Wed, 22 Feb 2017, Jeff King wrote:

quoted

[two beautiful patches]

I applied them and verified that the reported issue is fixed. Thank you!

Hopefully you do not mind that I cherry-picked them in preparation for
Git for Windows v2.12.0?

No, I don't mind. I'm happy that more people with a non-Basic setup are
verifying that they work. :)

Of the changes:

Obviously good, I should have included this in the original.

+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+	/*
+	 * Our libcurl is too old to do AUTH_ANY in the first place;
+	 * just default to turning the feature off.
+	 */
 #else
-		/*
-		 * Our libcurl is too old to do AUTH_ANY in the first place;
-		 * just default to turning the feature off.
-		 */

The ifdef reordering here is good.

+	/*
+	 * In the automatic case, kick in the empty-auth
+	 * hack as long as we would potentially try some
+	 * method more exotic than "Basic".
+	 *
+	 * But only do this when this is our second or
+	 * subsequent * request, as by then we know what
+	 * methods are available.
+	 */
+	if (http_auth_methods_restricted)
+		switch (http_auth_methods) {
+		case CURLAUTH_BASIC:
+		case CURLAUTH_DIGEST:
+#ifdef CURLAUTH_DIGEST_IE
+		case CURLAUTH_DIGEST_IE:
 #endif
[...]
+			return 0;
+		default:
+			return 1;
+		}

This is an improvement over my basic-only, but I think you actually want
to bitmask here. A server which advertises only BASIC|DIGEST should not
do empty-auth, but wouldn't match your switch statement.

Patch below.

Now, how to get this into upstream Git, too? Jeff, do you want to submit a
v2? In that case, would you please consider the fixup! I mentioned above?
Otherwise I'd be happy to take it from here.

I don't mind doing a v2. I'm unsure of whether we want to default to
"auto" or not upstream. It seems from your releases that you think it is
safe enough to do in Windows. And I guess nobody outside of that is
really doing NTLM. So it's OK, I guess?

<shrug> I don't have enough information to make an intelligent opinion,
so I'm happy to defer.

I'll send my v2 in a minute. Here's the interdiff/fixup if you need to
apply it separately:

diff --git a/http.c b/http.c
index 523c43cf9..dd637d031 100644
--- a/http.c
+++ b/http.c

@@ -126,6 +126,13 @@ static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
 static int http_auth_methods_restricted;
+/* Modes for which empty_auth cannot actually help us. */
+static unsigned long empty_auth_useless =
+	CURLAUTH_BASIC
+#ifdef CURLAUTH_DIGEST_IE
+	| CURLAUTH_DIGEST_IE
+#endif
+	| CURLAUTH_DIGEST;
 #endif
 
 static struct curl_slist *pragma_header;

@@ -400,23 +407,15 @@ static int curl_empty_auth_enabled(void)
 	/*
 	 * In the automatic case, kick in the empty-auth
 	 * hack as long as we would potentially try some
-	 * method more exotic than "Basic".
+	 * method more exotic than "Basic" or "Digest".
 	 *
 	 * But only do this when this is our second or
 	 * subsequent * request, as by then we know what
 	 * methods are available.
 	 */
-	if (http_auth_methods_restricted)
-		switch (http_auth_methods) {
-		case CURLAUTH_BASIC:
-		case CURLAUTH_DIGEST:
-#ifdef CURLAUTH_DIGEST_IE
-		case CURLAUTH_DIGEST_IE:
-#endif
-			return 0;
-		default:
-			return 1;
-		}
+	if (http_auth_methods_restricted &&
+	    (http_auth_methods & ~empty_auth_useless))
+		return 1;
 #endif
 	return 0;
 }