threads / discuss / 29678

Patchset NTLM-Authentication

Subject: Patchset NTLM-Authentication

## tl;dr

5 messages between Feb 21, 2012 and Feb 21, 2012.

replies: 4people: 4as markdown or json

Schmidt, Marco· Feb 21, 2012, 08:07 UTC · lore
Support NTLM-Authentication against proxy.

I have include a minimal patch set to support NTLM authentification against a proxy server.

The patch adds a boolean configuration variable "http.proxyauthany" to enable CURLOPT_PROXYAUTH = CURLAUTH_ANY.

I'm not conform with your patch submitting policy. I had no chance to track the git repository from my current location.

Marco Schmidt
From 8a3892eca69a0c603a53b3d3aa052f45c4a5b648 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 13:08:44 +0100
Subject: [PATCH 2/4] =?UTF-8?q?Einf=FChren=20der=20HTTP=20Proxy=20Authentifi?=
 =?UTF-8?q?zierung?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
 http.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index f3b2c90..dc51d41 100644
--- a/http.c
+++ b/http.c
@@ -41,6 +41,7 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static int curl_http_proxyauthany = 0;
 static const char *curl_cookie_file;
 static char *user_name, *user_pass, *description;
 static const char *user_agent;
@@ -222,6 +223,10 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
+	
+	if (!strcmp("http.proxyauthany", var))  {
+		curl_http_proxyauthany = git_config_bool(var, value);
+	}
 
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
@@ -330,6 +335,12 @@ static CURL *get_curl_handle(void)
 
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+	
+	if (curl_http_proxyauthany) {
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+		printf("CURLOPT_PROXYAUTH!!!!");
+	}
+
 
 	return result;
 }

-- 
1.7.8



From 2b7336c4959fc4df30da9e68f652f2c5941f0c91 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 14:35:15 +0100
Subject: [PATCH 3/4] First working step

---
 http.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index dc51d41..3dbd390 100644
--- a/http.c
+++ b/http.c
@@ -173,6 +173,8 @@ static int git_config_path(const char **result,
 
 static int http_options(const char *var, const char *value, void *cb)
 {
+	fprintf(stderr, "http_options: %s = %s\n", var, value);
+	
 	if (!strcmp("http.sslverify", var)) {
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
@@ -226,6 +228,8 @@ static int http_options(const char *var, const char *value, void *cb)
 	
 	if (!strcmp("http.proxyauthany", var))  {
 		curl_http_proxyauthany = git_config_bool(var, value);
+		fprintf(stderr, "http_options: curl_http_proxyauthany = %i", curl_http_proxyauthany);
+		return 0;
 	}
 
 	if (!strcmp("http.cookiefile", var))
@@ -336,9 +340,12 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	
+	fprintf(stderr, "DEBUG: curl_http_proxyauthany\n");
+	
+	/* http proxy could be set from enviroment - set CURLOPT_PROXYAUTH */
 	if (curl_http_proxyauthany) {
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-		printf("CURLOPT_PROXYAUTH!!!!");
+		fprintf(stderr, "curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n");
 	}
 
 
-- 
1.7.8



From 55b76b5f658904d240db29bb0d59f8c1dcedd4b5 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 14:40:57 +0100
Subject: [PATCH 4/4] remove debug information

---
 http.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index 3dbd390..fbc52f0 100644
--- a/http.c
+++ b/http.c
@@ -173,8 +173,6 @@ static int git_config_path(const char **result,
 
 static int http_options(const char *var, const char *value, void *cb)
 {
-	fprintf(stderr, "http_options: %s = %s\n", var, value);
-	
 	if (!strcmp("http.sslverify", var)) {
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
@@ -228,7 +226,6 @@ static int http_options(const char *var, const char *value, void *cb)
 	
 	if (!strcmp("http.proxyauthany", var))  {
 		curl_http_proxyauthany = git_config_bool(var, value);
-		fprintf(stderr, "http_options: curl_http_proxyauthany = %i", curl_http_proxyauthany);
 		return 0;
 	}
 
@@ -340,13 +337,12 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	
-	fprintf(stderr, "DEBUG: curl_http_proxyauthany\n");
-	
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY	
 	/* http proxy could be set from enviroment - set CURLOPT_PROXYAUTH */
 	if (curl_http_proxyauthany) {
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-		fprintf(stderr, "curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n");
 	}
+#endif
 
 
 	return result;
-- 
1.7.8
Thomas Rast· Feb 21, 2012, 09:22 UTC · re: Schmidt, Marco · lore

Re: Patchset NTLM-Authentication

"Schmidt, Marco" <Marco.Schmidt@cassidian.com> writes:
> I'm not conform with your patch submitting policy. I had no chance to
> track the git repository from my current location.

You should read Documentation/SubmittingPatches. If your location does not let you use any git:// or git+http:// ("smart http") transports, you can use the wereHamster's bundler service https://bundler.caurea.org/ to get a bundle that you can download over http. (If you cannot download *anything* over http, what is your internet access good for?)

Looking at the attachments:

[0002-Einf-hren-der-HTTP-Proxy-Authentifizierung.patch] [0003-First-working-step.patch] [0004-remove-debug-information.patch]

* You seem to have forgotten the first patch.  As it stands, the 0002
  one does not apply to my tree.
* The commit messages are useless.  The second one at least says
  "introduce HTTP proxy authentication" which tells me something about
  *what it does*, but in German.  It should also tell me why and how and
  possible other considerations.  See SubmittingPatches.
* The commits should "spring into perfect existence".  That is, in the
  above you should squash 3 and 4 into 2 so that 2 starts out working
  (presumably -- I'm only looking at the subjects) .  You can use rebase
  -i to clean everything up.
-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Thomas Rast· Feb 21, 2012, 13:28 UTC · lore

Re: Patchset NTLM-Authentication

"Schmidt, Marco" <Marco.Schmidt@cassidian.com> writes:
> Thanks for your reply. I have used the wereHamster bundler service and
> created the following patch. May I ask you to test the patch and check
> again for more style violations?

Please keep these discussions public, so that others can learn and (heaven forbid...) point out _my_ mistakes.

I'll discuss the file you attached below. Keep in mind that unless there is a compelling reason not to do that, you should send your patches inline. This allows for much easier review on the list.

Show 5 quoted lines
> Subject: [PATCH] Allow NTLM-Authentication against a http-proxy server - Add
>  config option "http.proxyauthany" - Set CURLOPT_PROXYAUTH
>  to CURLAUTH_ANY if option curl_http_proxyauthany is true.
> 
> ---
You can already see that you did not adhere to the standard format of a
commit message: one line summary, followed by a blank line, followed by
a long description.  So the above might have been written (dropping the
Subject: pseudoheader tag now):

} Allow NTLM-Authentication against a http-proxy server } } - Add config option "http.proxyauthany" } } - Set CURLOPT_PROXYAUTH to CURLAUTH_ANY if option } curl_http_proxyauthany is true.

Now bear in mind that I have no clue about curl (except that it downloads stuff ;-) and especially about its authentication parts. But allow me to sketch a different commit message that highlights what I would like to see answered. I am merely using the knowledge I could glean from 2min of googling around; I am not saying that anything of it is correct, so you should fix it as needed!

  http/curl: let user configure "any" proxy authentication
  Normally, curl uses only the "basic" authentication scheme when
  talking to proxies, which may not be desirable (it sends the password
  in cleartext) or sufficient (the author needs NTLM authentication for
  his proxy).
  Introduce the config setting http.proxyAuthAny.  When enabled, we tell
  curl to use any authentication scheme supported by the proxy.
  This mostly parallels http.authAny which was introduced in b8ac923
  (Add an option for using any HTTP authentication scheme, not only
  basic, 2009-11-27).  http.authAny was removed, and its feature
  unconditionally enabled, in 525ecd2 (Remove http.authAny, 2009-12-28).
  However the reasoning of the latter does not apply here because XXXX.

Some notes on the code/patch: There was some trailing whitespace, please make sure you remove it.

Usually we introduce a setting (command line option or environment variable) that can be tweaked to override the configuration. b8ac923 did this if you need some inspiration :-) Otherwise you should make an argument why this is not needed.

 +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	if (curl_http_proxyauthany) {
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +	}
> +#endif

