Cannot clone the git repository shared over http with authorization.

Cannot clone the git repository shared over http with authorization.

[Artur R. Czechowski]

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

Hello,

arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git
Cloning into 'test'...
Debug: Remote helper: -> capabilities
Debug: Remote helper: Waiting...
Debug: Remote helper: <- fetch
Debug: Got cap fetch
Debug: Remote helper: Waiting...
Debug: Remote helper: <- option
Debug: Got cap option
Debug: Remote helper: Waiting...
Debug: Remote helper: <- push
Debug: Got cap push
Debug: Remote helper: Waiting...
Debug: Remote helper: <- 
Debug: Capabilities complete.
Debug: Remote helper: Waiting...
Username for 'http://blabluga.hell.pl': test
Password for 'http://test@blabluga.hell.pl': 
Debug: Remote helper: <- @refs/heads/master HEAD
Debug: Remote helper: Waiting...
Debug: Remote helper: <- c64bcf957545f61436d405326d985521dc45058f refs/heads/master
Debug: Remote helper: Waiting...
Debug: Remote helper: <- 
Debug: Read ref listing.
Debug: Remote helper: -> option progress true
Debug: Remote helper: Waiting...
Debug: Remote helper: <- ok
Debug: Remote helper: -> option verbosity 1
Debug: Remote helper: Waiting...
Debug: Remote helper: <- ok
Debug: Remote helper: -> fetch c64bcf957545f61436d405326d985521dc45058f HEAD
fetch c64bcf957545f61436d405326d985521dc45058f refs/heads/master

Debug: Remote helper: Waiting...
error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)
error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git
Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588
while processing commit c64bcf957545f61436d405326d985521dc45058f.
error: Fetch failed.

The apache log from the action:

192.168.109.13 - - [01/Apr/2012:20:33:42 +0200] "GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/info/refs?service=git-upload-pack HTTP/1.1" 200 294 "-" "git/1.7.9.1"
192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/HEAD HTTP/1.1" 200 257 "-" "git/1.7.9.1"
192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/c6/4bcf957545f61436d405326d985521dc45058f HTTP/1.1" 200 403 "-" "git/1.7.9.1"
192.168.109.13 - test [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/b8/ab890a5432053f85d74a3b4ff798011260e222 HTTP/1.1" 200 288 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/fe/0e1075a026f19269bd958b05f586e252af7635 HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/e8/84293079beab9f2583b59b4e05479fc84fc588 HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/info/packs HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/info/http-alternates HTTP/1.1" 401 694 "-" "git/1.7.9.1"
192.168.109.13 - - [01/Apr/2012:20:33:50 +0200] "GET /git/test.git/objects/info/alternates HTTP/1.1" 401 694 "-" "git/1.7.9.1"

On the client side I'm using 1.7.9.1 from Debian package. Repository is
shared from apache using a DAV on directive (a person from #git channel
told me it's called a dumb http).

It's worth to say, that I've already asked for support on #git channel
and the problem exists with git/1.7.9.5 and git/1.7.10.rc3.3.g19a6c (at
least those versions can be seen in my access.log).

If necessary I can arrange an access to the repository.

Regards
        Artur
-- 
(ac) ,<                                             
    _ó_____     We all live in a yellow submarine...  
   <_______>                                             

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: Cannot clone the git repository shared over http with authorization.

[Clemens Buchacher]

On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote:
> 
> arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git
> Cloning into 'test'...
> error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)
> error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git
> Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588
> while processing commit c64bcf957545f61436d405326d985521dc45058f.
> error: Fetch failed.

I've been looking at this a bit. It's probably worth mentioning that the
problem does _not_ happen if username and password are specified in the
URL or in the .netrc. In both of those cases, curl is taking care of the
credentials itself.

So far I figured out that setting 'git config http.maxRequests 1' fixes
the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,
it seems that some GET requests use the credentials, while others do
not. My guess is that the CURLOPT_USERPWD option does not apply to all
threads.

Clemens

Re: Cannot clone the git repository shared over http with authorization.

[Clemens Buchacher]

On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:
> On Sun, Apr 01, 2012 at 08:48:04PM +0200, Artur R. Czechowski wrote:
> > 
> > arturcz@szczaw:/tmp$ git clone http://blabluga.hell.pl/git/test.git
> > Cloning into 'test'...
> > error: The requested URL returned error: 401 (curl_result = 22, http_code = 401, sha1 = e884293079beab9f2583b59b4e05479fc84fc588)
> > error: Unable to find e884293079beab9f2583b59b4e05479fc84fc588 under http://blabluga.hell.pl/git/test.git
> > Cannot obtain needed commit e884293079beab9f2583b59b4e05479fc84fc588
> > while processing commit c64bcf957545f61436d405326d985521dc45058f.
> > error: Fetch failed.

This is the minimal test case I could find to reproduce this reliably on
in our test suite. It seems that a certain number of loose objects is
also required to trigger the issue.

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 3c12b05..d31c083 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -77,6 +77,13 @@ SSLMutex file:ssl_mutex
 SSLEngine On
 </IfDefine>
 
+<Location /dumb/auth/>
+	AuthType Basic
+	AuthName "git-auth"
+	AuthUserFile passwd
+	Require valid-user
+</Location>
+
 <Location /auth/>
 	AuthType Basic
 	AuthName "git-auth"
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index e5e6b8f..3eb8753 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
 start_httpd
 
 test_expect_success 'setup repository' '
-	echo content >file &&
+	echo content1 >file &&
 	git add file &&
 	git commit -m one
+	echo content2 >file &&
+	git add file &&
+	git commit -m two
 '
 
 test_expect_success 'create http-accessible bare repository' '
-	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	cp -r .git "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-	 git --bare init &&
+	 git config core.bare true &&
+	 mv hooks-disabled hooks &&
 	 echo "exec git update-server-info" >hooks/post-update &&
-	 chmod +x hooks/post-update
+	 chmod +x hooks/post-update &&
+	 hooks/post-update
 	) &&
 	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	git push public master:master
