[PATCH 1/2] http: allow selection of proxy authentication method

[PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
with a 307 redirect to an error page instead of a 407 listing supported
authentication methods. Therefore, allow the authentication method to be set
using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
variables http.proxy-authmethod and remote.<name>.proxy-authmethod (in analogy
to http.proxy and remote.<name>.proxy).

The following values are supported:

* anyauth (default)
* basic
* digest
* negotiate
* ntlm

Signed-off-by: Knut Franke <k.franke@science-computing.de>
---
 Documentation/config.txt | 28 ++++++++++++++++++++++
 http.c                   | 61 ++++++++++++++++++++++++++++++++++++++++--------
 remote.c                 |  3 +++
 remote.h                 |  1 +
 4 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..9dff88d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1597,6 +1597,29 @@ http.proxy::
 	`curl(1)`).  This can be overridden on a per-remote basis; see
 	remote.<name>.proxy
 
+http.proxy-authmethod::
+	Set the method with which to authenticate against the HTTP proxy. This only
+    takes effect if the configured proxy URI contains a user name part (i.e. is
+    of the form 'user@host' or 'user@host:port'). This can be overridden on a
+    per-remote basis; see `remote.<name>.proxy-authmethod`. Both can be
+    overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
+    Possible values are:
++
+--
+* `anyauth` - Automatically pick a suitable authentication method. It is
+  assumed that the proxy answers an unauthenticated request with a 407
+  status code and one or more Proxy-authenticate headers with supported
+  authentication methods. This is the default.
+* `basic` - HTTP Basic authentication
+* `digest` - HTTP Digest authentication; this prevents the password from being
+  transmitted to the proxy in clear text
+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
+  of `curl(1)`)
+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
+--
++
+
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
@@ -2390,6 +2413,11 @@ remote.<name>.proxy::
 	the proxy to use for that remote.  Set to the empty string to
 	disable proxying for that remote.
 
+remote.<name>.proxy-authmethod::
+    For remotes that require curl (http, https and ftp), the method to use for
+    authenticating against the proxy in use (probably set in
+    `remote.<name>.proxy`). See `http.proxy-authmethod`.
+
 remote.<name>.fetch::
 	The default set of "refspec" for linkgit:git-fetch[1]. See
 	linkgit:git-fetch[1].
diff --git a/http.c b/http.c
index 7da76ed..f7366d0 100644
--- a/http.c
+++ b/http.c
@@ -63,6 +63,7 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static const char *http_proxy_authmethod = NULL;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -257,6 +258,9 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
+	if (!strcmp("http.proxy-authmethod", var))
+		return git_config_string(&http_proxy_authmethod, var, value);
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
@@ -305,6 +309,43 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+static void set_from_env(const char **var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val)
+		*var = val;
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+    set_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
+
+	if (http_proxy_authmethod) {
+		if (!strcmp(http_proxy_authmethod, "basic"))
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_BASIC);
+		else if (!strcmp(http_proxy_authmethod, "digest"))
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_DIGEST);
+		else if (!strcmp(http_proxy_authmethod, "negotiate"))
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_GSSNEGOTIATE);
+		else if (!strcmp(http_proxy_authmethod, "ntlm"))
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_NTLM);
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+		else if (!strcmp(http_proxy_authmethod, "anyauth"))
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+#endif
+		// CURLAUTH_DIGEST_IE has no corresponding command-line option in
+		// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
+		// here, too
+		else
+			error("Unknown proxy authentication method: %s",
+			      http_proxy_authmethod);
+	}
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	else
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+#endif
+}
+
 static int has_cert_password(void)
 {
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
@@ -379,6 +420,7 @@ static CURL *get_curl_handle(void)
 	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 #endif
 
+
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
 
@@ -466,22 +508,13 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	}
-#if LIBCURL_VERSION_NUM >= 0x070a07
-	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
+	init_curl_proxy_auth(result);
 
 	set_curl_keepalive(result);
 
 	return result;
 }
 
-static void set_from_env(const char **var, const char *envname)
-{
-	const char *val = getenv(envname);
-	if (val)
-		*var = val;
-}
-
 void http_init(struct remote *remote, const char *url, int proactive_auth)
 {
 	char *low_speed_limit;
@@ -509,6 +542,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote && remote->http_proxy)
 		curl_http_proxy = xstrdup(remote->http_proxy);
 
+	if (remote && remote->http_proxy_authmethod)
+		http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);
+
 	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
@@ -607,6 +643,11 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (http_proxy_authmethod) {
+		free((void *)http_proxy_authmethod);
+		http_proxy_authmethod = NULL;
+	}
+
 	if (cert_auth.password != NULL) {
 		memset(cert_auth.password, 0, strlen(cert_auth.password));
 		free(cert_auth.password);
diff --git a/remote.c b/remote.c
index 1101f82..5704f86 100644
--- a/remote.c
+++ b/remote.c
@@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 	} else if (!strcmp(subkey, ".proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
+	} else if (!strcmp(subkey, ".proxy-authmethod")) {
+		return git_config_string((const char **)&remote->http_proxy_authmethod,
+					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
diff --git a/remote.h b/remote.h
index 312b7ca..a221c5a 100644
--- a/remote.h
+++ b/remote.h
@@ -54,6 +54,7 @@ struct remote {
 	 * for curl remotes only
 	 */
 	char *http_proxy;
+	char *http_proxy_authmethod;
 };
 
 struct remote *remote_get(const char *name);
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

Currently, the only way to pass proxy credentials to curl is by including them
in the proxy URL. Usually, this means they will end up on disk unencrypted, one
way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
proxy authentication often uses a domain user, credentials can be security
sensitive; therefore, a safer way of passing credentials is desirable.

If the configured proxy contains a username but not a password, query the
credential API for one. Also, make sure we approve/reject proxy credentials
properly.

For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
environment variables, which would otherwise be evaluated as a fallback by curl.
Without this, we would have different semantics for git configuration and
environment variables.

Signed-off-by: Knut Franke <k.franke@science-computing.de>
---
 http.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 http.h |  1 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index f7366d0..6e20017 100644
--- a/http.c
+++ b/http.c
@@ -64,6 +64,7 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *http_proxy_authmethod = NULL;
+struct credential http_proxy_auth = CREDENTIAL_INIT;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -161,6 +162,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 #else
 		slot->results->auth_avail = 0;
 #endif
+
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+			&slot->results->http_connectcode);
 	}
 
 	/* Run callback if appropriate */
@@ -318,6 +322,25 @@ static void set_from_env(const char **var, const char *envname)
 
 static void init_curl_proxy_auth(CURL *result)
 {
+	if (http_proxy_auth.username) {
+		if (!http_proxy_auth.password) {
+			credential_fill(&http_proxy_auth);
+		}
+#if LIBCURL_VERSION_NUM >= 0x071301
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+			http_proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+			http_proxy_auth.password);
+#else
+		struct strbuf up = STRBUF_INIT;
+		strbuf_reset(&up);
+		strbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);
+		strbuf_addch(&up, ':');
+		strbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);
+#endif
+	}
+
     set_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
 
 	if (http_proxy_authmethod) {
@@ -505,8 +528,36 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif
 
+	/*
+	 * curl also examines these variables as a fallback; but we need to query
+	 * them here in order to decide whether to prompt for missing password (cf.
+	 * init_curl_proxy_auth()).
+	 */
+	if (!curl_http_proxy) {
+		if (!strcmp(http_auth.protocol, "https")) {
+			set_from_env(&curl_http_proxy, "HTTPS_PROXY");
+			set_from_env(&curl_http_proxy, "https_proxy");
+		} else {
+			set_from_env(&curl_http_proxy, "http_proxy");
+		}
+		if (!curl_http_proxy) {
+			set_from_env(&curl_http_proxy, "ALL_PROXY");
+			set_from_env(&curl_http_proxy, "all_proxy");
+		}
+	}
+
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		if (strstr(curl_http_proxy, "://"))
+			credential_from_url(&http_proxy_auth, curl_http_proxy);
+		else {
+			struct strbuf url = STRBUF_INIT;
+			strbuf_reset(&url);
+			strbuf_addstr(&url, "http://");
+			strbuf_addstr(&url, curl_http_proxy);
+			credential_from_url(&http_proxy_auth, url.buf);
+		}
+
+		curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);
 	}
 	init_curl_proxy_auth(result);
 
