[PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[Nelson Benitez Leon]

CURLAUTH_ANY option automatically chooses the best auth method
from among those the server supports, that means curl
will ask the proxy and use the appropiate, and it will only do that if
you are using a proxy (i.e. you've set CURLOPT_PROXY or you have http_proxy
env var), also curl will not try to authenticate if you've not provided
username or password in the proxy string, as told here[1]..

so, setting CURLOPT_PROXYAUTH = CURLAUTH_ANY will not affect the speed of
normal curl use, only if 1) you are using a proxy and 2) your proxy requires
authentication, only then curl will just make two or three roundtrips to find out
the auth methods the proxy is using, that is a tiny cost compared to having the
user find out the proxy auth type and set manually a specific config option to
enable that type.

So as CURLAUTH_ANY provide us out-of-the-box proxy support without affecting speed,
we don't want it activated manually from a config option, instead we added it
automatically when a proxy is being used.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=769254#c6

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

diff --git a/http.c b/http.c
index 0ffd79c..8ac8eb6 100644
--- a/http.c
+++ b/http.c
@@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-	if (curl_http_proxy)
+	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+	}
 
 	return result;
 }
-- 
1.7.7.6

Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[Junio C Hamano]

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

Thanks; doesn't a missing space before http: above look ugly to you, by
the way?

> CURLAUTH_ANY option automatically chooses the best auth method from
> among those the server supports, that means curl will ask the proxy and
> use the appropiate, and it will only do that if you are using a proxy
> (i.e. you've set CURLOPT_PROXY or you have http_proxy env var), also
> curl will not try to authenticate if you've not provided username or
> password in the proxy string, as told here[1]..

The above may justify why you used CURLAUTH_ANY as opposed to
CURLAUTH_BASIC or other types, but without any description of the problem
you are trying to solve before that paragraph, it does not justify why you
are adding a code to use CURLOPT_PROXYAUTH in the first place.

This is my *guess* of the problem you are trying to solve.

	When the proxy server specified by the http.proxy configuration or
	the http_proxy environment variable requires authentication, git
	failed to connect to the proxy, because we did not configure the
	cURL handle with CURLOPT_PROXYAUTH.

It may or may not match the reality, but either case, the explanation
should have something like that at the beginning of the log.

Instead of "as told here[1]..", I would have preferred for _you_ to say "I
ran with this patch against a proxy that requires authentication and
another that does not, and made sure that there was no prompt or any extra
network traffic when no username or password is in in the proxy URL to
trigger the authentication", or something like that.  The item you refer
to with your "as told here[1].." only has this to say on this issue:

   I don't think PROXYAUTH=HTTP_ANY must be conditional.  curl parses the
   proxy string, and if there's no user/pass, I'm quite sure it will ignore
   PROXYAUTH anyway.

which is *not* convincing enough to be worth even referring to.  It is
just expressing one person's educated guess.

> so, setting CURLOPT_PROXYAUTH = CURLAUTH_ANY will not affect the speed of
> normal curl use, only if 1) you are using a proxy and 2) your proxy requires
> authentication, only then curl will just make two or three roundtrips to find out
> the auth methods the proxy is using, that is a tiny cost compared to having the
> user find out the proxy auth type and set manually a specific config option to
> enable that type.
>
> So as CURLAUTH_ANY provide us out-of-the-box proxy support without affecting speed,
> we don't want it activated manually from a config option, instead we added it
> automatically when a proxy is being used.

I think you are discussing the latency (not the speed) here.  Also the
first few sentences are very hard to parse.  Perhaps you meant to say
something like this?

        When a proxy is in use, and you tell git that the proxy requires
	authentication by having username in the http.proxy configuration,
	an extra request needs to be made to the proxy to find out what
	authentication method it supports, as this patch uses CURLAUTH_ANY
	to let the library pick the most secure method supported by the
	proxy server.

        The extra round-trip adds extra latency, but relieves the user
	from the burden to configure a specific authentication method.  If
	it becomes problem, a later patch could add a configuration option
	to specify what method to use, but let's start simple for the time
	being.

Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[Nelson Benitez Leon]

