{"thread":{"id":"29937","subject":"[PATCH v5 2/5] http: handle proxy proactive authentication","startedAt":"2012-03-13T14:03:54Z","lastAt":"2012-04-19T17:09:14Z","messageCount":13,"participants":["Nelson Benitez Leon","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":5,"patchTotal":5},"messages":[{"id":"186856","messageId":"4F5F53CA.7090003@seap.minhap.es","threadId":"29937","inReplyTo":null,"subject":"[PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-13T14:03:54Z","receivedAt":"2012-03-13T14:03:54Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"If http_proactive_auth flag is set and there is a username\nbut no password in the proxy url, then interactively ask for\nthe password.\n\nThis makes possible to not have the password written down in\nhttp_proxy env var or in http.proxy config option.\n\nAlso take care that CURLOPT_PROXY don't include username or\npassword, as we now set them in the new set_proxy_auth() function\nwhere we use their specific cURL options.\n\nSigned-off-by: Nelson Benitez Leon \n---\n http.c | 27 ++++++++++++++++++++++++++-\n 1 files changed, 26 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex be88acb..e7410f8 100644\n--- a/http.c\n+++ b/http.c\n@@ -44,6 +44,7 @@ static const char *curl_http_proxy;\n static const char *curl_cookie_file;\n static struct credential cre_url = CREDENTIAL_INIT;\n static struct credential http_auth = CREDENTIAL_INIT;\n+static struct credential proxy_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n \n@@ -233,6 +234,20 @@ static int has_cert_password(void)\n \treturn 1;\n }\n \n+static void set_proxy_auth(CURL *result)\n+{\n+\tif (proxy_auth.username && proxy_auth.password) {\n+#if LIBCURL_VERSION_NUM >= 0x071901\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);\n+#else\n+\t\tstruct strbuf userpwd = STRBUF_INIT;\n+\t\tstrbuf_addf(&userpwd, \"%s:%s\", proxy_auth.username, proxy_auth.password);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERPWD, strbuf_detach(&userpwd, NULL));\n+#endif\n+\t}\n+}\n+\n static CURL *get_curl_handle(const char *url)\n {\n \tCURL *result = curl_easy_init();\n@@ -317,8 +332,18 @@ static CURL *get_curl_handle(const char *url)\n \t\tfree(env_proxy_var);\n \t}\n \tif (curl_http_proxy) {\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n+\t\tif (!proxy_auth.host) /* check to parse only once */\n+\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\n+\t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n+\t\t\t/* proxy string has username but no password, ask for password */\n+\t\t\tcredential_fill(&proxy_auth);\n+\n+\t\tstruct strbuf proxyhost = STRBUF_INIT;\n+\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n+\t\tset_proxy_auth(result);\n \t}\n \n \treturn result;\n-- \n1.7.7.6\n"},{"id":"188820","messageId":"7v398cvb30.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"4F5F53CA.7090003@seap.minhap.es","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-09T21:39:47Z","receivedAt":"2012-04-09T21:39:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> \tif (curl_http_proxy) {\n> -\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n> +\t\tif (!proxy_auth.host) /* check to parse only once */\n> +\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\n> +\t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n> +\t\t\t/* proxy string has username but no password, ask for password */\n> +\t\t\tcredential_fill(&proxy_auth);\n> +\n> +\t\tstruct strbuf proxyhost = STRBUF_INIT;\n> +\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n\nHow well has this code been tested? The documentation for CURLOPT_PROXY\nsays this:\n\n CURLOPT_PROXY\n\n Set HTTP proxy to use. The parameter should be a char * to a zero\n terminated string holding the host name or dotted IP address. To\n specify port number in this string, append :[port] to the end of the\n host name. The proxy string may be prefixed with [protocol]:// since\n any such prefix will be ignored. The proxy's port number may optionally\n be specified with the separate option. If not specified, libcurl will\n default to using port 1080 for proxies. CURLOPT_PROXYPORT.\n\nIf the user has been happily using \"127.0.0.1:4321\" in curl_http_proxy\n(i.e. without the meaningless :// part), the original code would\nhave called curl_easy_setopt with that string, and that would have been\nhow everything used to work.\n\nIf you haven't figured out proxy_auth.host at this point in the codepath,\nyou call credential_from_url() but the function only knows how to parse\nthe value for\n\n\t\"://[[:]@][:]/...\"\n\nSpecifically, it will punt with anything without \"://\" in it.\n\nAnd then you use proxy_auth.protocol and proxy_auth.host to build\nproxyhost.buf that presumably mimick the original curl_http_proxy (but\nwithout the credential part).\n\nI haven't formed an opinion on what the proper solution should be, but\neither the credential_from_url() function needs to be updated to accept\nthe scp style [user@]: argument, or this specific caller\nshould take the responsibility to do special case the syntax.\n\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n> +\t\tset_proxy_auth(result);\n> \t}\n> \n> \treturn result;\n"},{"id":"188832","messageId":"7vsjgcs8pq.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"7v398cvb30.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-10T00:59:13Z","receivedAt":"2012-04-10T00:59:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I haven't formed an opinion on what the proper solution should be, but\n> either the credential_from_url() function needs to be updated to accept\n> the scp style [user@]: argument, or this specific caller\n> should take the responsibility to do special case the syntax.\n\nWell, calling the above \"scp\" style is a mistake (it is not scp style at\nall), but the patch to teach the credentail_from_url() to handle the proxy\nspecification may look like this:\n\n credential.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 62d1c56..482ae88 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -324,11 +324,13 @@ void credential_from_url(struct credential *c, const char *url)\n \t * (1) proto:///...\n \t * (2) proto://@/...\n \t * (3) proto://:@/...\n+\t * or \"proto://\"-less variants of the above for *_proxy variables.\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end)\n-\t\treturn;\n-\tcp = proto_end + 3;\n+\tif (proto_end)\n+\t\tcp = proto_end + 3;\n+\telse\n+\t\tcp = url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -348,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)\n \t\thost = at + 1;\n \t}\n \n-\tif (proto_end - url > 0)\n+\tif (proto_end && proto_end != url)\n \t\tc->protocol = xmemdupz(url, proto_end - url);\n \tif (slash - host > 0)\n \t\tc->host = url_decode_mem(host, slash - host);\n"},{"id":"189112","messageId":"7vwr5leyj5.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"7vsjgcs8pq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T15:54:22Z","receivedAt":"2012-04-12T15:54:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> I haven't formed an opinion on what the proper solution should be, but\n>> either the credential_from_url() function needs to be updated to accept\n>> the scp style [user@]: argument, or this specific caller\n>> should take the responsibility to do special case the syntax.\n>\n> Well, calling the above \"scp\" style is a mistake (it is not scp style at\n> all), but the patch to teach the credentail_from_url() to handle the proxy\n> specification may look like this:\n\nJeff, do you have an opinion on this? I briefly wondered if we also want\nto teach the traditional [user@]host:/path/to/repo to this function (it is\nnot a URL in RFC1738 sense, but it is in the remote.$name.url sense), but\nbecause SSH does its own thing interacting with agents, perhaps it may not\nhelp to teach our credential layer to store and supply cached passphrases\n(or passwords, if the authentication is done by merely sending password\nover the encrypted channel).\n\nA safer approach might be to keep externally visible API to this function\nas before, but add another function only for the use of http_proxy and\nfriends (whose kosher format is \"host:address\" without the \"://\"\npart), and call it from the codepath broken by the patch.\n\n> credential.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/credential.c b/credential.c\n> index 62d1c56..482ae88 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -324,11 +324,13 @@ void credential_from_url(struct credential *c, const char *url)\n> \t * (1) proto:///...\n> \t * (2) proto://@/...\n> \t * (3) proto://:@/...\n> +\t * or \"proto://\"-less variants of the above for *_proxy variables.\n> \t */\n> \tproto_end = strstr(url, \"://\");\n> -\tif (!proto_end)\n> -\t\treturn;\n> -\tcp = proto_end + 3;\n> +\tif (proto_end)\n> +\t\tcp = proto_end + 3;\n> +\telse\n> +\t\tcp = url;\n> \tat = strchr(cp, '@');\n> \tcolon = strchr(cp, ':');\n> \tslash = strchrnul(cp, '/');\n> @@ -348,7 +350,7 @@ void credential_from_url(struct credential *c, const char *url)\n> \t\thost = at + 1;\n> \t}\n> \n> -\tif (proto_end - url > 0)\n> +\tif (proto_end && proto_end != url)\n> \t\tc->protocol = xmemdupz(url, proto_end - url);\n> \tif (slash - host > 0)\n> \t\tc->host = url_decode_mem(host, slash - host);\n"},{"id":"189142","messageId":"20120412205836.GB21018@sigill.intra.peff.net","threadId":"29937","inReplyTo":"7vwr5leyj5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T20:58:36Z","receivedAt":"2012-04-12T20:58:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 08:54:22AM -0700, Junio C Hamano wrote:\n\n> >> I haven't formed an opinion on what the proper solution should be, but\n> >> either the credential_from_url() function needs to be updated to accept\n> >> the scp style [user@]: argument, or this specific caller\n> >> should take the responsibility to do special case the syntax.\n> >\n> > Well, calling the above \"scp\" style is a mistake (it is not scp style at\n> > all), but the patch to teach the credentail_from_url() to handle the proxy\n> > specification may look like this:\n> \n> Jeff, do you have an opinion on this? I briefly wondered if we also want\n> to teach the traditional [user@]host:/path/to/repo to this function (it is\n> not a URL in RFC1738 sense, but it is in the remote.$name.url sense), but\n> because SSH does its own thing interacting with agents, perhaps it may not\n> help to teach our credential layer to store and supply cached passphrases\n> (or passwords, if the authentication is done by merely sending password\n> over the encrypted channel).\n\nMy first instinct was \"that is not a URL, and should be handled outside\nthis function\". In particular, it has no protocol field, and that is an\nimportant part of the credential-matching process. It would be up to the\ncaller to supply something sane in the protocol portion. In this case,\nit would probably be \"http\" (unless we want to distinguish http proxies\nfrom http end-points in the credential store, but I doubt that is\nuseful). But for an ssh-style URL, it would be \"ssh\". So already the\nabstraction is a little bit leaky.\n\nAs far as parsing ssh goes, I'm not sure that is unambiguous with the\nproxy syntax. If you see \"127.0.0.1:1234\", is that short for\n\"http://127.0.0.1:1234/\" (what the proxy code wants), or for\n\"ssh://127.0.0.1/1234\" (what ssh code would want)? The former seems less\nodd to me, as it really is just a URL missing some components. The\nlatter is a true alternative syntax.\n\nLike you said, I don't think we will ever want to handle ssh\ncredentials, though. There is already a solution for people who don't\nwant to input ssh passwords, and it is much more advanced and\nwell-supported within the community than what we would provide.\n\n> A safer approach might be to keep externally visible API to this function\n> as before, but add another function only for the use of http_proxy and\n> friends (whose kosher format is \"host:address\" without the \"://\"\n> part), and call it from the codepath broken by the patch.\n\nI think that would be cleaner conceptually, but it also means\nreimplementing the user/password-parsing logic. And given that I don't\nthink we want to handle ssh, and that the semantics in your patch are\nthe only sane ones to me, it is not so bad. The caller just needs to be\naware of filling in the \"protocol\" field. Perhaps we could have an\nalternate version that supplies a \"default protocol\" parameter. The\npresence of that parameter would activate this code-path and\nautomatically fill in the protocol field.\n\n-Peff\n"},{"id":"189147","messageId":"7vpqbc4p8n.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"20120412205836.GB21018@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T21:25:12Z","receivedAt":"2012-04-12T21:25:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My first instinct was \"that is not a URL, and should be handled outside\n> this function\". In particular, it has no protocol field, and that is an\n> important part of the credential-matching process. It would be up to the\n> caller to supply something sane in the protocol portion. In this case,\n> it would probably be \"http\"...\n\nOutside git, these actually come from things like these:\n\n\thttp_proxy=127.0.0.1:1080\n HTTPS_PROXY=127.0.0.1:1080\n\nAnd http.proxy configuration variable we have is a substitute for\nhttp_proxy. So if we want to keep the credentials for destination servers\nand the credentials for proxies, \"http.proxy\" codepath should be asking\nyou with \"http\". If we are looking at HTTPS_PROXY, you should get \"https\".\nThe patch that broke the unauthenticated proxy access does neither.\n\n> ... (unless we want to distinguish http proxies\n> from http end-points in the credential store, but I doubt that is\n> useful).\n\nThat is something we may want to think carefully about.\n\nIf it is better to separate them, then we can easily invent \"http-proxy\",\n\"https-proxy\" etc. for them, e.g.\n\n\tHTTPS_PROXY=http://127.0.0.1:1080\n\tgit fetch https://over.there.xz/repo/sito/ry.git\n\nwould ask you for a credential to access 127.0.0.1:1080 in \"https-proxy\"\ndomain, and another to access over.there.xz in \"https\" domain.\n\nIn either case, the last example will not use \"http\" anywhere, even though\nthe value of the proxy has noiseword \"http://\" in front of it, which is\nignored. So in that sense, even if we ignored the breakage for the proxy\nspecification without noiseword which Shawn noticed, the patch is broken,\nas it asks credential for http://127.0.0.1:1080 and you parse it for \"http\"\nprotocol.\n"},{"id":"189150","messageId":"20120412220516.GG21018@sigill.intra.peff.net","threadId":"29937","inReplyTo":"7vpqbc4p8n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T22:05:16Z","receivedAt":"2012-04-12T22:05:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 02:25:12PM -0700, Junio C Hamano wrote:\n\n> Outside git, these actually come from things like these:\n> \n> \thttp_proxy=127.0.0.1:1080\n> \tHTTPS_PROXY=127.0.0.1:1080\n> \n> And http.proxy configuration variable we have is a substitute for\n> http_proxy. So if we want to keep the credentials for destination servers\n> and the credentials for proxies, \"http.proxy\" codepath should be asking\n> you with \"http\". If we are looking at HTTPS_PROXY, you should get \"https\".\n> The patch that broke the unauthenticated proxy access does neither.\n\nHmm. Does the distinction between http and https actually matter to\ncurl? My reading of the code and documentation is that only \"http\" is\nmeaningful (actually, anything besides socks*:// gets converted to\nhttp).\n\nSo as far as I can tell, these are equivalent:\n\n http_proxy=http://127.0.0.1:1080\n http_proxy=https://127.0.0.1:1080\n http_proxy=foobar://127.0.0.1:1080\n\nAnd furthermore, the decision to use http_proxy versus https_proxy is\nabout what the _target_ connection wants to do. So if you see this:\n\n HTTPS_PROXY=127.0.0.1:1080\n\nit is still an http proxy; it is just that it is used for requests going\nto https:// servers, and it will ask to tunnel via CONNECT instead of\nGET. But in either case, the conversation with the proxy is over\nstraight http.\n\nSo the value should always be \"http\" in that case. This is a credential\nwe are handing to the proxy, not to the target server, and it is done\nover http, not https.\n\nI can't see that there is a way to tell curl to speak SSL to the proxy\nitself. Maybe I am missing it, but I couldn't find anything in the\ncode, nor make it work with \"curl -x\" to an \"openssl s_server\" instance.\n\n> That is something we may want to think carefully about.\n> \n> If it is better to separate them, then we can easily invent \"http-proxy\",\n> \"https-proxy\" etc. for them, e.g.\n> \n> \tHTTPS_PROXY=http://127.0.0.1:1080\n> \tgit fetch https://over.there.xz/repo/sito/ry.git\n> \n> would ask you for a credential to access 127.0.0.1:1080 in \"https-proxy\"\n> domain, and another to access over.there.xz in \"https\" domain.\n\nNo, it should ask for the credential for 127.0.0.1:1080 in the \"http\"\ndomain, per the above discussion.\n\nNot splitting \"http\" and \"http-proxy\" does have a slight confusion, as\nthe default proxy port is \"1080\". So a proxy of \"http://127.0.0.1\" would\nmean \"http://127.0.0.1:1080\", whereas a regular request would mean\n\"http://127.0.0.1:80\". The credential code includes the port as part of\nthe unique hostname, but since the default-port magic happens inside\ncurl, we have no access to it (short of re-implementing it ourselves).\n\nIn practice, I doubt it matters much; do people really have different\ncredentials for proxies and regular servers on the same host? And if so,\nthere is already a workaround by using the port number in the proxy\nspecification.\n\nI really wish curl's credential-handling was implemented as a callback;\nthis would be much simpler if could let curl decipher the request and\ncome to us with the complete request (protocol, host, port, path, etc).\nBut even if we got such a feature in curl, we are stuck supporting the\nold way for a while anyway.\n\n-Peff\n"},{"id":"189152","messageId":"7vd37c4msm.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"20120412220516.GG21018@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-12T22:18:01Z","receivedAt":"2012-04-12T22:18:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 12, 2012 at 02:25:12PM -0700, Junio C Hamano wrote:\n>\n>> Outside git, these actually come from things like these:\n>> \n>> \thttp_proxy=127.0.0.1:1080\n>> \tHTTPS_PROXY=127.0.0.1:1080\n>> \n>> And http.proxy configuration variable we have is a substitute for\n>> http_proxy. So if we want to keep the credentials for destination servers\n>> and the credentials for proxies, \"http.proxy\" codepath should be asking\n>> you with \"http\". If we are looking at HTTPS_PROXY, you should get \"https\".\n>> The patch that broke the unauthenticated proxy access does neither.\n>\n> Hmm. Does the distinction between http and https actually matter to\n> curl? My reading of the code and documentation is that only \"http\" is\n> meaningful (actually, anything besides socks*:// gets converted to\n> http).\n>\n> So as far as I can tell, these are equivalent:\n>\n> http_proxy=http://127.0.0.1:1080\n> http_proxy=https://127.0.0.1:1080\n> http_proxy=foobar://127.0.0.1:1080\n\nYes, that is exactly what I was trying to say. The foobar:// part does\nnot matter; \"http\" in \"http_proxy\" is what matters, as it is how you can\nspecify two separate proxies depending on what destination you are going\nvia what protocol.\n\n> Not splitting \"http\" and \"http-proxy\" does have a slight confusion, as\n> the default proxy port is \"1080\". So a proxy of \"http://127.0.0.1\" would\n> mean \"http://127.0.0.1:1080\", whereas a regular request would mean\n> \"http://127.0.0.1:80\". The credential code includes the port as part of\n> the unique hostname, but since the default-port magic happens inside\n> curl, we have no access to it (short of re-implementing it ourselves).\n\nOk, so how about this as a replacement patch for what I have had for the\npast few days?\n\n credential.c | 44 +++++++++++++++++++++++++++++++-------------\n credential.h | 1 +\n http.c | 10 +++++++---\n 3 files changed, 39 insertions(+), 16 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 62d1c56..5803645 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -313,22 +313,17 @@ void credential_reject(struct credential *c)\n \tc->approved = 0;\n }\n \n-void credential_from_url(struct credential *c, const char *url)\n+static void credential_for_dest(struct credential *c, const char *dest)\n {\n-\tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n-\n-\tcredential_clear(c);\n+\tconst char *at, *colon, *cp, *slash, *host;\n \n \t/*\n \t * Match one of:\n-\t * (1) proto:///...\n-\t * (2) proto://@/...\n-\t * (3) proto://:@/...\n+\t * (1) /...\n+\t * (2) @/...\n+\t * (3) :@/...\n \t */\n-\tproto_end = strstr(url, \"://\");\n-\tif (!proto_end)\n-\t\treturn;\n-\tcp = proto_end + 3;\n+\tcp = dest;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -348,10 +343,9 @@ void credential_from_url(struct credential *c, const char *url)\n \t\thost = at + 1;\n \t}\n \n-\tif (proto_end - url > 0)\n-\t\tc->protocol = xmemdupz(url, proto_end - url);\n \tif (slash - host > 0)\n \t\tc->host = url_decode_mem(host, slash - host);\n+\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n@@ -363,3 +357,27 @@ void credential_from_url(struct credential *c, const char *url)\n \t\t\t*p-- = '\\0';\n \t}\n }\n+\n+void credential_for_destination(struct credential *c, const char *dest, const char *proto)\n+{\n+\tcredential_clear(c);\n+\tc->protocol = xstrdup(proto);\n+\tcredential_for_dest(c, dest);\n+}\n+\n+void credential_from_url(struct credential *c, const char *url)\n+{\n+\tconst char *proto_end;\n+\n+\tcredential_clear(c);\n+\n+\t/*\n+\t * Strip \"proto://\" part and let credential_for_dest()\n+\t * parse the remainder.\n+\t */\n+\tproto_end = strstr(url, \"://\");\n+\tif (!proto_end)\n+\t\treturn;\n+\tc->protocol = xmemdupz(url, proto_end - url);\n+\tcredential_for_dest(c, proto_end + 3);\n+}\ndiff --git a/credential.h b/credential.h\nindex 96ea41b..4b1c320 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -27,6 +27,7 @@ void credential_reject(struct credential *);\n \n int credential_read(struct credential *, FILE *);\n void credential_from_url(struct credential *, const char *url);\n+void credential_for_destination(struct credential *, const char *dest, const char *proto);\n int credential_match(const struct credential *have,\n \t\t const struct credential *want);\n \ndiff --git a/http.c b/http.c\nindex 1c71edb..752b6ea 100644\n--- a/http.c\n+++ b/http.c\n@@ -336,14 +336,18 @@ static CURL *get_curl_handle(const char *url)\n \tif (curl_http_proxy) {\n \t\tstruct strbuf proxyhost = STRBUF_INIT;\n \n-\t\tif (!proxy_auth.host) /* check to parse only once */\n-\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\tif (!proxy_auth.host) {\n+\t\t\tconst char *cp;\n+\t\t\tcp = strstr(curl_http_proxy, \"://\");\n+\t\t\tcp = cp ? (cp + 3) : curl_http_proxy;\n+\t\t\tcredential_for_destination(&proxy_auth, cp, \"http-proxy\");\n+\t\t}\n \n \t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n \t\t\t/* proxy string has username but no password, ask for password */\n \t\t\tcredential_fill(&proxy_auth);\n \n-\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n+\t\tstrbuf_addstr(&proxyhost, proxy_auth.host);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t\tset_proxy_auth(result);\n"},{"id":"189155","messageId":"20120412224230.GA22988@sigill.intra.peff.net","threadId":"29937","inReplyTo":"7vd37c4msm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-12T22:42:30Z","receivedAt":"2012-04-12T22:42:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 12, 2012 at 03:18:01PM -0700, Junio C Hamano wrote:\n\n> > So as far as I can tell, these are equivalent:\n> >\n> > http_proxy=http://127.0.0.1:1080\n> > http_proxy=https://127.0.0.1:1080\n> > http_proxy=foobar://127.0.0.1:1080\n> \n> Yes, that is exactly what I was trying to say. The foobar:// part does\n> not matter; \"http\" in \"http_proxy\" is what matters, as it is how you can\n> specify two separate proxies depending on what destination you are going\n> via what protocol.\n\nBut you snipped the later part of my message, which is that the \"http\"\nin \"http_proxy\" does _not_ matter. It is about which destinations to\napply the proxy to, not how you talk to the proxy (and the latter is what\nshould matter for the credentials).\n\n> > Not splitting \"http\" and \"http-proxy\" does have a slight confusion, as\n> > the default proxy port is \"1080\". So a proxy of \"http://127.0.0.1\" would\n> > mean \"http://127.0.0.1:1080\", whereas a regular request would mean\n> > \"http://127.0.0.1:80\". The credential code includes the port as part of\n> > the unique hostname, but since the default-port magic happens inside\n> > curl, we have no access to it (short of re-implementing it ourselves).\n> \n> Ok, so how about this as a replacement patch for what I have had for the\n> past few days?\n\nMy other message argued \"the http-proxy distinction might be important,\nbut probably isn't\". But I didn't talk about \"the http-proxy distinction\nmight break helpers\". The stock helpers will be fine; they are totally\nclueless about what the protocol means, and just treat it as a string to\nbe matched. But for something like osxkeychain, where it is converting\nthe protocol string into some OS-specific magic value, it does matter,\nand http-proxy would cause it to exit in confusion.\n\nIt looks like OS X defines a SOCKS type and an HTTPProxy type for its\nkeychain API. So in either case, it should probably be updated to handle\nthese new types. And I guess that argues for making the distinction,\nsince at least one helper does want to care about it.\n\n-Peff\n"},{"id":"189210","messageId":"7viph32znu.fsf@alter.siamese.dyndns.org","threadId":"29937","inReplyTo":"20120412224230.GA22988@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-13T19:35:17Z","receivedAt":"2012-04-13T19:35:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But you snipped the later part of my message, which is that the \"http\"\n> in \"http_proxy\" does _not_ matter. It is about which destinations to\n> apply the proxy to, not how you talk to the proxy (and the latter is what\n> should matter for the credentials).\n\nOh, yes, I am in violent agreement. The language the http clients\n(browsers etc) talk to the proxy may be part of HTTP specification, but it\nis definitely different from the \"http\" talked with the origin servers.\n\n>> > Not splitting \"http\" and \"http-proxy\" does have a slight confusion,...\n>> \n>> Ok, so how about this as a replacement patch for what I have had for the\n>> past few days?\n>\n> My other message argued \"the http-proxy distinction might be important,\n> but probably isn't\". But I didn't talk about \"the http-proxy distinction\n> might break helpers\". The stock helpers will be fine; they are totally\n> clueless about what the protocol means, and just treat it as a string to\n> be matched. But for something like osxkeychain, where it is converting\n> the protocol string into some OS-specific magic value, it does matter,\n> and http-proxy would cause it to exit in confusion.\n>\n> It looks like OS X defines a SOCKS type and an HTTPProxy type for its\n> keychain API. So in either case, it should probably be updated to handle\n> these new types. And I guess that argues for making the distinction,\n> since at least one helper does want to care about it.\n\nOK. Sounds like we are in agreement.\n\nNelson, care to re-roll the series, with fixes discussed in this thread\nrolled into the second patch?\n"},{"id":"189214","messageId":"20120413202359.GA5962@sigill.intra.peff.net","threadId":"29937","inReplyTo":"7viph32znu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T20:23:59Z","receivedAt":"2012-04-13T20:23:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 12:35:17PM -0700, Junio C Hamano wrote:\n\n> > It looks like OS X defines a SOCKS type and an HTTPProxy type for its\n> > keychain API. So in either case, it should probably be updated to handle\n> > these new types. And I guess that argues for making the distinction,\n> > since at least one helper does want to care about it.\n> \n> OK. Sounds like we are in agreement.\n> \n> Nelson, care to re-roll the series, with fixes discussed in this thread\n> rolled into the second patch?\n\nI think there is a bug in the patch you posted, though:\n\n> diff --git a/http.c b/http.c\n> index 1c71edb..752b6ea 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -336,14 +336,18 @@ static CURL *get_curl_handle(const char *url)\n> \tif (curl_http_proxy) {\n> \t\tstruct strbuf proxyhost = STRBUF_INIT;\n> \n> -\t\tif (!proxy_auth.host) /* check to parse only once */\n> -\t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n> +\t\tif (!proxy_auth.host) {\n> +\t\t\tconst char *cp;\n> +\t\t\tcp = strstr(curl_http_proxy, \"://\");\n> +\t\t\tcp = cp ? (cp + 3) : curl_http_proxy;\n> +\t\t\tcredential_for_destination(&proxy_auth, cp, \"http-proxy\");\n> +\t\t}\n\nWhat happens if the URL starts with \"socks://\"? We would want to\npreserve that, and have our protocol end as \"socks://\".\n\n> \t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n> \t\t\t/* proxy string has username but no password, ask for password */\n> \t\t\tcredential_fill(&proxy_auth);\n> \n> -\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n> +\t\tstrbuf_addstr(&proxyhost, proxy_auth.host);\n> \t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n\nSame here. If we get \"socks://127.0.0.1\", we will feed \"127.0.0.1\" to\ncurl, which will then assume that it's http.\n\nBTW, do we actually need to strip the username out of the URL? We do not\ndo so with regular URLs, and curl takes the auth we give it over what is\nin the URL. Can this be simplified to just:\n\n curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n\n?\n\nAt any rate, I think the rule we want for parsing the url is to actually\nparse the protocol from the url, and if it's not there, assume it's\nhttp. And then convert all \"http\" to \"http-proxy\", because this is a\nweird alternate use of \"http\". So we can just take your old patch to\nrelax the credential_from_url parsing, and then fix it up on the calling\nside. Like this:\n\ndiff --git a/credential.c b/credential.c\nindex 62d1c56..813e3cf 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -324,11 +324,15 @@ void credential_from_url(struct credential *c, const char *url)\n \t * (1) proto:///...\n \t * (2) proto://@/...\n \t * (3) proto://:@/...\n+\t * or \"proto://\"-less variants of the above. They are not technically\n+\t * URLs, but the caller may have some context-specific knowledge about\n+\t * what protocol is in use.\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end)\n-\t\treturn;\n-\tcp = proto_end + 3;\n+\tif (proto_end)\n+\t\tcp = proto_end + 3;\n+\telse\n+\t\tcp = url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -348,7 +352,7 @@ void credential_from_url(struct credential *c, const char *url)\n \t\thost = at + 1;\n \t}\n \n-\tif (proto_end - url > 0)\n+\tif (proto_end && proto_end != url)\n \t\tc->protocol = xmemdupz(url, proto_end - url);\n \tif (slash - host > 0)\n \t\tc->host = url_decode_mem(host, slash - host);\ndiff --git a/http.c b/http.c\nindex 1c71edb..a164f79 100644\n--- a/http.c\n+++ b/http.c\n@@ -334,17 +334,20 @@ static CURL *get_curl_handle(const char *url)\n \t\tfree(env_proxy_var);\n \t}\n \tif (curl_http_proxy) {\n-\t\tstruct strbuf proxyhost = STRBUF_INIT;\n-\n-\t\tif (!proxy_auth.host) /* check to parse only once */\n+\t\tif (!proxy_auth.host) {\n \t\t\tcredential_from_url(&proxy_auth, curl_http_proxy);\n+\t\t\tif (!proxy_auth.protocol ||\n+\t\t\t !strcmp(proxy_auth.protocol, \"http\")) {\n+\t\t\t\tfree(proxy_auth.protocol);\n+\t\t\t\tproxy_auth.protocol = xstrdup(\"http-proxy\");\n+\t\t\t}\n+\t\t}\n \n \t\tif (http_proactive_auth && proxy_auth.username && !proxy_auth.password)\n \t\t\t/* proxy string has username but no password, ask for password */\n \t\t\tcredential_fill(&proxy_auth);\n \n-\t\tstrbuf_addf(&proxyhost, \"%s://%s\", proxy_auth.protocol, proxy_auth.host);\n-\t\tcurl_easy_setopt(result, CURLOPT_PROXY, strbuf_detach(&proxyhost, NULL));\n+\t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n \t\tset_proxy_auth(result);\n \t}\n\nNote that curl will treat \"foobar://\" (or any protocol it does not\nunderstand) as an http proxy. I didn't want to get into white-listing\n\"socks:// is ok, but foobar:// is really just http in disguise\" based on\ncurl's internal rules. So you can use \"foobar://\" if you want, but it's\nnot going to share credentials with \"http://\" (even though curl will use\nthem in exactly the same way).\n\n-Peff\n"},{"id":"189217","messageId":"20120413205649.GC7919@sigill.intra.peff.net","threadId":"29937","inReplyTo":"4F5F53CA.7090003@seap.minhap.es","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T20:56:50Z","receivedAt":"2012-04-13T20:56:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 13, 2012 at 03:03:54PM +0100, Nelson Benitez Leon wrote:\n\n> Also take care that CURLOPT_PROXY don't include username or\n> password, as we now set them in the new set_proxy_auth() function\n> where we use their specific cURL options.\n\nDid you test that this is necessary? We don't do it for the regular URL\ncase, and it makes the code much simpler if we can avoid munging what we\nhand to curl.\n\n> +static void set_proxy_auth(CURL *result)\n> +{\n> +\tif (proxy_auth.username && proxy_auth.password) {\n> +#if LIBCURL_VERSION_NUM >= 0x071901\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);\n> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);\n> +#else\n\nIs that version check right? You are giving a hexadecimal number, so\n7.19.1 would be 071301.\n\n-Peff\n"},{"id":"189711","messageId":"xmqqbomnsl6t.fsf@junio.mtv.corp.google.com","threadId":"29937","inReplyTo":"20120413205649.GC7919@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/5] http: handle proxy proactive authentication","fromName":"Junio C Hamano","fromEmail":"jch@google.com","sentAt":"2012-04-19T17:09:14Z","receivedAt":"2012-04-19T17:09:14Z","isPatch":true,"sender":{"key":"jch@google.com","avatar":null},"body":"Jeff King writes:\n\n> On Tue, Mar 13, 2012 at 03:03:54PM +0100, Nelson Benitez Leon wrote:\n>\n>> Also take care that CURLOPT_PROXY don't include username or\n>> password, as we now set them in the new set_proxy_auth() function\n>> where we use their specific cURL options.\n>\n> Did you test that this is necessary? We don't do it for the regular URL\n> case, and it makes the code much simpler if we can avoid munging what we\n> hand to curl.\n>\n>> +static void set_proxy_auth(CURL *result)\n>> +{\n>> +\tif (proxy_auth.username && proxy_auth.password) {\n>> +#if LIBCURL_VERSION_NUM >= 0x071901\n>> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYUSERNAME, proxy_auth.username);\n>> +\t\tcurl_easy_setopt(result, CURLOPT_PROXYPASSWORD, proxy_auth.password);\n>> +#else\n>\n> Is that version check right? You are giving a hexadecimal number, so\n> 7.19.1 would be 071301.\n\nI notice that I missed this comment, and I think the version queued in\n'pu' still has this incorrect. \n\nCURLOPT_PROXYUSERNAME is marked as Introduced at 7.19.1 in\n\n https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions\n\nso I agree that the above would need to be 0x071301.\n"}]}