@@ -643,6 +694,12 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (http_proxy_auth.password) {
+		memset(http_proxy_auth.password, 0, strlen(http_proxy_auth.password));
+		free(http_proxy_auth.password);
+		http_proxy_auth.password = NULL;
+	}
+
 	if (http_proxy_authmethod) {
 		free((void *)http_proxy_authmethod);
 		http_proxy_authmethod = NULL;
@@ -976,6 +1033,8 @@ static int handle_curl_result(struct slot_results *results)
 
 	if (results->curl_result == CURLE_OK) {
 		credential_approve(&http_auth);
+		if (http_proxy_auth.password)
+			credential_approve(&http_proxy_auth);
 		return HTTP_OK;
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
@@ -990,6 +1049,8 @@ static int handle_curl_result(struct slot_results *results)
 			return HTTP_REAUTH;
 		}
 	} else {
+		if (results->http_connectcode == 407)
+			credential_reject(&http_proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
diff --git a/http.h b/http.h
index 49afe39..7352a9e 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@ struct slot_results {
 	CURLcode curl_result;
 	long http_code;
 	long auth_avail;
+	long http_connectcode;
 };
 
 struct active_request_slot {
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxy-authmethod and remote.<name>.proxy-authmethod (in analogy
> to http.proxy and remote.<name>.proxy).

Please do not name configuration variable with dashes (or
underscores).  There still may be existing aberrations that are
deprecated and need to be removed over time that are misnamed, but
the core git variables are named without dashes and underscores.

Call yours "http.proxyAuthmethod" in the documentation, and use
strcmp("http.proxyauthmethod", var) in the options callback code.

> +static void init_curl_proxy_auth(CURL *result)
> +{
> +    set_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Strange indentation here...

> +
> +	if (http_proxy_authmethod) {
> +		if (!strcmp(http_proxy_authmethod, "basic"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_BASIC);
> +		else if (!strcmp(http_proxy_authmethod, "digest"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_DIGEST);
> +		else if (!strcmp(http_proxy_authmethod, "negotiate"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_GSSNEGOTIATE);
> +		else if (!strcmp(http_proxy_authmethod, "ntlm"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_NTLM);
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +		else if (!strcmp(http_proxy_authmethod, "anyauth"))
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +		// CURLAUTH_DIGEST_IE has no corresponding command-line option in
> +		// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> +		// here, too
> +		else
> +			error("Unknown proxy authentication method: %s",
> +			      http_proxy_authmethod);

Along the same line as how we do sslversions[] instead of a long
if/else if/ chain is preferrable.

Thansk.

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

On 2015-10-26 13:33, Junio C Hamano wrote:
> Call yours "http.proxyAuthmethod" in the documentation, and use
> strcmp("http.proxyauthmethod", var) in the options callback code.
[...]
> Strange indentation here...
[...]
> Along the same line as how we do sslversions[] instead of a long
> if/else if/ chain is preferrable.

Thanks for the feedback, I'll fix these in the next iteration.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH v2] http proxy authentication improvements

[Knut Franke]

Fixes in the second iteration:

* rename http.proxy-authmethod to http.proxyAuthmethod for consistency with
  other core git variables

* issue warning() instead of error() for unsupported authentication method, for
  consistency with http.sslVersion

* fix some code formatting / style issues
    
* fix memory management bug
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
with a 307 redirect to an error page instead of a 407 listing supported
authentication methods. Therefore, allow the authentication method to be set
using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
to http.proxy and remote.<name>.proxy).

The following values are supported:

* anyauth (default)
* basic
* digest
* negotiate
* ntlm

Signed-off-by: Knut Franke <k.franke@science-computing.de>
---
 Documentation/config.txt | 28 ++++++++++++++++++++++
 http.c                   | 62 +++++++++++++++++++++++++++++++++++++++++++++---
 remote.c                 |  3 +++
 remote.h                 |  1 +
 4 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..f2644d1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1597,6 +1597,29 @@ http.proxy::
 	`curl(1)`).  This can be overridden on a per-remote basis; see
 	remote.<name>.proxy
 
+http.proxyAuthmethod::
+	Set the method with which to authenticate against the HTTP proxy. This only
+    takes effect if the configured proxy URI contains a user name part (i.e. is
+    of the form 'user@host' or 'user@host:port'). This can be overridden on a
+    per-remote basis; see `remote.<name>.proxyAuthmethod`. Both can be
+    overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
+    Possible values are:
++
+--
+* `anyauth` - Automatically pick a suitable authentication method. It is
+  assumed that the proxy answers an unauthenticated request with a 407
+  status code and one or more Proxy-authenticate headers with supported
+  authentication methods. This is the default.
+* `basic` - HTTP Basic authentication
+* `digest` - HTTP Digest authentication; this prevents the password from being
+  transmitted to the proxy in clear text
+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
+  of `curl(1)`)
+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
+--
++
+
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
@@ -2390,6 +2413,11 @@ remote.<name>.proxy::
 	the proxy to use for that remote.  Set to the empty string to
 	disable proxying for that remote.
 
+remote.<name>.proxyAuthmethod::
+    For remotes that require curl (http, https and ftp), the method to use for
+    authenticating against the proxy in use (probably set in
+    `remote.<name>.proxy`). See `http.proxyAuthmethod`.
+
 remote.<name>.fetch::
 	The default set of "refspec" for linkgit:git-fetch[1]. See
 	linkgit:git-fetch[1].
diff --git a/http.c b/http.c
index 7da76ed..4756bab 100644
--- a/http.c
+++ b/http.c
@@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static const char *http_proxy_authmethod = NULL;
+static struct {
+	const char *name;
+	long curlauth_param;
+} http_proxy_authmethods[] = {
+	{ "basic", CURLAUTH_BASIC },
+	{ "digest", CURLAUTH_DIGEST },
+	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
+	{ "ntlm", CURLAUTH_NTLM },
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	{ "anyauth", CURLAUTH_ANY },
+#endif
+	// CURLAUTH_DIGEST_IE has no corresponding command-line option in
+	// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
+	// here, too
+};
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -257,6 +273,9 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
+	if (!strcmp("http.proxyauthmethod", var))
+		return git_config_string(&http_proxy_authmethod, var, value);
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
@@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+static void copy_from_env(const char **var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val)
+		*var = xstrdup(val);
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
+
+	if (http_proxy_authmethod) {
+		int i;
+		for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
+			if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
+				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
+						http_proxy_authmethods[i].curlauth_param);
+				break;
+			}
+		}
+		if (i == ARRAY_SIZE(http_proxy_authmethods)) {
+			warning("unsupported proxy authentication method %s: using default",
+			      http_proxy_authmethod);
+		}
+	}
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	else
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+#endif
+}
+
 static int has_cert_password(void)
 {
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
@@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	}
-#if LIBCURL_VERSION_NUM >= 0x070a07
-	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
+	init_curl_proxy_auth(result);
 
 	set_curl_keepalive(result);
 
@@ -509,6 +557,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote && remote->http_proxy)
 		curl_http_proxy = xstrdup(remote->http_proxy);
 
+	if (remote && remote->http_proxy_authmethod)
+		http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);
+
 	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
@@ -607,6 +658,11 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (http_proxy_authmethod) {
+		free((void *)http_proxy_authmethod);
+		http_proxy_authmethod = NULL;
+	}
+
 	if (cert_auth.password != NULL) {
 		memset(cert_auth.password, 0, strlen(cert_auth.password));
 		free(cert_auth.password);
diff --git a/remote.c b/remote.c
index 1101f82..426c6d8 100644
--- a/remote.c
+++ b/remote.c
@@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 	} else if (!strcmp(subkey, ".proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
+	} else if (!strcmp(subkey, ".proxyAuthmethod")) {
+		return git_config_string((const char **)&remote->http_proxy_authmethod,
+					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
diff --git a/remote.h b/remote.h
index 312b7ca..a221c5a 100644
--- a/remote.h
+++ b/remote.h
@@ -54,6 +54,7 @@ struct remote {
 	 * for curl remotes only
 	 */
 	char *http_proxy;
+	char *http_proxy_authmethod;
 };
 
 struct remote *remote_get(const char *name);
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

Currently, the only way to pass proxy credentials to curl is by including them
in the proxy URL. Usually, this means they will end up on disk unencrypted, one
way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
proxy authentication often uses a domain user, credentials can be security
sensitive; therefore, a safer way of passing credentials is desirable.

If the configured proxy contains a username but not a password, query the
credential API for one. Also, make sure we approve/reject proxy credentials
properly.

For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
environment variables, which would otherwise be evaluated as a fallback by curl.
Without this, we would have different semantics for git configuration and
environment variables.

Signed-off-by: Knut Franke <k.franke@science-computing.de>
---
 http.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 http.h |  1 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 4756bab..11bebe1 100644
--- a/http.c
+++ b/http.c
@@ -79,6 +79,7 @@ static struct {
 	// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
 	// here, too
 };
+struct credential http_proxy_auth = CREDENTIAL_INIT;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 #else
 		slot->results->auth_avail = 0;
 #endif
+
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+			&slot->results->http_connectcode);
 	}
 
 	/* Run callback if appropriate */
@@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char *envname)
 
 static void init_curl_proxy_auth(CURL *result)
 {
+	if (http_proxy_auth.username) {
+		if (!http_proxy_auth.password) {
+			credential_fill(&http_proxy_auth);
+		}
+#if LIBCURL_VERSION_NUM >= 0x071301
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+			http_proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+			http_proxy_auth.password);
+#else
+		struct strbuf up = STRBUF_INIT;
+		strbuf_reset(&up);
+		strbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);
+		strbuf_addch(&up, ':');
+		strbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);
+#endif
+	}
+
 	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
 
 	if (http_proxy_authmethod) {
@@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif
 
+	/*
+	 * curl also examines these variables as a fallback; but we need to query
+	 * them here in order to decide whether to prompt for missing password (cf.
+	 * init_curl_proxy_auth()).
+	 */
+	if (!curl_http_proxy) {
+		if (!strcmp(http_auth.protocol, "https")) {
+			copy_from_env(&curl_http_proxy, "HTTPS_PROXY");
+			copy_from_env(&curl_http_proxy, "https_proxy");
+		} else {
+			copy_from_env(&curl_http_proxy, "http_proxy");
+		}
+		if (!curl_http_proxy) {
+			copy_from_env(&curl_http_proxy, "ALL_PROXY");
+			copy_from_env(&curl_http_proxy, "all_proxy");
+		}
+	}
+
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		if (strstr(curl_http_proxy, "://"))
+			credential_from_url(&http_proxy_auth, curl_http_proxy);
+		else {
+			struct strbuf url = STRBUF_INIT;
+			strbuf_reset(&url);
+			strbuf_addstr(&url, "http://");
+			strbuf_addstr(&url, curl_http_proxy);
+			credential_from_url(&http_proxy_auth, url.buf);
+		}
+
+		curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);
 	}
 	init_curl_proxy_auth(result);
 
@@ -658,6 +709,12 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (http_proxy_auth.password) {
+		memset(http_proxy_auth.password, 0, strlen(http_proxy_auth.password));
+		free(http_proxy_auth.password);
+		http_proxy_auth.password = NULL;
+	}
+
 	if (http_proxy_authmethod) {
 		free((void *)http_proxy_authmethod);
 		http_proxy_authmethod = NULL;
@@ -991,6 +1048,8 @@ static int handle_curl_result(struct slot_results *results)
 
 	if (results->curl_result == CURLE_OK) {
 		credential_approve(&http_auth);
+		if (http_proxy_auth.password)
+			credential_approve(&http_proxy_auth);
 		return HTTP_OK;
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
@@ -1005,6 +1064,8 @@ static int handle_curl_result(struct slot_results *results)
 			return HTTP_REAUTH;
 		}
 	} else {
+		if (results->http_connectcode == 407)
+			credential_reject(&http_proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
diff --git a/http.h b/http.h
index 49afe39..7352a9e 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@ struct slot_results {
 	CURLcode curl_result;
 	long http_code;
 	long auth_avail;
+	long http_connectcode;
 };
 
 struct active_request_slot {
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
> to http.proxy and remote.<name>.proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> ---

Thanks.

> +http.proxyAuthmethod::
> +	Set the method with which to authenticate against the HTTP proxy. This only
> +    takes effect if the configured proxy URI contains a user name part (i.e. is
> +    of the form 'user@host' or 'user@host:port'). This can be overridden on a
> +    per-remote basis; see `remote.<name>.proxyAuthmethod`. Both can be
> +    overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
> +    Possible values are:

I see inconsistent indentation here.  Indent with a tab, like you
did your first line, consistently up to this point, perhaps?

> @@ -2390,6 +2413,11 @@ remote.<name>.proxy::
>  	the proxy to use for that remote.  Set to the empty string to
>  	disable proxying for that remote.
>  
> +remote.<name>.proxyAuthmethod::
> +    For remotes that require curl (http, https and ftp), the method to use for
> +    authenticating against the proxy in use (probably set in
> +    `remote.<name>.proxy`). See `http.proxyAuthmethod`.
> +

Likewise (match the style of the surrounding paragraphs).

> diff --git a/http.c b/http.c
> index 7da76ed..4756bab 100644
> --- a/http.c
> +++ b/http.c
> @@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
> +static const char *http_proxy_authmethod = NULL;
> +static struct {
> +	const char *name;
> +	long curlauth_param;
> +} http_proxy_authmethods[] = {

Perhaps call this "proxy_authmethod[]"?  We won't be talking about
any other kinds of proxy authentication method in http.c file, and a
long name like this makes the line unnecessarily long in a nested
loop like you added to init_curl_proxy_auth().

> +	{ "basic", CURLAUTH_BASIC },
> +	{ "digest", CURLAUTH_DIGEST },
> +	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
> +	{ "ntlm", CURLAUTH_NTLM },
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	{ "anyauth", CURLAUTH_ANY },
> +#endif
> +	// CURLAUTH_DIGEST_IE has no corresponding command-line option in
> +	// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> +	// here, too
> +};

Please do not use // C++ comments.

> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +static void copy_from_env(const char **var, const char *envname)
> +{
> +	const char *val = getenv(envname);
> +	if (val)
> +		*var = xstrdup(val);
> +}
> +
> +static void init_curl_proxy_auth(CURL *result)
> +{
> +	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Unless this helper is used regularly from many other places, is use
makes it harder to follow the flow of the logic, as it does not
offer clear and obvious abstraction, especially with the name
"copy_from_env()".  I was forced to look at the implementation to
see what happens when the environment variable does not exist to
make sure the right thing happens (i.e. http_proxy_authmethod is
unchanged).

> +	if (http_proxy_authmethod) {
> +		int i;
> +		for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> +			if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +						http_proxy_authmethods[i].curlauth_param);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> +			warning("unsupported proxy authentication method %s: using default",
> +			      http_proxy_authmethod);
> +		}
> +	}
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	else
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}

This patch should take what 1c2dbf20 (http: support curl < 7.10.7,
2015-02-03) wanted to do into account.  Having the configuration
variable or the environment variable defined by itself, while
running a Git built with old cURL, shouldn't trigger any warning,
but the entire function should perhaps be ifdefed out or something?

Personally I find it a bit surprising that somebody still cares
about such an old version (7.10.7 is listed on 15 Aug 2003 in the
CHANGES file), but there apparently are happy users.  Let's not
knowingly break them.

> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  	}
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +	init_curl_proxy_auth(result);

