Re: [PATCH] Support various HTTP authentication methods

[Junio C Hamano <gitster@pobox.com>]

Moriyoshi Koizumi <mozo@mozo.jp> writes:

> Currently there is no way to specify the preferred authentication
> method for the HTTP backend and it always ends up with the CURL's
> default
> settings.
>
> This patch enables it if supported by CURL, adding a couple of new

"it" in "enables it" is a bit unclear...

> Signed-off-by: Moriyoshi Koizumi <mozo@mozo.jp>
> ---
>  http.c |  105
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 105 insertions(+), 0 deletions(-)

Linewrapped and whitespace damaged patch that would not apply.

> diff --git a/http.c b/http.c
> index ee58799..889135f 100644
> --- a/http.c
> +++ b/http.c
> @@ -25,6 +25,12 @@ static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv = 0;
>  static const char *curl_http_proxy = NULL;
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static const char *curl_http_auth = NULL;
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +static const char *curl_http_proxy_auth = NULL;
> +#endif

I am not a cURL expert, so I'd take your word for these version
dependencies.

We do not initialize static scope pointers to "= NULL" nor variables to 0,
instead we let BSS take care of that for us.  ftp_no_epsv we can see in
the context is doing unnecessary initialization that should be fixed.

>  static struct curl_slist *pragma_header;
>  
> @@ -153,11 +159,67 @@ static int http_options(const char *var, const
> char *value, void *cb)
>  			return git_config_string(&curl_http_proxy, var, value);
>  		return 0;
>  	}
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +	if (!strcmp("http.auth", var)) {
> +		if (curl_http_auth == NULL)

We tend to say "if (!pointer)".

I see you implemented "the first one wins" rule with this test, but I do
not think you want that.  We first read $HOME/.gitconfig and then
repository specific $GIT_DIR/config, so it is often more useful to use
"the last one wins" rule.

> +			return git_config_string(&curl_http_auth, var, value);
> +		return 0;
> +	}
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070a07
> +	if (!strcmp("http.proxy_auth", var)) {
> +		if (curl_http_proxy_auth == NULL)
> +			return git_config_string(&curl_http_proxy_auth, var, value);
> +		return 0;
> +	}
> +#endif
>  
>  	/* Fall back on the default ones */
>  	return git_default_config(var, value, cb);
>  }
>  
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static long get_curl_auth_bitmask(const char* auth_method)
> +{
> +	char *buf = xmalloc(strlen(auth_method) + 1);
> +	const unsigned char *p = (const unsigned char *)auth_method;
> +	long mask = CURLAUTH_NONE;

Our isspace() is a sane_isspace(), so you do not have to play casting
games between signed vs unsigned char.

> +	for (;;) {
> +		char *q = buf;
> +		while (*p && isspace(*p))
> +			++p;
> +
> +		while (*p && *p != ',')
> +			*q++ = tolower(*p++);
> +
> +		while (--q >= buf && isspace(*(unsigned char *)q));
> +		++q;
> +
> +		*q = '\0';
> +
> +		if (strcmp(buf, "basic") == 0)

Say !strcmp(buf, "literal") like you did in the configuration parsing part
earlier.

> +			mask |= CURLAUTH_BASIC;
> +		else if (strcmp(buf, "digest") == 0)
> +			mask |= CURLAUTH_DIGEST;
> +		else if (strcmp(buf, "gss") == 0)
> +			mask |= CURLAUTH_GSSNEGOTIATE;
> +		else if (strcmp(buf, "ntlm") == 0)
> +			mask |= CURLAUTH_NTLM;
> +		else if (strcmp(buf, "any") == 0)
> +			mask |= CURLAUTH_ANY;
> +		else if (strcmp(buf, "anysafe") == 0)
> +			mask |= CURLAUTH_ANYSAFE;
> +
> +		if (!*p)
> +			break;
> +		++p;
> +	}

You leak "buf" here you forgot to free.  The string you can possibly
accept is a known set with some maximum length, so you can use a on-stack
buf[] and reject any token longer than that maximum, right?

> +	return mask;
> +}
> +#endif
> +
>  static CURL* get_curl_handle(void)
>  {
>  	CURL* result = curl_easy_init();
> @@ -210,6 +272,20 @@ static CURL* get_curl_handle(void)
>  	if (curl_http_proxy)
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  
> +	if (curl_http_auth) {
> +		long n = get_curl_auth_bitmask(curl_http_auth);
> +		curl_easy_setopt(result, CURLOPT_HTTPAUTH, n);
> +	}
> +
> +	if (curl_http_proxy) {
> +		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +
> +		if (curl_http_proxy_auth) {
> +			long n = get_curl_auth_bitmask(curl_http_proxy_auth);
> +			curl_easy_setopt(result, CURLOPT_PROXYAUTH, n);
> +		}
> +	}
> +

This part does not have to be protected with the LIBCURL_VERSION_NUM
conditional?  I somehow find it unlikely...

Instead of parsing the string every time a curl handle is asked for, how
about parsing them once and store the masks in two file scope static longs
in http_init() and use that value to easy_setopt() call here?

That way you can free the two strings much early without waiting for
http_cleanup(), too, right?