v2, 3 partshttp: authenticate on NTLM proxies and others suppported by CuRL

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

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?

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.

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.

On 03/01/2012 08:07 PM, Junio C Hamano wrote:

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

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

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.

On 03/02/2012 07:50 PM, Junio C Hamano wrote:

I've dropped it in v3 patchset.

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!