Thanks.

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +static void copy_from_env(const char **var, const char *envname)
>> +{
>> +	const char *val = getenv(envname);
>> +	if (val)
>> +		*var = xstrdup(val);
>> +}
>> +
>> +static void init_curl_proxy_auth(CURL *result)
>> +{
>> +	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
>
> Unless this helper is used regularly from many other places, is use
> makes it harder to follow the flow of the logic, as it does not
> offer clear and obvious abstraction, especially with the name
> "copy_from_env()".  I was forced to look at the implementation to
> see what happens when the environment variable does not exist to
> make sure the right thing happens (i.e. http_proxy_authmethod is
> unchanged).

I see you use this liberally in 2/2, it is a handy helper to have,
and I do _not_ think it is a good idea to open-code this in 1/2 and
turn it into a helper in 2/2.  IOW, I am OK with this "one helper
with a single caller introduced and used in 1/2".  I primarily was
wishing that its name more clearly conveyed that it sets the
variable from the environment _only if_ the environment variable
exists, and otherwise it does not clobber.

The implementation of the helper seems to assume that the variable
must not be pointing at a free-able piece of memory when it is
called (that is why *var is assigned to without freeing the old
value).  That's another subtle thing the callers need to be aware of
(i.e. deserves at least a comment in front of the function, but as
always a good name that clearly conveys it would be more
preferrable, if we can find one).

Thanks.

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Eric Sunshine]

In addition to Junio's review comments...

On Wednesday, October 28, 2015, Knut Franke
<k.franke@science-computing.de> wrote:
> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
> to http.proxy and remote.<name>.proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..f2644d1 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1597,6 +1597,29 @@ http.proxy::
>         `curl(1)`).  This can be overridden on a per-remote basis; see
>         remote.<name>.proxy
>
> +http.proxyAuthmethod::

Should this be typed as "proxyAuthMethod"?

> +       Set the method with which to authenticate against the HTTP proxy. This only
> +    takes effect if the configured proxy URI contains a user name part (i.e. is
> +    of the form 'user@host' or 'user@host:port'). This can be overridden on a
> +    per-remote basis; see `remote.<name>.proxyAuthmethod`. Both can be
> +    overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
> +    Possible values are:
> ++
> +--
> +* `anyauth` - Automatically pick a suitable authentication method. It is
> +  assumed that the proxy answers an unauthenticated request with a 407
> +  status code and one or more Proxy-authenticate headers with supported
> +  authentication methods. This is the default.
> +* `basic` - HTTP Basic authentication
> +* `digest` - HTTP Digest authentication; this prevents the password from being
> +  transmitted to the proxy in clear text
> +* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
> +  of `curl(1)`)
> +* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
> +--

I think you can drop the unnecessary '--' lines here and above.

> ++
> +

No need for the extra unnecessary "+" line and empty line.

> +
>  http.cookieFile::
>         File containing previously stored cookie lines which should be used
>         in the Git http session, if they match the server. The file format
> diff --git a/http.c b/http.c
> index 7da76ed..4756bab 100644
> --- a/http.c
> +++ b/http.c
> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
> +static void init_curl_proxy_auth(CURL *result)
> +{
> +       copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
> +
> +       if (http_proxy_authmethod) {
> +               int i;
> +               for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> +                       if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> +                               curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +                                               http_proxy_authmethods[i].curlauth_param);
> +                               break;
> +                       }
> +               }
> +               if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> +                       warning("unsupported proxy authentication method %s: using default",
> +                             http_proxy_authmethod);

Does the user know what "default" means here? Does it mean
CURLAUTH_ANY? If so, do you need to invoke curl_easy_setopt(...,
CURLAUTH_ANY) as you do below when http_proxy is NULL?

> +               }
> +       }
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +       else
> +               curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}
> +
>  static int has_cert_password(void)
>  {
>         if (ssl_cert == NULL || ssl_cert_password_required != 1)
> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)
>         if (curl_http_proxy) {
>                 curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>         }
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -       curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +       init_curl_proxy_auth(result);
>
>         set_curl_keepalive(result);
>
> diff --git a/remote.c b/remote.c
> index 1101f82..426c6d8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)
>         } else if (!strcmp(subkey, ".proxy")) {
>                 return git_config_string((const char **)&remote->http_proxy,
>                                          key, value);
> +       } else if (!strcmp(subkey, ".proxyAuthmethod")) {

In documentation, write "proxyAuthMethod", but in code use
"proxyauthmethod" string literal.

> +               return git_config_string((const char **)&remote->http_proxy_authmethod,
> +                                        key, value);
>         } else if (!strcmp(subkey, ".vcs")) {
>                 return git_config_string(&remote->foreign_vcs, key, value);
>         }
> diff --git a/remote.h b/remote.h
> index 312b7ca..a221c5a 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -54,6 +54,7 @@ struct remote {
>          * for curl remotes only
>          */
>         char *http_proxy;
> +       char *http_proxy_authmethod;
>  };
>
>  struct remote *remote_get(const char *name);
> --
> 2.3.7

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Eric Sunshine]

On Wednesday, October 28, 2015, Knut Franke
<k.franke@science-computing.de> wrote:
> Currently, the only way to pass proxy credentials to curl is by including them
> in the proxy URL. Usually, this means they will end up on disk unencrypted, one
> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
> proxy authentication often uses a domain user, credentials can be security
> sensitive; therefore, a safer way of passing credentials is desirable.
>
> If the configured proxy contains a username but not a password, query the
> credential API for one. Also, make sure we approve/reject proxy credentials
> properly.
>
> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
> environment variables, which would otherwise be evaluated as a fallback by curl.
> Without this, we would have different semantics for git configuration and
> environment variables.
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> ---
> diff --git a/http.c b/http.c
> index 4756bab..11bebe1 100644
> --- a/http.c
> +++ b/http.c
> @@ -79,6 +79,7 @@ static struct {
>         // curl(1) and is not included in CURLAUTH_ANY, so we leave it out
>         // here, too
>  };
> +struct credential http_proxy_auth = CREDENTIAL_INIT;

s/^/static/

>  static const char *curl_cookie_file;
>  static int curl_save_cookies;
>  struct credential http_auth = CREDENTIAL_INIT;
> @@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot *slot)
>  #else
>                 slot->results->auth_avail = 0;
>  #endif
> +
> +               curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> +                       &slot->results->http_connectcode);
>         }
>
>         /* Run callback if appropriate */
> @@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char *envname)
>
>  static void init_curl_proxy_auth(CURL *result)
>  {
> +       if (http_proxy_auth.username) {
> +               if (!http_proxy_auth.password) {
> +                       credential_fill(&http_proxy_auth);
> +               }

Style: drop unnecessary braces

> +#if LIBCURL_VERSION_NUM >= 0x071301
> +               curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> +                       http_proxy_auth.username);
> +               curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> +                       http_proxy_auth.password);
> +#else
> +               struct strbuf up = STRBUF_INIT;

Minor: It took me a moment to figure out that "up" meant
user-password. I wonder if a simpler name such as 's' would suffice?

> +               strbuf_reset(&up);

Unnecessary strbuf_reset().

> +               strbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);
> +               strbuf_addch(&up, ':');
> +               strbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);
> +               curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);

Leaking 'up'. Insert strbuf_release(&up) here.

> +#endif
> +       }
> +
>         copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
>
>         if (http_proxy_authmethod) {
> @@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)
>                 curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
>  #endif
>
> +       /*
> +        * curl also examines these variables as a fallback; but we need to query
> +        * them here in order to decide whether to prompt for missing password (cf.
> +        * init_curl_proxy_auth()).
> +        */
> +       if (!curl_http_proxy) {
> +               if (!strcmp(http_auth.protocol, "https")) {
> +                       copy_from_env(&curl_http_proxy, "HTTPS_PROXY");
> +                       copy_from_env(&curl_http_proxy, "https_proxy");
> +               } else {
> +                       copy_from_env(&curl_http_proxy, "http_proxy");

To the casual reader, it's not obvious why you check upper- and
lowercase versions of the other environment variables but not this
one.

> +               }
> +               if (!curl_http_proxy) {
> +                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
> +                       copy_from_env(&curl_http_proxy, "all_proxy");
> +               }

If this sort of upper- and lowercase environment variable name
checking is indeed desirable, I wonder if it would make sense to fold
that functionality into the helper function.

> +       }
> +
>         if (curl_http_proxy) {
> -               curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +               if (strstr(curl_http_proxy, "://"))
> +                       credential_from_url(&http_proxy_auth, curl_http_proxy);
> +               else {
> +                       struct strbuf url = STRBUF_INIT;
> +                       strbuf_reset(&url);

Unnecessary strbuf_reset().

> +                       strbuf_addstr(&url, "http://");
> +                       strbuf_addstr(&url, curl_http_proxy);

strbuf_addf(&url, "http://%s", curl_http_proxy) might be more straightforward.

> +                       credential_from_url(&http_proxy_auth, url.buf);

Leaking 'url' here. Insert strbuf_release(&url).

> +               }
> +
> +               curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);
>         }
>         init_curl_proxy_auth(result);

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

Junio C Hamano wrote:
> > +	if (http_proxy_authmethod) {
> > +		int i;
> > +		for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> > +			if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> > +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> > +						http_proxy_authmethods[i].curlauth_param);
> > +				break;
> > +			}
> > +		}
> > +		if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> > +			warning("unsupported proxy authentication method %s: using default",
> > +			      http_proxy_authmethod);
> > +		}
> > +	}
> > +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> > +	else
> > +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> > +#endif
> > +}
> 
> This patch should take what 1c2dbf20 (http: support curl < 7.10.7,
> 2015-02-03) wanted to do into account.  Having the configuration
> variable or the environment variable defined by itself, while
> running a Git built with old cURL, shouldn't trigger any warning,
> but the entire function should perhaps be ifdefed out or something?

Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do
with LIBCURL_CAN_HANDLE_AUTH_ANY? If so, should this be a separate patch? With
the current (scattered) version dependencies, it took me a while to realize that
if the function is ifdefed out for LIBCURL_VERSION_NUM < 0x070a07, we don't need
to worry about default behavior in case LIBCURL_CAN_HANDLE_AUTH_ANY is not
defined. (on the other hand, looking at other curl-version-ifdefs, the define
for AUTH_ANY is the exception)

