Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Sam Vilain]

On 3/1/12 10:22 AM, Nelson Benitez Leon wrote:
> Support proxy urls with username but without a password, in which
> case we interactively ask for the password (using credential api).
> This makes possible to not have the password written down in
> http_proxy env var or in http.proxy config option.
>
> Signed-off-by: Nelson Benitez Leon<nbenitezl@gmail.com>
> ---
>   http.c |   16 +++++++++++++++-
>   1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8932da5..5916194 100644
> --- a/http.c
> +++ b/http.c
> @@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;
>   static const char *curl_http_proxy;
>   static const char *curl_cookie_file;
>   static struct credential http_auth = CREDENTIAL_INIT;
> +static struct credential proxy_auth = CREDENTIAL_INIT;
>   static int http_proactive_auth;
>   static const char *user_agent;
>
> @@ -303,7 +304,20 @@ static CURL *get_curl_handle(void)
>   		}
>   	}
>   	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		credential_from_url(&proxy_auth, curl_http_proxy);
> +		if (proxy_auth.username != NULL&&  proxy_auth.password == NULL) {
> +			/* proxy string has username but no password, ask for password */
> +			struct strbuf pbuf = STRBUF_INIT;
> +			credential_fill(&proxy_auth);

Wouldn't it be better to wait until the proxy returns a 403 before 
assuming that the proxy setting is incorrect/missing a password?  What 
if the administrator expects the user to fill in both the username and 
password?  That is the behaviour of a web browser.

Also, I think you should wait until that 403 to detect whether the proxy 
setting came from the environment, and only load it explicitly then.

Sam

> +			strbuf_addf(&pbuf, "%s://%s:%s@%s",proxy_auth.protocol,
> +				    proxy_auth.username, proxy_auth.password,
> +				    proxy_auth.host);
> +			free ((void *)curl_http_proxy);
> +			curl_http_proxy =  strbuf_detach(&pbuf, NULL);
> +			curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		} else {
> +			curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		}
>   		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
>   	}
>

[PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Nelson Benitez Leon]

Support proxy urls with username but without a password, in which
case we interactively ask for the password (using credential api).
This makes possible to not have the password written down in
http_proxy env var or in http.proxy config option.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
 http.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index 8932da5..5916194 100644
--- a/http.c
+++ b/http.c
@@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static struct credential http_auth = CREDENTIAL_INIT;
+static struct credential proxy_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -303,7 +304,20 @@ static CURL *get_curl_handle(void)
 		}
 	}
 	if (curl_http_proxy) {
-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		credential_from_url(&proxy_auth, curl_http_proxy);
+		if (proxy_auth.username != NULL && proxy_auth.password == NULL) {
+			/* proxy string has username but no password, ask for password */
+			struct strbuf pbuf = STRBUF_INIT;
+			credential_fill(&proxy_auth);
+			strbuf_addf(&pbuf, "%s://%s:%s@%s",proxy_auth.protocol,
+				    proxy_auth.username, proxy_auth.password,
+				    proxy_auth.host);
+			free ((void *)curl_http_proxy);
+			curl_http_proxy =  strbuf_detach(&pbuf, NULL);
+			curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		} else {
+			curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		}
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
 	}
 
-- 
1.7.7.6

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

> Support proxy urls with username but without a password, in which
> case we interactively ask for the password (using credential api).
> This makes possible to not have the password written down in
> http_proxy env var or in http.proxy config option.

How do other people's applications that use http_proxy environment
variable handle this situation?

With this patch and the previous 2/3, we are allowing people to set
"http_proxy=http://me@over.there/", but an environment variable is global
to the user's environment, so if other applications do not grok the "name
only" proxy URL the same way as this patch does, adding this code only to
Git does not make users' lives any better.

Of course the above does not apply to http.proxy configuration, which is
specific to Git.

