Re: [PATCH] Support various HTTP authentication methods

From: Moriyoshi Koizumi <mozo@mozo.jp>
Date: Jan 29, 2009, 13:59 UTC
Message-ID: <4981B63F.4040100@mozo.jp>
In-Reply-To: <7v3af2h1b0.fsf@gitster.siamese.dyndns.org>

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.

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 20 quoted lines

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

Show 21 quoted lines

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

Show 12 quoted lines

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