Thread (18 messages) flat view 18 messages, 3 authors, 2016-06-15

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

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

On Wednesday, October 28, 2015, Knut Franke
[off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted>
---
diff --git a/http.c b/http.c
index 4756bab..11bebe1 100644
--- a/http.c
+++ b/http.c
@@ -79,6 +79,7 @@ static struct {
        // curl(1) and is not included in CURLAUTH_ANY, so we leave it out
        // here, too
 };
+struct credential http_proxy_auth = CREDENTIAL_INIT;
s/^/static/
quoted hunk ↗ jump to hunk
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot *slot)
 #else
                slot->results->auth_avail = 0;
 #endif
+
+               curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+                       &slot->results->http_connectcode);
        }

        /* Run callback if appropriate */
@@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char *envname)

 static void init_curl_proxy_auth(CURL *result)
 {
+       if (http_proxy_auth.username) {
+               if (!http_proxy_auth.password) {
+                       credential_fill(&http_proxy_auth);
+               }
Style: drop unnecessary braces
+#if LIBCURL_VERSION_NUM >= 0x071301
+               curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+                       http_proxy_auth.username);
+               curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+                       http_proxy_auth.password);
+#else
+               struct strbuf up = STRBUF_INIT;
Minor: It took me a moment to figure out that "up" meant
user-password. I wonder if a simpler name such as 's' would suffice?
+               strbuf_reset(&up);
Unnecessary strbuf_reset().
+               strbuf_addstr_urlencode(&up, http_proxy_auth.username, 1);
+               strbuf_addch(&up, ':');
+               strbuf_addstr_urlencode(&up, http_proxy_auth.password, 1);
+               curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);
Leaking 'up'. Insert strbuf_release(&up) here.
quoted hunk ↗ jump to hunk
+#endif
+       }
+
        copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

        if (http_proxy_authmethod) {
@@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)
                curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif

+       /*
+        * curl also examines these variables as a fallback; but we need to query
+        * them here in order to decide whether to prompt for missing password (cf.
+        * init_curl_proxy_auth()).
+        */
+       if (!curl_http_proxy) {
+               if (!strcmp(http_auth.protocol, "https")) {
+                       copy_from_env(&curl_http_proxy, "HTTPS_PROXY");
+                       copy_from_env(&curl_http_proxy, "https_proxy");
+               } else {
+                       copy_from_env(&curl_http_proxy, "http_proxy");
To the casual reader, it's not obvious why you check upper- and
lowercase versions of the other environment variables but not this
one.
+               }
+               if (!curl_http_proxy) {
+                       copy_from_env(&curl_http_proxy, "ALL_PROXY");
+                       copy_from_env(&curl_http_proxy, "all_proxy");
+               }
If this sort of upper- and lowercase environment variable name
checking is indeed desirable, I wonder if it would make sense to fold
that functionality into the helper function.
+       }
+
        if (curl_http_proxy) {
-               curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+               if (strstr(curl_http_proxy, "://"))
+                       credential_from_url(&http_proxy_auth, curl_http_proxy);
+               else {
+                       struct strbuf url = STRBUF_INIT;
+                       strbuf_reset(&url);
Unnecessary strbuf_reset().
+                       strbuf_addstr(&url, "http://");
+                       strbuf_addstr(&url, curl_http_proxy);
strbuf_addf(&url, "http://%s", curl_http_proxy) might be more straightforward.
+                       credential_from_url(&http_proxy_auth, url.buf);
Leaking 'url' here. Insert strbuf_release(&url).
+               }
+
+               curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);
        }
        init_curl_proxy_auth(result);
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help