> >> +static void copy_from_env(const char **var, const char *envname)
> >> +{
> >> +  const char *val = getenv(envname);
> >> +  if (val)
> >> +      *var = xstrdup(val);
> >> +}
[...]
> I primarily was
> wishing that its name more clearly conveyed that it sets the
> variable from the environment _only if_ the environment variable
> exists, and otherwise it does not clobber.

How about env_override? Not perfect, but probably better. I don't think
squeezing in more information (maybe_env_override, override_from_env_var) would
help.

> The implementation of the helper seems to assume that the variable
> must not be pointing at a free-able piece of memory when it is
> called

In fact, if http.proxyAuthMethod (btw, I agree with Eric about capitalization)
is set, I do call it with *var pointing to free-able memory. Will fix this.

FWIW, set_from_env() has the same pre-condition, which doesn't seem to be
satisfied in all cases (namely when overwriting variables previously set by
git_config_string()); not to mention missing free()s in http_cleanup.


Otherwise, I'll make the suggested fixes. Thanks.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

On 2015-10-28 14:58, Eric Sunshine wrote:
> > +               }
> > +               if (!curl_http_proxy) {
> > +                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
> > +                       copy_from_env(&curl_http_proxy, "all_proxy");
> > +               }
> 
> If this sort of upper- and lowercase environment variable name
> checking is indeed desirable, I wonder if it would make sense to fold
> that functionality into the helper function.

It's just for consistency with libcurl here, not generally desirable; so I don't
think it makes sense to add it to the helper.

Otherwise, will fix. Thanks.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do
> with LIBCURL_CAN_HANDLE_AUTH_ANY?

Quite frankly, my first preference is to have a code that is clear
enough so that you do not need such an intermediate macro.  By
isolating implementation details that have to be version dependent
into a helper function whose purpose is well defined, the rest of
the code can be #ifdef free; it would become sufficiently clear to
switch based on curl version where implementation details matter.

By refraining from littering #ifdef all over the code, we do assign
to disable_gssnegotiate even though the value is not even used when
compiled for ancient version of libCURL, but the benefit of code
clarity outweighs such downside.  We may have to use #ifdef/#endif
in some places, but we should in general minimize their uses and
write code for the more up-to-date API.

For what I mean, see the attached patch outline to show how to get
rid of CAN_HANDLE_AUTH_ANY.

> How about env_override? Not perfect, but probably better.

Much better than anything I'd come up with myself (I am bad at
naming, even though I may sometimes be good at spotting a bad name).


 http.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/http.c b/http.c
index 7da76ed..d272b02 100644
--- a/http.c
+++ b/http.c
@@ -15,10 +15,6 @@ int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
-#if LIBCURL_VERSION_NUM >= 0x070a06
-#define LIBCURL_CAN_HANDLE_AUTH_ANY
-#endif
-
 static int min_curl_sessions = 1;
 static int curl_session_count;
 #ifdef USE_CURL_MULTI
@@ -79,9 +75,6 @@ static const char *user_agent;
 
 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;
-#endif
 
 static struct curl_slist *pragma_header;
 static struct curl_slist *no_pragma_header;
@@ -90,6 +83,19 @@ static struct active_request_slot *active_queue_head;
 
 static char *cached_accept_language;
 
+static int disable_gssnegotiate;
+
+static void set_httpauth_opt(CURL *curl)
+{
+#if LIBCURL_VERSION_NUM >= 0x070a06  /* Is CURLAUTH_ANY available? */
+	unsigned long auth_methods = CURLAUTH_ANY;
+
+	if (disable_gssnegotiate)
+		auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, auth_methods);
+#endif
+}
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -375,9 +381,7 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
 #endif
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-	curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
-#endif
+	set_httpauth_opt(result);
 
 	if (http_proactive_auth)
 		init_curl_http_auth(result);
@@ -681,9 +685,7 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-#endif
+	set_httpauth_opt(slot->curl);
 	if (http_auth.password)
 		init_curl_http_auth(slot->curl);
 
@@ -943,9 +945,7 @@ static int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
-			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
-#endif
+			disable_gssnegotiate = 1;
 			return HTTP_REAUTH;
 		}
 	} else {

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> On 2015-10-28 14:58, Eric Sunshine wrote:
>> > +               }
>> > +               if (!curl_http_proxy) {
>> > +                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
>> > +                       copy_from_env(&curl_http_proxy, "all_proxy");
>> > +               }
>> 
>> If this sort of upper- and lowercase environment variable name
>> checking is indeed desirable, I wonder if it would make sense to fold
>> that functionality into the helper function.
>
> It's just for consistency with libcurl here, not generally desirable; so I don't
> think it makes sense to add it to the helper.

I agree.  Unlike many environment variables, historically these
proxy environment variables were all lowercase only for quite a
while (found as early as in CERN libwww 2.15 March '94).

It appears that CURL did not know this and implemented only
uppercase variants, which was later corrected to take both
(http://sourceforge.net/p/curl/bugs/40/ shows a fix in Nov 2000).

So both unfortunately are used in user's environment and we need to
pay attention to both.  As lowercase version is the more kosher one,
looking at uppercase first and then overriding it with the lowercase
one is the right order, I think.

It's a mess, but it is limited to these proxy-ish variables and does
not deserve a helper to promote the pattern.

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Eric Sunshine]

On Fri, Oct 30, 2015 at 3:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Knut Franke <k.franke@science-computing.de> writes:
>> On 2015-10-28 14:58, Eric Sunshine wrote:
>>> > +               }
>>> > +               if (!curl_http_proxy) {
>>> > +                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
>>> > +                       copy_from_env(&curl_http_proxy, "all_proxy");
>>> > +               }
>>>
>>> If this sort of upper- and lowercase environment variable name
>>> checking is indeed desirable, I wonder if it would make sense to fold
>>> that functionality into the helper function.
>>
>> It's just for consistency with libcurl here, not generally desirable; so I don't
>> think it makes sense to add it to the helper.
>
> I agree.  Unlike many environment variables, historically these
> proxy environment variables were all lowercase only for quite a
> while (found as early as in CERN libwww 2.15 March '94).
>
> It appears that CURL did not know this and implemented only
> uppercase variants, which was later corrected to take both
> (http://sourceforge.net/p/curl/bugs/40/ shows a fix in Nov 2000).
>
> So both unfortunately are used in user's environment and we need to
> pay attention to both.  As lowercase version is the more kosher one,
> looking at uppercase first and then overriding it with the lowercase
> one is the right order, I think.
>
> It's a mess, but it is limited to these proxy-ish variables and does
> not deserve a helper to promote the pattern.

It would be great to have this explanation in the commit message or
in-code comment. Thanks.

[PATCH v3 0/2]

[Knut Franke]

Changes in the third iteration:

* don't break support for curl < 7.10.7

* fix some memory leaks

* explicitly set anyauth as fallback for unsupported proyx authmethod setting,
  and tell the user what we did

* clean up usage of curl version #ifdefs

* fix more code formatting / style / naming issues

* add in-code comment explaining the proxy variable uppercase/lowercase mess

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
with a 307 redirect to an error page instead of a 407 listing supported
authentication methods. Therefore, allow the authentication method to be set
using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
to http.proxy and remote.<name>.proxy).

The following values are supported:

* anyauth (default)
* basic
* digest
* negotiate
* ntlm

Signed-off-by: Knut Franke <k.franke@science-computing.de>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt | 26 +++++++++++++++++
 http.c                   | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
 remote.c                 |  3 ++
 remote.h                 |  1 +
 4 files changed, 99 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..6f29893 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1597,6 +1597,27 @@ http.proxy::
 	`curl(1)`).  This can be overridden on a per-remote basis; see
 	remote.<name>.proxy
 
+http.proxyAuthMethod::
+	Set the method with which to authenticate against the HTTP proxy. This only
+	takes effect if the configured proxy URI contains a user name part (i.e. is
+	of the form 'user@host' or 'user@host:port'). This can be overridden on a
+	per-remote basis; see `remote.<name>.proxyAuthMethod`. Both can be
+	overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
+	Possible values are:
++
+--
+* `anyauth` - Automatically pick a suitable authentication method. It is
+  assumed that the proxy answers an unauthenticated request with a 407
+  status code and one or more Proxy-authenticate headers with supported
+  authentication methods. This is the default.
+* `basic` - HTTP Basic authentication
+* `digest` - HTTP Digest authentication; this prevents the password from being
+  transmitted to the proxy in clear text
+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
+  of `curl(1)`)
+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
+--
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
@@ -2390,6 +2411,11 @@ remote.<name>.proxy::
 	the proxy to use for that remote.  Set to the empty string to
 	disable proxying for that remote.
 
+remote.<name>.proxyAuthMethod::
+	For remotes that require curl (http, https and ftp), the method to use for
+	authenticating against the proxy in use (probably set in
+	`remote.<name>.proxy`). See `http.proxyAuthMethod`.
+
 remote.<name>.fetch::
 	The default set of "refspec" for linkgit:git-fetch[1]. See
 	linkgit:git-fetch[1].
diff --git a/http.c b/http.c
index 7da76ed..1172819 100644
--- a/http.c
+++ b/http.c
@@ -63,6 +63,24 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static const char *http_proxy_authmethod = NULL;
+static struct {
+	const char *name;
+	long curlauth_param;
+} proxy_authmethods[] = {
+	{ "basic", CURLAUTH_BASIC },
+	{ "digest", CURLAUTH_DIGEST },
+	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
+	{ "ntlm", CURLAUTH_NTLM },
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	{ "anyauth", CURLAUTH_ANY },
+#endif
+	/*
+	 * CURLAUTH_DIGEST_IE has no corresponding command-line option in
+	 * curl(1) and is not included in CURLAUTH_ANY, so we leave it out
+	 * here, too
+	 */
+};
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -257,6 +275,9 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
+	if (!strcmp("http.proxyauthmethod", var))
+		return git_config_string(&http_proxy_authmethod, var, value);
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
@@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+/* assumes *var is either NULL or free-able */
+static void env_override(const char **var, const char *envname)
+{
+	const char *val = getenv(envname);
+	if (val) {
+		if (*var)
+			free((void*)*var);
+		*var = xstrdup(val);
+	}
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
+
+#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
+	if (http_proxy_authmethod) {
+		int i;
+		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
+			if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {
+				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
+						proxy_authmethods[i].curlauth_param);
+				break;
+			}
+		}
+		if (i == ARRAY_SIZE(proxy_authmethods)) {
+			warning("unsupported proxy authentication method %s: using anyauth",
+					http_proxy_authmethod);
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+		}
+	}
+	else
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+#endif
+}
+
 static int has_cert_password(void)
 {
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
@@ -466,9 +523,7 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	}
-#if LIBCURL_VERSION_NUM >= 0x070a07
-	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
+	init_curl_proxy_auth(result);
 
 	set_curl_keepalive(result);
 