My google is failing me as to when this feature was introduced or how common curls without it still are. But you could be extra doubly nice to the user and warn if s/he enabled the feature, but git cannot adhere to the request because curl doesn't support it. (Then again b8ac923 didn't go that far either.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Junio C Hamano· Feb 21, 2012, 18:02 UTC · re: Thomas Rast · lore

Re: Patchset NTLM-Authentication

Thomas Rast <trast@inf.ethz.ch> writes:
Show 5 quoted lines
>   This mostly parallels http.authAny which was introduced in b8ac923
>   (Add an option for using any HTTP authentication scheme, not only
>   basic, 2009-11-27).  http.authAny was removed, and its feature
>   unconditionally enabled, in 525ecd2 (Remove http.authAny, 2009-12-28).
>   However the reasoning of the latter does not apply here because XXXX.
Thanks, Thomas.

I think this paragraph is essential, especially the XXXX part, if we were to accept the proposed change and keep the new configuration. Otherwise we won't know what to do when somebody proposes to unconditionally enable this ;-)

If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY without compromising security, then an explanation why this does not have to be optional, similar to what justified 525ecd2, needs to be there instead, and the patch needs to be tweaked to drop the configuration bits.

Marco, I extracted your patch in the attachment and took a look at it before composing the above response.

 - Your log message seems to be indented by two spaces for some strange
   reason;
 - it does not have any justification like the example Thomas gave
   you; and
 - it also is missing your S-o-b.
Care to re-roll one more time?
Daniel Stenberg· Feb 21, 2012, 19:02 UTC · re: Junio C Hamano · lore

Re: Patchset NTLM-Authentication

On Tue, 21 Feb 2012, Junio C Hamano wrote:
> If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY 
> without compromising security, then an explanation why this does not have to 
> be optional, similar to what justified 525ecd2, needs to be there instead, 
> and the patch needs to be tweaked to drop the configuration bits.
Allow me to provide some libcurl info on this!

Setting it to ANY will unconditionally cause an extra roundtrip which you can avoid if you know what auth type the proxy wants and you set it at once. With ANY set, libcurl will first "probe" the proxy to figure out which type to use and then go on and actually do it in a second request (and possibly even a third request in some cases).

It can actually be seen as a security _improvement_ in some cases where for example Basic auth (user+password sent as plain text) can be avoided in preference to a more secure mechanism, but I think that's a rather rare case for git.

IMO, if ANY is considered fine for normal host authentication I think it could be considered fine for proxy authentication as well.

-- 
  / daniel.haxx.se

← back to recent threads