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

Re: [PATCH 1/2] http: allow selection of proxy authentication method

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 28, 2015, 16:51 UTC
Message-ID
<xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<1446025245-10128-2-git-send-email-k.franke@science-computing.de>
Knut Franke <k.franke@science-computing.de> writes:
Show 17 quoted lines
> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote.<name>.proxyAuthmethod (in analogy
> to http.proxy and remote.<name>.proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke <k.franke@science-computing.de>
> ---
Thanks.
Show 7 quoted lines
> +http.proxyAuthmethod::
> +	Set the method with which to authenticate against the HTTP proxy. This only
> +    takes effect if the configured proxy URI contains a user name part (i.e. is
> +    of the form 'user@host' or 'user@host:port'). This can be overridden on a
> +    per-remote basis; see `remote.<name>.proxyAuthmethod`. Both can be
> +    overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
> +    Possible values are:

I see inconsistent indentation here. Indent with a tab, like you did your first line, consistently up to this point, perhaps?

Show 9 quoted lines
> @@ -2390,6 +2413,11 @@ remote.<name>.proxy::
>  	the proxy to use for that remote.  Set to the empty string to
>  	disable proxying for that remote.
>  
> +remote.<name>.proxyAuthmethod::
> +    For remotes that require curl (http, https and ftp), the method to use for
> +    authenticating against the proxy in use (probably set in
> +    `remote.<name>.proxy`). See `http.proxyAuthmethod`.
> +
Likewise (match the style of the surrounding paragraphs).
Show 13 quoted lines
> diff --git a/http.c b/http.c
> index 7da76ed..4756bab 100644
> --- a/http.c
> +++ b/http.c
> @@ -63,6 +63,22 @@ 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 const char *http_proxy_authmethod = NULL;
> +static struct {
> +	const char *name;
> +	long curlauth_param;
> +} http_proxy_authmethods[] = {

Perhaps call this "proxy_authmethod[]"? We won't be talking about any other kinds of proxy authentication method in http.c file, and a long name like this makes the line unnecessarily long in a nested loop like you added to init_curl_proxy_auth().

Show 11 quoted lines
> +	{ "basic", CURLAUTH_BASIC },
> +	{ "digest", CURLAUTH_DIGEST },
> +	{ "negotiate", CURLAUTH_GSSNEGOTIATE },
> +	{ "ntlm", CURLAUTH_NTLM },
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	{ "anyauth", CURLAUTH_ANY },
> +#endif
> +	// CURLAUTH_DIGEST_IE has no corresponding command-line option in
> +	// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> +	// here, too
> +};
Please do not use // C++ comments.
Show 14 quoted lines
> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +static void copy_from_env(const char **var, const char *envname)
> +{
> +	const char *val = getenv(envname);
> +	if (val)
> +		*var = xstrdup(val);
> +}
> +
> +static void init_curl_proxy_auth(CURL *result)
> +{
> +	copy_from_env(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Unless this helper is used regularly from many other places, is use makes it harder to follow the flow of the logic, as it does not offer clear and obvious abstraction, especially with the name "copy_from_env()". I was forced to look at the implementation to see what happens when the environment variable does not exist to make sure the right thing happens (i.e. http_proxy_authmethod is unchanged).

Show 19 quoted lines
> +	if (http_proxy_authmethod) {
> +		int i;
> +		for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> +			if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> +				curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +						http_proxy_authmethods[i].curlauth_param);
> +				break;
> +			}
> +		}
> +		if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> +			warning("unsupported proxy authentication method %s: using default",
> +			      http_proxy_authmethod);
> +		}
> +	}
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	else
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}

This patch should take what 1c2dbf20 (http: support curl < 7.10.7, 2015-02-03) wanted to do into account. Having the configuration variable or the environment variable defined by itself, while running a Git built with old cURL, shouldn't trigger any warning, but the entire function should perhaps be ifdefed out or something?

Personally I find it a bit surprising that somebody still cares about such an old version (7.10.7 is listed on 15 Aug 2003 in the CHANGES file), but there apparently are happy users. Let's not knowingly break them.

Show 8 quoted lines
> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  	}
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -	curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +	init_curl_proxy_auth(result);
Thanks.
Previous: Knut FrankeNext: Junio C Hamano
Message 7 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.