@@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote && remote->http_proxy)
 		curl_http_proxy = xstrdup(remote->http_proxy);
 
+	if (remote && remote->http_proxy_authmethod) {
+		if (http_proxy_authmethod)
+			free((void*)http_proxy_authmethod);
+		http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);
+	}
+
 	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
@@ -607,6 +668,11 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (http_proxy_authmethod) {
+		free((void *)http_proxy_authmethod);
+		http_proxy_authmethod = NULL;
+	}
+
 	if (cert_auth.password != NULL) {
 		memset(cert_auth.password, 0, strlen(cert_auth.password));
 		free(cert_auth.password);
diff --git a/remote.c b/remote.c
index 10f1ffc..0e3bba8 100644
--- a/remote.c
+++ b/remote.c
@@ -428,6 +428,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 	} else if (!strcmp(subkey, ".proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
+	} else if (!strcmp(subkey, ".proxyauthmethod")) {
+		return git_config_string((const char **)&remote->http_proxy_authmethod,
+					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
diff --git a/remote.h b/remote.h
index 312b7ca..a221c5a 100644
--- a/remote.h
+++ b/remote.h
@@ -54,6 +54,7 @@ struct remote {
 	 * for curl remotes only
 	 */
 	char *http_proxy;
+	char *http_proxy_authmethod;
 };
 
 struct remote *remote_get(const char *name);
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

Currently, the only way to pass proxy credentials to curl is by including them
in the proxy URL. Usually, this means they will end up on disk unencrypted, one
way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
proxy authentication often uses a domain user, credentials can be security
sensitive; therefore, a safer way of passing credentials is desirable.

If the configured proxy contains a username but not a password, query the
credential API for one. Also, make sure we approve/reject proxy credentials
properly.

For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
environment variables, which would otherwise be evaluated as a fallback by curl.
Without this, we would have different semantics for git configuration and
environment variables.

Signed-off-by: Knut Franke <k.franke@science-computing.de>
Reviewed-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
---
 http.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 http.h |  1 +
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 1172819..5708c7a 100644
--- a/http.c
+++ b/http.c
@@ -62,7 +62,7 @@ static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
-static const char *curl_http_proxy;
+static const char *curl_http_proxy = NULL;
 static const char *http_proxy_authmethod = NULL;
 static struct {
 	const char *name;
@@ -81,6 +81,8 @@ static struct {
 	 * here, too
 	 */
 };
+static struct credential proxy_auth = CREDENTIAL_INIT;
+static const char *curl_proxyuserpwd = NULL;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -178,6 +180,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 #else
 		slot->results->auth_avail = 0;
 #endif
+
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+			&slot->results->http_connectcode);
 	}
 
 	/* Run callback if appropriate */
@@ -339,6 +344,24 @@ static void env_override(const char **var, const char *envname)
 
 static void init_curl_proxy_auth(CURL *result)
 {
+	if (proxy_auth.username) {
+		if (!proxy_auth.password)
+			credential_fill(&proxy_auth);
+#if LIBCURL_VERSION_NUM >= 0x071301
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+			proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+			proxy_auth.password);
+#else
+		struct strbuf s = STRBUF_INIT;
+		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
+		strbuf_addch(&s, ':');
+		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
+		curl_proxyuserpwd = strbuf_detach(&s, NULL);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
+#endif
+	}
+
 	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
 
 #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
@@ -520,8 +543,42 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif
 
+	/*
+	 * CURL also examines these variables as a fallback; but we need to query
+	 * them here in order to decide whether to prompt for missing password (cf.
+	 * init_curl_proxy_auth()).
+	 *
+	 * Unlike many other common environment variables, these are historically
+	 * lowercase only. It appears that CURL did not know this and implemented
+	 * only uppercase variants, which was later corrected to take both - with
+	 * the exception of http_proxy, which is lowercase only also in CURL. As
+	 * the lowercase versions are the historical quasi-standard, they take
+	 * precedence here, as in CURL.
+	 */
+	if (!curl_http_proxy) {
+		if (!strcmp(http_auth.protocol, "https")) {
+			env_override(&curl_http_proxy, "HTTPS_PROXY");
+			env_override(&curl_http_proxy, "https_proxy");
+		} else {
+			env_override(&curl_http_proxy, "http_proxy");
+		}
+		if (!curl_http_proxy) {
+			env_override(&curl_http_proxy, "ALL_PROXY");
+			env_override(&curl_http_proxy, "all_proxy");
+		}
+	}
+
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		if (strstr(curl_http_proxy, "://"))
+			credential_from_url(&proxy_auth, curl_http_proxy);
+		else {
+			struct strbuf url = STRBUF_INIT;
+			strbuf_addf(&url, "http://%s", curl_http_proxy);
+			credential_from_url(&proxy_auth, url.buf);
+			strbuf_release(&url);
+		}
+
+		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
 	}
 	init_curl_proxy_auth(result);
 
@@ -668,6 +725,17 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (proxy_auth.password) {
+		memset(proxy_auth.password, 0, strlen(proxy_auth.password));
+		free(proxy_auth.password);
+		proxy_auth.password = NULL;
+	}
+
+	if (curl_proxyuserpwd) {
+		free((void *)curl_proxyuserpwd);
+		curl_proxyuserpwd = NULL;
+	}
+
 	if (http_proxy_authmethod) {
 		free((void *)http_proxy_authmethod);
 		http_proxy_authmethod = NULL;
@@ -1001,6 +1069,8 @@ static int handle_curl_result(struct slot_results *results)
 
 	if (results->curl_result == CURLE_OK) {
 		credential_approve(&http_auth);
+		if (proxy_auth.password)
+			credential_approve(&proxy_auth);
 		return HTTP_OK;
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
@@ -1015,6 +1085,8 @@ static int handle_curl_result(struct slot_results *results)
 			return HTTP_REAUTH;
 		}
 	} else {
+		if (results->http_connectcode == 407)
+			credential_reject(&proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
diff --git a/http.h b/http.h
index 49afe39..7352a9e 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@ struct slot_results {
 	CURLcode curl_result;
 	long http_code;
 	long auth_avail;
+	long http_connectcode;
 };
 
 struct active_request_slot {
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
> to http.proxy and remote.<name>.proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>


> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Please add these only when you are doing the final submission,
sending the same version reviewed by these people after they said
the patch(es) look good.  To credit others for helping you to polish
your patch, Helped-by: would be more appropriate.

> @@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +/* assumes *var is either NULL or free-able */
> +static void env_override(const char **var, const char *envname)
> +{
> +	const char *val = getenv(envname);
> +	if (val) {
> +		if (*var)
> +			free((void*)*var);

Just
		free((void *)*var);

would be more idiomatic (freeing NULL is not a crime but a norm).
Also as you did elsewhere, have a space between void and the
asterisk.

> +static void init_curl_proxy_auth(CURL *result)
> +{
> +	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Shouldn't this also be part of the #if/#endif?

> +
> +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
> +	if (http_proxy_authmethod) {
> +		int i;
> +		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
> +			if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {
> +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +						proxy_authmethods[i].curlauth_param);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(proxy_authmethods)) {
> +			warning("unsupported proxy authentication method %s: using anyauth",
> +					http_proxy_authmethod);
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +		}
> +	}
> +	else
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}
> +
>  static int has_cert_password(void)
>  {
>  	if (ssl_cert == NULL || ssl_cert_password_required != 1)
> @@ -466,9 +523,7 @@ static CURL *get_curl_handle(void)
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  	}
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +	init_curl_proxy_auth(result);
>  
>  	set_curl_keepalive(result);
>  
> @@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  	if (remote && remote->http_proxy)
>  		curl_http_proxy = xstrdup(remote->http_proxy);
>  
> +	if (remote && remote->http_proxy_authmethod) {
> +		if (http_proxy_authmethod)
> +			free((void*)http_proxy_authmethod);

Just
		free((void *)http_proxy_authmethod);

without NULL-ness check.

But this makes me wonder if env_override() was a good abstraction.

That is, with this helper:

        /* existing value in *var must be freeable */
        static void var_override(const char **var, char *value)
        {
                if (value) {
                        free((void *)(*var));
                        var = xstrdup(value);
                }
        }

the beginning of the init_proxy_auth() would become:

        static void init_curl_proxy_auth(CURL *result)
        {
        #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
		...

and this code would be:

	if (remote)
		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);

which might be even cleaner.

> +		http_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);
> +	}
> +
>  	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
>  	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
>  
> @@ -607,6 +668,11 @@ void http_cleanup(void)
>  		curl_http_proxy = NULL;
>  	}
>  
> +	if (http_proxy_authmethod) {
> +		free((void *)http_proxy_authmethod);
> +		http_proxy_authmethod = NULL;
> +	}

No need for NULL-ness check here, either.

Other than the above nits, looks cleanly done.

Thanks.

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> Currently, the only way to pass proxy credentials to curl is by including them
> in the proxy URL. Usually, this means they will end up on disk unencrypted, one
> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
> proxy authentication often uses a domain user, credentials can be security
> sensitive; therefore, a safer way of passing credentials is desirable.
>
> If the configured proxy contains a username but not a password, query the
> credential API for one. Also, make sure we approve/reject proxy credentials
> properly.
>
> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
> environment variables, which would otherwise be evaluated as a fallback by curl.
> Without this, we would have different semantics for git configuration and
> environment variables.
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> Reviewed-by: Junio C Hamano <gitster@pobox.com>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

As 1/2, I never reviewed this version yet.

> ---
>  http.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  http.h |  1 +
>  2 files changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 1172819..5708c7a 100644
> --- a/http.c
> +++ b/http.c
> @@ -62,7 +62,7 @@ static const char *ssl_cainfo;
>  static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
> -static const char *curl_http_proxy;
> +static const char *curl_http_proxy = NULL;
>  static const char *http_proxy_authmethod = NULL;

We do not do unnecessary initialization of file-scope globals to 0
or NULL.  The existing definition of curl_http_proxy is correct;
the one for http_proxy_authmethod needs to be changed to match.

>  static void init_curl_proxy_auth(CURL *result)
>  {
> +	if (proxy_auth.username) {
> +		if (!proxy_auth.password)
> +			credential_fill(&proxy_auth);
> +#if LIBCURL_VERSION_NUM >= 0x071301
> +		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> +			proxy_auth.username);
> +		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> +			proxy_auth.password);
> +#else
> +		struct strbuf s = STRBUF_INIT;
> +		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
> +		strbuf_addch(&s, ':');
> +		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
> +		curl_proxyuserpwd = strbuf_detach(&s, NULL);
> +		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
> +#endif

I think #else clause of this thing would introduce decl-after-stmt
compilation error.

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

On 2015-11-02 14:46, Junio C Hamano wrote:
> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> 
> Please add these only when you are doing the final submission,
> sending the same version reviewed by these people after they said
> the patch(es) look good.  To credit others for helping you to polish
> your patch, Helped-by: would be more appropriate.

Sorry about that.

However, may I suggest that Documentation/SubmittingPatches could do with a
little rewording in this respect?

> Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and
> "Tested-by:" lines as necessary to credit people who helped your
> patch.

"Helped-by:" isn't even mentioned.

> > +static void init_curl_proxy_auth(CURL *result)
> > +{
> > +	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
> 
> Shouldn't this also be part of the #if/#endif?

The idea here was to have as little code as possible within the #if/#endif, as a
matter of principle. It may be a little construed in this case, but supposing
there's some subtle bug with env_override, or a future change introduces one,
having it occur only for certain CURL versions would tend to make it harder to
track down.

> and this code would be:
> 
> 	if (remote)
> 		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);

Good catch.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

On 2015-11-02 14:54, Junio C Hamano wrote:
> >  static void init_curl_proxy_auth(CURL *result)
> >  {
> > +	if (proxy_auth.username) {
> > +		if (!proxy_auth.password)
> > +			credential_fill(&proxy_auth);
> > +#if LIBCURL_VERSION_NUM >= 0x071301
> > +		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> > +			proxy_auth.username);
> > +		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> > +			proxy_auth.password);
> > +#else
> > +		struct strbuf s = STRBUF_INIT;
> > +		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
> > +		strbuf_addch(&s, ':');
> > +		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
> > +		curl_proxyuserpwd = strbuf_detach(&s, NULL);
> > +		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
> > +#endif
> 
> I think #else clause of this thing would introduce decl-after-stmt
> compilation error.

I've actually tested this with CURL 7.15.5 (0x070f05), and didn't get any
compilation error.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Eric Sunshine]

On Tue, Nov 3, 2015 at 4:31 AM, Knut Franke
<k.franke@science-computing.de> wrote:
> On 2015-11-02 14:54, Junio C Hamano wrote:
>> >  static void init_curl_proxy_auth(CURL *result)
>> >  {
>> > +   if (proxy_auth.username) {
>> > +           if (!proxy_auth.password)
>> > +                   credential_fill(&proxy_auth);
>> > +#if LIBCURL_VERSION_NUM >= 0x071301
>> > +           curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
>> > +                   proxy_auth.username);
>> > +           curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
>> > +                   proxy_auth.password);
>> > +#else
>> > +           struct strbuf s = STRBUF_INIT;
>> > +           strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
>> > +           strbuf_addch(&s, ':');
>> > +           strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
>> > +           curl_proxyuserpwd = strbuf_detach(&s, NULL);
>> > +           curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
>> > +#endif
>>
>> I think #else clause of this thing would introduce decl-after-stmt
>> compilation error.
>
> I've actually tested this with CURL 7.15.5 (0x070f05), and didn't get any
> compilation error.

Junio meant that, on this project, code still follows the rule that
all declarations in a block must come before statements in order to
appease older compilers. That is (good):

    {
        int foo; /* declaration */

        if (doodad) /* statement */
            snorg();
        glop(&foo);
    }

rather than (bad):

    {
        if (doodad) /* statement */
            snorg();

        int foo; /* declaration after statement */
        glop(&foo);
    }

The gcc option -Wdeclaration-after-statement can help.

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> On 2015-11-02 14:46, Junio C Hamano wrote:
>> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
>> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>> 
>> Please add these only when you are doing the final submission,
>> sending the same version reviewed by these people after they said
>> the patch(es) look good.  To credit others for helping you to polish
>> your patch, Helped-by: would be more appropriate.
>
> Sorry about that.
>
> However, may I suggest that Documentation/SubmittingPatches could do with a
> little rewording in this respect?
>
>> Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and
>> "Tested-by:" lines as necessary to credit people who helped your
>> patch.
>
> "Helped-by:" isn't even mentioned.

That is because it is not actively encouraged.

The only thing I care about is that people do not incorrectly use
Reviewed/Acked-by when reviewers did not say "this version looks
good"; that would mislead the maintainer to think "ah, if that
reviewer said this is good, whose judgment I can trust, then I do
not have to read it carefully myself."

[PATCH v4 0/2]

[Knut Franke]

Changes in the fourth iteration:

* update Documentation/technical/api-remote.txt to reflect the addition to
  struct remote

* update documentation of http.proxy in Documentation/config.txt to mention the
  possibility of having git fill in missing proxy password

* fix decl-after-stmt

* generalize env_override helper to var_override

* more coding style corrections
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
with a 307 redirect to an error page instead of a 407 listing supported
authentication methods. Therefore, allow the authentication method to be set
using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
to http.proxy and remote.<name>.proxy).

The following values are supported:

* anyauth (default)
* basic
* digest
* negotiate
* ntlm

Signed-off-by: Knut Franke <k.franke@science-computing.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt               | 26 ++++++++++++++
 Documentation/technical/api-remote.txt |  4 +++
 http.c                                 | 65 ++++++++++++++++++++++++++++++++--
 remote.c                               |  3 ++
 remote.h                               |  1 +
 5 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..4ad7f74 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1597,6 +1597,27 @@ http.proxy::
 	`curl(1)`).  This can be overridden on a per-remote basis; see
 	remote.<name>.proxy
 
