v2, 3 partshttp: when proxy url has username but no password, ask for password

> 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);
>   	}
>

> >  	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.

> 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.
> 

> 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 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?

> 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).

> -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

> > 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.

>
> 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) {