@@ -73,6 +78,13 @@ expect_askpass() {
 	test_cmp askpass-expect askpass-query
 }
 
+test_expect_success 'clone password-protected repository with dumb http' '
+	>askpass-query &&
+	echo user@host >askpass-response &&
+	GIT_CURL_VERBOSE=1 git clone "$HTTPD_URL/dumb/auth/repo.git" repo-dumb-auth &&
+	expect_askpass both user@host
+'
+
 test_expect_success 'cloning password-protected repository can fail' '
 	>askpass-query &&
 	echo wrong >askpass-response &&

Re: Cannot clone the git repository shared over http with authorization.

[Jeff King]

On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:

> So far I figured out that setting 'git config http.maxRequests 1' fixes
> the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,
> it seems that some GET requests use the credentials, while others do
> not. My guess is that the CURLOPT_USERPWD option does not apply to all
> threads.

Yes, that is exactly it. Furthermore, the multi-fetch code paths used by
the http walker do not have the magic "notice a 401 and retry". I think
something like this should fix it (it passes your tests):

diff --git a/http.c b/http.c
index f3f83d7..c6dc9b7 100644
--- a/http.c
+++ b/http.c
@@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+	if (http_auth.password)
+		init_curl_http_auth(slot->curl);
 
 	return slot;
 }

