Re: [PATCH v5 2/5] http: handle proxy proactive authentication
- From
Jeff King <peff@peff.net>
- Date
- Apr 13, 2012, 20:23 UTC
- Message-ID
- <20120413202359.GA5962@sigill.intra.peff.net>
- In-Reply-To
- <7viph32znu.fsf@alter.siamese.dyndns.org>
On Fri, Apr 13, 2012 at 12:35:17PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > It looks like OS X defines a SOCKS type and an HTTPProxy type for its > > keychain API. So in either case, it should probably be updated to handle > > these new types. And I guess that argues for making the distinction, > > since at least one helper does want to care about it. > > OK. Sounds like we are in agreement. > > Nelson, care to re-roll the series, with fixes discussed in this thread > rolled into the second patch?
I think there is a bug in the patch you posted, though:
Show 16 quoted lines
> diff --git a/http.c b/http.c
> index 1c71edb..752b6ea 100644
> --- a/http.c
> +++ b/http.c
> @@ -336,14 +336,18 @@ static CURL *get_curl_handle(const char *url)
> if (curl_http_proxy) {
> struct strbuf proxyhost = STRBUF_INIT;
>
> - if (!proxy_auth.host) /* check to parse only once */
> - credential_from_url(&proxy_auth, curl_http_proxy);
> + if (!proxy_auth.host) {
> + const char *cp;
> + cp = strstr(curl_http_proxy, "://");
> + cp = cp ? (cp + 3) : curl_http_proxy;
> + credential_for_destination(&proxy_auth, cp, "http-proxy");
> + }What happens if the URL starts with "socks://"? We would want to preserve that, and have our protocol end as "socks://".
Show 7 quoted lines
> if (http_proactive_auth && proxy_auth.username && !proxy_auth.password) > /* proxy string has username but no password, ask for password */ > credential_fill(&proxy_auth); > > - strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, proxy_auth.host); > + strbuf_addstr(&proxyhost, proxy_auth.host); > curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));
Same here. If we get "socks://127.0.0.1", we will feed "127.0.0.1" to curl, which will then assume that it's http.
BTW, do we actually need to strip the username out of the URL? We do not do so with regular URLs, and curl takes the auth we give it over what is in the URL. Can this be simplified to just:
curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
?
At any rate, I think the rule we want for parsing the url is to actually parse the protocol from the url, and if it's not there, assume it's http. And then convert all "http" to "http-proxy", because this is a weird alternate use of "http". So we can just take your old patch to relax the credential_from_url parsing, and then fix it up on the calling side. Like this:
diff --git a/credential.c b/credential.c index 62d1c56..813e3cf 100644 --- a/credential.c +++ b/credential.c @@ -324,11 +324,15 @@ void credential_from_url(struct credential *c, const char *url) * (1) proto://<host>/... * (2) proto://<user>@<host>/... * (3) proto://<user>:<pass>@<host>/... + * or "proto://"-less variants of the above. They are not technically + * URLs, but the caller may have some context-specific knowledge about + * what protocol is in use. */ proto_end = strstr(url, "://"); - if (!proto_end) - return; - cp = proto_end + 3; + if (proto_end) + cp = proto_end + 3; + else + cp = url; at = strchr(cp, '@'); colon = strchr(cp, ':'); slash = strchrnul(cp, '/'); @@ -348,7 +352,7 @@ void credential_from_url(struct credential *c, const char *url) host = at + 1; } - if (proto_end - url > 0) + if (proto_end && proto_end != url) c->protocol = xmemdupz(url, proto_end - url); if (slash - host > 0) c->host = url_decode_mem(host, slash - host); diff --git a/http.c b/http.c index 1c71edb..a164f79 100644 --- a/http.c +++ b/http.c @@ -334,17 +334,20 @@ static CURL *get_curl_handle(const char *url) free(env_proxy_var); } if (curl_http_proxy) { - struct strbuf proxyhost = STRBUF_INIT; - - if (!proxy_auth.host) /* check to parse only once */ + if (!proxy_auth.host) { credential_from_url(&proxy_auth, curl_http_proxy); + if (!proxy_auth.protocol || + !strcmp(proxy_auth.protocol, "http")) { + free(proxy_auth.protocol); + proxy_auth.protocol = xstrdup("http-proxy"); + } + } if (http_proactive_auth && proxy_auth.username && !proxy_auth.password) /* proxy string has username but no password, ask for password */ credential_fill(&proxy_auth); - strbuf_addf(&proxyhost, "%s://%s", proxy_auth.protocol, proxy_auth.host); - curl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL)); + curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy); curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY); set_proxy_auth(result); } Note that curl will treat "foobar://" (or any protocol it does not understand) as an http proxy. I didn't want to get into white-listing "socks:// is ok, but foobar:// is really just http in disguise" based on curl's internal rules. So you can use "foobar://" if you want, but it's not going to share credentials with "http://" (even though curl will use them in exactly the same way). -Peff