From e2905f4a848ece49c36c4303c7c88d461946032c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 25 Feb 2017 12:31:54 +0100 Subject: [PATCH 1/4] Revert "http(s): automatically try NTLM authentication first" Turning on http.emptyAuth globally turned out to cause a regression in case of some servers that support Basic authentication but do not respond with *yet* another 401 upon receiving the empty user/password: https://github.com/git-for-windows/git/issues/1034 Jeff King came up with a superior approach. Let's revert our change in preparation for including those patches. This reverts commit de4b3c5efa24385aba42e051b8fa10116f8928d4. Signed-off-by: Johannes Schindelin --- http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.c b/http.c index 53d33a43e7dd98..96171ebaf0a963 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 = 1; +static int curl_empty_auth; enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL; From 4574329e16ce7635c2b7be3109c0e81cdc5fe5e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Feb 2017 18:34:37 -0500 Subject: [PATCH 2/4] http: restrict auth methods to what the server advertises 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 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 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 Signed-off-by: Johannes Schindelin --- http.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/http.c b/http.c index 96171ebaf0a963..ca6985f58ba602 100644 --- a/http.c +++ b/http.c @@ -1340,6 +1340,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; } From 0f93447a1cece7d31359efc3fb59201b06a0f976 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Feb 2017 18:40:59 -0500 Subject: [PATCH 3/4] http: add an "auto" mode for http.emptyauth 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 Signed-off-by: Johannes Schindelin --- http.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/http.c b/http.c index ca6985f58ba602..f8eb0f23d6cc18 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; } @@ -1072,7 +1100,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; @@ -1340,8 +1368,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; } From 44ae0bcae5eed2a92be7b9434346cff979a49a53 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 25 Feb 2017 09:41:26 +0100 Subject: [PATCH 4/4] 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 --- http.c | 55 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/http.c b/http.c index f8eb0f23d6cc18..fb94c444c805ea 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)