+http.proxyAuthMethod::
+	Set the method with which to authenticate against the HTTP proxy. This
+	only takes effect if the configured proxy string contains a user name part
+	(i.e. is of the form 'user@host' or 'user@host:port'). This can be
+	overridden on a per-remote basis; see `remote.<name>.proxyAuthMethod`.
+	Both can be overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment
+	variable.  Possible values are:
++
+--
+* `anyauth` - Automatically pick a suitable authentication method. It is
+  assumed that the proxy answers an unauthenticated request with a 407
+  status code and one or more Proxy-authenticate headers with supported
+  authentication methods. This is the default.
+* `basic` - HTTP Basic authentication
+* `digest` - HTTP Digest authentication; this prevents the password from being
+  transmitted to the proxy in clear text
+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
+  of `curl(1)`)
+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
+--
+
 http.cookieFile::
 	File containing previously stored cookie lines which should be used
 	in the Git http session, if they match the server. The file format
@@ -2390,6 +2411,11 @@ remote.<name>.proxy::
 	the proxy to use for that remote.  Set to the empty string to
 	disable proxying for that remote.
 
+remote.<name>.proxyAuthMethod::
+	For remotes that require curl (http, https and ftp), the method to use for
+	authenticating against the proxy in use (probably set in
+	`remote.<name>.proxy`). See `http.proxyAuthMethod`.
+
 remote.<name>.fetch::
 	The default set of "refspec" for linkgit:git-fetch[1]. See
 	linkgit:git-fetch[1].
diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt
index 2cfdd22..f10941b 100644
--- a/Documentation/technical/api-remote.txt
+++ b/Documentation/technical/api-remote.txt
@@ -51,6 +51,10 @@ struct remote
 
 	The proxy to use for curl (http, https, ftp, etc.) URLs.
 
+`http_proxy_authmethod`::
+
+	The method used for authenticating against `http_proxy`.
+
 struct remotes can be found by name with remote_get(), and iterated
 through with for_each_remote(). remote_get(NULL) will return the
 default remote, given the current branch and configuration.
diff --git a/http.c b/http.c
index 7da76ed..a786802 100644
--- a/http.c
+++ b/http.c
@@ -63,6 +63,24 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static const char *http_proxy_authmethod;
+static struct {
+	const char *name;
+	long curlauth_param;
+} proxy_authmethods[] = {
+	{ "basic", CURLAUTH_BASIC },
+	{ "digest", CURLAUTH_DIGEST },
+	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
+	{ "ntlm", CURLAUTH_NTLM },
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	{ "anyauth", CURLAUTH_ANY },
+#endif
+	/*
+	 * CURLAUTH_DIGEST_IE has no corresponding command-line option in
+	 * curl(1) and is not included in CURLAUTH_ANY, so we leave it out
+	 * here, too
+	 */
+};
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -257,6 +275,9 @@ static int http_options(const char *var, const char *value, void *cb)
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
 
+	if (!strcmp("http.proxyauthmethod", var))
+		return git_config_string(&http_proxy_authmethod, var, value);
+
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
 	if (!strcmp("http.savecookies", var)) {
@@ -305,6 +326,40 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+/* assumes *var is free-able */
+static void var_override(const char **var, char *value)
+{
+	if (value) {
+		free((void*) *var);
+		*var = xstrdup(value);
+	}
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
+
+#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
+	if (http_proxy_authmethod) {
+		int i;
+		for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
+			if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {
+				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
+						proxy_authmethods[i].curlauth_param);
+				break;
+			}
+		}
+		if (i == ARRAY_SIZE(proxy_authmethods)) {
+			warning("unsupported proxy authentication method %s: using anyauth",
+					http_proxy_authmethod);
+			curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+		}
+	}
+	else
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+#endif
+}
+
 static int has_cert_password(void)
 {
 	if (ssl_cert == NULL || ssl_cert_password_required != 1)
@@ -466,9 +521,7 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	}
-#if LIBCURL_VERSION_NUM >= 0x070a07
-	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-#endif
+	init_curl_proxy_auth(result);
 
 	set_curl_keepalive(result);
 
@@ -509,6 +562,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote && remote->http_proxy)
 		curl_http_proxy = xstrdup(remote->http_proxy);
 
+	if (remote)
+		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
+
 	pragma_header = curl_slist_append(pragma_header, "Pragma: no-cache");
 	no_pragma_header = curl_slist_append(no_pragma_header, "Pragma:");
 
@@ -607,6 +663,9 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	free((void *) http_proxy_authmethod);
+	http_proxy_authmethod = NULL;
+
 	if (cert_auth.password != NULL) {
 		memset(cert_auth.password, 0, strlen(cert_auth.password));
 		free(cert_auth.password);
diff --git a/remote.c b/remote.c
index 10f1ffc..0e3bba8 100644
--- a/remote.c
+++ b/remote.c
@@ -428,6 +428,9 @@ static int handle_config(const char *key, const char *value, void *cb)
 	} else if (!strcmp(subkey, ".proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
+	} else if (!strcmp(subkey, ".proxyauthmethod")) {
+		return git_config_string((const char **)&remote->http_proxy_authmethod,
+					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
diff --git a/remote.h b/remote.h
index 312b7ca..a221c5a 100644
--- a/remote.h
+++ b/remote.h
@@ -54,6 +54,7 @@ struct remote {
 	 * for curl remotes only
 	 */
 	char *http_proxy;
+	char *http_proxy_authmethod;
 };
 
 struct remote *remote_get(const char *name);
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

[PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

Currently, the only way to pass proxy credentials to curl is by including them
in the proxy URL. Usually, this means they will end up on disk unencrypted, one
way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
proxy authentication often uses a domain user, credentials can be security
sensitive; therefore, a safer way of passing credentials is desirable.

If the configured proxy contains a username but not a password, query the
credential API for one. Also, make sure we approve/reject proxy credentials
properly.

For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
environment variables, which would otherwise be evaluated as a fallback by curl.
Without this, we would have different semantics for git configuration and
environment variables.

Signed-off-by: Knut Franke <k.franke@science-computing.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt | 10 +++++--
 http.c                   | 72 +++++++++++++++++++++++++++++++++++++++++++++++-
 http.h                   |  1 +
 3 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4ad7f74..320ce9a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1593,9 +1593,13 @@ help.htmlPath::
 
 http.proxy::
 	Override the HTTP proxy, normally configured using the 'http_proxy',
-	'https_proxy', and 'all_proxy' environment variables (see
-	`curl(1)`).  This can be overridden on a per-remote basis; see
-	remote.<name>.proxy
+	'https_proxy', and 'all_proxy' environment variables (see `curl(1)`). In
+	addition to the syntax understood by curl, it is possible to specify a
+	proxy string with a user name but no password, in which case git will
+	attempt to acquire one in the same way it does for other credentials. See
+	linkgit:gitcredentials[7] for more information. The syntax thus is
+	'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden
+	on a per-remote basis; see remote.<name>.proxy
 
 http.proxyAuthMethod::
 	Set the method with which to authenticate against the HTTP proxy. This
diff --git a/http.c b/http.c
index a786802..714a59a 100644
--- a/http.c
+++ b/http.c
@@ -81,6 +81,8 @@ static struct {
 	 * here, too
 	 */
 };
+static struct credential proxy_auth = CREDENTIAL_INIT;
+static const char *curl_proxyuserpwd;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -178,6 +180,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 #else
 		slot->results->auth_avail = 0;
 #endif
+
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+			&slot->results->http_connectcode);
 	}
 
 	/* Run callback if appropriate */
@@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)
 
 static void init_curl_proxy_auth(CURL *result)
 {
+	if (proxy_auth.username) {
+		struct strbuf s = STRBUF_INIT;
+		if (!proxy_auth.password)
+			credential_fill(&proxy_auth);
+#if LIBCURL_VERSION_NUM >= 0x071301
+		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+			proxy_auth.username);
+		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+			proxy_auth.password);
+#else
+		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
+		strbuf_addch(&s, ':');
+		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
+		curl_proxyuserpwd = strbuf_detach(&s, NULL);
+		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
+#endif
+	}
+
 	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
 
 #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
