Re: [PATCH v4 1/8] http: read HTTP WWW-Authenticate response headers

From: Matthew John Cheetham <mjcheetham@outlook.com>
To: Victoria Dye <vdye@github.com>,
	Matthew John Cheetham via GitGitGadget  <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.com>,
	M Hickford <mirth.hickford@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>,
	Glen Choo <chooglen@google.com>,
	Matthew John Cheetham <mjcheetham@github.com>
Subject: Re: [PATCH v4 1/8] http: read HTTP WWW-Authenticate response headers
Date: Wed, 11 Jan 2023 14:09:14 -0800	[thread overview]
Message-ID: <AS2PR03MB9815234B918C9C566F21FABBC0FC9@AS2PR03MB9815.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <c255896d-637d-f7b0-8698-10a2112852c1@github.com>

On 2022-12-14 15:15, Victoria Dye wrote:

> Matthew John Cheetham via GitGitGadget wrote:
>> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
>> +{
>> +	size_t size = eltsize * nmemb;
>> +	struct strvec *values = &http_auth.wwwauth_headers;
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char *val;
>> +	const char *z = NULL;
>> +
>> +	/*
>> +	 * Header lines may not come NULL-terminated from libcurl so we must
>> +	 * limit all scans to the maximum length of the header line, or leverage
>> +	 * strbufs for all operations.
>> +	 *
>> +	 * In addition, it is possible that header values can be split over
>> +	 * multiple lines as per RFC 2616 (even though this has since been
>> +	 * deprecated in RFC 7230). A continuation header field value is
>> +	 * identified as starting with a space or horizontal tab.
>> +	 *
>> +	 * The formal definition of a header field as given in RFC 2616 is:
>> +	 *
>> +	 *   message-header = field-name ":" [ field-value ]
>> +	 *   field-name     = token
>> +	 *   field-value    = *( field-content | LWS )
>> +	 *   field-content  = <the OCTETs making up the field-value
>> +	 *                    and consisting of either *TEXT or combinations
>> +	 *                    of token, separators, and quoted-string>
>> +	 */
>> +
>> +	strbuf_add(&buf, ptr, size);
>> +
>> +	/* Strip the CRLF that should be present at the end of each field */
>> +	strbuf_trim_trailing_newline(&buf);
>> +
>> +	/* Start of a new WWW-Authenticate header */
>> +	if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
>> +		while (isspace(*val))
>> +			val++;
> 
> Per the RFC [1]: 
> 
>> The field value MAY be preceded by any amount of LWS, though a single SP
>> is preferred.
> 
> And LWS (linear whitespace) is defined as:
> 
>> CRLF           = CR LF 
>> LWS            = [CRLF] 1*( SP | HT )
> 
> and 'isspace()' includes CR, LF, SP, and HT [2]. 
> 
> Looks good!
> 
> [1] https://datatracker.ietf.org/doc/html/rfc2616#section-4-2
> [2] https://linux.die.net/man/3/isspace
> 
>> +
>> +		strvec_push(values, val);
> 
> I had the same question about "what happens with an empty 'val' here?" as
> Stolee did earlier [3], but I *think* the "zero length" (i.e., single null
> terminator) will be copied successfully. It's probably worth testing that
> explicitly, though (I see you set up tests in later patches - ideally a 
> "www-authenticate:<mix of whitespace>" line could be tested there).
> 
> [3] https://lore.kernel.org/git/9fded44b-c503-f8e5-c6a6-93e882d50e27@github.com/

There is a bug here. Empty header values would indeed be appended
successfully, but this eventually results in empty values for `wwwauth[]`
being sent over to credential helpers (which should treat the empty value as
a reset of the existing list!!)

Really, empty values should be ignored.
My next iteration should hopefully be a bit more careful around these cases.

>> +		http_auth.header_is_last_match = 1;
>> +		goto exit;
>> +	}
>> +
>> +	/*
>> +	 * This line could be a continuation of the previously matched header
>> +	 * field. If this is the case then we should append this value to the
>> +	 * end of the previously consumed value.
>> +	 */
>> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
>> +		const char **v = values->v + values->nr - 1;
>> +		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
> 
> In this case (where the line is a continuation of a 'www-authenticate'
> header), it looks like the code here expects *exactly* one LWS at the start
> of the line ('isspace(*buf.buf)' requiring at least one space to append the
> header, 'ptr + 1' skipping no more than one). But, according to the RFC, it
> could be more than one:
> 
>> Header fields can be extended over multiple lines by preceding each extra
>> line with at least one SP or HT.
> 
> So I think 'buf.buf' might need to have all preceding spaces removed, like
> you did in the "Start of a new WWW-Authenticate header" block.
> 
> Also, if you're copying 'ptr' into 'buf' to avoid issues from a missing null
> terminator, wouldn't you want to use 'buf.buf' (instead of 'ptr') in
> 'xstrfmt()'?

Sure! Good points.

>> +
>> +		free((void*)*v);
>> +		*v = append;
> 
> I was about to suggest (optionally) rewriting this to use 'strvec_pop()' and
> 'strvec_push_nodup()':
> 
> 	strvec_pop(values); 
> 	strvec_push_nodup(values, append);
> 
> to maybe make this a bit easier to follow, but unfortunately
> 'strvec_push_nodup()' isn't available outside of 'strvec.c'. If you did want
> to use 'strvec' functions, you could remove the 'static' from
> 'strvec_push_nodup()' and add it to 'strvec.h' it in a later reroll, but I
> don't consider that change "blocking" or even important enough to warrant
> its own reroll. 

That wouldn't be too much effort, and would help simplify overall the move
to using `strbuf_` functions. Check my next iteration for this.

>> +
>> +		goto exit;
>> +	}
>> +
>> +	/* This is the start of a new header we don't care about */
>> +	http_auth.header_is_last_match = 0;
>> +
>> +	/*
>> +	 * If this is a HTTP status line and not a header field, this signals
>> +	 * a different HTTP response. libcurl writes all the output of all
>> +	 * response headers of all responses, including redirects.
>> +	 * We only care about the last HTTP request response's headers so clear
>> +	 * the existing array.
>> +	 */
>> +	if (skip_iprefix(buf.buf, "http/", &z))
>> +		strvec_clear(values);
> 
> The comments describing the intended behavior (as well as the commit
> message) are clear and explain the somewhat esoteric (at least to my
> untrained eye ;) ) code. Thanks!
> 
>> +
>> +exit:
>> +	strbuf_release(&buf);
>> +	return size;
>> +}
>> +
>>  size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
>>  {
>>  	return nmemb;
>> @@ -1864,6 +1940,8 @@ static int http_request(const char *url,
>>  					 fwrite_buffer);
>>  	}
>>  
>> +	curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
>> +
>>  	accept_language = http_get_accept_language_header();
>>  
>>  	if (accept_language)
> 

Thanks,
Matthew