Patchset NTLM-Authentication

Patchset NTLM-Authentication

[Schmidt, Marco]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

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

[-- Attachment #2: 0002-Einf-hren-der-HTTP-Proxy-Authentifizierung.patch --]
[-- Type: application/octet-stream, Size: 1553 bytes --]

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


[-- Attachment #3: 0003-First-working-step.patch --]
[-- Type: application/octet-stream, Size: 1519 bytes --]

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


[-- Attachment #4: 0004-remove-debug-information.patch --]
[-- Type: application/octet-stream, Size: 1512 bytes --]

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

Re: Patchset NTLM-Authentication

[Thomas Rast]

"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

Re: Patchset NTLM-Authentication

[Thomas Rast]

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

> 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

Re: Patchset NTLM-Authentication

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

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

Re: Patchset NTLM-Authentication

[Daniel Stenberg]

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