@@ -518,8 +541,42 @@ static CURL *get_curl_handle(void)
 		curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif
 
+	/*
+	 * CURL also examines these variables as a fallback; but we need to query
+	 * them here in order to decide whether to prompt for missing password (cf.
+	 * init_curl_proxy_auth()).
+	 *
+	 * Unlike many other common environment variables, these are historically
+	 * lowercase only. It appears that CURL did not know this and implemented
+	 * only uppercase variants, which was later corrected to take both - with
+	 * the exception of http_proxy, which is lowercase only also in CURL. As
+	 * the lowercase versions are the historical quasi-standard, they take
+	 * precedence here, as in CURL.
+	 */
+	if (!curl_http_proxy) {
+		if (!strcmp(http_auth.protocol, "https")) {
+			var_override(&curl_http_proxy, getenv("HTTPS_PROXY"));
+			var_override(&curl_http_proxy, getenv("https_proxy"));
+		} else {
+			var_override(&curl_http_proxy, getenv("http_proxy"));
+		}
+		if (!curl_http_proxy) {
+			var_override(&curl_http_proxy, getenv("ALL_PROXY"));
+			var_override(&curl_http_proxy, getenv("all_proxy"));
+		}
+	}
+
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		if (strstr(curl_http_proxy, "://"))
+			credential_from_url(&proxy_auth, curl_http_proxy);
+		else {
+			struct strbuf url = STRBUF_INIT;
+			strbuf_addf(&url, "http://%s", curl_http_proxy);
+			credential_from_url(&proxy_auth, url.buf);
+			strbuf_release(&url);
+		}
+
+		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
 	}
 	init_curl_proxy_auth(result);
 
@@ -663,6 +720,15 @@ void http_cleanup(void)
 		curl_http_proxy = NULL;
 	}
 
+	if (proxy_auth.password) {
+		memset(proxy_auth.password, 0, strlen(proxy_auth.password));
+		free(proxy_auth.password);
+		proxy_auth.password = NULL;
+	}
+
+	free((void *)curl_proxyuserpwd);
+	curl_proxyuserpwd = NULL;
+
 	free((void *) http_proxy_authmethod);
 	http_proxy_authmethod = NULL;
 
@@ -994,6 +1060,8 @@ static int handle_curl_result(struct slot_results *results)
 
 	if (results->curl_result == CURLE_OK) {
 		credential_approve(&http_auth);
+		if (proxy_auth.password)
+			credential_approve(&proxy_auth);
 		return HTTP_OK;
 	} else if (missing_target(results))
 		return HTTP_MISSING_TARGET;
@@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)
 			return HTTP_REAUTH;
 		}
 	} else {
+		if (results->http_connectcode == 407)
+			credential_reject(&proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x070c00
 		if (!curl_errorstr[0])
 			strlcpy(curl_errorstr,
diff --git a/http.h b/http.h
index 49afe39..7352a9e 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@ struct slot_results {
 	CURLcode curl_result;
 	long http_code;
 	long auth_avail;
+	long http_connectcode;
 };
 
 struct active_request_slot {
-- 
2.3.7

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Eric Sunshine]

On Wednesday, November 4, 2015, Knut Franke
<k.franke@science-computing.de> wrote:
> Currently, the only way to pass proxy credentials to curl is by including them
> in the proxy URL. Usually, this means they will end up on disk unencrypted, one
> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
> proxy authentication often uses a domain user, credentials can be security
> sensitive; therefore, a safer way of passing credentials is desirable.
>
> If the configured proxy contains a username but not a password, query the
> credential API for one. Also, make sure we approve/reject proxy credentials
> properly.
>
> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
> environment variables, which would otherwise be evaluated as a fallback by curl.
> Without this, we would have different semantics for git configuration and
> environment variables.
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1593,9 +1593,13 @@ help.htmlPath::
>
>  http.proxy::
>         Override the HTTP proxy, normally configured using the 'http_proxy',
> -       'https_proxy', and 'all_proxy' environment variables (see
> -       `curl(1)`).  This can be overridden on a per-remote basis; see
> -       remote.<name>.proxy
> +       'https_proxy', and 'all_proxy' environment variables (see `curl(1)`). In
> +       addition to the syntax understood by curl, it is possible to specify a
> +       proxy string with a user name but no password, in which case git will
> +       attempt to acquire one in the same way it does for other credentials. See
> +       linkgit:gitcredentials[7] for more information. The syntax thus is
> +       '[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden
> +       on a per-remote basis; see remote.<name>.proxy

s/$/./ maybe?

>  http.proxyAuthMethod::
>         Set the method with which to authenticate against the HTTP proxy. This
> diff --git a/http.c b/http.c
> @@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)
>
>  static void init_curl_proxy_auth(CURL *result)
>  {
> +       if (proxy_auth.username) {
> +               struct strbuf s = STRBUF_INIT;
> +               if (!proxy_auth.password)
> +                       credential_fill(&proxy_auth);
> +#if LIBCURL_VERSION_NUM >= 0x071301
> +               curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> +                       proxy_auth.username);
> +               curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> +                       proxy_auth.password);

The strbuf does not get released in this #if branch, but since no
content was added, it doesn't need to be released, thus nothing is
leaked. Good.

Does the compiler warn about unused variable 's' in this #if branch?

> +#else
> +               strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
> +               strbuf_addch(&s, ':');
> +               strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
> +               curl_proxyuserpwd = strbuf_detach(&s, NULL);
> +               curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);

And this branch detaches content from the strbuf, and that memory is
released later in http_cleanup(), so also nothing is leaked, thus all
is good.

> +#endif
> +       }
> +
>         var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
>
>  #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> diff --git a/http.c b/http.c
> index 7da76ed..a786802 100644
> --- a/http.c
> +++ b/http.c
> @@ -305,6 +326,40 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +/* assumes *var is free-able */

This is not just "assumes", but it is wrong for the callers to pass
unfreeable memory, so

	/* *var must be free-able */

> +static void var_override(const char **var, char *value)
> +{
> +	if (value) {
> +		free((void*) *var);

There may be a similar whitespace damage but I happened to notice
this one.

	free((void *)*var);

> +static void init_curl_proxy_auth(CURL *result)
> +{
> +	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));

If your libcurl does not understand CURLOPT_PROXYAUTH, do you need
to do this var_override()?  Shouldn't this be inside the #if..#endif
below?

> +
> +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */
> +	if (http_proxy_authmethod) {
> +...
> +	else
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}

Other than that, looks cleanly done.  Thanks.

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Junio C Hamano]

Knut Franke <k.franke@science-computing.de> writes:

> @@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)
>  
>  static void init_curl_proxy_auth(CURL *result)
>  {
> +	if (proxy_auth.username) {
> +		struct strbuf s = STRBUF_INIT;

Having this variable triggers compilation error with newer libcurl
version as it is only used in the #else clause X-<.

> +		if (!proxy_auth.password)
> +			credential_fill(&proxy_auth);
> +#if LIBCURL_VERSION_NUM >= 0x071301
> +		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> +			proxy_auth.username);
> +		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> +			proxy_auth.password);
> +#else
> +		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
> +		strbuf_addch(&s, ':');
> +		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
> +		curl_proxyuserpwd = strbuf_detach(&s, NULL);
> +		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
> +#endif
> +	}

It is probably easier to follow the flow of the logic of the primary
interface (i.e. init_curl_proxy_auth()) if you split this part into
its own helper function that deals with implementation detail, e.g.

 http.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index 1f269b0..fad2ba5 100644
--- a/http.c
+++ b/http.c
@@ -340,24 +340,30 @@ static void var_override(const char **var, char *value)
 	}
 }
 
-static void init_curl_proxy_auth(CURL *result)
+static void set_proxyauth_name_password(CURL *result)
 {
-	if (proxy_auth.username) {
-		struct strbuf s = STRBUF_INIT;
-		if (!proxy_auth.password)
-			credential_fill(&proxy_auth);
 #if LIBCURL_VERSION_NUM >= 0x071301
 		curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
 			proxy_auth.username);
 		curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
 			proxy_auth.password);
 #else
+		struct strbuf s = STRBUF_INIT;
+
 		strbuf_addstr_urlencode(&s, proxy_auth.username, 1);
 		strbuf_addch(&s, ':');
 		strbuf_addstr_urlencode(&s, proxy_auth.password, 1);
 		curl_proxyuserpwd = strbuf_detach(&s, NULL);
 		curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
 #endif
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+	if (proxy_auth.username) {
+		if (!proxy_auth.password)
+			credential_fill(&proxy_auth);
+		set_proxyauth_name_password(result);
 	}
 
 	var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Jeff King]

On Wed, Nov 04, 2015 at 10:13:25AM +0100, Knut Franke wrote:

> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
> environment variables, which would otherwise be evaluated as a fallback by curl.
> Without this, we would have different semantics for git configuration and
> environment variables.

I can't say I'm excited about this, but I don't think there's a better
way.

There was a series similar to yours in 2012, and it ran into the same
problems. There's sadly no good way to ask curl "what is the proxy you
ended up using?".

There was also some discussion with curl upstream of providing a new
authentication interface, where we would provide curl with
authentication callbacks, and it would trigger them if and when
credentials were needed. Somebody upstream was working on a patch, but I
don't think it ever got merged. :(

Here's a relevant bit from that old series (which doesn't seem threaded,
but you can search for the author if you want to see more):

  http://thread.gmane.org/gmane.comp.version-control.git/192246

I have a few comments/questions below.

> +
> +		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> +			&slot->results->http_connectcode);

It looks like you use this to see the remote side's HTTP 407 code.  In
the 2012 series, I think we simply looked for a 407 in the HTTP return
code (I assume that if we fail in the CONNECT, we can't get any other
HTTP code and so curl just returns the proxy code).

