[v6,11/12] http: read HTTP WWW-Authenticate response headers

Commit Message

Matthew John Cheetham Jan. 18, 2023, 3:30 a.m. UTC

From: Matthew John Cheetham <[e-mail]>

Read and store the HTTP WWW-Authenticate response headers made for
a particular request.

This will allow us to pass important authentication challenge
information to credential helpers or others that would otherwise have
been lost.

According to RFC2616 Section 4.2 [1], header field names are not
case-sensitive meaning when collecting multiple values for the same
field name, we can just use the case of the first observed instance of
each field name and no normalisation is required.

libcurl only provides us with the ability to read all headers recieved
for a particular request, including any intermediate redirect requests
or proxies. The lines returned by libcurl include HTTP status lines
delinating any intermediate requests such as "HTTP/1.1 200". We use
these lines to reset the strvec of WWW-Authenticate header values as
we encounter them in order to only capture the final response headers.

The collection of all header values matching the WWW-Authenticate
header is complicated by the fact that it is legal for header fields to
be continued over multiple lines, but libcurl only gives us one line at
a time.

In the future [2] we may be able to leverage functions to read headers
from libcurl itself, but as of today we must do this ourselves.

[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/

Signed-off-by: Matthew John Cheetham <[e-mail]>
---
 credential.c |  1 +
 credential.h | 15 +++++++++
 http.c       | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Jan. 18, 2023, 11:42 a.m. UTC | #1

On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:

> From: Matthew John Cheetham <[e-mail]>

> +	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++;
> +
> +		strvec_push(values, val);
> +		http_auth.header_is_last_match = 1;
> +		goto exit;
> [...]
> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> +		/* Trim leading whitespace from this continuation hdr line. */
> +		strbuf_ltrim(&buf);


The mixture of this isspace() loop and then strbuf_ltrim() seems odd,
why not stick with the strbuf API?

I.e. after skip_iprefix() strbuf_splice() the start of the string away,
then use strbuf_ltrim() in the first "if" branch here?

Likewise this is open-coding the "isspace" in strbuf_ltrim() for the
second "if". Maybe run the strbuf_ltrim() unconditionally, save away the
length before, and then:

	if (http_auth.header_is_last_match && prev_len != buf.len) { ...

?

Matthew John Cheetham Jan. 20, 2023, 10:05 p.m. UTC | #2

On 2023-01-18 03:42, Ævar Arnfjörð Bjarmason wrote:

> 
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> 
>> From: Matthew John Cheetham <[e-mail]>
> 
>> +	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++;
>> +
>> +		strvec_push(values, val);
>> +		http_auth.header_is_last_match = 1;
>> +		goto exit;
>> [...]
>> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
>> +		/* Trim leading whitespace from this continuation hdr line. */
>> +		strbuf_ltrim(&buf);
> 
> 
> The mixture of this isspace() loop and then strbuf_ltrim() seems odd,
> why not stick with the strbuf API?
> 
> I.e. after skip_iprefix() strbuf_splice() the start of the string away,
> then use strbuf_ltrim() in the first "if" branch here?

You mean like this?

        size_t size = st_mult(eltsize, nmemb);
        struct strvec *values = &http_auth.wwwauth_headers;
        struct strbuf buf = STRBUF_INIT;
-       const char *val;
 
        /*
         * Header lines may not come NULL-terminated from libcurl so we must
@@ -216,11 +215,11 @@ static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
        strbuf_trim_trailing_newline(&buf);
 
        /* Start of a new WWW-Authenticate header */
-       if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
-               while (isspace(*val))
-                       val++;
+       if (istarts_with(buf.buf, "www-authenticate:")) {
+               strbuf_splice(&buf, 0, 17, NULL, 0);
+               strbuf_ltrim(&buf);
 
-               strvec_push(values, val);
+               strvec_push(values, buf.buf);
                http_auth.header_is_last_match = 1;
                goto exit;
        }


I don't particularly like this given we're now introducing the 'magic' number
17 that's the length of `www-authenticate:`, plus `strbuf_splice` is doing
a lot more work moving pieces of memory around rather than just producing
a new starting pointer to the start of the value (skipping leading whitespace).

> Likewise this is open-coding the "isspace" in strbuf_ltrim() for the
> second "if". Maybe run the strbuf_ltrim() unconditionally, save away the
> length before, and then:
> 
> 	if (http_auth.header_is_last_match && prev_len != buf.len) { ...
> 
> ?

The suggestion of trimming and comparing lengths just makes a piece of code
handling a little-known edge case less immediately obvious in its intent in
my opinion. The current implementation of "if starts with a single space"
matches the definition of continuation header lines, rather than re-piecing
together this from "why are we trimming and comparing lengths?"
Perf-wise the current implementation is only adding one extra `isspace`
call which we're potentially about to do in a loop inside of `strbuf_ltrim`
anyway. Plus, the common case will be a single space anyway.

Thanks,
Matthew

Patch

diff --git a/credential.c b/credential.c
index f6389a50684..897b4679333 100644
--- a/credential.c
+++ b/credential.c
@@ -22,6 +22,7 @@  void credential_clear(struct credential *c)
 	free(c->username);
 	free(c->password);
 	string_list_clear(&c->helpers, 0);
+	strvec_clear(&c->wwwauth_headers);
 
 	credential_init(c);
 }
diff --git a/credential.h b/credential.h
index f430e77fea4..6f2e5bc610b 100644
--- a/credential.h
+++ b/credential.h
@@ -2,6 +2,7 @@ 
 #define CREDENTIAL_H
 
 #include "string-list.h"
+#include "strvec.h"
 
 /**
  * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,19 @@  struct credential {
 	 */
 	struct string_list helpers;
 
+	/**
+	 * A `strvec` of WWW-Authenticate header values. Each string
+	 * is the value of a WWW-Authenticate header in an HTTP response,
+	 * in the order they were received in the response.
+	 */
+	struct strvec wwwauth_headers;
+
+	/**
+	 * Internal use only. Used to keep track of split header fields
+	 * in order to fold multiple lines into one value.
+	 */
+	unsigned header_is_last_match:1;
+
 	unsigned approved:1,
 		 configured:1,
 		 quit:1,
@@ -130,6 +144,7 @@  struct credential {
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.wwwauth_headers = STRVEC_INIT, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */
diff --git a/http.c b/http.c
index a2a80318bb2..595c93bc7a3 100644
--- a/http.c
+++ b/http.c
@@ -183,6 +183,98 @@  size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	return nmemb;
 }
 
+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
+{
+	size_t size = st_mult(eltsize, nmemb);
+	struct strvec *values = &http_auth.wwwauth_headers;
+	struct strbuf buf = STRBUF_INIT;
+	const char *val;
+
+	/*
+	 * 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++;
+
+		strvec_push(values, val);
+		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.
+	 * Continuation lines start with at least one whitespace, maybe more,
+	 * so we should collapse these down to a single SP (valid per the spec).
+	 */
+	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
+		/* Trim leading whitespace from this continuation hdr line. */
+		strbuf_ltrim(&buf);
+
+		/*
+		 * At this point we should always have at least one existing
+		 * value, even if it is empty. Do not bother appending the new
+		 * value if this continuation header is itself empty.
+		 */
+		if (!values->nr) {
+			BUG("should have at least one existing header value");
+		} else if (buf.len) {
+			char *prev = xstrdup(values->v[values->nr - 1]);
+
+			/* Join two non-empty values with a single space. */
+			const char *const sp = *prev ? " " : "";
+
+			strvec_pop(values);
+			strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
+			free(prev);
+		}
+
+		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 (istarts_with(buf.buf, "http/"))
+		strvec_clear(values);
+
+exit:
+	strbuf_release(&buf);
+	return size;
+}
+
 size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
 {
 	return nmemb;
@@ -1864,6 +1956,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)