which is basically just double-checking that we set CURLOPT_USERPWD
whenever we get a slot (I wish there was some way of asking "does this
curl handle have USERPWD already set?", but I couldn't fine one).

It still doesn't do 401-handling itself, but it works OK in practice
because the first request will be a non-multi fetch (to get info/refs),
which recognizes the 401 and prompts the user, and then that credential
gets used subsequently.

-Peff

Re: Cannot clone the git repository shared over http with authorization.

[Clemens Buchacher]

On Mon, Apr 02, 2012 at 04:31:15AM -0400, Jeff King wrote:
> On Sun, Apr 01, 2012 at 09:45:36PM +0200, Clemens Buchacher wrote:
> 
> > So far I figured out that setting 'git config http.maxRequests 1' fixes
> > the problem as well. Looking at the output with GIT_CURL_VERBOSE=1 set,
> > it seems that some GET requests use the credentials, while others do
> > not. My guess is that the CURLOPT_USERPWD option does not apply to all
> > threads.
> 
> Yes, that is exactly it. Furthermore, the multi-fetch code paths used by
> the http walker do not have the magic "notice a 401 and retry". I think
> something like this should fix it (it passes your tests),
> which is basically just double-checking that we set CURLOPT_USERPWD
> whenever we get a slot (I wish there was some way of asking "does this
> curl handle have USERPWD already set?", but I couldn't fine one).

I think this is the way to go. I am sending the updated patch for the test
(much of my changes were unnecessary), and your fix on top.

[PATCH 1/2] http auth fails with multiple curl handles
[PATCH 2/2] fix http auth with multiple curl handles

Clemens

[PATCH 1/2] http auth fails with multiple curl handles

[Clemens Buchacher]

Create a repo with multiple loose objects in order to demonstrate http
authentication breakage.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 t/t5550-http-fetch.sh |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index e5e6b8f..1d9ff1e 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -13,17 +13,22 @@ LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
 start_httpd
 
 test_expect_success 'setup repository' '
-	echo content >file &&
+	echo content1 >file &&
 	git add file &&
 	git commit -m one
+	echo content2 >file &&
+	git add file &&
+	git commit -m two
 '
 
-test_expect_success 'create http-accessible bare repository' '
-	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+test_expect_success 'create http-accessible bare repository with loose objects' '
+	cp -a .git "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-	 git --bare init &&
+	 git config core.bare true &&
+	 mkdir -p hooks &&
 	 echo "exec git update-server-info" >hooks/post-update &&
-	 chmod +x hooks/post-update
+	 chmod +x hooks/post-update &&
+	 hooks/post-update
 	) &&
 	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
 	git push public master:master
@@ -87,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' '
 	expect_askpass none
 '
 
-test_expect_success 'http auth can use just user in URL' '
+test_expect_failure 'http auth can use just user in URL' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
 	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass &&
 	expect_askpass pass user@host
 '
 
-test_expect_success 'http auth can request both user and pass' '
+test_expect_failure 'http auth can request both user and pass' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
 	git clone "$HTTPD_URL/auth/repo.git" clone-auth-both &&
 	expect_askpass both user@host
 '
 
-test_expect_success 'http auth respects credential helper config' '
+test_expect_failure 'http auth respects credential helper config' '
 	test_config_global credential.helper "!f() {
 		cat >/dev/null
 		echo username=user@host
@@ -113,7 +118,7 @@ test_expect_success 'http auth respects credential helper config' '
 	expect_askpass none
 '
 
-test_expect_success 'http auth can get username from config' '
+test_expect_failure 'http auth can get username from config' '
 	test_config_global "credential.$HTTPD_URL.username" user@host &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
@@ -121,7 +126,7 @@ test_expect_success 'http auth can get username from config' '
 	expect_askpass pass user@host
 '
 
-test_expect_success 'configured username does not override URL' '
+test_expect_failure 'configured username does not override URL' '
 	test_config_global "credential.$HTTPD_URL.username" wrong &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
-- 
1.7.9.6

[PATCH 2/2] fix http auth with multiple curl handles

[Clemens Buchacher]

From: Jeff King <peff@peff.net>

HTTP authentication is currently handled by get_refs and fetch_ref, but
not by fetch_object, fetch_pack or fetch_alternates. In the
single-threaded case, this is not an issue, since get_refs is always
called first. It recognigzes the 401 and prompts the user for
credentials, which will then be used subsequently.

If the curl multi interface is used, however, only the multi handle used
by get_refs will have credentials configured. Requests made by other
handles fail with an authentication error.

Fix this by setting CURLOPT_USERPWD whenever a slot is requested.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
 http.c                |    2 ++
 t/t5550-http-fetch.sh |   10 +++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/http.c b/http.c
index f3f83d7..c6dc9b7 100644
--- a/http.c
+++ b/http.c
@@ -494,6 +494,8 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+	if (http_auth.password)
+		init_curl_http_auth(slot->curl);
 
 	return slot;
 }
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index 1d9ff1e..b06f817 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -92,21 +92,21 @@ test_expect_success 'http auth can use user/pass in URL' '
 	expect_askpass none
 '
 
