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

From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:05

In addition to Junio's review comments...

On Wednesday, October 28, 2015, Knut Franke
[off-list ref] wrote:

quoted hunk ↗ jump to hunk

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

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.

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

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

quoted hunk ↗ jump to hunk

+               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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help