| Message ID | b5b56ccd9419353a4bf5bc9d751a711af07d2197.1670880984.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
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/ > + 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()'? > + > + 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. > + > + 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)
On Mon, Dec 12 2022, Matthew John Cheetham via GitGitGadget wrote: > From: Matthew John Cheetham <mjcheetham@outlook.com> > [...] > /* Initialize a credential structure, setting all fields to empty. */ > diff --git a/http.c b/http.c > index 8a5ba3f4776..c4e9cd73e14 100644 > --- a/http.c > +++ b/http.c > @@ -183,6 +183,82 @@ 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 = eltsize * nmemb; Just out of general paranoia: use st_mult() here, not "*" (checks for overflows)? > + struct strvec *values = &http_auth.wwwauth_headers; > + struct strbuf buf = STRBUF_INIT; > + const char *val; > + const char *z = NULL; Why NULL-init the "z" here, but not the "val"? Both look like they should be un-init'd. We also tend to call a throw-away char pointer "p", not "z", but anyway (more below).... > + > + /* > + * 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++; As we already have a "struct strbuf" here, maybe we can instead consistently use the strbuf functions, e.g. strbuf_ltrim() in this case. I haven't reviewed this in detail, maybe it's not easy or worth it here... > + > + 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. > + */ > + if (http_auth.header_is_last_match && isspace(*buf.buf)) { > + const char **v = values->v + values->nr - 1; It makes no difference to the compiler, but perhaps using []-indexing here is more idiomatic, for getting the nth member of this strvec? > + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1); > + > + free((void*)*v); Is this reaching into the strvec & manually memory-managing it unavoidable, or can we use strvec_pop() etc? > + *v = append; > + > + 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)) ...Don't you want to just skip this "z" variable altogether and use istarts_with() instead? All you seem to care about is whether it starts with it, not what the offset is.
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
On 2022-12-15 01:27, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Dec 12 2022, Matthew John Cheetham via GitGitGadget wrote: > >> From: Matthew John Cheetham <mjcheetham@outlook.com> >> [...] >> /* Initialize a credential structure, setting all fields to empty. */ >> diff --git a/http.c b/http.c >> index 8a5ba3f4776..c4e9cd73e14 100644 >> --- a/http.c >> +++ b/http.c >> @@ -183,6 +183,82 @@ 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 = eltsize * nmemb; > > Just out of general paranoia: use st_mult() here, not "*" (checks for > overflows)? Sure! Good point. >> + struct strvec *values = &http_auth.wwwauth_headers; >> + struct strbuf buf = STRBUF_INIT; >> + const char *val; >> + const char *z = NULL; > > Why NULL-init the "z" here, but not the "val"? Both look like they > should be un-init'd. We also tend to call a throw-away char pointer "p", > not "z", but anyway (more below).... > >> + >> + /* >> + * 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++; > > As we already have a "struct strbuf" here, maybe we can instead > consistently use the strbuf functions, e.g. strbuf_ltrim() in this case. That's a good point. I can move to using strbuf functions entirely. > I haven't reviewed this in detail, maybe it's not easy or worth it > here... > >> + >> + 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. >> + */ >> + if (http_auth.header_is_last_match && isspace(*buf.buf)) { >> + const char **v = values->v + values->nr - 1; > > It makes no difference to the compiler, but perhaps using []-indexing > here is more idiomatic, for getting the nth member of this strvec? Sure! >> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1); >> + >> + free((void*)*v); > > Is this reaching into the strvec & manually memory-managing it > unavoidable, or can we use strvec_pop() etc? Again, good point. I can rework this to pop and push a new, joined value. >> + *v = append; >> + >> + 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)) > > ...Don't you want to just skip this "z" variable altogether and use > istarts_with() instead? All you seem to care about is whether it starts > with it, not what the offset is. > Again, a good point. Thanks for the suggestions. My next iteration will include this. Thanks, Matthew
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 8a5ba3f4776..c4e9cd73e14 100644 --- a/http.c +++ b/http.c @@ -183,6 +183,82 @@ 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 = 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++; + + 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. + */ + 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); + + free((void*)*v); + *v = append; + + 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); + +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)