Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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 <johannes.schindelin@gmx.de>
---
 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:
> 
> > > +		 * 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