-test_expect_failure 'http auth can use just user in URL' '
+test_expect_success 'http auth can use just user in URL' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
 	git clone "$HTTPD_URL_USER/auth/repo.git" clone-auth-pass &&
 	expect_askpass pass user@host
 '
 
-test_expect_failure 'http auth can request both user and pass' '
+test_expect_success 'http auth can request both user and pass' '
 	>askpass-query &&
 	echo user@host >askpass-response &&
 	git clone "$HTTPD_URL/auth/repo.git" clone-auth-both &&
 	expect_askpass both user@host
 '
 
-test_expect_failure 'http auth respects credential helper config' '
+test_expect_success 'http auth respects credential helper config' '
 	test_config_global credential.helper "!f() {
 		cat >/dev/null
 		echo username=user@host
@@ -118,7 +118,7 @@ test_expect_failure 'http auth respects credential helper config' '
 	expect_askpass none
 '
 
-test_expect_failure 'http auth can get username from config' '
+test_expect_success 'http auth can get username from config' '
 	test_config_global "credential.$HTTPD_URL.username" user@host &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
@@ -126,7 +126,7 @@ test_expect_failure 'http auth can get username from config' '
 	expect_askpass pass user@host
 '
 
-test_expect_failure 'configured username does not override URL' '
+test_expect_success 'configured username does not override URL' '
 	test_config_global "credential.$HTTPD_URL.username" wrong &&
 	>askpass-query &&
 	echo user@host >askpass-response &&
-- 
1.7.9.6

Re: [PATCH 2/2] fix http auth with multiple curl handles

[Jeff King]

On Tue, Apr 10, 2012 at 11:53:40AM +0200, Clemens Buchacher wrote:

> HTTP authentication is currently handled by get_refs and fetch_ref, but
> not by fetch_object, fetch_pack or fetch_alternates. In the
> single-threaded case, this is not an issue, since get_refs is always
> called first. It recognigzes the 401 and prompts the user for
> credentials, which will then be used subsequently.
> 
> If the curl multi interface is used, however, only the multi handle used
> by get_refs will have credentials configured. Requests made by other
> handles fail with an authentication error.
> 
> Fix this by setting CURLOPT_USERPWD whenever a slot is requested.

The reason I didn't like my initial patch was that by calling
curl_easy_setopt() for every request, we end up leaking the password
buffer once per request.

Unfortunately, I don't think there's a way to ask curl whether it has
anything in CURLOPT_USERPWD. So we have to overwrite. Recent versions of
curl will actually copy the string we give it anyway, so the existing
code is already leaking a little bit (but once per process, not once per
request). I wish we could get away with just handing curl the data and
assuming it would copy, but that code came about in curl 7.17.0, in
2007. According to our #if statements, we handle much older versions of
curl, so that is a non-option.

I think the best we can do is to put the auth data in a static buffer
and feed that to curl. We end up rewriting the auth data into our buffer
over and over, but at least we don't re-malloc it. Like this:

diff --git a/http.c b/http.c
index f3f83d7..374c3bb 100644
--- a/http.c
+++ b/http.c
@@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)
 static void init_curl_http_auth(CURL *result)
 {
 	if (http_auth.username) {
-		struct strbuf up = STRBUF_INIT;
+		static struct strbuf up = STRBUF_INIT;
 		credential_fill(&http_auth);
+		strbuf_reset(&up);
 		strbuf_addf(&up, "%s:%s",
 			    http_auth.username, http_auth.password);
-		curl_easy_setopt(result, CURLOPT_USERPWD,
-				 strbuf_detach(&up, NULL));
+		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
 	}
 }
 

By the way, this touches on an area that I noticed while refactoring the
http auth code a while ago, but decided not to tackle at the time. We
fill in the auth information early, and then never bother to revisit it
as URLs change. So for example, if I got a redirect from host A to host
B, we would continue to use the credential for host A and host B. Which
is maybe convenient, and maybe a security issue.

