Re: [PATCH 2/2] http: use credential API to handle proxy authentication
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Nov 4, 2015, 19:41 UTC
- Message-ID
- <CAPig+cS9PtibBQYAPXmG1GEBAMfd741FApYo8YnsNeVzw-Wt-A@mail.gmail.com>
- In-Reply-To
- <1446628405-8070-3-git-send-email-k.franke@science-computing.de>
On Wednesday, November 4, 2015, Knut Franke <k.franke@science-computing.de> wrote:
Show 34 quoted lines
> Currently, the only way to pass proxy credentials to curl is by including them > in the proxy URL. Usually, this means they will end up on disk unencrypted, one > way or another (by inclusion in ~/.gitconfig, shell profile or history). Since > proxy authentication often uses a domain user, credentials can be security > sensitive; therefore, a safer way of passing credentials is desirable. > > If the configured proxy contains a username but not a password, query the > credential API for one. Also, make sure we approve/reject proxy credentials > properly. > > For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy > environment variables, which would otherwise be evaluated as a fallback by curl. > Without this, we would have different semantics for git configuration and > environment variables. > > Signed-off-by: Knut Franke <k.franke@science-computing.de> > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -1593,9 +1593,13 @@ help.htmlPath:: > > http.proxy:: > Override the HTTP proxy, normally configured using the 'http_proxy', > - 'https_proxy', and 'all_proxy' environment variables (see > - `curl(1)`). This can be overridden on a per-remote basis; see > - remote.<name>.proxy > + 'https_proxy', and 'all_proxy' environment variables (see `curl(1)`). In > + addition to the syntax understood by curl, it is possible to specify a > + proxy string with a user name but no password, in which case git will > + attempt to acquire one in the same way it does for other credentials. See > + linkgit:gitcredentials[7] for more information. The syntax thus is > + '[protocol://][user[:password]@]proxyhost[:port]'. This can be overridden > + on a per-remote basis; see remote.<name>.proxy
s/$/./ maybe?
Show 16 quoted lines
> http.proxyAuthMethod::
> Set the method with which to authenticate against the HTTP proxy. This
> diff --git a/http.c b/http.c
> @@ -337,6 +342,24 @@ static void var_override(const char **var, char *value)
>
> static void init_curl_proxy_auth(CURL *result)
> {
> + if (proxy_auth.username) {
> + struct strbuf s = STRBUF_INIT;
> + if (!proxy_auth.password)
> + credential_fill(&proxy_auth);
> +#if LIBCURL_VERSION_NUM >= 0x071301
> + curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> + proxy_auth.username);
> + curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> + proxy_auth.password);The strbuf does not get released in this #if branch, but since no content was added, it doesn't need to be released, thus nothing is leaked. Good.
Does the compiler warn about unused variable 's' in this #if branch?
Show 6 quoted lines
> +#else > + strbuf_addstr_urlencode(&s, proxy_auth.username, 1); > + strbuf_addch(&s, ':'); > + strbuf_addstr_urlencode(&s, proxy_auth.password, 1); > + curl_proxyuserpwd = strbuf_detach(&s, NULL); > + curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, curl_proxyuserpwd);
And this branch detaches content from the strbuf, and that memory is released later in http_cleanup(), so also nothing is leaked, thus all is good.
Show 6 quoted lines
> +#endif
> + }
> +
> var_override(&http_proxy_authmethod, getenv("GIT_HTTP_PROXY_AUTHMETHOD"));
>
> #if LIBCURL_VERSION_NUM >= 0x070a07 /* CURLOPT_PROXYAUTH and CURLAUTH_ANY */