Re: [PATCH] Support various HTTP authentication methods

[Moriyoshi Koizumi <mozo@mozo.jp>]

First thank you for the advice. I am not familiar to the code base and
definitely doing something wrong.

Junio C Hamano wrote:
> Moriyoshi Koizumi <mozo@mozo.jp> writes:
>> This patch enables it if supported by CURL, adding a couple of new
> 
> "it" in "enables it" is a bit unclear...

I was a bit in a rush and I thought I got to get this done before I went
home.  This patch dnables various HTTP authentication methods namely
basic, digest, GSS and NTLM that are supported by cURL.  cURL tries to
use basic authentication if the option is not explicitly provided.

> Linewrapped and whitespace damaged patch that would not apply.

Sorry for the crap. I'll try to send the correct one next week.

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

Right.

>> +#if LIBCURL_VERSION_NUM >= 0x070a06
>> +	if (!strcmp("http.auth", var)) {
>> +		if (curl_http_auth == NULL)
> 
> We tend to say "if (!pointer)".

I'll fix this too.

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

The environment variables win over gitconfigs. I thought that's what I
implemented and what we want.

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

I tend to like this way, and the reason for the inconsistency between
them is that the earlier one is pasted from the nearby code.  I'm
willing to fix this as well if I should.

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

That one is really silly and shameful :-( I first thought I could go
with the on-stack buffer, but I eventually did this because the input
might contain an indefinite set of tokens, and thought safer to alloc it.

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

The block that starts with if (curl_http_proxy_auth) {} should've been
enclosed by the conditinals. The leading part had been there in the
first place.

> 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 has to be the way to go, but the question is where I am supposed to
parse the strings and store them to the globals?

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

Regards,
Moriyoshi