It has been that way since the beginning of git, and nobody has seemed
to care. So maybe it is not worth dealing with. But if we did want to,
the right way would be to keep several credentials on hand, and match
each URL to them as we were about to request it. That would also provide
a fix to the problem we are fixing here. I don't know if it is worth
doing or not.

-Peff

Re: [PATCH 2/2] fix http auth with multiple curl handles

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...
> I think the best we can do is to put the auth data in a static buffer
> and feed that to curl. We end up rewriting the auth data into our buffer
> over and over, but at least we don't re-malloc it. Like this:
>
> diff --git a/http.c b/http.c
> index f3f83d7..374c3bb 100644
> --- a/http.c
> +++ b/http.c
> @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)
>  static void init_curl_http_auth(CURL *result)
>  {
>  	if (http_auth.username) {
> -		struct strbuf up = STRBUF_INIT;
> +		static struct strbuf up = STRBUF_INIT;
>  		credential_fill(&http_auth);
> +		strbuf_reset(&up);
>  		strbuf_addf(&up, "%s:%s",
>  			    http_auth.username, http_auth.password);
> -		curl_easy_setopt(result, CURLOPT_USERPWD,
> -				 strbuf_detach(&up, NULL));
> +		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
>  	}
>  }

Yeah, that's sad but I agree that is probably the best we could do.  Do
you want me to squash it in?

> By the way, this touches on an area that I noticed while refactoring the
> http auth code a while ago, but decided not to tackle at the time. We
> fill in the auth information early, and then never bother to revisit it
> as URLs change. So for example, if I got a redirect from host A to host
> B, we would continue to use the credential for host A and host B. Which
> is maybe convenient, and maybe a security issue.

Good point.  Do we follow redirects, though?

Re: [PATCH 2/2] fix http auth with multiple curl handles

[Jeff King]

On Thu, Apr 12, 2012 at 03:50:08PM -0700, Junio C Hamano wrote:

> > diff --git a/http.c b/http.c
> > index f3f83d7..374c3bb 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)
> >  static void init_curl_http_auth(CURL *result)
> >  {
> >  	if (http_auth.username) {
> > -		struct strbuf up = STRBUF_INIT;
> > +		static struct strbuf up = STRBUF_INIT;
> >  		credential_fill(&http_auth);
> > +		strbuf_reset(&up);
> >  		strbuf_addf(&up, "%s:%s",
> >  			    http_auth.username, http_auth.password);
> > -		curl_easy_setopt(result, CURLOPT_USERPWD,
> > -				 strbuf_detach(&up, NULL));
> > +		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
> >  	}
> >  }
> 
> Yeah, that's sad but I agree that is probably the best we could do.  Do
> you want me to squash it in?

We can use an #if-macro and only do the static thing for older versions
of curl (newer versions copy for us). But since we'd have to carry the
old code anyway, it doesn't clean up much.  However, since there is an
even newer interface for giving curl the credential, it might be worth
conditionally including that.

I'd rather not squash this bit. It's sufficiently subtle that I'd rather
do it on top as a separate patch. Plus I gotta keep up my shortlog
stats. ;)

So here are the two patches I think we should apply on top of what you
have in ct/http-multi-curl-auth.

  [1/2]: http: clean up leak in init_curl_http_auth
  [2/2]: http: use newer curl options for setting credentials

> > By the way, this touches on an area that I noticed while refactoring the
> > http auth code a while ago, but decided not to tackle at the time. We
> > fill in the auth information early, and then never bother to revisit it
> > as URLs change. So for example, if I got a redirect from host A to host
> > B, we would continue to use the credential for host A and host B. Which
> > is maybe convenient, and maybe a security issue.
> 
> Good point.  Do we follow redirects, though?

