Re: [PATCH] http: always use any proxy auth method available

Enrique Tobis [off-list ref] writes:

Thanks.  I wonder why this was addressed me directly (i.e. I am not
an area expert, and I haven't seen this patch discussed here and
reviewed by other people), but anyway...

By default, libcurl honors some environment variables that specify a
proxy (e.g. http_proxy, https_proxy). Also by default, libcurl will
only try to authenticate with a proxy using the Basic method. 

OK, that is a statement of two facts.

What's missing here is what they relate to this change.  Are these
two good things that we want to keep?  Are these bad things we need
to tweak out by changing our software?  Or some combination?  Some
third key information that is left untold?

This
change makes libcurl always try the most secure proxy authentication
method available. As a consequence, you can use environment variables
to instruct git to use a proxy that uses an authentication method
different from Basic (e.g. Negotiate).

That is a worthy goal, but the description of the current problem
seems lacking.  Perhaps you meant something like this:

	We use CURLOPT_PROXYAUTH to ask for the most secure
        authentication method with proxy only when we have
        curl_http_proxy set, by http.proxy or remote.*.proxy
        configuration variables.  However, libcurl also allows users
        to use http proxies by setting some environment variables,
        and by default the authentication with the proxy uses Basic
        auth (unless specified with CURLOPT_PROXYAUTH, that is).

	By always using CURLOPT_PROXYAUTH to ask for the most secure
	authentication method, even when we are not aware that we
	are using proxy (because there is no configuration that
	tells us so), we can allow users to tell libcurl to use
	a proxy with more secure method without setting http.proxy
        or remote.*.proxy configuration variables.

But I am just guessing; as I said, I am not an expert in this area
of the code.

The authoritative source of truth:

  https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions

matches this version number, so there is nothing wrong per-se on
this line, but it makes me wonder why we didn't do

	#ifdef CURLOPT_PROXYAUTH

instead.  That's not something that should be changed with this
change, though.

-		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

 #endif
-	}
 
 	set_curl_keepalive(result);

Assuming that I guessed your justification for this change corretly
in the earlier part of this message, I think the change makes sense.

Thanks.

Thanks for your feedback!

-----Original Message-----
From: Junio C Hamano [mailto:jch2355@gmail.com] On Behalf Of Junio C
Hamano
Sent: Friday, June 26, 2015 15:25
To: Enrique Tobis
Cc: 'git@vger.kernel.org'; 'Nelson Benitez Leon'
Subject: Re: [PATCH] http: always use any proxy auth method available

Enrique Tobis [off-list ref] writes:

Thanks.  I wonder why this was addressed me directly (i.e. I am not an area
expert, and I haven't seen this patch discussed here and reviewed by other
people), but anyway...

I apologize for that. I misunderstood part of the instructions for submitting patches.

quoted

By default, libcurl honors some environment variables that specify a
proxy (e.g. http_proxy, https_proxy). Also by default, libcurl will
only try to authenticate with a proxy using the Basic method.

OK, that is a statement of two facts.

What's missing here is what they relate to this change.  Are these two good
things that we want to keep?  Are these bad things we need to tweak out by
changing our software?  Or some combination?  Some third key information
that is left untold?

quoted

This
change makes libcurl always try the most secure proxy authentication
method available. As a consequence, you can use environment variables
to instruct git to use a proxy that uses an authentication method
different from Basic (e.g. Negotiate).

That is a worthy goal, but the description of the current problem seems
lacking.  Perhaps you meant something like this:

	We use CURLOPT_PROXYAUTH to ask for the most secure
        authentication method with proxy only when we have
        curl_http_proxy set, by http.proxy or remote.*.proxy
        configuration variables.  However, libcurl also allows users
        to use http proxies by setting some environment variables,
        and by default the authentication with the proxy uses Basic
        auth (unless specified with CURLOPT_PROXYAUTH, that is).

	By always using CURLOPT_PROXYAUTH to ask for the most secure
	authentication method, even when we are not aware that we
	are using proxy (because there is no configuration that
	tells us so), we can allow users to tell libcurl to use
	a proxy with more secure method without setting http.proxy
        or remote.*.proxy configuration variables.

But I am just guessing; as I said, I am not an expert in this area of the code.

How about this?

"
We set CURLOPT_PROXYAUTH to use the most secure authentication method available only when the user has set configuration variables to specify a proxy. However, libcurl also supports specifying a proxy through environment variables. In that case libcurl defaults to only using the Basic proxy authentication method.

This change sets CURLOPT_PROXYAUTH to always use the most secure authentication method available, even when there is no git configuration telling us to use a proxy. This allows the user to use environment variables to configure a proxy that requires an authentication method different from Basic.
"

quoted

Signed-off-by: Enrique A. Tobis <redacted>
---
 http.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index f0c5bbc..e9c6fdd 100644
--- a/http.c
+++ b/http.c

@@ -416,10 +416,10 @@ static CURL *get_curl_handle(void)

 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+	}
 #if LIBCURL_VERSION_NUM >= 0x070a07

The authoritative source of truth:

  https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-
versions

matches this version number, so there is nothing wrong per-se on this line,
but it makes me wonder why we didn't do

	#ifdef CURLOPT_PROXYAUTH

instead.  That's not something that should be changed with this change,
though.

quoted

-		curl_easy_setopt(result, CURLOPT_PROXYAUTH,

CURLAUTH_ANY);

quoted

+	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

quoted

 #endif
-	}

 	set_curl_keepalive(result);

Assuming that I guessed your justification for this change corretly in the
earlier part of this message, I think the change makes sense.

You did guess correctly. As you said you are not an expert in this area, should I wait until someone else chimes in or is this enough to resubmit for inclusion? Assuming my revised explanation is acceptable, of course.
 

Thanks.

Thank you!

Enrique