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

[Johannes Schindelin]

Hi,

On Mon, 27 Feb 2017, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > 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.
> 
> IOW, people who care about an extra round-trip have this workaround,
> which is good.
> 
> This, along with the possible security implications, may want to be
> added to the documentation but that is outside the topic of this change,
> and I think we would want to see such an update come from those who
> actually use NTLM (or Kerberos, but they know they have minimum security
> implications).
> 
> > +#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
> 
> I'll drop the '*' that you left while line-wrapping ;-)
> 
> > +* methods are available.  + */
> 
> Thanks.  This looks good.

I replaced the previous version in Git for Windows' `master` branch with
the one in `pu`.

Thanks,
Johannes

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

[Junio C Hamano]

Jeff King  writes:

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

IOW, people who care about an extra round-trip have this workaround,
which is good.

This, along with the possible security implications, may want to be
added to the documentation but that is outside the topic of this
change, and I think we would want to see such an update come from
those who actually use NTLM (or Kerberos, but they know they have
minimum security implications).

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

I'll drop the '*' that you left while line-wrapping ;-)

> +  * methods are available.
> +  */

Thanks.  This looks good.

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

[Jeff King]

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 
Signed-off-by: Jeff King 
---
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(_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