Curl follows them for us, since we set CURLOPT_FOLLOWLOCATION. However,
it does say this under CURLOPT_USERPWD (which I somehow missed the last
time I looked into this issue):

  When using HTTP and CURLOPT_FOLLOWLOCATION, libcurl might perform
  several requests to possibly different hosts. libcurl will only send
  this user and password information to hosts using the initial host
  name (unless CURLOPT_UNRESTRICTED_AUTH is set), so if libcurl
  follows locations to other hosts it will not send the user and
  password to those. This is enforced to prevent accidental information
  leakage.

So it looks like we are already doing the safe thing (unless we redirect
ourselves outside of curl, but I don't think we ever do so). We won't
properly retry auth if we get a 401 on a redirect (we never retry auth
if we already had both a username and password; we complain and exit).
But nobody has been complaining about that, so I guess it is a
non-issue.

-Peff

[PATCH 1/2] http: clean up leak in init_curl_http_auth

[Jeff King]

When we have a credential to give to curl, we must copy it
into a "user:pass" buffer and then hand the buffer to curl.
Old versions of curl did not copy the buffer, and we were
expected to keep it valid. Newer versions of curl will copy
the buffer.

Our solution was to use a strbuf and detach it, giving
ownership of the resulting buffer to curl. However, this
meant that we were leaking the buffer on newer versions of
curl, since curl was just copying it and throwing away the
string we passed. Furthermore, when we replaced a
credential (e.g., because our original one was rejected), we
were also leaking on both old and new versions of curl.

This got even worse in the last patch, which started
replacing the credential (and thus leaking) on every http
request.

Instead, let's use a static buffer to make the ownership
more clear and less leaky.  We already keep a static "struct
credential", so we are only handling a single credential at
a time, anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
The "old" version was prior to 7.17.0, from 2007. That is probably still
new enough to care about. But at some point we should probably make a
sweep through http.c and get rid of conditional blocks for really old
cruft.

 http.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index c6dc9b7..eaf7f40 100644
--- a/http.c
+++ b/http.c
@@ -211,12 +211,12 @@ static int http_options(const char *var, const char *value, void *cb)
 static void init_curl_http_auth(CURL *result)
 {
 	if (http_auth.username) {
-		struct strbuf up = STRBUF_INIT;
+		static struct strbuf up = STRBUF_INIT;
 		credential_fill(&http_auth);
+		strbuf_reset(&up);
 		strbuf_addf(&up, "%s:%s",
 			    http_auth.username, http_auth.password);
-		curl_easy_setopt(result, CURLOPT_USERPWD,
-				 strbuf_detach(&up, NULL));
+		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
 	}
 }
 
-- 
1.7.9.6.6.g6b3b56

[PATCH 2/2] http: use newer curl options for setting credentials

[Jeff King]

We give the username and password to curl by sticking them
in a buffer of the form "user:pass" and handing the result
to CURLOPT_USERPWD. Since curl 7.19.1, there is a split
mechanism, where you can specify each element individually.

This has the advantage that a username can contain a ":"
character. It also is less code for us, since we can hand
our strings over to curl directly. And since curl 7.17.0 and
higher promise to copy the strings for us, we we don't even
have to worry about memory ownership issues.

Unfortunately, we have to keep the ugly code for old curl
around, but as it is now nicely #if'd out, we can easily get
rid of it when we decide that 7.19.1 is "old enough".

Signed-off-by: Jeff King <peff@peff.net>
---
For reference, 7.19.1 is from late 2008.

 http.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index eaf7f40..2ec3789 100644
--- a/http.c
+++ b/http.c
@@ -210,14 +210,23 @@ static int http_options(const char *var, const char *value, void *cb)
 
 static void init_curl_http_auth(CURL *result)
 {
-	if (http_auth.username) {
+	if (!http_auth.username)
+		return;
+
+	credential_fill(&http_auth);
+
+#if LIBCURL_VERSION_NUM >= 0x071301
+	curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);
+	curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);
+#else
+	{
 		static struct strbuf up = STRBUF_INIT;
-		credential_fill(&http_auth);
 		strbuf_reset(&up);
 		strbuf_addf(&up, "%s:%s",
 			    http_auth.username, http_auth.password);
 		curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
 	}
+#endif
 }
 
 static int has_cert_password(void)
-- 
1.7.9.6.6.g6b3b56