On 03/01/2012 08:07 PM, Junio C Hamano wrote:
> Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:
> 
> Thanks; doesn't a missing space before http: above look ugly to you, by
> the way?
> 
>> CURLAUTH_ANY option automatically chooses the best auth method from
>> among those the server supports, that means curl will ask the proxy and
>> use the appropiate, and it will only do that if you are using a proxy
>> (i.e. you've set CURLOPT_PROXY or you have http_proxy env var), also
>> curl will not try to authenticate if you've not provided username or
>> password in the proxy string, as told here[1]..
> 
> The above may justify why you used CURLAUTH_ANY as opposed to
> CURLAUTH_BASIC or other types, but without any description of the problem
> you are trying to solve before that paragraph, it does not justify why you
> are adding a code to use CURLOPT_PROXYAUTH in the first place.
> 
> This is my *guess* of the problem you are trying to solve.

I've ammended the commit message with your wording, text as follows:

When the proxy server specified by the http.proxy configuration or
the http_proxy environment variable requires authentication, git
failed to connect to the proxy, because we did not configure the
cURL handle with CURLOPT_PROXYAUTH.

When a proxy is in use, and you tell git that the proxy requires
authentication by having username in the http.proxy configuration,
an extra request needs to be made to the proxy to find out what
authentication method it supports, as this patch uses CURLAUTH_ANY
to let the library pick the most secure method supported by the
proxy server.

The extra round-trip adds extra latency, but relieves the user
from the burden to configure a specific authentication method.  If
it becomes problem, a later patch could add a configuration option
to specify what method to use, but let's start simple for the time
being.

So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't
want it activated manually from a config option, instead we added it
automatically when a proxy is being used.

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

diff --git a/http.c b/http.c
index 0ffd79c..8ac8eb6 100644
--- a/http.c
+++ b/http.c
@@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
-	if (curl_http_proxy)
+	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+	}
 
 	return result;
 }
-- 
1.7.7.6

Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[Junio C Hamano]

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

Is there anything specific about NTLM in this patch?  Let's retitle it to
something like

	http: support proxies that needs authentication

> ...
> So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't
> want it activated manually from a config option, instead we added it
> automatically when a proxy is being used.

I can sort of parse this paragraph but cannot follow the logic.

 - Because we use CURLAUTH_ANY, a proxy that requires authentication is
   supported out-of-the-box.

 - We do not want having to manually enable it.

 - Instead of adding a manual configuration, we do so automatically.

It sounds like it is saying the same thing three times.  Do we even need
that paragraph?  Shouldn't we drop it instead?

Otherwise it all looks good, so you can either

 (1) explain why the above suggestions are wrong and why I should apply
     the message I am responding to as-is;

 (2) send another re-roll; or

 (3) say "yeah, your suggestions all sound good to me", in which case I'll
     just locally amend and apply.

Thanks.

>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
>  http.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 0ffd79c..8ac8eb6 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)
>  	if (curl_ftp_no_epsv)
>  		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>  
> -	if (curl_http_proxy)
> +	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +	}
>  
>  	return result;
>  }

Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

[Nelson Benitez Leon]

On 03/02/2012 07:50 PM, Junio C Hamano wrote:
> Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:
> 
> Is there anything specific about NTLM in this patch?  Let's retitle it to
> something like
> 
> 	http: support proxies that needs authentication
> 
>> ...
>> So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't
>> want it activated manually from a config option, instead we added it
>> automatically when a proxy is being used.
> 
> I can sort of parse this paragraph but cannot follow the logic.
> 
>  - Because we use CURLAUTH_ANY, a proxy that requires authentication is
>    supported out-of-the-box.
> 
>  - We do not want having to manually enable it.
> 
>  - Instead of adding a manual configuration, we do so automatically.
> 
> It sounds like it is saying the same thing three times.  Do we even need
> that paragraph?  Shouldn't we drop it instead?

I've dropped it in v3 patchset.

> 
> Otherwise it all looks good, so you can either
> 
>  (1) explain why the above suggestions are wrong and why I should apply
>      the message I am responding to as-is;
> 
>  (2) send another re-roll; or
> 
>  (3) say "yeah, your suggestions all sound good to me", in which case I'll
>      just locally amend and apply.
> 

I've re-rolled a new patchset and included your suggestions about wording,
you're welcome to better fine-tune the messages and commit if you find them
suitable.

Thanks!