git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

From
KFKnut Franke <k.franke@science-computing.de>
Date
Nov 5, 2015, 11:56 UTC
Message-ID
<20151105115654.GA6786@science-computing.de>
In-Reply-To
<20151105082421.GB6819@sigill.intra.peff.net>
On 2015-11-05 03:24, Jeff King wrote:
Show 5 quoted lines
> There was also some discussion with curl upstream of providing a new
> authentication interface, where we would provide curl with
> authentication callbacks, and it would trigger them if and when
> credentials were needed. Somebody upstream was working on a patch, but I
> don't think it ever got merged. :(

That would certainly be nice, also with respect to other credentials, such as SSL key passphrase (presuming that'd be possible without modifying the SSL lib as well).

> Here's a relevant bit from that old series (which doesn't seem threaded,
> but you can search for the author if you want to see more):
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/192246

My main takeaway from this, apart from the points you mention below, is that it'd be good to have a test case, similar to t/lib-httpd.sh. Since none of the existent proxy-related code has an automated test, I think this would be an improvement on top of the other patches. I'd need to look into how easy/hard this would be to implement.

Show 7 quoted lines
> > +
> > +		curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> > +			&slot->results->http_connectcode);
> 
> It looks like you use this to see the remote side's HTTP 407 code.  In
> the 2012 series, I think we simply looked for a 407 in the HTTP return
> code

I'm not sure why that worked for the author of the old series - possibly curl semantics changed at some point. In my setup at least (with curl 7.15.5), after a failed proxy authentication, CURLINFO_HTTP_CODE returns 0 while CURLINFO_HTTP_CONNECTCODE returns the 407. This is also consistent with the curl documentation for CURLINFO_RESPONSE_CODE (which has replaced CURLINFO_HTTP_CODE in 7.10.7, though the compatibility #define is still there): "Note that a proxy's CONNECT response should be read with CURLINFO_HTTP_CONNECTCODE and not this."

> If we do need CONNECTCODE, do we need to protect it with an #ifdef on
> the curl version? The manpage says it came in 7.10.7, which was released
> in 2003. That's probably old enough not to worry about.

As Junio pointed out earlier, since some people still care about ancient curl versions, we don't want to knowingly break compatibility. So yes, an #ifdef would be in oder here.

Show 10 quoted lines
> 
> > +	if (proxy_auth.password) {
> > +		memset(proxy_auth.password, 0, strlen(proxy_auth.password));
> > +		free(proxy_auth.password);
> 
> My understanding is that memset() like this is not sufficient for
> zero-ing sensitive data, as they can be optimized out by the compiler. I
> don't think there's a portable alternative, though, so it may be the
> best we can do. OTOH, the rest of git does not worry about such zero-ing
> anyway, so we could also simply omit it here.

For what it's worth, that's the same as we do for cert_auth (while, as far as I can see, no attempt is made for http_auth). I tend to think it's better than nothing. Maybe an in-code comment stating it's not reliable would be in order, to prevent the passing reader from putting too much trust in it.

Show 5 quoted lines
> > +	free((void *)curl_proxyuserpwd);
> 
> This cast is necessary because curl_proxyuserpwd is declared const. But
> I do not see anywhere that it needs to be const (we detach a strbuf into
> it). Can we simply change the declaration?
Right.
> For that matter, it is not clear to me why this needs to be a global at
> all. Once we hand the value to curl_easy_setopt, curl keeps its own
> copy.

That's true only for relatively recent curl versions; before 7.17.0, strings were not copied.

Show 15 quoted lines
> > @@ -1008,6 +1076,8 @@ static int handle_curl_result(struct slot_results *results)
> >  			return HTTP_REAUTH;
> >  		}
> >  	} else {
> > +		if (results->http_connectcode == 407)
> > +			credential_reject(&proxy_auth);
> 
> Rejecting on a 407 makes sense (though again, can we check
> results->http_code?). But if we get a 407 and we _don't_ have a
> password, shouldn't we then prompt for one, similar to what we do with a
> 401?
> 
> That will require some refactoring around http_request_reauth, though
> (because now we might potentially retry twice: once to get past the
> proxy auth, and once to get past the real site's auth).

I think this would also require changes to post_rpc in remote-curl.c, which apparently does something similar to http_request_reauth. Probably something along the lines of adding a HTTP_PROXY_REAUTH return code, plus some refactoring in order to prevent code duplication between the different code parts handling (proxy) reauth. :-/

Show 6 quoted lines
> You prompt unconditionally for the password earlier, but only if the
> proxy URL contains a username. We used to do the same thing for regular
> http, but people got annoyed that they had to specify half the
> credential in the URL. Perhaps it would be less so with proxies (which
> are changed a lot less), so I don't think making this work is an
> absolute requirement.

As far as I understand, the issue was around unconditionally prompting for the password even if it was listed in ~/.netrc. As far as I can see, curl doesn't read ~/.netrc for proxy credentials, so I don't think it would make a difference here.

Thanks, Knut

-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196
Previous: Jeff KingNext: Jeff King
Message 33 of 34 in “http: allow selection of proxy authentication method”
  1. 1/2 http: allow selection of proxy authentication methodKnut Franke, Oct 26, 2015
  2. 2/2 http: use credential API to handle proxy authenticationKnut Franke, Oct 26, 2015
  3. Junio C HamanoOct 26, 2015
  4. Knut FrankeOct 27, 2015
  5. http proxy authentication improvementsKnut Franke, Oct 28, 2015
  6. 1/2 http: allow selection of proxy authentication methodKnut Franke, Oct 28, 2015
  7. Junio C HamanoOct 28, 2015
  8. Junio C HamanoOct 28, 2015
  9. Knut FrankeOct 30, 2015
  10. Junio C HamanoOct 30, 2015
  11. Eric SunshineOct 28, 2015
  12. 2/2 http: use credential API to handle proxy authenticationKnut Franke, Oct 28, 2015
  13. Eric SunshineOct 28, 2015
  14. Knut FrankeOct 30, 2015
  15. Junio C HamanoOct 30, 2015
  16. Eric SunshineOct 30, 2015
  17. 0/2 Knut Franke, Nov 2, 2015
  18. 1/2 http: allow selection of proxy authentication methodKnut Franke, Nov 2, 2015
  19. Junio C HamanoNov 2, 2015
  20. Knut FrankeNov 3, 2015
  21. Junio C HamanoNov 3, 2015
  22. 2/2 http: use credential API to handle proxy authenticationKnut Franke, Nov 2, 2015
  23. Junio C HamanoNov 2, 2015
  24. Knut FrankeNov 3, 2015
  25. Eric SunshineNov 3, 2015
  26. 0/2 Knut Franke, Nov 4, 2015
  27. 1/2 http: allow selection of proxy authentication methodKnut Franke, Nov 4, 2015
  28. Junio C HamanoNov 4, 2015
  29. 2/2 http: use credential API to handle proxy authenticationKnut Franke, Nov 4, 2015
  30. Eric SunshineNov 4, 2015
  31. Junio C HamanoNov 4, 2015
  32. Jeff KingNov 5, 2015
  33. Knut FrankeNov 5, 2015
  34. Jeff KingNov 5, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.