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

From: Jeff King <peff@peff.net>
Date: Apr 13, 2012, 06:16 UTC
Message-ID: <20120413061622.GA27591@sigill.intra.peff.net>
In-Reply-To: <7v4nso4lb3.fsf@alter.siamese.dyndns.org>

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

Show 22 quoted lines

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

Show 8 quoted lines

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