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

[Jeff King <peff@peff.net>]

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