I don't have a proxy to test against, but would that work (it's entirely
possible the other series was simply wrong)?

If we do need CONNECTCODE, do we need to protect it with an #ifdef on
the curl version? The manpage says it came in 7.10.7, which was released
in 2003. That's probably old enough not to worry about.

> +	if (proxy_auth.password) {
> +		memset(proxy_auth.password, 0, strlen(proxy_auth.password));
> +		free(proxy_auth.password);

My understanding is that memset() like this is not sufficient for
zero-ing sensitive data, as they can be optimized out by the compiler. I
don't think there's a portable alternative, though, so it may be the
best we can do. OTOH, the rest of git does not worry about such zero-ing
anyway, so we could also simply omit it here.

> +	free((void *)curl_proxyuserpwd);

This cast is necessary because curl_proxyuserpwd is declared const. But
I do not see anywhere that it needs to be const (we detach a strbuf into
it). Can we simply change the declaration?

For that matter, it is not clear to me why this needs to be a global at
all. Once we hand the value to curl_easy_setopt, curl keeps its own
copy.

>  	free((void *) http_proxy_authmethod);

This one unfortunately does need to remain const, as it is used with
git_config_string (though given the number of void casts necessary in
patch 1, it may be less painful to simply cast it in the call to
git_config_string()).

> @@ -994,6 +1060,8 @@ static int handle_curl_result(struct slot_results *results)
>  
>  	if (results->curl_result == CURLE_OK) {
>  		credential_approve(&http_auth);
> +		if (proxy_auth.password)
> +			credential_approve(&proxy_auth);
>  		return HTTP_OK;

Approving on success. Makes sense. You can drop the conditional here;
credential_approve() is a noop if the password isn't set.

> @@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)
>  			return HTTP_REAUTH;
>  		}
>  	} else {
> +		if (results->http_connectcode == 407)
> +			credential_reject(&proxy_auth);

Rejecting on a 407 makes sense (though again, can we check
results->http_code?). But if we get a 407 and we _don't_ have a
password, shouldn't we then prompt for one, similar to what we do with a
401?

That will require some refactoring around http_request_reauth, though
(because now we might potentially retry twice: once to get past the
proxy auth, and once to get past the real site's auth).

You prompt unconditionally for the password earlier, but only if the
proxy URL contains a username. We used to do the same thing for regular
http, but people got annoyed that they had to specify half the
credential in the URL. Perhaps it would be less so with proxies (which
are changed a lot less), so I don't think making this work is an
absolute requirement.

-Peff

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

On 2015-11-05 03:24, Jeff King wrote:
> There was also some discussion with curl upstream of providing a new
> authentication interface, where we would provide curl with
> authentication callbacks, and it would trigger them if and when
> credentials were needed. Somebody upstream was working on a patch, but I
> don't think it ever got merged. :(

That would certainly be nice, also with respect to other credentials, such as
SSL key passphrase (presuming that'd be possible without modifying the SSL lib
as well).

> Here's a relevant bit from that old series (which doesn't seem threaded,
> but you can search for the author if you want to see more):
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/192246

My main takeaway from this, apart from the points you mention below, is that
it'd be good to have a test case, similar to t/lib-httpd.sh. Since none of the
existent proxy-related code has an automated test, I think this would be an
improvement on top of the other patches. I'd need to look into how easy/hard
this would be to implement.

> > +
> > +		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> > +			&slot->results->http_connectcode);
> 
> It looks like you use this to see the remote side's HTTP 407 code.  In
> the 2012 series, I think we simply looked for a 407 in the HTTP return
> code

I'm not sure why that worked for the author of the old series - possibly curl
semantics changed at some point. In my setup at least (with curl 7.15.5), after
a failed proxy authentication, CURLINFO_HTTP_CODE returns 0 while
CURLINFO_HTTP_CONNECTCODE returns the 407. This is also consistent with the curl
documentation for CURLINFO_RESPONSE_CODE (which has replaced CURLINFO_HTTP_CODE
in 7.10.7, though the compatibility #define is still there): "Note that a
proxy's CONNECT response should be read with CURLINFO_HTTP_CONNECTCODE and not
this."

> If we do need CONNECTCODE, do we need to protect it with an #ifdef on
> the curl version? The manpage says it came in 7.10.7, which was released
> in 2003. That's probably old enough not to worry about.

As Junio pointed out earlier, since some people still care about ancient curl
versions, we don't want to knowingly break compatibility. So yes, an #ifdef
would be in oder here.

> 
> > +	if (proxy_auth.password) {
> > +		memset(proxy_auth.password, 0, strlen(proxy_auth.password));
> > +		free(proxy_auth.password);
> 
> My understanding is that memset() like this is not sufficient for
> zero-ing sensitive data, as they can be optimized out by the compiler. I
> don't think there's a portable alternative, though, so it may be the
> best we can do. OTOH, the rest of git does not worry about such zero-ing
> anyway, so we could also simply omit it here.

For what it's worth, that's the same as we do for cert_auth (while, as far as I
can see, no attempt is made for http_auth). I tend to think it's better than
nothing. Maybe an in-code comment stating it's not reliable would be in order,
to prevent the passing reader from putting too much trust in it.

> > +	free((void *)curl_proxyuserpwd);
> 
> This cast is necessary because curl_proxyuserpwd is declared const. But
> I do not see anywhere that it needs to be const (we detach a strbuf into
> it). Can we simply change the declaration?

Right.

> For that matter, it is not clear to me why this needs to be a global at
> all. Once we hand the value to curl_easy_setopt, curl keeps its own
> copy.

That's true only for relatively recent curl versions; before 7.17.0, strings
were not copied.

> > @@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)
> >  			return HTTP_REAUTH;
> >  		}
> >  	} else {
> > +		if (results->http_connectcode == 407)
> > +			credential_reject(&proxy_auth);
> 
> Rejecting on a 407 makes sense (though again, can we check
> results->http_code?). But if we get a 407 and we _don't_ have a
> password, shouldn't we then prompt for one, similar to what we do with a
> 401?
> 
> That will require some refactoring around http_request_reauth, though
> (because now we might potentially retry twice: once to get past the
> proxy auth, and once to get past the real site's auth).

I think this would also require changes to post_rpc in remote-curl.c, which
apparently does something similar to http_request_reauth. Probably something
along the lines of adding a HTTP_PROXY_REAUTH return code, plus some refactoring
in order to prevent code duplication between the different code parts handling
(proxy) reauth. :-/

> You prompt unconditionally for the password earlier, but only if the
> proxy URL contains a username. We used to do the same thing for regular
> http, but people got annoyed that they had to specify half the
> credential in the URL. Perhaps it would be less so with proxies (which
> are changed a lot less), so I don't think making this work is an
> absolute requirement.

As far as I understand, the issue was around unconditionally prompting for the
password even if it was listed in ~/.netrc. As far as I can see, curl doesn't
read ~/.netrc for proxy credentials, so I don't think it would make a difference
here.


Thanks,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Jeff King]

On Thu, Nov 05, 2015 at 12:56:54PM +0100, Knut Franke wrote:

> My main takeaway from this, apart from the points you mention below, is that
> it'd be good to have a test case, similar to t/lib-httpd.sh. Since none of the
> existent proxy-related code has an automated test, I think this would be an
> improvement on top of the other patches. I'd need to look into how easy/hard
> this would be to implement.

Yeah, tests would be wonderful. I think the main challenge will be
configuring Apache as a proxy (and failing gracefully when mod_proxy is
not available).

If there's another proxy that is easy to configure for a one-shot test,
that would be fine, too. It's nice if it's something that's commonly
available, though, so more people can actually run the test.

> > It looks like you use this to see the remote side's HTTP 407 code.  In
> > the 2012 series, I think we simply looked for a 407 in the HTTP return
> > code
> 
> I'm not sure why that worked for the author of the old series - possibly curl
> semantics changed at some point.

It's not clear to me that the original _did_ work in all cases. So I'll
trust your experiments now much more than that old thread. :)

> > My understanding is that memset() like this is not sufficient for
> > zero-ing sensitive data, as they can be optimized out by the compiler. I
> > don't think there's a portable alternative, though, so it may be the
> > best we can do. OTOH, the rest of git does not worry about such zero-ing
> > anyway, so we could also simply omit it here.
> 
> For what it's worth, that's the same as we do for cert_auth (while, as far as I
> can see, no attempt is made for http_auth). I tend to think it's better than
> nothing. Maybe an in-code comment stating it's not reliable would be in order,
> to prevent the passing reader from putting too much trust in it.

Rather than just a comment, can we do something like:

  void clear_password(void *buf, size_t len)
  {
	/*
	 * TODO: This is known to be insufficient, but perhaps better
	 * than nothing, and at least portable. We should use a more
	 * secure variant on systems that provide it.
	 */
	memset(buf, 0, len);
  }

That will make it easier to find such sites and improve them later
(adding this function in a separate translation unit might actually be
enough to make it work, as the compiler cannot omit a call to the opaque
clear_password, and clear_password itself does not know the results will
not be used).

> > For that matter, it is not clear to me why this needs to be a global at
> > all. Once we hand the value to curl_easy_setopt, curl keeps its own
> > copy.
> 
> That's true only for relatively recent curl versions; before 7.17.0, strings
> were not copied.

Yeah, I remembered that, but I thought for some reason it was old enough
that we didn't need to worry about it. I have a feeling that there may
be other places where we do not handle it that well.

7.17.0 is from 2007. I wonder if it is time we bumped our minimum
required curl version. Supporting older installations is nice, but at
some point it is not really helping anybody, and that has to be
balanced by the increase in code complexity (and especially we are not
helping those people if there are subtle bugs that nobody else is
exercising).

That's a separate topic from your patch, though (though I would not mind
at all if you wanted to work on it :) ).

> > That will require some refactoring around http_request_reauth, though
> > (because now we might potentially retry twice: once to get past the
> > proxy auth, and once to get past the real site's auth).
> 
> I think this would also require changes to post_rpc in remote-curl.c, which
> apparently does something similar to http_request_reauth. Probably something
> along the lines of adding a HTTP_PROXY_REAUTH return code, plus some refactoring
> in order to prevent code duplication between the different code parts handling
> (proxy) reauth. :-/

Yeah, that would work. I think we could also just loop on HTTP_REAUTH.
The code in handle_curl_result that returns HTTP_REAUTH will only do so
if it looks like we could make progress by trying again.

> > You prompt unconditionally for the password earlier, but only if the
> > proxy URL contains a username. We used to do the same thing for regular
> > http, but people got annoyed that they had to specify half the
> > credential in the URL. Perhaps it would be less so with proxies (which
> > are changed a lot less), so I don't think making this work is an
> > absolute requirement.
> 
> As far as I understand, the issue was around unconditionally prompting for the
> password even if it was listed in ~/.netrc. As far as I can see, curl doesn't
> read ~/.netrc for proxy credentials, so I don't think it would make a difference
> here.

The .netrc thing came up recently-ish, but the HTTP prompting issues are
much older than that. Basically, does:

  git config http.proxy http://example.com:8080

work out of the box if example.com requires a username and password? I
think with your patch it doesn't. You need to use
"http://user@example.com:8080" to convince git to prompt at all.

-Peff