>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
>  http.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8932da5..5916194 100644
> --- a/http.c
> +++ b/http.c
> @@ -43,6 +43,7 @@ static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
>  static const char *curl_cookie_file;
>  static struct credential http_auth = CREDENTIAL_INIT;
> +static struct credential proxy_auth = CREDENTIAL_INIT;
>  static int http_proactive_auth;
>  static const char *user_agent;
>  
> @@ -303,7 +304,20 @@ static CURL *get_curl_handle(void)
>  		}
>  	}
>  	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		credential_from_url(&proxy_auth, curl_http_proxy);
> +		if (proxy_auth.username != NULL && proxy_auth.password == NULL) {

Just a style, but 

		if (proxy_auth.username && !proxy_auth.password) {

is much more preferred.

> +			free ((void *)curl_http_proxy);

I think somebody already pointed out interaction of this with 2/3.

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Jeff King]

On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:

> >  	if (curl_http_proxy) {
> >-		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> >+		credential_from_url(&proxy_auth, curl_http_proxy);
> >+		if (proxy_auth.username != NULL&&  proxy_auth.password == NULL) {
> >+			/* proxy string has username but no password, ask for password */
> >+			struct strbuf pbuf = STRBUF_INIT;
> >+			credential_fill(&proxy_auth);
> 
> Wouldn't it be better to wait until the proxy returns a 403 before
> assuming that the proxy setting is incorrect/missing a password?
> What if the administrator expects the user to fill in both the
> username and password?  That is the behaviour of a web browser.
> 
> Also, I think you should wait until that 403 to detect whether the
> proxy setting came from the environment, and only load it explicitly
> then.

It's worth looking at what the http auth code does here.

In the beginning (2005), git saw that there was a username in the URL
and prompted for a password unconditionally before making a request. If
you didn't have a username, you didn't do auth, period.

Later, 42653c0 (Prompt for a username when an HTTP request 401s,
2010-04-01) taught git to handle 401s, for when the URL does not contain
a username. We kept the unconditional pre-prompt, though; it has the
nice side effect of avoiding a round-trip to the server.

Then, in 986bbc0 (http: don't always prompt for password, 2011-11-04),
the unconditional pre-prompt was taken away. While avoiding the
round-trip is nice, it circumvented curl's reading of the .netrc file,
which means git would prompt unnecessarily, even when curl could
eventually read out of the netrc.

However, the code for dumb http push-over-dav doesn't handle the 401
properly. As a work-around, a4ddbc3 (http-push: enable "proactive auth",
2011-12-13) re-enabled the pre-prompt, but only for the code paths that
need it (and those code paths are now broken for .netrc, as everything
was before 986bbc0). This is a hack, and in the long run it would be
nice to have everything handle 401s properly, but the dav code is
somewhat obsolete these days, and I suspect nobody really wants to
overhaul it.

Complicating all of this is the fact that I think Nelson's original
patch was based on an older, pre-986bbc0 version of git, which is why he
followed the pre-prompt route, copying the style of regular http auth.

So there's the history lesson. What should proxy auth do?

  1. Definitely respond to HTTP 407 by prompting on the fly; this code
     should go along-side the HTTP 401 code in http.c.

  2. Definitely do the pre-prompt thing when http_proactive_auth is set
     (which is used only by http-push). Unless somebody really feels
     like re-writing http-push to handle retries for authentication.

  3. Consider doing the pre-prompt thing when http_proactive_auth is not
     set. This can save a round-trip, but we should not do it if there
     is a good reason not to. The two possible reasons I can think of
     are:

       a. Like http auth, if curl will read the proxy credentials from
          .netrc, then we should not do it for the same reasons
          mentioned in 986bbc0.

       b. If people realistically have proxy URLs with usernames but do
          _not_ want to ask for a password, then the prompt will be
          annoying. I'm not sure that anybody expects that.

I consider (3) to be a "meh, if you are really interested in looking
into this" step, as it is really just a possible optimization (and I
suspect curl _does_ use netrc for proxy credentials, but I haven't
checked). But we definitely want to get (1) and (2) right.

-Peff

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Jeff King]

On Fri, Mar 02, 2012 at 02:33:53PM +0100, Nelson Benitez Leon wrote:

> > So there's the history lesson. What should proxy auth do?
> > 
> >   1. Definitely respond to HTTP 407 by prompting on the fly; this code
> >      should go along-side the HTTP 401 code in http.c.
> > 
> >   2. Definitely do the pre-prompt thing when http_proactive_auth is set
> >      (which is used only by http-push). Unless somebody really feels
> >      like re-writing http-push to handle retries for authentication.
> > 
> >   3. Consider doing the pre-prompt thing when http_proactive_auth is not
> >      set. This can save a round-trip, but we should not do it if there
> >      is a good reason not to. The two possible reasons I can think of
> >      are:
> > 
> >        a. Like http auth, if curl will read the proxy credentials from
> >           .netrc, then we should not do it for the same reasons
> >           mentioned in 986bbc0.
> > 
> >        b. If people realistically have proxy URLs with usernames but do
> >           _not_ want to ask for a password, then the prompt will be
> >           annoying. I'm not sure that anybody expects that.
> 
> So, trying to sum up, I will try to redo patch-set as follows:
> - Ignore PATCH 2/3 , that is, we won't read any env var.
> - Let cURL try to connect and if that fails with 407 , then do a credential_fill
> and try to reconnect.
> 
> Is that ok? or do I need to do something more?

I think you'll still need to read the env var, because you'll need to
know the proxy URL when getting the password (to ask credential helpers
properly, and to prompt the user).

Also, I think you'll need to call credential_fill() when
http_proactive_auth is set. Otherwise http-push will not be able to do
proxy auth.

-Peff

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Nelson Benitez Leon]

On 03/01/2012 10:58 PM, Jeff King wrote:
> On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:
> 
>>>  	if (curl_http_proxy) {
>>> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>>> +		credential_from_url(&proxy_auth, curl_http_proxy);
>>> +		if (proxy_auth.username != NULL&&  proxy_auth.password == NULL) {
>>> +			/* proxy string has username but no password, ask for password */
>>> +			struct strbuf pbuf = STRBUF_INIT;
>>> +			credential_fill(&proxy_auth);
>>
>> Wouldn't it be better to wait until the proxy returns a 403 before
>> assuming that the proxy setting is incorrect/missing a password?
>> What if the administrator expects the user to fill in both the
>> username and password?  That is the behaviour of a web browser.
>>
>> Also, I think you should wait until that 403 to detect whether the
>> proxy setting came from the environment, and only load it explicitly
>> then.
> 
> It's worth looking at what the http auth code does here.
> 
[snip]
> overhaul it.
> 
> Complicating all of this is the fact that I think Nelson's original
> patch was based on an older, pre-986bbc0 version of git, which is why he
> followed the pre-prompt route, copying the style of regular http auth.
> 
> So there's the history lesson. What should proxy auth do?
> 
>   1. Definitely respond to HTTP 407 by prompting on the fly; this code
>      should go along-side the HTTP 401 code in http.c.
> 
>   2. Definitely do the pre-prompt thing when http_proactive_auth is set
>      (which is used only by http-push). Unless somebody really feels
>      like re-writing http-push to handle retries for authentication.
> 
>   3. Consider doing the pre-prompt thing when http_proactive_auth is not
>      set. This can save a round-trip, but we should not do it if there
>      is a good reason not to. The two possible reasons I can think of
>      are:
> 
>        a. Like http auth, if curl will read the proxy credentials from
>           .netrc, then we should not do it for the same reasons
>           mentioned in 986bbc0.
> 
>        b. If people realistically have proxy URLs with usernames but do
>           _not_ want to ask for a password, then the prompt will be
>           annoying. I'm not sure that anybody expects that.

So, trying to sum up, I will try to redo patch-set as follows:
- Ignore PATCH 2/3 , that is, we won't read any env var.
- Let cURL try to connect and if that fails with 407 , then do a credential_fill
and try to reconnect.

Is that ok? or do I need to do something more?

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Jeff King]

On Fri, Mar 02, 2012 at 03:05:17PM +0100, Nelson Benitez Leon wrote:

> > I think you'll still need to read the env var, because you'll need to
> > know the proxy URL when getting the password (to ask credential helpers
> > properly, and to prompt the user).
> 
> Ok, but I can read it after receiving the 407 (and in case we were not
> using http.proxy) so discarding PATCH 2/3 still applies, ok? or we need
> to read it first-hand for the http_proactive_auth you mention below?

You will need it for proactive_auth.

> > Also, I think you'll need to call credential_fill() when
> > http_proactive_auth is set. Otherwise http-push will not be able to do
> > proxy auth.
> 
> I still don't get what proactive_auth is about, will ask you when I get
> to that part of the patch.

It is a flag that, when true, instructs the http code to do auth if we
have a non-NULL username, even before we get an http 401. It is only set
for http-push, because the http-push-over-dav code does not properly
detect and retry on a 401 (and I don't expect it will be easy to
properly detect and retry on a 407, either). Whereas the smart-http code
and the dumb http fetch code properly detect the 401.

-Peff

Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Nelson Benitez Leon]

On 03/02/2012 01:45 PM, Jeff King wrote:
> On Fri, Mar 02, 2012 at 02:33:53PM +0100, Nelson Benitez Leon wrote:
> 
>>> So there's the history lesson. What should proxy auth do?
>>>
>>>   1. Definitely respond to HTTP 407 by prompting on the fly; this code
>>>      should go along-side the HTTP 401 code in http.c.
>>>
>>>   2. Definitely do the pre-prompt thing when http_proactive_auth is set
>>>      (which is used only by http-push). Unless somebody really feels
>>>      like re-writing http-push to handle retries for authentication.
>>>
>>>   3. Consider doing the pre-prompt thing when http_proactive_auth is not
>>>      set. This can save a round-trip, but we should not do it if there
>>>      is a good reason not to. The two possible reasons I can think of
>>>      are:
>>>
>>>        a. Like http auth, if curl will read the proxy credentials from
>>>           .netrc, then we should not do it for the same reasons
>>>           mentioned in 986bbc0.
>>>
>>>        b. If people realistically have proxy URLs with usernames but do
>>>           _not_ want to ask for a password, then the prompt will be
>>>           annoying. I'm not sure that anybody expects that.
>>
>> So, trying to sum up, I will try to redo patch-set as follows:
>> - Ignore PATCH 2/3 , that is, we won't read any env var.
>> - Let cURL try to connect and if that fails with 407 , then do a credential_fill
>> and try to reconnect.
>>
>> Is that ok? or do I need to do something more?
> 
> I think you'll still need to read the env var, because you'll need to
> know the proxy URL when getting the password (to ask credential helpers
> properly, and to prompt the user).

Ok, but I can read it after receiving the 407 (and in case we were not
using http.proxy) so discarding PATCH 2/3 still applies, ok? or we need
to read it first-hand for the http_proactive_auth you mention below?

> 
> Also, I think you'll need to call credential_fill() when
> http_proactive_auth is set. Otherwise http-push will not be able to do
> proxy auth.

I still don't get what proactive_auth is about, will ask you when I get
to that part of the patch.

Thank you,


> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html