{"thread":{"id":"40647","subject":"[PATCH 2/2] http: use credential API to handle proxy authentication","startedAt":"2015-10-26T17:55:08Z","lastAt":"2015-11-05T17:30:22Z","messageCount":34,"participants":["Knut Franke","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272249","messageId":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":null,"subject":"[PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-26T17:55:08Z","receivedAt":"2015-10-26T17:55:08Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\nwith a 307 redirect to an error page instead of a 407 listing supported\nauthentication methods. Therefore, allow the authentication method to be set\nusing the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\nvariables http.proxy-authmethod and remote..proxy-authmethod (in analogy\nto http.proxy and remote..proxy).\n\nThe following values are supported:\n\n* anyauth (default)\n* basic\n* digest\n* negotiate\n* ntlm\n\nSigned-off-by: Knut Franke \n---\n Documentation/config.txt | 28 ++++++++++++++++++++++\n http.c | 61 ++++++++++++++++++++++++++++++++++++++++--------\n remote.c | 3 +++\n remote.h | 1 +\n 4 files changed, 83 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..9dff88d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1597,6 +1597,29 @@ http.proxy::\n \t`curl(1)`). This can be overridden on a per-remote basis; see\n \tremote..proxy\n \n+http.proxy-authmethod::\n+\tSet the method with which to authenticate against the HTTP proxy. This only\n+ takes effect if the configured proxy URI contains a user name part (i.e. is\n+ of the form 'user@host' or 'user@host:port'). This can be overridden on a\n+ per-remote basis; see `remote..proxy-authmethod`. Both can be\n+ overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.\n+ Possible values are:\n++\n+--\n+* `anyauth` - Automatically pick a suitable authentication method. It is\n+ assumed that the proxy answers an unauthenticated request with a 407\n+ status code and one or more Proxy-authenticate headers with supported\n+ authentication methods. This is the default.\n+* `basic` - HTTP Basic authentication\n+* `digest` - HTTP Digest authentication; this prevents the password from being\n+ transmitted to the proxy in clear text\n+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option\n+ of `curl(1)`)\n+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)\n+--\n++\n+\n+\n http.cookieFile::\n \tFile containing previously stored cookie lines which should be used\n \tin the Git http session, if they match the server. The file format\n@@ -2390,6 +2413,11 @@ remote..proxy::\n \tthe proxy to use for that remote. Set to the empty string to\n \tdisable proxying for that remote.\n \n+remote..proxy-authmethod::\n+ For remotes that require curl (http, https and ftp), the method to use for\n+ authenticating against the proxy in use (probably set in\n+ `remote..proxy`). See `http.proxy-authmethod`.\n+\n remote..fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\ndiff --git a/http.c b/http.c\nindex 7da76ed..f7366d0 100644\n--- a/http.c\n+++ b/http.c\n@@ -63,6 +63,7 @@ static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n+static const char *http_proxy_authmethod = NULL;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -257,6 +258,9 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.proxy\", var))\n \t\treturn git_config_string(&curl_http_proxy, var, value);\n \n+\tif (!strcmp(\"http.proxy-authmethod\", var))\n+\t\treturn git_config_string(&http_proxy_authmethod, var, value);\n+\n \tif (!strcmp(\"http.cookiefile\", var))\n \t\treturn git_config_string(&curl_cookie_file, var, value);\n \tif (!strcmp(\"http.savecookies\", var)) {\n@@ -305,6 +309,43 @@ static void init_curl_http_auth(CURL *result)\n #endif\n }\n \n+static void set_from_env(const char **var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val)\n+\t\t*var = val;\n+}\n+\n+static void init_curl_proxy_auth(CURL *result)\n+{\n+ set_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n+\n+\tif (http_proxy_authmethod) {\n+\t\tif (!strcmp(http_proxy_authmethod, \"basic\"))\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_BASIC);\n+\t\telse if (!strcmp(http_proxy_authmethod, \"digest\"))\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_DIGEST);\n+\t\telse if (!strcmp(http_proxy_authmethod, \"negotiate\"))\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_GSSNEGOTIATE);\n+\t\telse if (!strcmp(http_proxy_authmethod, \"ntlm\"))\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_NTLM);\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t\telse if (!strcmp(http_proxy_authmethod, \"anyauth\"))\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+#endif\n+\t\t// CURLAUTH_DIGEST_IE has no corresponding command-line option in\n+\t\t// curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n+\t\t// here, too\n+\t\telse\n+\t\t\terror(\"Unknown proxy authentication method: %s\",\n+\t\t\t http_proxy_authmethod);\n+\t}\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\telse\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+#endif\n+}\n+\n static int has_cert_password(void)\n {\n \tif (ssl_cert == NULL || ssl_cert_password_required != 1)\n@@ -379,6 +420,7 @@ static CURL *get_curl_handle(void)\n \tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n #endif\n \n+\n \tif (http_proactive_auth)\n \t\tinit_curl_http_auth(result);\n \n@@ -466,22 +508,13 @@ static CURL *get_curl_handle(void)\n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t}\n-#if LIBCURL_VERSION_NUM >= 0x070a07\n-\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n-#endif\n+\tinit_curl_proxy_auth(result);\n \n \tset_curl_keepalive(result);\n \n \treturn result;\n }\n \n-static void set_from_env(const char **var, const char *envname)\n-{\n-\tconst char *val = getenv(envname);\n-\tif (val)\n-\t\t*var = val;\n-}\n-\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n \tchar *low_speed_limit;\n@@ -509,6 +542,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (remote && remote->http_proxy)\n \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n \n+\tif (remote && remote->http_proxy_authmethod)\n+\t\thttp_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);\n+\n \tpragma_header = curl_slist_append(pragma_header, \"Pragma: no-cache\");\n \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n \n@@ -607,6 +643,11 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (http_proxy_authmethod) {\n+\t\tfree((void *)http_proxy_authmethod);\n+\t\thttp_proxy_authmethod = NULL;\n+\t}\n+\n \tif (cert_auth.password != NULL) {\n \t\tmemset(cert_auth.password, 0, strlen(cert_auth.password));\n \t\tfree(cert_auth.password);\ndiff --git a/remote.c b/remote.c\nindex 1101f82..5704f86 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t} else if (!strcmp(subkey, \".proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n+\t} else if (!strcmp(subkey, \".proxy-authmethod\")) {\n+\t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n+\t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\ndiff --git a/remote.h b/remote.h\nindex 312b7ca..a221c5a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -54,6 +54,7 @@ struct remote {\n \t * for curl remotes only\n \t */\n \tchar *http_proxy;\n+\tchar *http_proxy_authmethod;\n };\n \n struct remote *remote_get(const char *name);\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272248","messageId":"1445882109-18184-2-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-26T17:55:09Z","receivedAt":"2015-10-26T17:55:09Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Currently, the only way to pass proxy credentials to curl is by including them\nin the proxy URL. Usually, this means they will end up on disk unencrypted, one\nway or another (by inclusion in ~/.gitconfig, shell profile or history). Since\nproxy authentication often uses a domain user, credentials can be security\nsensitive; therefore, a safer way of passing credentials is desirable.\n\nIf the configured proxy contains a username but not a password, query the\ncredential API for one. Also, make sure we approve/reject proxy credentials\nproperly.\n\nFor consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\nenvironment variables, which would otherwise be evaluated as a fallback by curl.\nWithout this, we would have different semantics for git configuration and\nenvironment variables.\n\nSigned-off-by: Knut Franke \n---\n http.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 1 +\n 2 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex f7366d0..6e20017 100644\n--- a/http.c\n+++ b/http.c\n@@ -64,6 +64,7 @@ static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n static const char *http_proxy_authmethod = NULL;\n+struct credential http_proxy_auth = CREDENTIAL_INIT;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -161,6 +162,9 @@ static void finish_active_slot(struct active_request_slot *slot)\n #else\n \t\tslot->results->auth_avail = 0;\n #endif\n+\n+\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n+\t\t\t&slot->results->http_connectcode);\n \t}\n \n \t/* Run callback if appropriate */\n@@ -318,6 +322,25 @@ static void set_from_env(const char **var, const char *envname)\n \n static void init_curl_proxy_auth(CURL *result)\n {\n+\tif (http_proxy_auth.username) {\n+\t\tif (!http_proxy_auth.password) {\n+\t\t\tcredential_fill(&http_proxy_auth);\n+\t\t}\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n+\t\t\thttp_proxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n+\t\t\thttp_proxy_auth.password);\n+#else\n+\t\tstruct strbuf up = STRBUF_INIT;\n+\t\tstrbuf_reset(&up);\n+\t\tstrbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);\n+\t\tstrbuf_addch(&up, ':');\n+\t\tstrbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);\n+#endif\n+\t}\n+\n set_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n \n \tif (http_proxy_authmethod) {\n@@ -505,8 +528,36 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n #endif\n \n+\t/*\n+\t * curl also examines these variables as a fallback; but we need to query\n+\t * them here in order to decide whether to prompt for missing password (cf.\n+\t * init_curl_proxy_auth()).\n+\t */\n+\tif (!curl_http_proxy) {\n+\t\tif (!strcmp(http_auth.protocol, \"https\")) {\n+\t\t\tset_from_env(&curl_http_proxy, \"HTTPS_PROXY\");\n+\t\t\tset_from_env(&curl_http_proxy, \"https_proxy\");\n+\t\t} else {\n+\t\t\tset_from_env(&curl_http_proxy, \"http_proxy\");\n+\t\t}\n+\t\tif (!curl_http_proxy) {\n+\t\t\tset_from_env(&curl_http_proxy, \"ALL_PROXY\");\n+\t\t\tset_from_env(&curl_http_proxy, \"all_proxy\");\n+\t\t}\n+\t}\n+\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tif (strstr(curl_http_proxy, \"://\"))\n+\t\t\tcredential_from_url(&http_proxy_auth, curl_http_proxy);\n+\t\telse {\n+\t\t\tstruct strbuf url = STRBUF_INIT;\n+\t\t\tstrbuf_reset(&url);\n+\t\t\tstrbuf_addstr(&url, \"http://\");\n+\t\t\tstrbuf_addstr(&url, curl_http_proxy);\n+\t\t\tcredential_from_url(&http_proxy_auth, url.buf);\n+\t\t}\n+\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);\n \t}\n \tinit_curl_proxy_auth(result);\n \n@@ -643,6 +694,12 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (http_proxy_auth.password) {\n+\t\tmemset(http_proxy_auth.password, 0, strlen(http_proxy_auth.password));\n+\t\tfree(http_proxy_auth.password);\n+\t\thttp_proxy_auth.password = NULL;\n+\t}\n+\n \tif (http_proxy_authmethod) {\n \t\tfree((void *)http_proxy_authmethod);\n \t\thttp_proxy_authmethod = NULL;\n@@ -976,6 +1033,8 @@ static int handle_curl_result(struct slot_results *results)\n \n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n+\t\tif (http_proxy_auth.password)\n+\t\t\tcredential_approve(&http_proxy_auth);\n \t\treturn HTTP_OK;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n@@ -990,6 +1049,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n+\t\tif (results->http_connectcode == 407)\n+\t\t\tcredential_reject(&http_proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x070c00\n \t\tif (!curl_errorstr[0])\n \t\t\tstrlcpy(curl_errorstr,\ndiff --git a/http.h b/http.h\nindex 49afe39..7352a9e 100644\n--- a/http.h\n+++ b/http.h\n@@ -54,6 +54,7 @@ struct slot_results {\n \tCURLcode curl_result;\n \tlong http_code;\n \tlong auth_avail;\n+\tlong http_connectcode;\n };\n \n struct active_request_slot {\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272267","messageId":"xmqqk2q9fjyx.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-26T20:33:42Z","receivedAt":"2015-10-26T20:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\n> with a 307 redirect to an error page instead of a 407 listing supported\n> authentication methods. Therefore, allow the authentication method to be set\n> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\n> variables http.proxy-authmethod and remote..proxy-authmethod (in analogy\n> to http.proxy and remote..proxy).\n\nPlease do not name configuration variable with dashes (or\nunderscores). There still may be existing aberrations that are\ndeprecated and need to be removed over time that are misnamed, but\nthe core git variables are named without dashes and underscores.\n\nCall yours \"http.proxyAuthmethod\" in the documentation, and use\nstrcmp(\"http.proxyauthmethod\", var) in the options callback code.\n\n> +static void init_curl_proxy_auth(CURL *result)\n> +{\n> + set_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n\nStrange indentation here...\n\n> +\n> +\tif (http_proxy_authmethod) {\n> +\t\tif (!strcmp(http_proxy_authmethod, \"basic\"))\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_BASIC);\n> +\t\telse if (!strcmp(http_proxy_authmethod, \"digest\"))\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_DIGEST);\n> +\t\telse if (!strcmp(http_proxy_authmethod, \"negotiate\"))\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_GSSNEGOTIATE);\n> +\t\telse if (!strcmp(http_proxy_authmethod, \"ntlm\"))\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_NTLM);\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t\telse if (!strcmp(http_proxy_authmethod, \"anyauth\"))\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +#endif\n> +\t\t// CURLAUTH_DIGEST_IE has no corresponding command-line option in\n> +\t\t// curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n> +\t\t// here, too\n> +\t\telse\n> +\t\t\terror(\"Unknown proxy authentication method: %s\",\n> +\t\t\t http_proxy_authmethod);\n\nAlong the same line as how we do sslversions[] instead of a long\nif/else if/ chain is preferrable.\n\nThansk.\n"},{"id":"272292","messageId":"20151027084742.GA6492@science-computing.de","threadId":"40647","inReplyTo":"xmqqk2q9fjyx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-27T08:47:43Z","receivedAt":"2015-10-27T08:47:43Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"On 2015-10-26 13:33, Junio C Hamano wrote:\n> Call yours \"http.proxyAuthmethod\" in the documentation, and use\n> strcmp(\"http.proxyauthmethod\", var) in the options callback code.\n[...]\n> Strange indentation here...\n[...]\n> Along the same line as how we do sslversions[] instead of a long\n> if/else if/ chain is preferrable.\n\nThanks for the feedback, I'll fix these in the next iteration.\n\n\nCheers,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272401","messageId":"1446025245-10128-1-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH v2] http proxy authentication improvements","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-28T09:40:43Z","receivedAt":"2015-10-28T09:40:43Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Fixes in the second iteration:\n\n* rename http.proxy-authmethod to http.proxyAuthmethod for consistency with\n other core git variables\n\n* issue warning() instead of error() for unsupported authentication method, for\n consistency with http.sslVersion\n\n* fix some code formatting / style issues\n \n* fix memory management bug\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272399","messageId":"1446025245-10128-2-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446025245-10128-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-28T09:40:44Z","receivedAt":"2015-10-28T09:40:44Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\nwith a 307 redirect to an error page instead of a 407 listing supported\nauthentication methods. Therefore, allow the authentication method to be set\nusing the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\nvariables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\nto http.proxy and remote..proxy).\n\nThe following values are supported:\n\n* anyauth (default)\n* basic\n* digest\n* negotiate\n* ntlm\n\nSigned-off-by: Knut Franke \n---\n Documentation/config.txt | 28 ++++++++++++++++++++++\n http.c | 62 +++++++++++++++++++++++++++++++++++++++++++++---\n remote.c | 3 +++\n remote.h | 1 +\n 4 files changed, 91 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..f2644d1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1597,6 +1597,29 @@ http.proxy::\n \t`curl(1)`). This can be overridden on a per-remote basis; see\n \tremote..proxy\n \n+http.proxyAuthmethod::\n+\tSet the method with which to authenticate against the HTTP proxy. This only\n+ takes effect if the configured proxy URI contains a user name part (i.e. is\n+ of the form 'user@host' or 'user@host:port'). This can be overridden on a\n+ per-remote basis; see `remote..proxyAuthmethod`. Both can be\n+ overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.\n+ Possible values are:\n++\n+--\n+* `anyauth` - Automatically pick a suitable authentication method. It is\n+ assumed that the proxy answers an unauthenticated request with a 407\n+ status code and one or more Proxy-authenticate headers with supported\n+ authentication methods. This is the default.\n+* `basic` - HTTP Basic authentication\n+* `digest` - HTTP Digest authentication; this prevents the password from being\n+ transmitted to the proxy in clear text\n+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option\n+ of `curl(1)`)\n+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)\n+--\n++\n+\n+\n http.cookieFile::\n \tFile containing previously stored cookie lines which should be used\n \tin the Git http session, if they match the server. The file format\n@@ -2390,6 +2413,11 @@ remote..proxy::\n \tthe proxy to use for that remote. Set to the empty string to\n \tdisable proxying for that remote.\n \n+remote..proxyAuthmethod::\n+ For remotes that require curl (http, https and ftp), the method to use for\n+ authenticating against the proxy in use (probably set in\n+ `remote..proxy`). See `http.proxyAuthmethod`.\n+\n remote..fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\ndiff --git a/http.c b/http.c\nindex 7da76ed..4756bab 100644\n--- a/http.c\n+++ b/http.c\n@@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n+static const char *http_proxy_authmethod = NULL;\n+static struct {\n+\tconst char *name;\n+\tlong curlauth_param;\n+} http_proxy_authmethods[] = {\n+\t{ \"basic\", CURLAUTH_BASIC },\n+\t{ \"digest\", CURLAUTH_DIGEST },\n+\t{ \"negotiate\", CURLAUTH_GSSNEGOTIATE },\n+\t{ \"ntlm\", CURLAUTH_NTLM },\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t{ \"anyauth\", CURLAUTH_ANY },\n+#endif\n+\t// CURLAUTH_DIGEST_IE has no corresponding command-line option in\n+\t// curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n+\t// here, too\n+};\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -257,6 +273,9 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.proxy\", var))\n \t\treturn git_config_string(&curl_http_proxy, var, value);\n \n+\tif (!strcmp(\"http.proxyauthmethod\", var))\n+\t\treturn git_config_string(&http_proxy_authmethod, var, value);\n+\n \tif (!strcmp(\"http.cookiefile\", var))\n \t\treturn git_config_string(&curl_cookie_file, var, value);\n \tif (!strcmp(\"http.savecookies\", var)) {\n@@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)\n #endif\n }\n \n+static void copy_from_env(const char **var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val)\n+\t\t*var = xstrdup(val);\n+}\n+\n+static void init_curl_proxy_auth(CURL *result)\n+{\n+\tcopy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n+\n+\tif (http_proxy_authmethod) {\n+\t\tint i;\n+\t\tfor (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {\n+\t\t\tif (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {\n+\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n+\t\t\t\t\t\thttp_proxy_authmethods[i].curlauth_param);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (i == ARRAY_SIZE(http_proxy_authmethods)) {\n+\t\t\twarning(\"unsupported proxy authentication method %s: using default\",\n+\t\t\t http_proxy_authmethod);\n+\t\t}\n+\t}\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\telse\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+#endif\n+}\n+\n static int has_cert_password(void)\n {\n \tif (ssl_cert == NULL || ssl_cert_password_required != 1)\n@@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)\n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t}\n-#if LIBCURL_VERSION_NUM >= 0x070a07\n-\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n-#endif\n+\tinit_curl_proxy_auth(result);\n \n \tset_curl_keepalive(result);\n \n@@ -509,6 +557,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (remote && remote->http_proxy)\n \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n \n+\tif (remote && remote->http_proxy_authmethod)\n+\t\thttp_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);\n+\n \tpragma_header = curl_slist_append(pragma_header, \"Pragma: no-cache\");\n \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n \n@@ -607,6 +658,11 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (http_proxy_authmethod) {\n+\t\tfree((void *)http_proxy_authmethod);\n+\t\thttp_proxy_authmethod = NULL;\n+\t}\n+\n \tif (cert_auth.password != NULL) {\n \t\tmemset(cert_auth.password, 0, strlen(cert_auth.password));\n \t\tfree(cert_auth.password);\ndiff --git a/remote.c b/remote.c\nindex 1101f82..426c6d8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t} else if (!strcmp(subkey, \".proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n+\t} else if (!strcmp(subkey, \".proxyAuthmethod\")) {\n+\t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n+\t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\ndiff --git a/remote.h b/remote.h\nindex 312b7ca..a221c5a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -54,6 +54,7 @@ struct remote {\n \t * for curl remotes only\n \t */\n \tchar *http_proxy;\n+\tchar *http_proxy_authmethod;\n };\n \n struct remote *remote_get(const char *name);\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272400","messageId":"1446025245-10128-3-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446025245-10128-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-28T09:40:45Z","receivedAt":"2015-10-28T09:40:45Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Currently, the only way to pass proxy credentials to curl is by including them\nin the proxy URL. Usually, this means they will end up on disk unencrypted, one\nway or another (by inclusion in ~/.gitconfig, shell profile or history). Since\nproxy authentication often uses a domain user, credentials can be security\nsensitive; therefore, a safer way of passing credentials is desirable.\n\nIf the configured proxy contains a username but not a password, query the\ncredential API for one. Also, make sure we approve/reject proxy credentials\nproperly.\n\nFor consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\nenvironment variables, which would otherwise be evaluated as a fallback by curl.\nWithout this, we would have different semantics for git configuration and\nenvironment variables.\n\nSigned-off-by: Knut Franke \n---\n http.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 1 +\n 2 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 4756bab..11bebe1 100644\n--- a/http.c\n+++ b/http.c\n@@ -79,6 +79,7 @@ static struct {\n \t// curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n \t// here, too\n };\n+struct credential http_proxy_auth = CREDENTIAL_INIT;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot *slot)\n #else\n \t\tslot->results->auth_avail = 0;\n #endif\n+\n+\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n+\t\t\t&slot->results->http_connectcode);\n \t}\n \n \t/* Run callback if appropriate */\n@@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char *envname)\n \n static void init_curl_proxy_auth(CURL *result)\n {\n+\tif (http_proxy_auth.username) {\n+\t\tif (!http_proxy_auth.password) {\n+\t\t\tcredential_fill(&http_proxy_auth);\n+\t\t}\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n+\t\t\thttp_proxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n+\t\t\thttp_proxy_auth.password);\n+#else\n+\t\tstruct strbuf up = STRBUF_INIT;\n+\t\tstrbuf_reset(&up);\n+\t\tstrbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);\n+\t\tstrbuf_addch(&up, ':');\n+\t\tstrbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);\n+#endif\n+\t}\n+\n \tcopy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n \n \tif (http_proxy_authmethod) {\n@@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n #endif\n \n+\t/*\n+\t * curl also examines these variables as a fallback; but we need to query\n+\t * them here in order to decide whether to prompt for missing password (cf.\n+\t * init_curl_proxy_auth()).\n+\t */\n+\tif (!curl_http_proxy) {\n+\t\tif (!strcmp(http_auth.protocol, \"https\")) {\n+\t\t\tcopy_from_env(&curl_http_proxy, \"HTTPS_PROXY\");\n+\t\t\tcopy_from_env(&curl_http_proxy, \"https_proxy\");\n+\t\t} else {\n+\t\t\tcopy_from_env(&curl_http_proxy, \"http_proxy\");\n+\t\t}\n+\t\tif (!curl_http_proxy) {\n+\t\t\tcopy_from_env(&curl_http_proxy, \"ALL_PROXY\");\n+\t\t\tcopy_from_env(&curl_http_proxy, \"all_proxy\");\n+\t\t}\n+\t}\n+\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tif (strstr(curl_http_proxy, \"://\"))\n+\t\t\tcredential_from_url(&http_proxy_auth, curl_http_proxy);\n+\t\telse {\n+\t\t\tstruct strbuf url = STRBUF_INIT;\n+\t\t\tstrbuf_reset(&url);\n+\t\t\tstrbuf_addstr(&url, \"http://\");\n+\t\t\tstrbuf_addstr(&url, curl_http_proxy);\n+\t\t\tcredential_from_url(&http_proxy_auth, url.buf);\n+\t\t}\n+\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);\n \t}\n \tinit_curl_proxy_auth(result);\n \n@@ -658,6 +709,12 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (http_proxy_auth.password) {\n+\t\tmemset(http_proxy_auth.password, 0, strlen(http_proxy_auth.password));\n+\t\tfree(http_proxy_auth.password);\n+\t\thttp_proxy_auth.password = NULL;\n+\t}\n+\n \tif (http_proxy_authmethod) {\n \t\tfree((void *)http_proxy_authmethod);\n \t\thttp_proxy_authmethod = NULL;\n@@ -991,6 +1048,8 @@ static int handle_curl_result(struct slot_results *results)\n \n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n+\t\tif (http_proxy_auth.password)\n+\t\t\tcredential_approve(&http_proxy_auth);\n \t\treturn HTTP_OK;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n@@ -1005,6 +1064,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n+\t\tif (results->http_connectcode == 407)\n+\t\t\tcredential_reject(&http_proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x070c00\n \t\tif (!curl_errorstr[0])\n \t\t\tstrlcpy(curl_errorstr,\ndiff --git a/http.h b/http.h\nindex 49afe39..7352a9e 100644\n--- a/http.h\n+++ b/http.h\n@@ -54,6 +54,7 @@ struct slot_results {\n \tCURLcode curl_result;\n \tlong http_code;\n \tlong auth_avail;\n+\tlong http_connectcode;\n };\n \n struct active_request_slot {\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272418","messageId":"xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1446025245-10128-2-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T16:51:12Z","receivedAt":"2015-10-28T16:51:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\n> with a 307 redirect to an error page instead of a 407 listing supported\n> authentication methods. Therefore, allow the authentication method to be set\n> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\n> variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\n> to http.proxy and remote..proxy).\n>\n> The following values are supported:\n>\n> * anyauth (default)\n> * basic\n> * digest\n> * negotiate\n> * ntlm\n>\n> Signed-off-by: Knut Franke \n> ---\n\nThanks.\n\n> +http.proxyAuthmethod::\n> +\tSet the method with which to authenticate against the HTTP proxy. This only\n> + takes effect if the configured proxy URI contains a user name part (i.e. is\n> + of the form 'user@host' or 'user@host:port'). This can be overridden on a\n> + per-remote basis; see `remote..proxyAuthmethod`. Both can be\n> + overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.\n> + Possible values are:\n\nI see inconsistent indentation here. Indent with a tab, like you\ndid your first line, consistently up to this point, perhaps?\n\n> @@ -2390,6 +2413,11 @@ remote..proxy::\n> \tthe proxy to use for that remote. Set to the empty string to\n> \tdisable proxying for that remote.\n> \n> +remote..proxyAuthmethod::\n> + For remotes that require curl (http, https and ftp), the method to use for\n> + authenticating against the proxy in use (probably set in\n> + `remote..proxy`). See `http.proxyAuthmethod`.\n> +\n\nLikewise (match the style of the surrounding paragraphs).\n\n> diff --git a/http.c b/http.c\n> index 7da76ed..4756bab 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;\n> static long curl_low_speed_time = -1;\n> static int curl_ftp_no_epsv;\n> static const char *curl_http_proxy;\n> +static const char *http_proxy_authmethod = NULL;\n> +static struct {\n> +\tconst char *name;\n> +\tlong curlauth_param;\n> +} http_proxy_authmethods[] = {\n\nPerhaps call this \"proxy_authmethod[]\"? We won't be talking about\nany other kinds of proxy authentication method in http.c file, and a\nlong name like this makes the line unnecessarily long in a nested\nloop like you added to init_curl_proxy_auth().\n\n> +\t{ \"basic\", CURLAUTH_BASIC },\n> +\t{ \"digest\", CURLAUTH_DIGEST },\n> +\t{ \"negotiate\", CURLAUTH_GSSNEGOTIATE },\n> +\t{ \"ntlm\", CURLAUTH_NTLM },\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\t{ \"anyauth\", CURLAUTH_ANY },\n> +#endif\n> +\t// CURLAUTH_DIGEST_IE has no corresponding command-line option in\n> +\t// curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n> +\t// here, too\n> +};\n\nPlease do not use // C++ comments.\n\n> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)\n> #endif\n> }\n> \n> +static void copy_from_env(const char **var, const char *envname)\n> +{\n> +\tconst char *val = getenv(envname);\n> +\tif (val)\n> +\t\t*var = xstrdup(val);\n> +}\n> +\n> +static void init_curl_proxy_auth(CURL *result)\n> +{\n> +\tcopy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n\nUnless this helper is used regularly from many other places, is use\nmakes it harder to follow the flow of the logic, as it does not\noffer clear and obvious abstraction, especially with the name\n\"copy_from_env()\". I was forced to look at the implementation to\nsee what happens when the environment variable does not exist to\nmake sure the right thing happens (i.e. http_proxy_authmethod is\nunchanged).\n\n> +\tif (http_proxy_authmethod) {\n> +\t\tint i;\n> +\t\tfor (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {\n> +\t\t\tif (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {\n> +\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n> +\t\t\t\t\t\thttp_proxy_authmethods[i].curlauth_param);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (i == ARRAY_SIZE(http_proxy_authmethods)) {\n> +\t\t\twarning(\"unsupported proxy authentication method %s: using default\",\n> +\t\t\t http_proxy_authmethod);\n> +\t\t}\n> +\t}\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> +\telse\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +#endif\n> +}\n\nThis patch should take what 1c2dbf20 (http: support curl < 7.10.7,\n2015-02-03) wanted to do into account. Having the configuration\nvariable or the environment variable defined by itself, while\nrunning a Git built with old cURL, shouldn't trigger any warning,\nbut the entire function should perhaps be ifdefed out or something?\n\nPersonally I find it a bit surprising that somebody still cares\nabout such an old version (7.10.7 is listed on 15 Aug 2003 in the\nCHANGES file), but there apparently are happy users. Let's not\nknowingly break them.\n\n> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)\n> \tif (curl_http_proxy) {\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> \t}\n> -#if LIBCURL_VERSION_NUM >= 0x070a07\n> -\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> -#endif\n> +\tinit_curl_proxy_auth(result);\n\nThanks.\n"},{"id":"272419","messageId":"xmqqoafjkjy3.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-28T16:59:48Z","receivedAt":"2015-10-28T16:59:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +static void copy_from_env(const char **var, const char *envname)\n>> +{\n>> +\tconst char *val = getenv(envname);\n>> +\tif (val)\n>> +\t\t*var = xstrdup(val);\n>> +}\n>> +\n>> +static void init_curl_proxy_auth(CURL *result)\n>> +{\n>> +\tcopy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n>\n> Unless this helper is used regularly from many other places, is use\n> makes it harder to follow the flow of the logic, as it does not\n> offer clear and obvious abstraction, especially with the name\n> \"copy_from_env()\". I was forced to look at the implementation to\n> see what happens when the environment variable does not exist to\n> make sure the right thing happens (i.e. http_proxy_authmethod is\n> unchanged).\n\nI see you use this liberally in 2/2, it is a handy helper to have,\nand I do _not_ think it is a good idea to open-code this in 1/2 and\nturn it into a helper in 2/2. IOW, I am OK with this \"one helper\nwith a single caller introduced and used in 1/2\". I primarily was\nwishing that its name more clearly conveyed that it sets the\nvariable from the environment _only if_ the environment variable\nexists, and otherwise it does not clobber.\n\nThe implementation of the helper seems to assume that the variable\nmust not be pointing at a free-able piece of memory when it is\ncalled (that is why *var is assigned to without freeing the old\nvalue). That's another subtle thing the callers need to be aware of\n(i.e. deserves at least a comment in front of the function, but as\nalways a good name that clearly conveys it would be more\npreferrable, if we can find one).\n\nThanks.\n"},{"id":"272430","messageId":"CAPig+cQhUgFLe+8JLwC4vc1upNT7YXO0JdWgXDhSGn+5xGwaSA@mail.gmail.com","threadId":"40647","inReplyTo":"1446025245-10128-2-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-28T18:54:04Z","receivedAt":"2015-10-28T18:54:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to Junio's review comments...\n\nOn Wednesday, October 28, 2015, Knut Franke\n wrote:\n> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\n> with a 307 redirect to an error page instead of a 407 listing supported\n> authentication methods. Therefore, allow the authentication method to be set\n> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\n> variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\n> to http.proxy and remote..proxy).\n>\n> The following values are supported:\n>\n> * anyauth (default)\n> * basic\n> * digest\n> * negotiate\n> * ntlm\n>\n> Signed-off-by: Knut Franke \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..f2644d1 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1597,6 +1597,29 @@ http.proxy::\n> `curl(1)`). This can be overridden on a per-remote basis; see\n> remote..proxy\n>\n> +http.proxyAuthmethod::\n\nShould this be typed as \"proxyAuthMethod\"?\n\n> + Set the method with which to authenticate against the HTTP proxy. This only\n> + takes effect if the configured proxy URI contains a user name part (i.e. is\n> + of the form 'user@host' or 'user@host:port'). This can be overridden on a\n> + per-remote basis; see `remote..proxyAuthmethod`. Both can be\n> + overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.\n> + Possible values are:\n> ++\n> +--\n> +* `anyauth` - Automatically pick a suitable authentication method. It is\n> + assumed that the proxy answers an unauthenticated request with a 407\n> + status code and one or more Proxy-authenticate headers with supported\n> + authentication methods. This is the default.\n> +* `basic` - HTTP Basic authentication\n> +* `digest` - HTTP Digest authentication; this prevents the password from being\n> + transmitted to the proxy in clear text\n> +* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option\n> + of `curl(1)`)\n> +* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)\n> +--\n\nI think you can drop the unnecessary '--' lines here and above.\n\n> ++\n> +\n\nNo need for the extra unnecessary \"+\" line and empty line.\n\n> +\n> http.cookieFile::\n> File containing previously stored cookie lines which should be used\n> in the Git http session, if they match the server. The file format\n> diff --git a/http.c b/http.c\n> index 7da76ed..4756bab 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)\n> +static void init_curl_proxy_auth(CURL *result)\n> +{\n> + copy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n> +\n> + if (http_proxy_authmethod) {\n> + int i;\n> + for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {\n> + if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {\n> + curl_easy_setopt(result, CURLOPT_PROXYAUTH,\n> + http_proxy_authmethods[i].curlauth_param);\n> + break;\n> + }\n> + }\n> + if (i == ARRAY_SIZE(http_proxy_authmethods)) {\n> + warning(\"unsupported proxy authentication method %s: using default\",\n> + http_proxy_authmethod);\n\nDoes the user know what \"default\" means here? Does it mean\nCURLAUTH_ANY? If so, do you need to invoke curl_easy_setopt(...,\nCURLAUTH_ANY) as you do below when http_proxy is NULL?\n\n> + }\n> + }\n> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> + else\n> + curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +#endif\n> +}\n> +\n> static int has_cert_password(void)\n> {\n> if (ssl_cert == NULL || ssl_cert_password_required != 1)\n> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)\n> if (curl_http_proxy) {\n> curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> }\n> -#if LIBCURL_VERSION_NUM >= 0x070a07\n> - curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> -#endif\n> + init_curl_proxy_auth(result);\n>\n> set_curl_keepalive(result);\n>\n> diff --git a/remote.c b/remote.c\n> index 1101f82..426c6d8 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -427,6 +427,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n> } else if (!strcmp(subkey, \".proxy\")) {\n> return git_config_string((const char **)&remote->http_proxy,\n> key, value);\n> + } else if (!strcmp(subkey, \".proxyAuthmethod\")) {\n\nIn documentation, write \"proxyAuthMethod\", but in code use\n\"proxyauthmethod\" string literal.\n\n> + return git_config_string((const char **)&remote->http_proxy_authmethod,\n> + key, value);\n> } else if (!strcmp(subkey, \".vcs\")) {\n> return git_config_string(&remote->foreign_vcs, key, value);\n> }\n> diff --git a/remote.h b/remote.h\n> index 312b7ca..a221c5a 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -54,6 +54,7 @@ struct remote {\n> * for curl remotes only\n> */\n> char *http_proxy;\n> + char *http_proxy_authmethod;\n> };\n>\n> struct remote *remote_get(const char *name);\n> --\n> 2.3.7\n"},{"id":"272432","messageId":"CAPig+cRK-EPpH4dUMpYBcjR22Wqw4RnNTYeBvw-M7h=CTFGviQ@mail.gmail.com","threadId":"40647","inReplyTo":"1446025245-10128-3-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-28T18:58:06Z","receivedAt":"2015-10-28T18:58:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wednesday, October 28, 2015, Knut Franke\n wrote:\n> Currently, the only way to pass proxy credentials to curl is by including them\n> in the proxy URL. Usually, this means they will end up on disk unencrypted, one\n> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since\n> proxy authentication often uses a domain user, credentials can be security\n> sensitive; therefore, a safer way of passing credentials is desirable.\n>\n> If the configured proxy contains a username but not a password, query the\n> credential API for one. Also, make sure we approve/reject proxy credentials\n> properly.\n>\n> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\n> environment variables, which would otherwise be evaluated as a fallback by curl.\n> Without this, we would have different semantics for git configuration and\n> environment variables.\n>\n> Signed-off-by: Knut Franke \n> ---\n> diff --git a/http.c b/http.c\n> index 4756bab..11bebe1 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -79,6 +79,7 @@ static struct {\n> // curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n> // here, too\n> };\n> +struct credential http_proxy_auth = CREDENTIAL_INIT;\n\ns/^/static/\n\n> static const char *curl_cookie_file;\n> static int curl_save_cookies;\n> struct credential http_auth = CREDENTIAL_INIT;\n> @@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot *slot)\n> #else\n> slot->results->auth_avail = 0;\n> #endif\n> +\n> + curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n> + &slot->results->http_connectcode);\n> }\n>\n> /* Run callback if appropriate */\n> @@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char *envname)\n>\n> static void init_curl_proxy_auth(CURL *result)\n> {\n> + if (http_proxy_auth.username) {\n> + if (!http_proxy_auth.password) {\n> + credential_fill(&http_proxy_auth);\n> + }\n\nStyle: drop unnecessary braces\n\n> +#if LIBCURL_VERSION_NUM >= 0x071301\n> + curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n> + http_proxy_auth.username);\n> + curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n> + http_proxy_auth.password);\n> +#else\n> + struct strbuf up = STRBUF_INIT;\n\nMinor: It took me a moment to figure out that \"up\" meant\nuser-password. I wonder if a simpler name such as 's' would suffice?\n\n> + strbuf_reset(&up);\n\nUnnecessary strbuf_reset().\n\n> + strbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);\n> + strbuf_addch(&up, ':');\n> + strbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);\n> + curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);\n\nLeaking 'up'. Insert strbuf_release(&up) here.\n\n> +#endif\n> + }\n> +\n> copy_from_env(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n>\n> if (http_proxy_authmethod) {\n> @@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)\n> curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n> #endif\n>\n> + /*\n> + * curl also examines these variables as a fallback; but we need to query\n> + * them here in order to decide whether to prompt for missing password (cf.\n> + * init_curl_proxy_auth()).\n> + */\n> + if (!curl_http_proxy) {\n> + if (!strcmp(http_auth.protocol, \"https\")) {\n> + copy_from_env(&curl_http_proxy, \"HTTPS_PROXY\");\n> + copy_from_env(&curl_http_proxy, \"https_proxy\");\n> + } else {\n> + copy_from_env(&curl_http_proxy, \"http_proxy\");\n\nTo the casual reader, it's not obvious why you check upper- and\nlowercase versions of the other environment variables but not this\none.\n\n> + }\n> + if (!curl_http_proxy) {\n> + copy_from_env(&curl_http_proxy, \"ALL_PROXY\");\n> + copy_from_env(&curl_http_proxy, \"all_proxy\");\n> + }\n\nIf this sort of upper- and lowercase environment variable name\nchecking is indeed desirable, I wonder if it would make sense to fold\nthat functionality into the helper function.\n\n> + }\n> +\n> if (curl_http_proxy) {\n> - curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> + if (strstr(curl_http_proxy, \"://\"))\n> + credential_from_url(&http_proxy_auth, curl_http_proxy);\n> + else {\n> + struct strbuf url = STRBUF_INIT;\n> + strbuf_reset(&url);\n\nUnnecessary strbuf_reset().\n\n> + strbuf_addstr(&url, \"http://\");\n> + strbuf_addstr(&url, curl_http_proxy);\n\nstrbuf_addf(&url, \"http://%s\", curl_http_proxy) might be more straightforward.\n\n> + credential_from_url(&http_proxy_auth, url.buf);\n\nLeaking 'url' here. Insert strbuf_release(&url).\n\n> + }\n> +\n> + curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);\n> }\n> init_curl_proxy_auth(result);\n"},{"id":"272540","messageId":"20151030180129.GA6425@science-computing.de","threadId":"40647","inReplyTo":"xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-30T18:01:30Z","receivedAt":"2015-10-30T18:01:30Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Junio C Hamano wrote:\n> > +\tif (http_proxy_authmethod) {\n> > +\t\tint i;\n> > +\t\tfor (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {\n> > +\t\t\tif (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {\n> > +\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n> > +\t\t\t\t\t\thttp_proxy_authmethods[i].curlauth_param);\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t\tif (i == ARRAY_SIZE(http_proxy_authmethods)) {\n> > +\t\t\twarning(\"unsupported proxy authentication method %s: using default\",\n> > +\t\t\t http_proxy_authmethod);\n> > +\t\t}\n> > +\t}\n> > +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n> > +\telse\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> > +#endif\n> > +}\n> \n> This patch should take what 1c2dbf20 (http: support curl < 7.10.7,\n> 2015-02-03) wanted to do into account. Having the configuration\n> variable or the environment variable defined by itself, while\n> running a Git built with old cURL, shouldn't trigger any warning,\n> but the entire function should perhaps be ifdefed out or something?\n\nMaybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do\nwith LIBCURL_CAN_HANDLE_AUTH_ANY? If so, should this be a separate patch? With\nthe current (scattered) version dependencies, it took me a while to realize that\nif the function is ifdefed out for LIBCURL_VERSION_NUM < 0x070a07, we don't need\nto worry about default behavior in case LIBCURL_CAN_HANDLE_AUTH_ANY is not\ndefined. (on the other hand, looking at other curl-version-ifdefs, the define\nfor AUTH_ANY is the exception)\n\n> >> +static void copy_from_env(const char **var, const char *envname)\n> >> +{\n> >> + const char *val = getenv(envname);\n> >> + if (val)\n> >> + *var = xstrdup(val);\n> >> +}\n[...]\n> I primarily was\n> wishing that its name more clearly conveyed that it sets the\n> variable from the environment _only if_ the environment variable\n> exists, and otherwise it does not clobber.\n\nHow about env_override? Not perfect, but probably better. I don't think\nsqueezing in more information (maybe_env_override, override_from_env_var) would\nhelp.\n\n> The implementation of the helper seems to assume that the variable\n> must not be pointing at a free-able piece of memory when it is\n> called\n\nIn fact, if http.proxyAuthMethod (btw, I agree with Eric about capitalization)\nis set, I do call it with *var pointing to free-able memory. Will fix this.\n\nFWIW, set_from_env() has the same pre-condition, which doesn't seem to be\nsatisfied in all cases (namely when overwriting variables previously set by\ngit_config_string()); not to mention missing free()s in http_cleanup.\n\n\nOtherwise, I'll make the suggested fixes. Thanks.\n\n\nCheers,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272553","messageId":"20151030182426.GA16389@science-computing.de","threadId":"40647","inReplyTo":"CAPig+cRK-EPpH4dUMpYBcjR22Wqw4RnNTYeBvw-M7h=CTFGviQ@mail.gmail.com","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-10-30T18:24:27Z","receivedAt":"2015-10-30T18:24:27Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"On 2015-10-28 14:58, Eric Sunshine wrote:\n> > + }\n> > + if (!curl_http_proxy) {\n> > + copy_from_env(&curl_http_proxy, \"ALL_PROXY\");\n> > + copy_from_env(&curl_http_proxy, \"all_proxy\");\n> > + }\n> \n> If this sort of upper- and lowercase environment variable name\n> checking is indeed desirable, I wonder if it would make sense to fold\n> that functionality into the helper function.\n\nIt's just for consistency with libcurl here, not generally desirable; so I don't\nthink it makes sense to add it to the helper.\n\nOtherwise, will fix. Thanks.\n\n\nCheers,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272565","messageId":"xmqqd1vwkvtz.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"20151030180129.GA6425@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T19:19:52Z","receivedAt":"2015-10-30T19:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do\n> with LIBCURL_CAN_HANDLE_AUTH_ANY?\n\nQuite frankly, my first preference is to have a code that is clear\nenough so that you do not need such an intermediate macro. By\nisolating implementation details that have to be version dependent\ninto a helper function whose purpose is well defined, the rest of\nthe code can be #ifdef free; it would become sufficiently clear to\nswitch based on curl version where implementation details matter.\n\nBy refraining from littering #ifdef all over the code, we do assign\nto disable_gssnegotiate even though the value is not even used when\ncompiled for ancient version of libCURL, but the benefit of code\nclarity outweighs such downside. We may have to use #ifdef/#endif\nin some places, but we should in general minimize their uses and\nwrite code for the more up-to-date API.\n\nFor what I mean, see the attached patch outline to show how to get\nrid of CAN_HANDLE_AUTH_ANY.\n\n> How about env_override? Not perfect, but probably better.\n\nMuch better than anything I'd come up with myself (I am bad at\nnaming, even though I may sometimes be good at spotting a bad name).\n\n\n http.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 7da76ed..d272b02 100644\n--- a/http.c\n+++ b/http.c\n@@ -15,10 +15,6 @@ int active_requests;\n int http_is_verbose;\n size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n \n-#if LIBCURL_VERSION_NUM >= 0x070a06\n-#define LIBCURL_CAN_HANDLE_AUTH_ANY\n-#endif\n-\n static int min_curl_sessions = 1;\n static int curl_session_count;\n #ifdef USE_CURL_MULTI\n@@ -79,9 +75,6 @@ static const char *user_agent;\n \n static struct credential cert_auth = CREDENTIAL_INIT;\n static int ssl_cert_password_required;\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-static unsigned long http_auth_methods = CURLAUTH_ANY;\n-#endif\n \n static struct curl_slist *pragma_header;\n static struct curl_slist *no_pragma_header;\n@@ -90,6 +83,19 @@ static struct active_request_slot *active_queue_head;\n \n static char *cached_accept_language;\n \n+static int disable_gssnegotiate;\n+\n+static void set_httpauth_opt(CURL *curl)\n+{\n+#if LIBCURL_VERSION_NUM >= 0x070a06 /* Is CURLAUTH_ANY available? */\n+\tunsigned long auth_methods = CURLAUTH_ANY;\n+\n+\tif (disable_gssnegotiate)\n+\t\tauth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n+\tcurl_easy_setopt(curl, CURLOPT_HTTPAUTH, auth_methods);\n+#endif\n+}\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -375,9 +381,7 @@ static CURL *get_curl_handle(void)\n #if LIBCURL_VERSION_NUM >= 0x070907\n \tcurl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);\n #endif\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\tcurl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);\n-#endif\n+\tset_httpauth_opt(result);\n \n \tif (http_proactive_auth)\n \t\tinit_curl_http_auth(result);\n@@ -681,9 +685,7 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\tcurl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);\n-#endif\n+\tset_httpauth_opt(slot->curl);\n \tif (http_auth.password)\n \t\tinit_curl_http_auth(slot->curl);\n \n@@ -943,9 +945,7 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n-\t\t\thttp_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;\n-#endif\n+\t\t\tdisable_gssnegotiate = 1;\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n"},{"id":"272568","messageId":"xmqq8u6kkvam.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"20151030182426.GA16389@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T19:31:29Z","receivedAt":"2015-10-30T19:31:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> On 2015-10-28 14:58, Eric Sunshine wrote:\n>> > + }\n>> > + if (!curl_http_proxy) {\n>> > + copy_from_env(&curl_http_proxy, \"ALL_PROXY\");\n>> > + copy_from_env(&curl_http_proxy, \"all_proxy\");\n>> > + }\n>> \n>> If this sort of upper- and lowercase environment variable name\n>> checking is indeed desirable, I wonder if it would make sense to fold\n>> that functionality into the helper function.\n>\n> It's just for consistency with libcurl here, not generally desirable; so I don't\n> think it makes sense to add it to the helper.\n\nI agree. Unlike many environment variables, historically these\nproxy environment variables were all lowercase only for quite a\nwhile (found as early as in CERN libwww 2.15 March '94).\n\nIt appears that CURL did not know this and implemented only\nuppercase variants, which was later corrected to take both\n(http://sourceforge.net/p/curl/bugs/40/ shows a fix in Nov 2000).\n\nSo both unfortunately are used in user's environment and we need to\npay attention to both. As lowercase version is the more kosher one,\nlooking at uppercase first and then overriding it with the lowercase\none is the right order, I think.\n\nIt's a mess, but it is limited to these proxy-ish variables and does\nnot deserve a helper to promote the pattern.\n"},{"id":"272571","messageId":"CAPig+cSzF0B1rP1ns7PqZjnOuAm50t+Wky1S0EUNh2VWAmdjNw@mail.gmail.com","threadId":"40647","inReplyTo":"xmqq8u6kkvam.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-30T19:35:26Z","receivedAt":"2015-10-30T19:35:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 30, 2015 at 3:31 PM, Junio C Hamano wrote:\n> Knut Franke writes:\n>> On 2015-10-28 14:58, Eric Sunshine wrote:\n>>> > + }\n>>> > + if (!curl_http_proxy) {\n>>> > + copy_from_env(&curl_http_proxy, \"ALL_PROXY\");\n>>> > + copy_from_env(&curl_http_proxy, \"all_proxy\");\n>>> > + }\n>>>\n>>> If this sort of upper- and lowercase environment variable name\n>>> checking is indeed desirable, I wonder if it would make sense to fold\n>>> that functionality into the helper function.\n>>\n>> It's just for consistency with libcurl here, not generally desirable; so I don't\n>> think it makes sense to add it to the helper.\n>\n> I agree. Unlike many environment variables, historically these\n> proxy environment variables were all lowercase only for quite a\n> while (found as early as in CERN libwww 2.15 March '94).\n>\n> It appears that CURL did not know this and implemented only\n> uppercase variants, which was later corrected to take both\n> (http://sourceforge.net/p/curl/bugs/40/ shows a fix in Nov 2000).\n>\n> So both unfortunately are used in user's environment and we need to\n> pay attention to both. As lowercase version is the more kosher one,\n> looking at uppercase first and then overriding it with the lowercase\n> one is the right order, I think.\n>\n> It's a mess, but it is limited to these proxy-ish variables and does\n> not deserve a helper to promote the pattern.\n\nIt would be great to have this explanation in the commit message or\nin-code comment. Thanks.\n"},{"id":"272737","messageId":"1446483264-15123-1-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH v3 0/2]","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-02T16:54:22Z","receivedAt":"2015-11-02T16:54:22Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Changes in the third iteration:\n\n* don't break support for curl < 7.10.7\n\n* fix some memory leaks\n\n* explicitly set anyauth as fallback for unsupported proyx authmethod setting,\n and tell the user what we did\n\n* clean up usage of curl version #ifdefs\n\n* fix more code formatting / style / naming issues\n\n* add in-code comment explaining the proxy variable uppercase/lowercase mess\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272738","messageId":"1446483264-15123-2-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446483264-15123-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-02T16:54:23Z","receivedAt":"2015-11-02T16:54:23Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\nwith a 307 redirect to an error page instead of a 407 listing supported\nauthentication methods. Therefore, allow the authentication method to be set\nusing the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\nvariables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\nto http.proxy and remote..proxy).\n\nThe following values are supported:\n\n* anyauth (default)\n* basic\n* digest\n* negotiate\n* ntlm\n\nSigned-off-by: Knut Franke \nReviewed-by: Junio C Hamano \nReviewed-by: Eric Sunshine \n---\n Documentation/config.txt | 26 +++++++++++++++++\n http.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--\n remote.c | 3 ++\n remote.h | 1 +\n 4 files changed, 99 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..6f29893 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1597,6 +1597,27 @@ http.proxy::\n \t`curl(1)`). This can be overridden on a per-remote basis; see\n \tremote..proxy\n \n+http.proxyAuthMethod::\n+\tSet the method with which to authenticate against the HTTP proxy. This only\n+\ttakes effect if the configured proxy URI contains a user name part (i.e. is\n+\tof the form 'user@host' or 'user@host:port'). This can be overridden on a\n+\tper-remote basis; see `remote..proxyAuthMethod`. Both can be\n+\toverridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.\n+\tPossible values are:\n++\n+--\n+* `anyauth` - Automatically pick a suitable authentication method. It is\n+ assumed that the proxy answers an unauthenticated request with a 407\n+ status code and one or more Proxy-authenticate headers with supported\n+ authentication methods. This is the default.\n+* `basic` - HTTP Basic authentication\n+* `digest` - HTTP Digest authentication; this prevents the password from being\n+ transmitted to the proxy in clear text\n+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option\n+ of `curl(1)`)\n+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)\n+--\n+\n http.cookieFile::\n \tFile containing previously stored cookie lines which should be used\n \tin the Git http session, if they match the server. The file format\n@@ -2390,6 +2411,11 @@ remote..proxy::\n \tthe proxy to use for that remote. Set to the empty string to\n \tdisable proxying for that remote.\n \n+remote..proxyAuthMethod::\n+\tFor remotes that require curl (http, https and ftp), the method to use for\n+\tauthenticating against the proxy in use (probably set in\n+\t`remote..proxy`). See `http.proxyAuthMethod`.\n+\n remote..fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\ndiff --git a/http.c b/http.c\nindex 7da76ed..1172819 100644\n--- a/http.c\n+++ b/http.c\n@@ -63,6 +63,24 @@ static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n+static const char *http_proxy_authmethod = NULL;\n+static struct {\n+\tconst char *name;\n+\tlong curlauth_param;\n+} proxy_authmethods[] = {\n+\t{ \"basic\", CURLAUTH_BASIC },\n+\t{ \"digest\", CURLAUTH_DIGEST },\n+\t{ \"negotiate\", CURLAUTH_GSSNEGOTIATE },\n+\t{ \"ntlm\", CURLAUTH_NTLM },\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t{ \"anyauth\", CURLAUTH_ANY },\n+#endif\n+\t/*\n+\t * CURLAUTH_DIGEST_IE has no corresponding command-line option in\n+\t * curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n+\t * here, too\n+\t */\n+};\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -257,6 +275,9 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.proxy\", var))\n \t\treturn git_config_string(&curl_http_proxy, var, value);\n \n+\tif (!strcmp(\"http.proxyauthmethod\", var))\n+\t\treturn git_config_string(&http_proxy_authmethod, var, value);\n+\n \tif (!strcmp(\"http.cookiefile\", var))\n \t\treturn git_config_string(&curl_cookie_file, var, value);\n \tif (!strcmp(\"http.savecookies\", var)) {\n@@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result)\n #endif\n }\n \n+/* assumes *var is either NULL or free-able */\n+static void env_override(const char **var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val) {\n+\t\tif (*var)\n+\t\t\tfree((void*)*var);\n+\t\t*var = xstrdup(val);\n+\t}\n+}\n+\n+static void init_curl_proxy_auth(CURL *result)\n+{\n+\tenv_override(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n+\n+#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n+\tif (http_proxy_authmethod) {\n+\t\tint i;\n+\t\tfor (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {\n+\t\t\tif (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {\n+\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n+\t\t\t\t\t\tproxy_authmethods[i].curlauth_param);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (i == ARRAY_SIZE(proxy_authmethods)) {\n+\t\t\twarning(\"unsupported proxy authentication method %s: using anyauth\",\n+\t\t\t\t\thttp_proxy_authmethod);\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t\t}\n+\t}\n+\telse\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+#endif\n+}\n+\n static int has_cert_password(void)\n {\n \tif (ssl_cert == NULL || ssl_cert_password_required != 1)\n@@ -466,9 +523,7 @@ static CURL *get_curl_handle(void)\n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t}\n-#if LIBCURL_VERSION_NUM >= 0x070a07\n-\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n-#endif\n+\tinit_curl_proxy_auth(result);\n \n \tset_curl_keepalive(result);\n \n@@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (remote && remote->http_proxy)\n \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n \n+\tif (remote && remote->http_proxy_authmethod) {\n+\t\tif (http_proxy_authmethod)\n+\t\t\tfree((void*)http_proxy_authmethod);\n+\t\thttp_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);\n+\t}\n+\n \tpragma_header = curl_slist_append(pragma_header, \"Pragma: no-cache\");\n \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n \n@@ -607,6 +668,11 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (http_proxy_authmethod) {\n+\t\tfree((void *)http_proxy_authmethod);\n+\t\thttp_proxy_authmethod = NULL;\n+\t}\n+\n \tif (cert_auth.password != NULL) {\n \t\tmemset(cert_auth.password, 0, strlen(cert_auth.password));\n \t\tfree(cert_auth.password);\ndiff --git a/remote.c b/remote.c\nindex 10f1ffc..0e3bba8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -428,6 +428,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t} else if (!strcmp(subkey, \".proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n+\t} else if (!strcmp(subkey, \".proxyauthmethod\")) {\n+\t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n+\t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\ndiff --git a/remote.h b/remote.h\nindex 312b7ca..a221c5a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -54,6 +54,7 @@ struct remote {\n \t * for curl remotes only\n \t */\n \tchar *http_proxy;\n+\tchar *http_proxy_authmethod;\n };\n \n struct remote *remote_get(const char *name);\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272739","messageId":"1446483264-15123-3-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446483264-15123-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-02T16:54:24Z","receivedAt":"2015-11-02T16:54:24Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Currently, the only way to pass proxy credentials to curl is by including them\nin the proxy URL. Usually, this means they will end up on disk unencrypted, one\nway or another (by inclusion in ~/.gitconfig, shell profile or history). Since\nproxy authentication often uses a domain user, credentials can be security\nsensitive; therefore, a safer way of passing credentials is desirable.\n\nIf the configured proxy contains a username but not a password, query the\ncredential API for one. Also, make sure we approve/reject proxy credentials\nproperly.\n\nFor consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\nenvironment variables, which would otherwise be evaluated as a fallback by curl.\nWithout this, we would have different semantics for git configuration and\nenvironment variables.\n\nSigned-off-by: Knut Franke \nReviewed-by: Junio C Hamano \nReviewed-by: Eric Sunshine \n---\n http.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n http.h | 1 +\n 2 files changed, 75 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 1172819..5708c7a 100644\n--- a/http.c\n+++ b/http.c\n@@ -62,7 +62,7 @@ static const char *ssl_cainfo;\n static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n-static const char *curl_http_proxy;\n+static const char *curl_http_proxy = NULL;\n static const char *http_proxy_authmethod = NULL;\n static struct {\n \tconst char *name;\n@@ -81,6 +81,8 @@ static struct {\n \t * here, too\n \t */\n };\n+static struct credential proxy_auth = CREDENTIAL_INIT;\n+static const char *curl_proxyuserpwd = NULL;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -178,6 +180,9 @@ static void finish_active_slot(struct active_request_slot *slot)\n #else\n \t\tslot->results->auth_avail = 0;\n #endif\n+\n+\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n+\t\t\t&slot->results->http_connectcode);\n \t}\n \n \t/* Run callback if appropriate */\n@@ -339,6 +344,24 @@ static void env_override(const char **var, const char *envname)\n \n static void init_curl_proxy_auth(CURL *result)\n {\n+\tif (proxy_auth.username) {\n+\t\tif (!proxy_auth.password)\n+\t\t\tcredential_fill(&proxy_auth);\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n+\t\t\tproxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n+\t\t\tproxy_auth.password);\n+#else\n+\t\tstruct strbuf s = STRBUF_INIT;\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n+\t\tstrbuf_addch(&s, ':');\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n+\t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n+#endif\n+\t}\n+\n \tenv_override(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n \n #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n@@ -520,8 +543,42 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n #endif\n \n+\t/*\n+\t * CURL also examines these variables as a fallback; but we need to query\n+\t * them here in order to decide whether to prompt for missing password (cf.\n+\t * init_curl_proxy_auth()).\n+\t *\n+\t * Unlike many other common environment variables, these are historically\n+\t * lowercase only. It appears that CURL did not know this and implemented\n+\t * only uppercase variants, which was later corrected to take both - with\n+\t * the exception of http_proxy, which is lowercase only also in CURL. As\n+\t * the lowercase versions are the historical quasi-standard, they take\n+\t * precedence here, as in CURL.\n+\t */\n+\tif (!curl_http_proxy) {\n+\t\tif (!strcmp(http_auth.protocol, \"https\")) {\n+\t\t\tenv_override(&curl_http_proxy, \"HTTPS_PROXY\");\n+\t\t\tenv_override(&curl_http_proxy, \"https_proxy\");\n+\t\t} else {\n+\t\t\tenv_override(&curl_http_proxy, \"http_proxy\");\n+\t\t}\n+\t\tif (!curl_http_proxy) {\n+\t\t\tenv_override(&curl_http_proxy, \"ALL_PROXY\");\n+\t\t\tenv_override(&curl_http_proxy, \"all_proxy\");\n+\t\t}\n+\t}\n+\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tif (strstr(curl_http_proxy, \"://\"))\n+\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\telse {\n+\t\t\tstruct strbuf url = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&url, \"http://%s\", curl_http_proxy);\n+\t\t\tcredential_from_url(&proxy_auth, url.buf);\n+\t\t\tstrbuf_release(&url);\n+\t\t}\n+\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n \t}\n \tinit_curl_proxy_auth(result);\n \n@@ -668,6 +725,17 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (proxy_auth.password) {\n+\t\tmemset(proxy_auth.password, 0, strlen(proxy_auth.password));\n+\t\tfree(proxy_auth.password);\n+\t\tproxy_auth.password = NULL;\n+\t}\n+\n+\tif (curl_proxyuserpwd) {\n+\t\tfree((void *)curl_proxyuserpwd);\n+\t\tcurl_proxyuserpwd = NULL;\n+\t}\n+\n \tif (http_proxy_authmethod) {\n \t\tfree((void *)http_proxy_authmethod);\n \t\thttp_proxy_authmethod = NULL;\n@@ -1001,6 +1069,8 @@ static int handle_curl_result(struct slot_results *results)\n \n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n+\t\tif (proxy_auth.password)\n+\t\t\tcredential_approve(&proxy_auth);\n \t\treturn HTTP_OK;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n@@ -1015,6 +1085,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n+\t\tif (results->http_connectcode == 407)\n+\t\t\tcredential_reject(&proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x070c00\n \t\tif (!curl_errorstr[0])\n \t\t\tstrlcpy(curl_errorstr,\ndiff --git a/http.h b/http.h\nindex 49afe39..7352a9e 100644\n--- a/http.h\n+++ b/http.h\n@@ -54,6 +54,7 @@ struct slot_results {\n \tCURLcode curl_result;\n \tlong http_code;\n \tlong auth_avail;\n+\tlong http_connectcode;\n };\n \n struct active_request_slot {\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272770","messageId":"xmqqfv0odnoc.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1446483264-15123-2-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-02T22:46:59Z","receivedAt":"2015-11-02T22:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\n> with a 307 redirect to an error page instead of a 407 listing supported\n> authentication methods. Therefore, allow the authentication method to be set\n> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\n> variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\n> to http.proxy and remote..proxy).\n>\n> The following values are supported:\n>\n> * anyauth (default)\n> * basic\n> * digest\n> * negotiate\n> * ntlm\n>\n> Signed-off-by: Knut Franke \n\n\n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Eric Sunshine \n\nPlease add these only when you are doing the final submission,\nsending the same version reviewed by these people after they said\nthe patch(es) look good. To credit others for helping you to polish\nyour patch, Helped-by: would be more appropriate.\n\n> @@ -305,6 +326,42 @@ static void init_curl_http_auth(CURL *result)\n> #endif\n> }\n> \n> +/* assumes *var is either NULL or free-able */\n> +static void env_override(const char **var, const char *envname)\n> +{\n> +\tconst char *val = getenv(envname);\n> +\tif (val) {\n> +\t\tif (*var)\n> +\t\t\tfree((void*)*var);\n\nJust\n\t\tfree((void *)*var);\n\nwould be more idiomatic (freeing NULL is not a crime but a norm).\nAlso as you did elsewhere, have a space between void and the\nasterisk.\n\n> +static void init_curl_proxy_auth(CURL *result)\n> +{\n> +\tenv_override(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n\nShouldn't this also be part of the #if/#endif?\n\n> +\n> +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n> +\tif (http_proxy_authmethod) {\n> +\t\tint i;\n> +\t\tfor (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {\n> +\t\t\tif (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {\n> +\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n> +\t\t\t\t\t\tproxy_authmethods[i].curlauth_param);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tif (i == ARRAY_SIZE(proxy_authmethods)) {\n> +\t\t\twarning(\"unsupported proxy authentication method %s: using anyauth\",\n> +\t\t\t\t\thttp_proxy_authmethod);\n> +\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +\t\t}\n> +\t}\n> +\telse\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +#endif\n> +}\n> +\n> static int has_cert_password(void)\n> {\n> \tif (ssl_cert == NULL || ssl_cert_password_required != 1)\n> @@ -466,9 +523,7 @@ static CURL *get_curl_handle(void)\n> \tif (curl_http_proxy) {\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> \t}\n> -#if LIBCURL_VERSION_NUM >= 0x070a07\n> -\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> -#endif\n> +\tinit_curl_proxy_auth(result);\n> \n> \tset_curl_keepalive(result);\n> \n> @@ -509,6 +564,12 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n> \tif (remote && remote->http_proxy)\n> \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n> \n> +\tif (remote && remote->http_proxy_authmethod) {\n> +\t\tif (http_proxy_authmethod)\n> +\t\t\tfree((void*)http_proxy_authmethod);\n\nJust\n\t\tfree((void *)http_proxy_authmethod);\n\nwithout NULL-ness check.\n\nBut this makes me wonder if env_override() was a good abstraction.\n\nThat is, with this helper:\n\n /* existing value in *var must be freeable */\n static void var_override(const char **var, char *value)\n {\n if (value) {\n free((void *)(*var));\n var = xstrdup(value);\n }\n }\n\nthe beginning of the init_proxy_auth() would become:\n\n static void init_curl_proxy_auth(CURL *result)\n {\n #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n\tvar_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n\t\t...\n\nand this code would be:\n\n\tif (remote)\n\t\tvar_override(&http_proxy_authmethod, remote->http_proxy_authmethod);\n\nwhich might be even cleaner.\n\n> +\t\thttp_proxy_authmethod = xstrdup(remote->http_proxy_authmethod);\n> +\t}\n> +\n> \tpragma_header = curl_slist_append(pragma_header, \"Pragma: no-cache\");\n> \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n> \n> @@ -607,6 +668,11 @@ void http_cleanup(void)\n> \t\tcurl_http_proxy = NULL;\n> \t}\n> \n> +\tif (http_proxy_authmethod) {\n> +\t\tfree((void *)http_proxy_authmethod);\n> +\t\thttp_proxy_authmethod = NULL;\n> +\t}\n\nNo need for NULL-ness check here, either.\n\nOther than the above nits, looks cleanly done.\n\nThanks.\n"},{"id":"272771","messageId":"xmqqbnbcdnb8.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1446483264-15123-3-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-02T22:54:51Z","receivedAt":"2015-11-02T22:54:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> Currently, the only way to pass proxy credentials to curl is by including them\n> in the proxy URL. Usually, this means they will end up on disk unencrypted, one\n> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since\n> proxy authentication often uses a domain user, credentials can be security\n> sensitive; therefore, a safer way of passing credentials is desirable.\n>\n> If the configured proxy contains a username but not a password, query the\n> credential API for one. Also, make sure we approve/reject proxy credentials\n> properly.\n>\n> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\n> environment variables, which would otherwise be evaluated as a fallback by curl.\n> Without this, we would have different semantics for git configuration and\n> environment variables.\n>\n> Signed-off-by: Knut Franke \n> Reviewed-by: Junio C Hamano \n> Reviewed-by: Eric Sunshine \n\nAs 1/2, I never reviewed this version yet.\n\n> ---\n> http.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n> http.h | 1 +\n> 2 files changed, 75 insertions(+), 2 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 1172819..5708c7a 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -62,7 +62,7 @@ static const char *ssl_cainfo;\n> static long curl_low_speed_limit = -1;\n> static long curl_low_speed_time = -1;\n> static int curl_ftp_no_epsv;\n> -static const char *curl_http_proxy;\n> +static const char *curl_http_proxy = NULL;\n> static const char *http_proxy_authmethod = NULL;\n\nWe do not do unnecessary initialization of file-scope globals to 0\nor NULL. The existing definition of curl_http_proxy is correct;\nthe one for http_proxy_authmethod needs to be changed to match.\n\n> static void init_curl_proxy_auth(CURL *result)\n> {\n> +\tif (proxy_auth.username) {\n> +\t\tif (!proxy_auth.password)\n> +\t\t\tcredential_fill(&proxy_auth);\n> +#if LIBCURL_VERSION_NUM >= 0x071301\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n> +\t\t\tproxy_auth.username);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n> +\t\t\tproxy_auth.password);\n> +#else\n> +\t\tstruct strbuf s = STRBUF_INIT;\n> +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n> +\t\tstrbuf_addch(&s, ':');\n> +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n> +\t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n> +#endif\n\nI think #else clause of this thing would introduce decl-after-stmt\ncompilation error.\n"},{"id":"272822","messageId":"20151103090724.GA6354@science-computing.de","threadId":"40647","inReplyTo":"xmqqfv0odnoc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-03T09:07:24Z","receivedAt":"2015-11-03T09:07:24Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"On 2015-11-02 14:46, Junio C Hamano wrote:\n> > Reviewed-by: Junio C Hamano \n> > Reviewed-by: Eric Sunshine \n> \n> Please add these only when you are doing the final submission,\n> sending the same version reviewed by these people after they said\n> the patch(es) look good. To credit others for helping you to polish\n> your patch, Helped-by: would be more appropriate.\n\nSorry about that.\n\nHowever, may I suggest that Documentation/SubmittingPatches could do with a\nlittle rewording in this respect?\n\n> Do not forget to add trailers such as \"Acked-by:\", \"Reviewed-by:\" and\n> \"Tested-by:\" lines as necessary to credit people who helped your\n> patch.\n\n\"Helped-by:\" isn't even mentioned.\n\n> > +static void init_curl_proxy_auth(CURL *result)\n> > +{\n> > +\tenv_override(&http_proxy_authmethod, \"GIT_HTTP_PROXY_AUTHMETHOD\");\n> \n> Shouldn't this also be part of the #if/#endif?\n\nThe idea here was to have as little code as possible within the #if/#endif, as a\nmatter of principle. It may be a little construed in this case, but supposing\nthere's some subtle bug with env_override, or a future change introduces one,\nhaving it occur only for certain CURL versions would tend to make it harder to\ntrack down.\n\n> and this code would be:\n> \n> \tif (remote)\n> \t\tvar_override(&http_proxy_authmethod, remote->http_proxy_authmethod);\n\nGood catch.\n\n\nCheers,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272824","messageId":"20151103093103.GB6354@science-computing.de","threadId":"40647","inReplyTo":"xmqqbnbcdnb8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-03T09:31:04Z","receivedAt":"2015-11-03T09:31:04Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"On 2015-11-02 14:54, Junio C Hamano wrote:\n> > static void init_curl_proxy_auth(CURL *result)\n> > {\n> > +\tif (proxy_auth.username) {\n> > +\t\tif (!proxy_auth.password)\n> > +\t\t\tcredential_fill(&proxy_auth);\n> > +#if LIBCURL_VERSION_NUM >= 0x071301\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n> > +\t\t\tproxy_auth.username);\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n> > +\t\t\tproxy_auth.password);\n> > +#else\n> > +\t\tstruct strbuf s = STRBUF_INIT;\n> > +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n> > +\t\tstrbuf_addch(&s, ':');\n> > +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n> > +\t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n> > +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n> > +#endif\n> \n> I think #else clause of this thing would introduce decl-after-stmt\n> compilation error.\n\nI've actually tested this with CURL 7.15.5 (0x070f05), and didn't get any\ncompilation error.\n\n\nCheers,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272840","messageId":"CAPig+cQ2u49oLCy6e+1G+xx0K5WjY_U5qmUqnjWZe8XjQH6xPA@mail.gmail.com","threadId":"40647","inReplyTo":"20151103093103.GB6354@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-03T18:12:31Z","receivedAt":"2015-11-03T18:12:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 3, 2015 at 4:31 AM, Knut Franke\n wrote:\n> On 2015-11-02 14:54, Junio C Hamano wrote:\n>> > static void init_curl_proxy_auth(CURL *result)\n>> > {\n>> > + if (proxy_auth.username) {\n>> > + if (!proxy_auth.password)\n>> > + credential_fill(&proxy_auth);\n>> > +#if LIBCURL_VERSION_NUM >= 0x071301\n>> > + curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n>> > + proxy_auth.username);\n>> > + curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n>> > + proxy_auth.password);\n>> > +#else\n>> > + struct strbuf s = STRBUF_INIT;\n>> > + strbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n>> > + strbuf_addch(&s, ':');\n>> > + strbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n>> > + curl_proxyuserpwd = strbuf_detach(&s, NULL);\n>> > + curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n>> > +#endif\n>>\n>> I think #else clause of this thing would introduce decl-after-stmt\n>> compilation error.\n>\n> I've actually tested this with CURL 7.15.5 (0x070f05), and didn't get any\n> compilation error.\n\nJunio meant that, on this project, code still follows the rule that\nall declarations in a block must come before statements in order to\nappease older compilers. That is (good):\n\n {\n int foo; /* declaration */\n\n if (doodad) /* statement */\n snorg();\n glop(&foo);\n }\n\nrather than (bad):\n\n {\n if (doodad) /* statement */\n snorg();\n\n int foo; /* declaration after statement */\n glop(&foo);\n }\n\nThe gcc option -Wdeclaration-after-statement can help.\n"},{"id":"272845","messageId":"xmqqr3k6dfwx.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"20151103090724.GA6354@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-03T19:46:54Z","receivedAt":"2015-11-03T19:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> On 2015-11-02 14:46, Junio C Hamano wrote:\n>> > Reviewed-by: Junio C Hamano \n>> > Reviewed-by: Eric Sunshine \n>> \n>> Please add these only when you are doing the final submission,\n>> sending the same version reviewed by these people after they said\n>> the patch(es) look good. To credit others for helping you to polish\n>> your patch, Helped-by: would be more appropriate.\n>\n> Sorry about that.\n>\n> However, may I suggest that Documentation/SubmittingPatches could do with a\n> little rewording in this respect?\n>\n>> Do not forget to add trailers such as \"Acked-by:\", \"Reviewed-by:\" and\n>> \"Tested-by:\" lines as necessary to credit people who helped your\n>> patch.\n>\n> \"Helped-by:\" isn't even mentioned.\n\nThat is because it is not actively encouraged.\n\nThe only thing I care about is that people do not incorrectly use\nReviewed/Acked-by when reviewers did not say \"this version looks\ngood\"; that would mislead the maintainer to think \"ah, if that\nreviewer said this is good, whose judgment I can trust, then I do\nnot have to read it carefully myself.\"\n"},{"id":"272890","messageId":"1446628405-8070-1-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1445882109-18184-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH v4 0/2]","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-04T09:13:23Z","receivedAt":"2015-11-04T09:13:23Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Changes in the fourth iteration:\n\n* update Documentation/technical/api-remote.txt to reflect the addition to\n struct remote\n\n* update documentation of http.proxy in Documentation/config.txt to mention the\n possibility of having git fill in missing proxy password\n\n* fix decl-after-stmt\n\n* generalize env_override helper to var_override\n\n* more coding style corrections\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272889","messageId":"1446628405-8070-2-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446628405-8070-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-04T09:13:24Z","receivedAt":"2015-11-04T09:13:24Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"CURLAUTH_ANY does not work with proxies which answer unauthenticated requests\nwith a 307 redirect to an error page instead of a 407 listing supported\nauthentication methods. Therefore, allow the authentication method to be set\nusing the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration\nvariables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy\nto http.proxy and remote..proxy).\n\nThe following values are supported:\n\n* anyauth (default)\n* basic\n* digest\n* negotiate\n* ntlm\n\nSigned-off-by: Knut Franke \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \n---\n Documentation/config.txt | 26 ++++++++++++++\n Documentation/technical/api-remote.txt | 4 +++\n http.c | 65 ++++++++++++++++++++++++++++++++--\n remote.c | 3 ++\n remote.h | 1 +\n 5 files changed, 96 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..4ad7f74 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1597,6 +1597,27 @@ http.proxy::\n \t`curl(1)`). This can be overridden on a per-remote basis; see\n \tremote..proxy\n \n+http.proxyAuthMethod::\n+\tSet the method with which to authenticate against the HTTP proxy. This\n+\tonly takes effect if the configured proxy string contains a user name part\n+\t(i.e. is of the form 'user@host' or 'user@host:port'). This can be\n+\toverridden on a per-remote basis; see `remote..proxyAuthMethod`.\n+\tBoth can be overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment\n+\tvariable. Possible values are:\n++\n+--\n+* `anyauth` - Automatically pick a suitable authentication method. It is\n+ assumed that the proxy answers an unauthenticated request with a 407\n+ status code and one or more Proxy-authenticate headers with supported\n+ authentication methods. This is the default.\n+* `basic` - HTTP Basic authentication\n+* `digest` - HTTP Digest authentication; this prevents the password from being\n+ transmitted to the proxy in clear text\n+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option\n+ of `curl(1)`)\n+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)\n+--\n+\n http.cookieFile::\n \tFile containing previously stored cookie lines which should be used\n \tin the Git http session, if they match the server. The file format\n@@ -2390,6 +2411,11 @@ remote..proxy::\n \tthe proxy to use for that remote. Set to the empty string to\n \tdisable proxying for that remote.\n \n+remote..proxyAuthMethod::\n+\tFor remotes that require curl (http, https and ftp), the method to use for\n+\tauthenticating against the proxy in use (probably set in\n+\t`remote..proxy`). See `http.proxyAuthMethod`.\n+\n remote..fetch::\n \tThe default set of \"refspec\" for linkgit:git-fetch[1]. See\n \tlinkgit:git-fetch[1].\ndiff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt\nindex 2cfdd22..f10941b 100644\n--- a/Documentation/technical/api-remote.txt\n+++ b/Documentation/technical/api-remote.txt\n@@ -51,6 +51,10 @@ struct remote\n \n \tThe proxy to use for curl (http, https, ftp, etc.) URLs.\n \n+`http_proxy_authmethod`::\n+\n+\tThe method used for authenticating against `http_proxy`.\n+\n struct remotes can be found by name with remote_get(), and iterated\n through with for_each_remote(). remote_get(NULL) will return the\n default remote, given the current branch and configuration.\ndiff --git a/http.c b/http.c\nindex 7da76ed..a786802 100644\n--- a/http.c\n+++ b/http.c\n@@ -63,6 +63,24 @@ static long curl_low_speed_limit = -1;\n static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n+static const char *http_proxy_authmethod;\n+static struct {\n+\tconst char *name;\n+\tlong curlauth_param;\n+} proxy_authmethods[] = {\n+\t{ \"basic\", CURLAUTH_BASIC },\n+\t{ \"digest\", CURLAUTH_DIGEST },\n+\t{ \"negotiate\", CURLAUTH_GSSNEGOTIATE },\n+\t{ \"ntlm\", CURLAUTH_NTLM },\n+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n+\t{ \"anyauth\", CURLAUTH_ANY },\n+#endif\n+\t/*\n+\t * CURLAUTH_DIGEST_IE has no corresponding command-line option in\n+\t * curl(1) and is not included in CURLAUTH_ANY, so we leave it out\n+\t * here, too\n+\t */\n+};\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -257,6 +275,9 @@ static int http_options(const char *var, const char *value, void *cb)\n \tif (!strcmp(\"http.proxy\", var))\n \t\treturn git_config_string(&curl_http_proxy, var, value);\n \n+\tif (!strcmp(\"http.proxyauthmethod\", var))\n+\t\treturn git_config_string(&http_proxy_authmethod, var, value);\n+\n \tif (!strcmp(\"http.cookiefile\", var))\n \t\treturn git_config_string(&curl_cookie_file, var, value);\n \tif (!strcmp(\"http.savecookies\", var)) {\n@@ -305,6 +326,40 @@ static void init_curl_http_auth(CURL *result)\n #endif\n }\n \n+/* assumes *var is free-able */\n+static void var_override(const char **var, char *value)\n+{\n+\tif (value) {\n+\t\tfree((void*) *var);\n+\t\t*var = xstrdup(value);\n+\t}\n+}\n+\n+static void init_curl_proxy_auth(CURL *result)\n+{\n+\tvar_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n+\n+#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n+\tif (http_proxy_authmethod) {\n+\t\tint i;\n+\t\tfor (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {\n+\t\t\tif (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {\n+\t\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH,\n+\t\t\t\t\t\tproxy_authmethods[i].curlauth_param);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tif (i == ARRAY_SIZE(proxy_authmethods)) {\n+\t\t\twarning(\"unsupported proxy authentication method %s: using anyauth\",\n+\t\t\t\t\thttp_proxy_authmethod);\n+\t\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t\t}\n+\t}\n+\telse\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+#endif\n+}\n+\n static int has_cert_password(void)\n {\n \tif (ssl_cert == NULL || ssl_cert_password_required != 1)\n@@ -466,9 +521,7 @@ static CURL *get_curl_handle(void)\n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t}\n-#if LIBCURL_VERSION_NUM >= 0x070a07\n-\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n-#endif\n+\tinit_curl_proxy_auth(result);\n \n \tset_curl_keepalive(result);\n \n@@ -509,6 +562,9 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \tif (remote && remote->http_proxy)\n \t\tcurl_http_proxy = xstrdup(remote->http_proxy);\n \n+\tif (remote)\n+\t\tvar_override(&http_proxy_authmethod, remote->http_proxy_authmethod);\n+\n \tpragma_header = curl_slist_append(pragma_header, \"Pragma: no-cache\");\n \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n \n@@ -607,6 +663,9 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tfree((void *) http_proxy_authmethod);\n+\thttp_proxy_authmethod = NULL;\n+\n \tif (cert_auth.password != NULL) {\n \t\tmemset(cert_auth.password, 0, strlen(cert_auth.password));\n \t\tfree(cert_auth.password);\ndiff --git a/remote.c b/remote.c\nindex 10f1ffc..0e3bba8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -428,6 +428,9 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t} else if (!strcmp(subkey, \".proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n+\t} else if (!strcmp(subkey, \".proxyauthmethod\")) {\n+\t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n+\t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\ndiff --git a/remote.h b/remote.h\nindex 312b7ca..a221c5a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -54,6 +54,7 @@ struct remote {\n \t * for curl remotes only\n \t */\n \tchar *http_proxy;\n+\tchar *http_proxy_authmethod;\n };\n \n struct remote *remote_get(const char *name);\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272891","messageId":"1446628405-8070-3-git-send-email-k.franke@science-computing.de","threadId":"40647","inReplyTo":"1446628405-8070-1-git-send-email-k.franke@science-computing.de","subject":"[PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-04T09:13:25Z","receivedAt":"2015-11-04T09:13:25Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"Currently, the only way to pass proxy credentials to curl is by including them\nin the proxy URL. Usually, this means they will end up on disk unencrypted, one\nway or another (by inclusion in ~/.gitconfig, shell profile or history). Since\nproxy authentication often uses a domain user, credentials can be security\nsensitive; therefore, a safer way of passing credentials is desirable.\n\nIf the configured proxy contains a username but not a password, query the\ncredential API for one. Also, make sure we approve/reject proxy credentials\nproperly.\n\nFor consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\nenvironment variables, which would otherwise be evaluated as a fallback by curl.\nWithout this, we would have different semantics for git configuration and\nenvironment variables.\n\nSigned-off-by: Knut Franke \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \n---\n Documentation/config.txt | 10 +++++--\n http.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 1 +\n 3 files changed, 79 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4ad7f74..320ce9a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1593,9 +1593,13 @@ help.htmlPath::\n \n http.proxy::\n \tOverride the HTTP proxy, normally configured using the 'http_proxy',\n-\t'https_proxy', and 'all_proxy' environment variables (see\n-\t`curl(1)`). This can be overridden on a per-remote basis; see\n-\tremote..proxy\n+\t'https_proxy', and 'all_proxy' environment variables (see `curl(1)`). In\n+\taddition to the syntax understood by curl, it is possible to specify a\n+\tproxy string with a user name but no password, in which case git will\n+\tattempt to acquire one in the same way it does for other credentials. See\n+\tlinkgit:gitcredentials[7] for more information. The syntax thus is\n+\t'[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n+\ton a per-remote basis; see remote..proxy\n \n http.proxyAuthMethod::\n \tSet the method with which to authenticate against the HTTP proxy. This\ndiff --git a/http.c b/http.c\nindex a786802..714a59a 100644\n--- a/http.c\n+++ b/http.c\n@@ -81,6 +81,8 @@ static struct {\n \t * here, too\n \t */\n };\n+static struct credential proxy_auth = CREDENTIAL_INIT;\n+static const char *curl_proxyuserpwd;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n struct credential http_auth = CREDENTIAL_INIT;\n@@ -178,6 +180,9 @@ static void finish_active_slot(struct active_request_slot *slot)\n #else\n \t\tslot->results->auth_avail = 0;\n #endif\n+\n+\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n+\t\t\t&slot->results->http_connectcode);\n \t}\n \n \t/* Run callback if appropriate */\n@@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)\n \n static void init_curl_proxy_auth(CURL *result)\n {\n+\tif (proxy_auth.username) {\n+\t\tstruct strbuf s = STRBUF_INIT;\n+\t\tif (!proxy_auth.password)\n+\t\t\tcredential_fill(&proxy_auth);\n+#if LIBCURL_VERSION_NUM >= 0x071301\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n+\t\t\tproxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n+\t\t\tproxy_auth.password);\n+#else\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n+\t\tstrbuf_addch(&s, ':');\n+\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n+\t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n+#endif\n+\t}\n+\n \tvar_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n \n #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n@@ -518,8 +541,42 @@ static CURL *get_curl_handle(void)\n \t\tcurl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);\n #endif\n \n+\t/*\n+\t * CURL also examines these variables as a fallback; but we need to query\n+\t * them here in order to decide whether to prompt for missing password (cf.\n+\t * init_curl_proxy_auth()).\n+\t *\n+\t * Unlike many other common environment variables, these are historically\n+\t * lowercase only. It appears that CURL did not know this and implemented\n+\t * only uppercase variants, which was later corrected to take both - with\n+\t * the exception of http_proxy, which is lowercase only also in CURL. As\n+\t * the lowercase versions are the historical quasi-standard, they take\n+\t * precedence here, as in CURL.\n+\t */\n+\tif (!curl_http_proxy) {\n+\t\tif (!strcmp(http_auth.protocol, \"https\")) {\n+\t\t\tvar_override(&curl_http_proxy, getenv(\"HTTPS_PROXY\"));\n+\t\t\tvar_override(&curl_http_proxy, getenv(\"https_proxy\"));\n+\t\t} else {\n+\t\t\tvar_override(&curl_http_proxy, getenv(\"http_proxy\"));\n+\t\t}\n+\t\tif (!curl_http_proxy) {\n+\t\t\tvar_override(&curl_http_proxy, getenv(\"ALL_PROXY\"));\n+\t\t\tvar_override(&curl_http_proxy, getenv(\"all_proxy\"));\n+\t\t}\n+\t}\n+\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tif (strstr(curl_http_proxy, \"://\"))\n+\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\telse {\n+\t\t\tstruct strbuf url = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&url, \"http://%s\", curl_http_proxy);\n+\t\t\tcredential_from_url(&proxy_auth, url.buf);\n+\t\t\tstrbuf_release(&url);\n+\t\t}\n+\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);\n \t}\n \tinit_curl_proxy_auth(result);\n \n@@ -663,6 +720,15 @@ void http_cleanup(void)\n \t\tcurl_http_proxy = NULL;\n \t}\n \n+\tif (proxy_auth.password) {\n+\t\tmemset(proxy_auth.password, 0, strlen(proxy_auth.password));\n+\t\tfree(proxy_auth.password);\n+\t\tproxy_auth.password = NULL;\n+\t}\n+\n+\tfree((void *)curl_proxyuserpwd);\n+\tcurl_proxyuserpwd = NULL;\n+\n \tfree((void *) http_proxy_authmethod);\n \thttp_proxy_authmethod = NULL;\n \n@@ -994,6 +1060,8 @@ static int handle_curl_result(struct slot_results *results)\n \n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n+\t\tif (proxy_auth.password)\n+\t\t\tcredential_approve(&proxy_auth);\n \t\treturn HTTP_OK;\n \t} else if (missing_target(results))\n \t\treturn HTTP_MISSING_TARGET;\n@@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n+\t\tif (results->http_connectcode == 407)\n+\t\t\tcredential_reject(&proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x070c00\n \t\tif (!curl_errorstr[0])\n \t\t\tstrlcpy(curl_errorstr,\ndiff --git a/http.h b/http.h\nindex 49afe39..7352a9e 100644\n--- a/http.h\n+++ b/http.h\n@@ -54,6 +54,7 @@ struct slot_results {\n \tCURLcode curl_result;\n \tlong http_code;\n \tlong auth_avail;\n+\tlong http_connectcode;\n };\n \n struct active_request_slot {\n-- \n2.3.7\n\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272902","messageId":"CAPig+cS9PtibBQYAPXmG1GEBAMfd741FApYo8YnsNeVzw-Wt-A@mail.gmail.com","threadId":"40647","inReplyTo":"1446628405-8070-3-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-04T19:41:13Z","receivedAt":"2015-11-04T19:41:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wednesday, November 4, 2015, Knut Franke\n wrote:\n> Currently, the only way to pass proxy credentials to curl is by including them\n> in the proxy URL. Usually, this means they will end up on disk unencrypted, one\n> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since\n> proxy authentication often uses a domain user, credentials can be security\n> sensitive; therefore, a safer way of passing credentials is desirable.\n>\n> If the configured proxy contains a username but not a password, query the\n> credential API for one. Also, make sure we approve/reject proxy credentials\n> properly.\n>\n> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\n> environment variables, which would otherwise be evaluated as a fallback by curl.\n> Without this, we would have different semantics for git configuration and\n> environment variables.\n>\n> Signed-off-by: Knut Franke \n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1593,9 +1593,13 @@ help.htmlPath::\n>\n> http.proxy::\n> Override the HTTP proxy, normally configured using the 'http_proxy',\n> - 'https_proxy', and 'all_proxy' environment variables (see\n> - `curl(1)`). This can be overridden on a per-remote basis; see\n> - remote..proxy\n> + 'https_proxy', and 'all_proxy' environment variables (see `curl(1)`). In\n> + addition to the syntax understood by curl, it is possible to specify a\n> + proxy string with a user name but no password, in which case git will\n> + attempt to acquire one in the same way it does for other credentials. See\n> + linkgit:gitcredentials[7] for more information. The syntax thus is\n> + '[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden\n> + on a per-remote basis; see remote..proxy\n\ns/$/./ maybe?\n\n> http.proxyAuthMethod::\n> Set the method with which to authenticate against the HTTP proxy. This\n> diff --git a/http.c b/http.c\n> @@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)\n>\n> static void init_curl_proxy_auth(CURL *result)\n> {\n> + if (proxy_auth.username) {\n> + struct strbuf s = STRBUF_INIT;\n> + if (!proxy_auth.password)\n> + credential_fill(&proxy_auth);\n> +#if LIBCURL_VERSION_NUM >= 0x071301\n> + curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n> + proxy_auth.username);\n> + curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n> + proxy_auth.password);\n\nThe strbuf does not get released in this #if branch, but since no\ncontent was added, it doesn't need to be released, thus nothing is\nleaked. Good.\n\nDoes the compiler warn about unused variable 's' in this #if branch?\n\n> +#else\n> + strbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n> + strbuf_addch(&s, ':');\n> + strbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n> + curl_proxyuserpwd = strbuf_detach(&s, NULL);\n> + curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n\nAnd this branch detaches content from the strbuf, and that memory is\nreleased later in http_cleanup(), so also nothing is leaked, thus all\nis good.\n\n> +#endif\n> + }\n> +\n> var_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n>\n> #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n"},{"id":"272903","messageId":"xmqqlhada6v8.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1446628405-8070-2-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 1/2] http: allow selection of proxy authentication method","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T19:42:51Z","receivedAt":"2015-11-04T19:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> diff --git a/http.c b/http.c\n> index 7da76ed..a786802 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -305,6 +326,40 @@ static void init_curl_http_auth(CURL *result)\n> #endif\n> }\n> \n> +/* assumes *var is free-able */\n\nThis is not just \"assumes\", but it is wrong for the callers to pass\nunfreeable memory, so\n\n\t/* *var must be free-able */\n\n> +static void var_override(const char **var, char *value)\n> +{\n> +\tif (value) {\n> +\t\tfree((void*) *var);\n\nThere may be a similar whitespace damage but I happened to notice\nthis one.\n\n\tfree((void *)*var);\n\n> +static void init_curl_proxy_auth(CURL *result)\n> +{\n> +\tvar_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n\nIf your libcurl does not understand CURLOPT_PROXYAUTH, do you need\nto do this var_override()? Shouldn't this be inside the #if..#endif\nbelow?\n\n> +\n> +#if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */\n> +\tif (http_proxy_authmethod) {\n> +...\n> +\telse\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +#endif\n> +}\n\nOther than that, looks cleanly done. Thanks.\n"},{"id":"272906","messageId":"xmqqd1vpa6d1.fsf@gitster.mtv.corp.google.com","threadId":"40647","inReplyTo":"1446628405-8070-3-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-04T19:53:46Z","receivedAt":"2015-11-04T19:53:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Knut Franke writes:\n\n> @@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)\n> \n> static void init_curl_proxy_auth(CURL *result)\n> {\n> +\tif (proxy_auth.username) {\n> +\t\tstruct strbuf s = STRBUF_INIT;\n\nHaving this variable triggers compilation error with newer libcurl\nversion as it is only used in the #else clause X-<.\n\n> +\t\tif (!proxy_auth.password)\n> +\t\t\tcredential_fill(&proxy_auth);\n> +#if LIBCURL_VERSION_NUM >= 0x071301\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n> +\t\t\tproxy_auth.username);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n> +\t\t\tproxy_auth.password);\n> +#else\n> +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n> +\t\tstrbuf_addch(&s, ':');\n> +\t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n> +\t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n> +#endif\n> +\t}\n\nIt is probably easier to follow the flow of the logic of the primary\ninterface (i.e. init_curl_proxy_auth()) if you split this part into\nits own helper function that deals with implementation detail, e.g.\n\n http.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 1f269b0..fad2ba5 100644\n--- a/http.c\n+++ b/http.c\n@@ -340,24 +340,30 @@ static void var_override(const char **var, char *value)\n \t}\n }\n \n-static void init_curl_proxy_auth(CURL *result)\n+static void set_proxyauth_name_password(CURL *result)\n {\n-\tif (proxy_auth.username) {\n-\t\tstruct strbuf s = STRBUF_INIT;\n-\t\tif (!proxy_auth.password)\n-\t\t\tcredential_fill(&proxy_auth);\n #if LIBCURL_VERSION_NUM >= 0x071301\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME,\n \t\t\tproxy_auth.username);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD,\n \t\t\tproxy_auth.password);\n #else\n+\t\tstruct strbuf s = STRBUF_INIT;\n+\n \t\tstrbuf_addstr_urlencode(&s, proxy_auth.username, 1);\n \t\tstrbuf_addch(&s, ':');\n \t\tstrbuf_addstr_urlencode(&s, proxy_auth.password, 1);\n \t\tcurl_proxyuserpwd = strbuf_detach(&s, NULL);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);\n #endif\n+}\n+\n+static void init_curl_proxy_auth(CURL *result)\n+{\n+\tif (proxy_auth.username) {\n+\t\tif (!proxy_auth.password)\n+\t\t\tcredential_fill(&proxy_auth);\n+\t\tset_proxyauth_name_password(result);\n \t}\n \n \tvar_override(&http_proxy_authmethod, getenv(\"GIT_HTTP_PROXY_AUTHMETHOD\"));\n"},{"id":"272950","messageId":"20151105082421.GB6819@sigill.intra.peff.net","threadId":"40647","inReplyTo":"1446628405-8070-3-git-send-email-k.franke@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-05T08:24:21Z","receivedAt":"2015-11-05T08:24:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 04, 2015 at 10:13:25AM +0100, Knut Franke wrote:\n\n> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy\n> environment variables, which would otherwise be evaluated as a fallback by curl.\n> Without this, we would have different semantics for git configuration and\n> environment variables.\n\nI can't say I'm excited about this, but I don't think there's a better\nway.\n\nThere was a series similar to yours in 2012, and it ran into the same\nproblems. There's sadly no good way to ask curl \"what is the proxy you\nended up using?\".\n\nThere was also some discussion with curl upstream of providing a new\nauthentication interface, where we would provide curl with\nauthentication callbacks, and it would trigger them if and when\ncredentials were needed. Somebody upstream was working on a patch, but I\ndon't think it ever got merged. :(\n\nHere's a relevant bit from that old series (which doesn't seem threaded,\nbut you can search for the author if you want to see more):\n\n http://thread.gmane.org/gmane.comp.version-control.git/192246\n\nI have a few comments/questions below.\n\n> +\n> +\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n> +\t\t\t&slot->results->http_connectcode);\n\nIt looks like you use this to see the remote side's HTTP 407 code. In\nthe 2012 series, I think we simply looked for a 407 in the HTTP return\ncode (I assume that if we fail in the CONNECT, we can't get any other\nHTTP code and so curl just returns the proxy code).\n\nI don't have a proxy to test against, but would that work (it's entirely\npossible the other series was simply wrong)?\n\nIf we do need CONNECTCODE, do we need to protect it with an #ifdef on\nthe curl version? The manpage says it came in 7.10.7, which was released\nin 2003. That's probably old enough not to worry about.\n\n> +\tif (proxy_auth.password) {\n> +\t\tmemset(proxy_auth.password, 0, strlen(proxy_auth.password));\n> +\t\tfree(proxy_auth.password);\n\nMy understanding is that memset() like this is not sufficient for\nzero-ing sensitive data, as they can be optimized out by the compiler. I\ndon't think there's a portable alternative, though, so it may be the\nbest we can do. OTOH, the rest of git does not worry about such zero-ing\nanyway, so we could also simply omit it here.\n\n> +\tfree((void *)curl_proxyuserpwd);\n\nThis cast is necessary because curl_proxyuserpwd is declared const. But\nI do not see anywhere that it needs to be const (we detach a strbuf into\nit). Can we simply change the declaration?\n\nFor that matter, it is not clear to me why this needs to be a global at\nall. Once we hand the value to curl_easy_setopt, curl keeps its own\ncopy.\n\n> \tfree((void *) http_proxy_authmethod);\n\nThis one unfortunately does need to remain const, as it is used with\ngit_config_string (though given the number of void casts necessary in\npatch 1, it may be less painful to simply cast it in the call to\ngit_config_string()).\n\n> @@ -994,6 +1060,8 @@ static int handle_curl_result(struct slot_results *results)\n> \n> \tif (results->curl_result == CURLE_OK) {\n> \t\tcredential_approve(&http_auth);\n> +\t\tif (proxy_auth.password)\n> +\t\t\tcredential_approve(&proxy_auth);\n> \t\treturn HTTP_OK;\n\nApproving on success. Makes sense. You can drop the conditional here;\ncredential_approve() is a noop if the password isn't set.\n\n> @@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)\n> \t\t\treturn HTTP_REAUTH;\n> \t\t}\n> \t} else {\n> +\t\tif (results->http_connectcode == 407)\n> +\t\t\tcredential_reject(&proxy_auth);\n\nRejecting on a 407 makes sense (though again, can we check\nresults->http_code?). But if we get a 407 and we _don't_ have a\npassword, shouldn't we then prompt for one, similar to what we do with a\n401?\n\nThat will require some refactoring around http_request_reauth, though\n(because now we might potentially retry twice: once to get past the\nproxy auth, and once to get past the real site's auth).\n\nYou prompt unconditionally for the password earlier, but only if the\nproxy URL contains a username. We used to do the same thing for regular\nhttp, but people got annoyed that they had to specify half the\ncredential in the URL. Perhaps it would be less so with proxies (which\nare changed a lot less), so I don't think making this work is an\nabsolute requirement.\n\n-Peff\n"},{"id":"272954","messageId":"20151105115654.GA6786@science-computing.de","threadId":"40647","inReplyTo":"20151105082421.GB6819@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Knut Franke","fromEmail":"k.franke@science-computing.de","sentAt":"2015-11-05T11:56:54Z","receivedAt":"2015-11-05T11:56:54Z","isPatch":true,"sender":{"key":"knut.franke@gmx.de","avatar":null},"body":"On 2015-11-05 03:24, Jeff King wrote:\n> There was also some discussion with curl upstream of providing a new\n> authentication interface, where we would provide curl with\n> authentication callbacks, and it would trigger them if and when\n> credentials were needed. Somebody upstream was working on a patch, but I\n> don't think it ever got merged. :(\n\nThat would certainly be nice, also with respect to other credentials, such as\nSSL key passphrase (presuming that'd be possible without modifying the SSL lib\nas well).\n\n> Here's a relevant bit from that old series (which doesn't seem threaded,\n> but you can search for the author if you want to see more):\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/192246\n\nMy main takeaway from this, apart from the points you mention below, is that\nit'd be good to have a test case, similar to t/lib-httpd.sh. Since none of the\nexistent proxy-related code has an automated test, I think this would be an\nimprovement on top of the other patches. I'd need to look into how easy/hard\nthis would be to implement.\n\n> > +\n> > +\t\tcurl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,\n> > +\t\t\t&slot->results->http_connectcode);\n> \n> It looks like you use this to see the remote side's HTTP 407 code. In\n> the 2012 series, I think we simply looked for a 407 in the HTTP return\n> code\n\nI'm not sure why that worked for the author of the old series - possibly curl\nsemantics changed at some point. In my setup at least (with curl 7.15.5), after\na failed proxy authentication, CURLINFO_HTTP_CODE returns 0 while\nCURLINFO_HTTP_CONNECTCODE returns the 407. This is also consistent with the curl\ndocumentation for CURLINFO_RESPONSE_CODE (which has replaced CURLINFO_HTTP_CODE\nin 7.10.7, though the compatibility #define is still there): \"Note that a\nproxy's CONNECT response should be read with CURLINFO_HTTP_CONNECTCODE and not\nthis.\"\n\n> If we do need CONNECTCODE, do we need to protect it with an #ifdef on\n> the curl version? The manpage says it came in 7.10.7, which was released\n> in 2003. That's probably old enough not to worry about.\n\nAs Junio pointed out earlier, since some people still care about ancient curl\nversions, we don't want to knowingly break compatibility. So yes, an #ifdef\nwould be in oder here.\n\n> \n> > +\tif (proxy_auth.password) {\n> > +\t\tmemset(proxy_auth.password, 0, strlen(proxy_auth.password));\n> > +\t\tfree(proxy_auth.password);\n> \n> My understanding is that memset() like this is not sufficient for\n> zero-ing sensitive data, as they can be optimized out by the compiler. I\n> don't think there's a portable alternative, though, so it may be the\n> best we can do. OTOH, the rest of git does not worry about such zero-ing\n> anyway, so we could also simply omit it here.\n\nFor what it's worth, that's the same as we do for cert_auth (while, as far as I\ncan see, no attempt is made for http_auth). I tend to think it's better than\nnothing. Maybe an in-code comment stating it's not reliable would be in order,\nto prevent the passing reader from putting too much trust in it.\n\n> > +\tfree((void *)curl_proxyuserpwd);\n> \n> This cast is necessary because curl_proxyuserpwd is declared const. But\n> I do not see anywhere that it needs to be const (we detach a strbuf into\n> it). Can we simply change the declaration?\n\nRight.\n\n> For that matter, it is not clear to me why this needs to be a global at\n> all. Once we hand the value to curl_easy_setopt, curl keeps its own\n> copy.\n\nThat's true only for relatively recent curl versions; before 7.17.0, strings\nwere not copied.\n\n> > @@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)\n> > \t\t\treturn HTTP_REAUTH;\n> > \t\t}\n> > \t} else {\n> > +\t\tif (results->http_connectcode == 407)\n> > +\t\t\tcredential_reject(&proxy_auth);\n> \n> Rejecting on a 407 makes sense (though again, can we check\n> results->http_code?). But if we get a 407 and we _don't_ have a\n> password, shouldn't we then prompt for one, similar to what we do with a\n> 401?\n> \n> That will require some refactoring around http_request_reauth, though\n> (because now we might potentially retry twice: once to get past the\n> proxy auth, and once to get past the real site's auth).\n\nI think this would also require changes to post_rpc in remote-curl.c, which\napparently does something similar to http_request_reauth. Probably something\nalong the lines of adding a HTTP_PROXY_REAUTH return code, plus some refactoring\nin order to prevent code duplication between the different code parts handling\n(proxy) reauth. :-/\n\n> You prompt unconditionally for the password earlier, but only if the\n> proxy URL contains a username. We used to do the same thing for regular\n> http, but people got annoyed that they had to specify half the\n> credential in the URL. Perhaps it would be less so with proxies (which\n> are changed a lot less), so I don't think making this work is an\n> absolute requirement.\n\nAs far as I understand, the issue was around unconditionally prompting for the\npassword even if it was listed in ~/.netrc. As far as I can see, curl doesn't\nread ~/.netrc for proxy credentials, so I don't think it would make a difference\nhere.\n\n\nThanks,\nKnut\n-- \nVorstandsvorsitzender/Chairman of the board of management:\nGerd-Lothar Leonhart\nVorstand/Board of Management:\nDr. Bernd Finkbeiner, Dr. Arno Steitz\nVorsitzender des Aufsichtsrats/\nChairman of the Supervisory Board:\nPhilippe Miltin\nSitz/Registered Office: Tuebingen\nRegistergericht/Registration Court: Stuttgart\nRegisternummer/Commercial Register No.: HRB 382196\n"},{"id":"272959","messageId":"20151105173021.GA2168@sigill.intra.peff.net","threadId":"40647","inReplyTo":"20151105115654.GA6786@science-computing.de","subject":"Re: [PATCH 2/2] http: use credential API to handle proxy authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-05T17:30:22Z","receivedAt":"2015-11-05T17:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 05, 2015 at 12:56:54PM +0100, Knut Franke wrote:\n\n> My main takeaway from this, apart from the points you mention below, is that\n> it'd be good to have a test case, similar to t/lib-httpd.sh. Since none of the\n> existent proxy-related code has an automated test, I think this would be an\n> improvement on top of the other patches. I'd need to look into how easy/hard\n> this would be to implement.\n\nYeah, tests would be wonderful. I think the main challenge will be\nconfiguring Apache as a proxy (and failing gracefully when mod_proxy is\nnot available).\n\nIf there's another proxy that is easy to configure for a one-shot test,\nthat would be fine, too. It's nice if it's something that's commonly\navailable, though, so more people can actually run the test.\n\n> > It looks like you use this to see the remote side's HTTP 407 code. In\n> > the 2012 series, I think we simply looked for a 407 in the HTTP return\n> > code\n> \n> I'm not sure why that worked for the author of the old series - possibly curl\n> semantics changed at some point.\n\nIt's not clear to me that the original _did_ work in all cases. So I'll\ntrust your experiments now much more than that old thread. :)\n\n> > My understanding is that memset() like this is not sufficient for\n> > zero-ing sensitive data, as they can be optimized out by the compiler. I\n> > don't think there's a portable alternative, though, so it may be the\n> > best we can do. OTOH, the rest of git does not worry about such zero-ing\n> > anyway, so we could also simply omit it here.\n> \n> For what it's worth, that's the same as we do for cert_auth (while, as far as I\n> can see, no attempt is made for http_auth). I tend to think it's better than\n> nothing. Maybe an in-code comment stating it's not reliable would be in order,\n> to prevent the passing reader from putting too much trust in it.\n\nRather than just a comment, can we do something like:\n\n void clear_password(void *buf, size_t len)\n {\n\t/*\n\t * TODO: This is known to be insufficient, but perhaps better\n\t * than nothing, and at least portable. We should use a more\n\t * secure variant on systems that provide it.\n\t */\n\tmemset(buf, 0, len);\n }\n\nThat will make it easier to find such sites and improve them later\n(adding this function in a separate translation unit might actually be\nenough to make it work, as the compiler cannot omit a call to the opaque\nclear_password, and clear_password itself does not know the results will\nnot be used).\n\n> > For that matter, it is not clear to me why this needs to be a global at\n> > all. Once we hand the value to curl_easy_setopt, curl keeps its own\n> > copy.\n> \n> That's true only for relatively recent curl versions; before 7.17.0, strings\n> were not copied.\n\nYeah, I remembered that, but I thought for some reason it was old enough\nthat we didn't need to worry about it. I have a feeling that there may\nbe other places where we do not handle it that well.\n\n7.17.0 is from 2007. I wonder if it is time we bumped our minimum\nrequired curl version. Supporting older installations is nice, but at\nsome point it is not really helping anybody, and that has to be\nbalanced by the increase in code complexity (and especially we are not\nhelping those people if there are subtle bugs that nobody else is\nexercising).\n\nThat's a separate topic from your patch, though (though I would not mind\nat all if you wanted to work on it :) ).\n\n> > That will require some refactoring around http_request_reauth, though\n> > (because now we might potentially retry twice: once to get past the\n> > proxy auth, and once to get past the real site's auth).\n> \n> I think this would also require changes to post_rpc in remote-curl.c, which\n> apparently does something similar to http_request_reauth. Probably something\n> along the lines of adding a HTTP_PROXY_REAUTH return code, plus some refactoring\n> in order to prevent code duplication between the different code parts handling\n> (proxy) reauth. :-/\n\nYeah, that would work. I think we could also just loop on HTTP_REAUTH.\nThe code in handle_curl_result that returns HTTP_REAUTH will only do so\nif it looks like we could make progress by trying again.\n\n> > You prompt unconditionally for the password earlier, but only if the\n> > proxy URL contains a username. We used to do the same thing for regular\n> > http, but people got annoyed that they had to specify half the\n> > credential in the URL. Perhaps it would be less so with proxies (which\n> > are changed a lot less), so I don't think making this work is an\n> > absolute requirement.\n> \n> As far as I understand, the issue was around unconditionally prompting for the\n> password even if it was listed in ~/.netrc. As far as I can see, curl doesn't\n> read ~/.netrc for proxy credentials, so I don't think it would make a difference\n> here.\n\nThe .netrc thing came up recently-ish, but the HTTP prompting issues are\nmuch older than that. Basically, does:\n\n git config http.proxy http://example.com:8080\n\nwork out of the box if example.com requires a username and password? I\nthink with your patch it doesn't. You need to use\n\"http://user@example.com:8080\" to convince git to prompt at all.\n\n-Peff\n"}]}