git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: David Turner <dturner@twosigma.com>,
	git@vger.kernel.org, sandals@crustytoothpaste.net,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
Date: Wed, 22 Feb 2017 18:40:59 -0500	[thread overview]
Message-ID: <20170222234059.iajn2zuwzkzjxit2@sigill.intra.peff.net> (raw)
In-Reply-To: <20170222233333.dx5lknw4fpopu5hy@sigill.intra.peff.net>

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 <peff@peff.net>
---
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

  parent reply	other threads:[~2017-02-22 23:42 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-22 17:39 [PATCH] http(s): automatically try NTLM authentication first David Turner
2017-02-22 20:19 ` Junio C Hamano
2017-02-22 21:04   ` David Turner
2017-02-22 21:16     ` Junio C Hamano
2017-02-22 21:34       ` Jeff King
2017-02-23 17:08         ` Johannes Schindelin
2017-02-23 19:06           ` Junio C Hamano
2017-02-23 19:42           ` Jeff King
2017-02-23 20:37             ` Junio C Hamano
2017-02-23 20:48               ` Jeff King
2017-02-25 11:51                 ` Johannes Schindelin
2017-02-22 23:34     ` brian m. carlson
2017-02-22 23:42       ` Jeff King
2017-02-23  2:15         ` Junio C Hamano
2017-02-23 19:11         ` Junio C Hamano
2017-02-23 19:35           ` Jeff King
2017-02-23  1:03       ` David Turner
2017-02-23  4:19         ` brian m. carlson
2017-02-23  9:13         ` Mantas Mikulėnas
2017-02-22 21:06   ` Jeff King
2017-02-22 21:25     ` Junio C Hamano
2017-02-22 21:35       ` Jeff King
2017-02-22 21:57         ` Junio C Hamano
2017-02-22 21:58           ` Jeff King
2017-02-22 22:35             ` Junio C Hamano
2017-02-22 23:33               ` Jeff King
2017-02-22 23:34                 ` [PATCH 1/2] http: restrict auth methods to what the server advertises Jeff King
2017-02-22 23:40                 ` Jeff King [this message]
2017-02-23  1:16                   ` [PATCH 2/2] http: add an "auto" mode for http.emptyauth David Turner
2017-02-23  1:37                     ` Jeff King
2017-02-23 16:31                       ` David Turner
2017-02-23 19:44                         ` Jeff King
2017-02-23 20:05                           ` David Turner
2017-02-25 11:48                       ` Johannes Schindelin
2017-02-25 19:15                         ` Jeff King
2017-02-25 19:18                           ` [PATCH] " Jeff King
2017-02-27 18:35                             ` Junio C Hamano
2017-02-28 10:18                               ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170222234059.iajn2zuwzkzjxit2@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=dturner@twosigma.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=sandals@crustytoothpaste.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).