Enhance credential helper protocol to include auth headers #1352
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @mjcheetham, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
/allow |
|
User mjcheetham is now allowed to use GitGitGadget. |
mjcheetham
force-pushed
the
emu
branch
3 times, most recently
from
f8a0e5e
to
7f82706
Compare
|
/submit |
|
Submitted as pull.1352.git.1663097156.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This branch is now known as |
|
This patch series was integrated into seen via git@a4ed9ef. |
|
This patch series was integrated into seen via git@038b030. |
|
This patch series was integrated into seen via git@fb50673. |
|
There was a status update in the "New Topics" section about the branch RFC source: <pull.1352.git.1663097156.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@2106621. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> Hello! I have an RFC to update the existing credential helper design in
> order to allow for some new scenarios, and future evolution of auth methods
> that Git hosts may wish to provide. I outline the background, summary of
> changes and some challenges below. I also attach a series of patches to
> illustrate the design proposal.
It's unfortunate that we didn't get to talk about this during the
contributor summit, but it is super-technical and worth looking closely
at all the details.
> One missing element from the patches are extensive tests of the new
> behaviour. It appears existing tests focus either on the credential helper
> protocol/format, or rely on testing basic authentication only via an Apache
> webserver. In order to have a full end to end test coverage of these new
> features it make be that we need a more comprehensive test bed to mock these
> more nuanced authentication methods. I lean on the experts on the list for
> advice here.
The microsoft/git fork has a feature (the GVFS Protocol) that requires a
custom HTTP server as a test helper. We might need a similar test helper
to return these WWW-Authenticate headers and check the full request list
from Git matches the spec. Doing that while also executing the proper Git
commands to serve the HTTP bodies is hopefully not too large. It might be
nice to adapt such a helper to replace the need for a full Apache install
in our test suite, but that's an independent concern from this RFC.
> Limitations
> ===========
>
> Because this credential model was built mostly for password based
> authentication systems, it's somewhat limited. In particular:
>
> 1. To generate valid credentials, additional information about the request
> (or indeed the requestee and their device) may be required. For example,
> OAuth is based around scopes. A scope, like "git.read", might be
> required to read data from the remote. However, the remote cannot tell
> the credential helper what scope is required for this request.
>
> 2. This system is not fully extensible. Each time a new type of
> authentication (like OAuth Bearer) is invented, Git needs updates before
> credential helpers can take advantage of it (or leverage a new
> capability in libcurl).
>
>
> Goals
> =====
>
> * As a user with multiple federated cloud identities:
I'm not sure if you mentioned it anywhere else, but this is specifically
for cases where a user might have multiple identities _on the same host
by DNS name_. The credential.useHttpPath config option might seem like it
could help here, but the credential helper might pick the wrong identity
that is the most-recent login. Either this workflow will require the user
to re-login with every new URL or the fetches/clones will fail when the
guess is wrong and the user would need to learn how to log into that other
identity.
Please correct me if I'm wrong about any of this, but the details of your
goals make it clear that the workflow will be greatly improved:
> * Reach out to a remote and have my credential helper automatically
> prompt me for the correct identity.
> * Leverage existing authentication systems built-in to many operating
> systems and devices to boost security and reduce reliance on passwords.
>
> * As a Git host and/or cloud identity provider:
>
> * Leverage newest identity standards, enhancements, and threat
> mitigations - all without updating Git.
> * Enforce security policies (like requiring two-factor authentication)
> dynamically.
> * Allow integration with third party standard based identity providers in
> enterprises allowing customers to have a single plane of control for
> critical identities with access to source code.
I had a question with this part of your proposal:
> Because the extra information forms an ordered list, and the existing
> credential helper I/O format only provides for simple key=value pairs,
> we introduce a new convention for transmitting an ordered list of
> values. Key names that are suffixed with a C-style array syntax should
> have values considered to form an order list, i.e. key[n]=value, where n
> is a zero based index of the values.
>
> For the WWW-Authenticate header values we opt to use the key wwwauth[n].
...
> Git sends over standard input:
>
> protocol=https
> host=example.com
> wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
> wwwauth[1]=Basic realm="login.example"
The important part here is that we provide a way to specify a multi-valued
key as opposed to a "last one wins" key, right?
Using empty braces (wwwauth[]) would suffice to indicate this, right? That
allows us to not care about the values inside the braces. The biggest
issues I see with a value in the braces are:
1. What if it isn't an integer?
2. What if we are missing a value?
3. What if they come out of order?
Without a value inside, then the order in which they appear provides
implicit indices in their multi-valued list.
Other than that, I support this idea and will start looking at the code
now.
Thanks,
-Stolee |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -159,6 +159,11 @@ static void read_credential(void) | |||
| username = xstrdup(v); | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Like in all the other credential helpers, the osxkeychain helper
> ignores unknown credential lines.
>
> Add a comment (a la the other helpers) to make it clear and explicit
> that this is the desired behaviour.
I recommend that these first three patches be submitted for full
review and merging, since they seem important independent of this
RFC.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Matthew John Cheetham wrote (reply to this):
On 2022-09-19 09:12, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Like in all the other credential helpers, the osxkeychain helper
>> ignores unknown credential lines.
>>
>> Add a comment (a la the other helpers) to make it clear and explicit
>> that this is the desired behaviour.
>
> I recommend that these first three patches be submitted for full
> review and merging, since they seem important independent of this
> RFC.
>
> Thanks,
> -Stolee
That's a fair point. I will submit these independently.
Thanks,
Matthew
| @@ -22,6 +22,7 @@ void credential_clear(struct credential *c) | |||
| free(c->username); | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> + /**
> + * 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;
I like this careful documentation.
> + unsigned header_is_last_match:1;
But then this member is unclear how it is attached. It could use its
own "for internal use" comment if we don't want to describe it in full
detail here.
> +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 */
Is it really a CRLF? Or just an LF?
> + strbuf_trim_trailing_newline(&buf);
Thankfully, this will trim an LF _or_ CR/LF pair, so either way would be fine.
> + /* Start of a new WWW-Authenticate header */
> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> + while (isspace(*val)) val++;
Break the "val++;" to its own line:
while (isspace(*val))
val++;
While we are here, do we need to be careful about the end of the string at
this point? Is it possible that the server will send all spaces up until the
maximum header size (as mentioned in the message)?
> +
> + 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;
I suppose we expect leading spaces as critical to this header, right?
> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
We might have better luck using a strbuf, initializing it with the expected
size and using strbuf_add() to append the strings. Maybe I'm just prematurely
optimizing, though.
> +
> + 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;
> @@ -1829,6 +1904,8 @@ static int http_request(const char *url,
> fwrite_buffer);
> }
>
> + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
Nice integration point!
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Matthew John Cheetham wrote (reply to this):
On 2022-09-19 09:21, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>
>> + /**
>> + * 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;
>
> I like this careful documentation.
>
>> + unsigned header_is_last_match:1;
>
> But then this member is unclear how it is attached. It could use its
> own "for internal use" comment if we don't want to describe it in full
> detail here.
A fair point. I will update in a future iteration.
>> +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 */
>
> Is it really a CRLF? Or just an LF?
It is indeed an CRLF, agnostic of platform. HTTP defines CRLF as the
end-of-line marker for all entities other than the body.
See RFC 2616 section 2.2: https://www.rfc-editor.org/rfc/rfc2616#section-2.2
>> + strbuf_trim_trailing_newline(&buf);
>
> Thankfully, this will trim an LF _or_ CR/LF pair, so either way would be fine.
>
>> + /* Start of a new WWW-Authenticate header */
>> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
>> + while (isspace(*val)) val++;
>
> Break the "val++;" to its own line:
>
> while (isspace(*val))
> val++;
Sure! Sorry I missed this one.
> While we are here, do we need to be careful about the end of the string at
> this point? Is it possible that the server will send all spaces up until the
> maximum header size (as mentioned in the message)?
>
>> +
>> + 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;
>
> I suppose we expect leading spaces as critical to this header, right?
Leading (and trailing) spaces are not part of the header value.
From RFC 2616 section 2.2 regarding header field values:
"All linear white space, including folding, has the same semantics as SP.
A recipient MAY replace any linear white space with a single SP before
interpreting the field value or forwarding the message downstream."
>> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
>
> We might have better luck using a strbuf, initializing it with the expected
> size and using strbuf_add() to append the strings. Maybe I'm just prematurely
> optimizing, though.
This code path is used to re-join/fold a header value continuation, which is
pretty rare in the wild (if at all with modern web servers).
>> +
>> + 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;
>> @@ -1829,6 +1904,8 @@ static int http_request(const char *url,
>> fwrite_buffer);
>> }
>>
>> + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
>
> Nice integration point!
>
> Thanks,
> -Stolee
Thanks,
MatthewThere was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/21/2022 6:24 PM, Matthew John Cheetham wrote:
> On 2022-09-19 09:21, Derrick Stolee wrote:
>> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>>> +
>>> + 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;
>>
>> I suppose we expect leading spaces as critical to this header, right?
>
> Leading (and trailing) spaces are not part of the header value.
>
> From RFC 2616 section 2.2 regarding header field values:
>
> "All linear white space, including folding, has the same semantics as SP.
> A recipient MAY replace any linear white space with a single SP before
> interpreting the field value or forwarding the message downstream."
>
>>> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
>>
>> We might have better luck using a strbuf, initializing it with the expected
>> size and using strbuf_add() to append the strings. Maybe I'm just prematurely
>> optimizing, though.
>
> This code path is used to re-join/fold a header value continuation, which is
> pretty rare in the wild (if at all with modern web servers).
I think the point is that I noticed that you removed the leading whitespace
in a header's first line, but additional whitespace after this first space
will be included in the concatenated content of the header value.
As long as that is the intention, then I'm happy here.
Thanks,
-Stolee
Documentation/git-credential.txt
Outdated
| @@ -151,6 +151,15 @@ Git understands the following attributes: | |||
| were read (e.g., `url=https://example.com` would behave as if | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> In this case we send multiple `wwwauth[n]` properties where `n` is a
> zero-indexed number, reflecting the order the WWW-Authenticate headers
> appeared in the HTTP response.
> @@ -151,6 +151,15 @@ Git understands the following attributes:
> were read (e.g., `url=https://example.com` would behave as if
> `protocol=https` and `host=example.com` had been provided). This
> can help callers avoid parsing URLs themselves.
> +
> +`wwwauth[n]`::
> +
> + When an HTTP response is received that includes one or more
> + 'WWW-Authenticate' authentication headers, these can be passed to Git
> + (and subsequent credential helpers) with these attributes.
> + Each 'WWW-Authenticate' header value should be passed as a separate
> + attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> + appear in the HTTP response.
> +
> Note that specifying a protocol is mandatory and if the URL
> doesn't specify a hostname (e.g., "cert:///path/to/file") the
This "+" means that this paragraph should be connected to the previous
one, so it seems that you've inserted your new value in the middle of
the `url` key. You'll want to move yours to be after those two connected
paragraphs. Your diff hunk should look like this:
--- >8 ---
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f..127ae29be3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -160,6 +160,15 @@ empty string.
Components which are missing from the URL (e.g., there is no
username in the example above) will be left unset.
+`wwwauth[n]`::
+
+ When an HTTP response is received that includes one or more
+ 'WWW-Authenticate' authentication headers, these can be passed to Git
+ (and subsequent credential helpers) with these attributes.
+ Each 'WWW-Authenticate' header value should be passed as a separate
+ attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+ appear in the HTTP response.
+
GIT
---
Part of the linkgit:git[1] suite
--- >8 ---
> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 497b9b9d927..fe118d76f98 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -235,6 +235,19 @@ SSLEngine On
> Require valid-user
> </LocationMatch>
>
> +# Advertise two additional auth methods above "Basic".
> +# Neither of them actually work but serve test cases showing these
> +# additional auth headers are consumed correctly.
> +<Location /auth-wwwauth/>
> + AuthType Basic
> + AuthName "git-auth"
> + AuthUserFile passwd
> + Require valid-user
> + SetEnvIf Authorization "^\S+" authz
> + Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
> + Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
> +</Location>
> +
This is cool that you've figured out how to make our Apache tests
add these headers! Maybe we won't need that extra test helper like
I thought (unless we want to confirm the second request sends the
right information).
> +test_expect_success 'http auth sends www-auth headers to credential helper' '
> + write_script git-credential-tee <<-\EOF &&
> + cmd=$1
> + teefile=credential-$cmd
> + if [ -f "$teefile" ]; then
I think we prefer using "test" over the braces (and linebreak
before then) like this:
if test -n "$teefile"
then
> + rm $teefile
> + fi
Alternatively, you could always run "rm -f $teefile" for
simplicity.
> + (
> + while read line;
> + do
> + if [ -z "$line" ]; then
> + exit 0
> + fi
> + echo "$line" >> $teefile
> + echo $line
> + done
> + ) | git credential-store $cmd
Since I'm not sure, I'll ask the question: do we need the sub-shell
here, or could we pipe directly off of the "done"? Like this:
while read line;
do
if [ -z "$line" ]; then
exit 0
fi
echo "$line" >> $teefile
echo $line
done | git credential-store $cmd
> + EOF
> + cat >expected-get <<-EOF &&
> + protocol=http
> + host=127.0.0.1:5551
> + wwwauth[0]=Bearer authority=https://login.example.com
> + wwwauth[1]=FooAuth foo=bar baz=1
> + wwwauth[2]=Basic realm="git-auth"
> + EOF
> +
> + cat >expected-store <<-EOF &&
> + protocol=http
> + host=127.0.0.1:5551
> + username=user@host
> + password=pass@host
> + EOF
> +
> + rm -f .git-credentials &&
> + test_config credential.helper tee &&
> + set_askpass user@host pass@host &&
> + (
> + PATH="$PWD:$PATH" &&
> + git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
> + ) &&
> + expect_askpass both user@host &&
> + test_cmp expected-get credential-get &&
> + test_cmp expected-store credential-store
Elegant check for both calls.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Matthew John Cheetham wrote (reply to this):
On 2022-09-19 09:33, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
>> In this case we send multiple `wwwauth[n]` properties where `n` is a
>> zero-indexed number, reflecting the order the WWW-Authenticate headers
>> appeared in the HTTP response.
>> @@ -151,6 +151,15 @@ Git understands the following attributes:
>> were read (e.g., `url=https://example.com` would behave as if
>> `protocol=https` and `host=example.com` had been provided). This
>> can help callers avoid parsing URLs themselves.
>> +
>> +`wwwauth[n]`::
>> +
>> + When an HTTP response is received that includes one or more
>> + 'WWW-Authenticate' authentication headers, these can be passed to Git
>> + (and subsequent credential helpers) with these attributes.
>> + Each 'WWW-Authenticate' header value should be passed as a separate
>> + attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
>> + appear in the HTTP response.
>> +
>> Note that specifying a protocol is mandatory and if the URL
>> doesn't specify a hostname (e.g., "cert:///path/to/file") the
>
> This "+" means that this paragraph should be connected to the previous
> one, so it seems that you've inserted your new value in the middle of
> the `url` key. You'll want to move yours to be after those two connected
> paragraphs. Your diff hunk should look like this:
>
> --- >8 ---
>
> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
> index f18673017f..127ae29be3 100644
> --- a/Documentation/git-credential.txt
> +++ b/Documentation/git-credential.txt
> @@ -160,6 +160,15 @@ empty string.
> Components which are missing from the URL (e.g., there is no
> username in the example above) will be left unset.
>
> +`wwwauth[n]`::
> +
> + When an HTTP response is received that includes one or more
> + 'WWW-Authenticate' authentication headers, these can be passed to Git
> + (and subsequent credential helpers) with these attributes.
> + Each 'WWW-Authenticate' header value should be passed as a separate
> + attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> + appear in the HTTP response.
> +
> GIT
> ---
> Part of the linkgit:git[1] suite
>
>
> --- >8 ---
Thanks for catching!
>> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
>> index 497b9b9d927..fe118d76f98 100644
>> --- a/t/lib-httpd/apache.conf
>> +++ b/t/lib-httpd/apache.conf
>> @@ -235,6 +235,19 @@ SSLEngine On
>> Require valid-user
>> </LocationMatch>
>>
>> +# Advertise two additional auth methods above "Basic".
>> +# Neither of them actually work but serve test cases showing these
>> +# additional auth headers are consumed correctly.
>> +<Location /auth-wwwauth/>
>> + AuthType Basic
>> + AuthName "git-auth"
>> + AuthUserFile passwd
>> + Require valid-user
>> + SetEnvIf Authorization "^\S+" authz
>> + Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
>> + Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
>> +</Location>
>> +
>
> This is cool that you've figured out how to make our Apache tests
> add these headers! Maybe we won't need that extra test helper like
> I thought (unless we want to confirm the second request sends the
> right information).
This will exercise the new header parsing and passing the info to the helper
but will indeed not test the response. I feel like a test helper would be
beneficial still.. what I've done here doesn't feel 100% clean or complete.
>> +test_expect_success 'http auth sends www-auth headers to credential helper' '
>> + write_script git-credential-tee <<-\EOF &&
>> + cmd=$1
>> + teefile=credential-$cmd
>> + if [ -f "$teefile" ]; then
>
> I think we prefer using "test" over the braces (and linebreak
> before then) like this:
>
> if test -n "$teefile"
> then
>
>> + rm $teefile
>> + fi
>
> Alternatively, you could always run "rm -f $teefile" for
> simplicity.
I like simple :-)
>> + (
>> + while read line;
>> + do
>> + if [ -z "$line" ]; then
>> + exit 0
>> + fi
>> + echo "$line" >> $teefile
>> + echo $line
>> + done
>> + ) | git credential-store $cmd
>
> Since I'm not sure, I'll ask the question: do we need the sub-shell
> here, or could we pipe directly off of the "done"? Like this:
>
> while read line;
> do
> if [ -z "$line" ]; then
> exit 0
> fi
> echo "$line" >> $teefile
> echo $line
> done | git credential-store $cmd
That we can.. I will update in next iteration.
>> + EOF
>
>
>> + cat >expected-get <<-EOF &&
>> + protocol=http
>> + host=127.0.0.1:5551
>> + wwwauth[0]=Bearer authority=https://login.example.com
>> + wwwauth[1]=FooAuth foo=bar baz=1
>> + wwwauth[2]=Basic realm="git-auth"
>> + EOF
>> +
>> + cat >expected-store <<-EOF &&
>> + protocol=http
>> + host=127.0.0.1:5551
>> + username=user@host
>> + password=pass@host
>> + EOF
>> +
>> + rm -f .git-credentials &&
>> + test_config credential.helper tee &&
>> + set_askpass user@host pass@host &&
>> + (
>> + PATH="$PWD:$PATH" &&
>> + git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
>> + ) &&
>> + expect_askpass both user@host &&
>> + test_cmp expected-get credential-get &&
>> + test_cmp expected-store credential-store
>
> Elegant check for both calls.
>
> Thanks,
> -Stolee
Thanks,
Matthew
Documentation/git-credential.txt
Outdated
| @@ -151,6 +151,24 @@ Git understands the following attributes: | |||
| were read (e.g., `url=https://example.com` would behave as if | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Introduce a new credential field `authtype` that can be used by
> credential helpers to indicate the type of the credential or
> authentication mechanism to use for a request.
>
> Modify http.c to now specify the correct authentication scheme or
> credential type when authenticating the curl handle. If the new
> `authtype` field in the credential structure is `NULL` or "Basic" then
> use the existing username/password options. If the field is "Bearer"
> then use the OAuth bearer token curl option. Otherwise, the `authtype`
> field is the authentication scheme and the `password` field is the
> raw, unencoded value.
> @@ -524,8 +525,25 @@ static void init_curl_http_auth(struct active_request_slot *slot)
>
> credential_fill(&http_auth);
>
> - curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
> - curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
> + if (!http_auth.authtype || !strcasecmp(http_auth.authtype, "basic")
> + || !strcasecmp(http_auth.authtype, "digest")) {
> + curl_easy_setopt(slot->curl, CURLOPT_USERNAME,
> + http_auth.username);
> + curl_easy_setopt(slot->curl, CURLOPT_PASSWORD,
> + http_auth.password);
> +#ifdef GIT_CURL_HAVE_CURLAUTH_BEARER
> + } else if (!strcasecmp(http_auth.authtype, "bearer")) {
> + curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
> + curl_easy_setopt(slot->curl, CURLOPT_XOAUTH2_BEARER,
> + http_auth.password);
> +#endif
> + } else {
> + struct strbuf auth = STRBUF_INIT;
> + strbuf_addf(&auth, "Authorization: %s %s",
> + http_auth.authtype, http_auth.password);
> + slot->headers = curl_slist_append(slot->headers, auth.buf);
> + strbuf_release(&auth);
> + }
> }
It would be good to have a test here, and the only way I can think
to add it would be to modify one of the test credential helpers to
indicate that OAuth is being used.
The test would somehow need to be careful about the curl version,
though, and I'm not sure if we have prior work for writing prereqs
based on the linked curl version.
Thanks,
-Stolee
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 9/19/2022 12:08 PM, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> protocol=https
>> host=example.com
>> wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
>> wwwauth[1]=Basic realm="login.example"
>
> The important part here is that we provide a way to specify a multi-valued
> key as opposed to a "last one wins" key, right?
>
> Using empty braces (wwwauth[]) would suffice to indicate this, right? That
> allows us to not care about the values inside the braces. The biggest
> issues I see with a value in the braces are:
>
> 1. What if it isn't an integer?
> 2. What if we are missing a value?
> 3. What if they come out of order?
>
> Without a value inside, then the order in which they appear provides
> implicit indices in their multi-valued list.
After looking at the code, it would not be difficult at all to make this
change in-place for these patches. But I won't push too hard if there is
some reason to keep the index values.
> Other than that, I support this idea and will start looking at the code
> now.
I took a look and provided feedback as I could. Patches 6 and 7 eluded
me only because I'm so unfamiliar with the http.c code and don't have
time to learn it today.
I mentioned that patches 1-3 could easily be picked up as a topic while
the rest of the series is considered carefully.
I tried to add some mentions of testing, but you've already tested more
than I expected, by adding the headers to the Apache output.
Thanks,
-Stolee
|
|
This patch series was integrated into seen via git@1579b5d. |
|
This patch series was integrated into seen via git@d005462. |
|
On the Git mailing list, Lessley Dennington wrote (reply to this): This is a really exciting idea! Based on your patches, it seems to be a
great opportunity to add extensibility and flexibility to the credential
helper model without huge disruptions to the codebase. Well done!
On 9/13/22 12:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> 3. Teach Git to specify authentication schemes other than Basic in
> subsequent HTTP requests based on credential helper responses.
> This!! Yes!!
> > ...
> wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
> I think sending the fields individually (as you describe in this doc and
implement in your patches) is the right call. In my opinion, it's more
legible, consistent with the remote response, and aligns with your goal of
minimizing authentication-related actions in Git.
Best,
Lessley |
|
User |
|
There was a status update in the "Cooking" section about the branch RFC source: <pull.1352.git.1663097156.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@32aafc9. |
|
Submitted as pull.1352.v11.git.1677518420.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@99cba60. |
|
This patch series was integrated into seen via git@2734818. |
|
This patch series was integrated into seen via git@3e34931. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Feb 27, 2023 at 05:20:17PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> Updates in v11
> ==============
>
> * Delete custom-auth.valid and .challenge explicitly in test cleanup.
>
> * Use tolower over strncasecmp in implementation of skip_iprefix_mem.
>
> * Use skip_iprefix_mem to match "HTTP/" header lines.
Thanks, I looked over all three changes and the whole thing looks good
to me. The first one isn't strictly necessary if we're not renaming the
script, but I agree that it is probably worth being a bit more strict
when deleting in $HTTPD_ROOT_PATH.
-Peff |
|
This patch series was integrated into seen via git@e4ad18d. |
|
This patch series was integrated into next via git@89c9bd4. |
|
This patch series was integrated into seen via git@d479e81. |
|
This patch series was integrated into seen via git@8cd6248. |
|
There was a status update in the "Cooking" section about the branch Allow information carried on the WWW-AUthenticate header to be passed to the credential helpers. Will cook in 'next'. source: <pull.1352.v11.git.1677518420.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@95ae0ef. |
|
This patch series was integrated into seen via git@f905cfc. |
|
This patch series was integrated into seen via git@d68a3b8. |
|
This patch series was integrated into seen via git@3e3a7af. |
|
There was a status update in the "Cooking" section about the branch Allow information carried on the WWW-AUthenticate header to be passed to the credential helpers. Will cook in 'next'. source: <pull.1352.v11.git.1677518420.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@26c6ea8. |
|
This patch series was integrated into seen via git@9c18eea. |
|
This patch series was integrated into seen via git@d0beb59. |
|
This patch series was integrated into seen via git@22c92f0. |
|
There was a status update in the "Cooking" section about the branch Allow information carried on the WWW-AUthenticate header to be passed to the credential helpers. Will cook in 'next'. source: <pull.1352.v11.git.1677518420.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@293019e. |
|
This patch series was integrated into seen via git@92c56da. |
|
This patch series was integrated into master via git@92c56da. |
|
This patch series was integrated into next via git@92c56da. |
|
Closed via 92c56da. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Ævar,
On Mon, 6 Feb 2023, Ævar Arnfjörð Bjarmason wrote:
> we currently have CI tests running Apache on *nix boxes, but you're
> suggesting a loss of coverage on Windows
>
> Is it really harder to just install (or even ship our own package of)
> Apache for Windows than it is to embark on PID file handling, logging,
> timeout management and the long tail of "80% is easy, the rest is really
> hard" of writing our own production-class httpd (as the suggestion is to
> have it eventually mature beyond the test suite)?
Yes, it _is_ that much harder, and it would result in yet more painful
increases of the build times which have really gotten out of hand in the
past year.
Ciao,
Johannes |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Jeff,
On Fri, 3 Feb 2023, Jeff King wrote:
> On Thu, Feb 02, 2023 at 11:14:33AM +0100, Johannes Schindelin wrote:
>
> > > I do not mind reverting the merge to 'next' to have an improved
> > > version. Your "do we really want to add a custom server based on
> > > questionable codebase whose quality as a test-bed for real world
> > > usage is dubious?" is a valid concern.
> >
> > Except.
> >
> > Except that this code base would have made for a fine base to potentially
> > implement an HTTPS-based replacement for the aging and insecure
> > git-daemon.
>
> I'm skeptical that it is a good idea for Git to implement a custom http
> server from scratch.
To be clear: I never suggested to implement a generic HTTP server.
All I wanted was to have a replacement for `git daemon` that speaks
https:// instead of git://. It does not have to speak to every browser out
there, it only needs to respond well when speaking to Git clients. That is
a much, much smaller surface than "production-ready server, HTTP/2 and so
on".
And while the proposed test helper was not quite complete in that way, and
while it may have had much of the `git daemon` code that you would love to
lose, it would have offered an incremental way forward.
I am afraid that this way forward is now blocked, and we're further away
from dropping that `git daemon` code you wanted to drop than ever.
Ciao,
Johannes |
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Mar 27, 2023 at 11:10:40AM +0200, Johannes Schindelin wrote:
> > I'm skeptical that it is a good idea for Git to implement a custom http
> > server from scratch.
>
> To be clear: I never suggested to implement a generic HTTP server.
>
> All I wanted was to have a replacement for `git daemon` that speaks
> https:// instead of git://. It does not have to speak to every browser out
> there, it only needs to respond well when speaking to Git clients. That is
> a much, much smaller surface than "production-ready server, HTTP/2 and so
> on".
I guess I don't see the point of having this in our test suite, though.
We do want to test things like HTTP/2, SSL, and so on in our test suite.
So either we have a split in our tests (some use apache, some don't,
which presumably means many tests are still not run on Windows), or this
custom HTTP server eventually grows to do all of those other things.
I can see the utility outside the tests of a quick "let me stand up an
HTTP server to access Git" tool. But even there, I'd be considered with
feature creep as regular users ignore any warnings about its lack of
encryption/robustness, and so on. And it feels like something that could
utilize work already done by others in making a web server. Yes, that's
a new dependency for the tool, but there are a lot of options out there.
Surely one of them is worth building on?
> And while the proposed test helper was not quite complete in that way, and
> while it may have had much of the `git daemon` code that you would love to
> lose, it would have offered an incremental way forward.
>
> I am afraid that this way forward is now blocked, and we're further away
> from dropping that `git daemon` code you wanted to drop than ever.
I don't see how pushing the same code into an http server helps. If we
could have incrementally improved it there, we could incrementally
improve it in git-daemon, too.
-Peff |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following from my original RFC submission [0], this submission is considered
ready for full review. This patch series is now based on top of current
master(9c32cfb) that includes my now separately
submitted patches [1] to fix up the other credential helpers' behaviour.
In this patch series I update the existing credential helper design in order to
allow for some new scenarios, and future evolution of auth methods that Git
hosts may wish to provide. I outline the background, summary of changes and some
challenges below.
Testing these new additions, I use a small CGI shell script that acts as a
frontend to
git-http-backend; simple authentication is configurable by files.Background
Git uses a variety of protocols [2]: local, Smart HTTP, Dumb HTTP, SSH, and Git.
Here I focus on the Smart HTTP protocol, and attempt to enhance the
authentication capabilities of this protocol to address limitations (see below).
The Smart HTTP protocol in Git supports a few different types of HTTP
authentication - Basic and Digest (RFC 2617) [3], and Negotiate (RFC 2478) [4].
Git uses a extensible model where credential helpers can provide credentials for
protocols [5]. Several helpers support alternatives such as OAuth
authentication (RFC 6749) [6], but this is typically done as an extension.
For example, a helper might use basic auth and set the password to an OAuth
Bearer access token. Git uses standard input and output to communicate with
credential helpers.
After a HTTP 401 response, Git would call a credential helper with the following
over standard input:
And then a credential helper would return over standard output:
Git then the following request to the remote, including the standard HTTP
Authorizationheader (RFC 7235 Section 4.2) [7]:Credential helpers are encouraged (see gitcredentials.txt) to return the minimum
information necessary.
Limitations
Because this credential model was built mostly for password based authentication
systems, it's somewhat limited. In particular:
To generate valid credentials, additional information about the request (or
indeed the requestee and their device) may be required.
For example, OAuth is based around scopes. A scope, like "git.read", might be
required to read data from the remote. However, the remote cannot tell the
credential helper what scope is required for this request.
This system is not fully extensible. Each time a new type of authentication
(like OAuth Bearer) is invented, Git needs updates before credential
helpers can take advantage of it (or leverage a new capability in libcurl).
Goals
As a user with multiple federated cloud identities:
for the correct identity.
authentication/authorization challenge types, even from the same DNS
hostname (and without needing to use
credential.useHttpPath).and devices to boost security and reduce reliance on passwords.
As a Git host and/or cloud identity provider:
dynamically.
enterprises allowing customers to have a single plane of control for
critical identities with access to source code.
Design Principles
model.
authentication space.
extensibility.
Proposed Changes
Teach Git to read HTTP response headers, specifically the standard
WWW-Authenticate(RFC 7235 Section 4.1) headers.Teach Git to include extra information about HTTP responses that require
authentication when calling credential helpers. Specifically the
WWW-Authenticateheader information.Because the extra information forms an ordered list, and the existing
credential helper I/O format only provides for simple key=value pairs, we
introduce a new convention for transmitting an ordered list of values.
Key names that are suffixed with a C-style array syntax should have values
considered to form an order list, i.e.
key[]=value, where the order of thekey=value pairs in the stream specifies the order.
For the
WWW-Authenticateheader values we opt to use the keywwwauth[].Handling the WWW-Authenticate header in detail
RFC 6750 [8] envisions that OAuth Bearer resource servers would give responses
that include
WWW-Authenticateheaders, for example:Specifically, a
WWW-Authenticateheader consists of a scheme and arbitraryattributes, depending on the scheme. This pattern enables generic OAuth or
OpenID Connect [9] authorities. Note that it is possible to have several
WWW-Authenticatechallenges in a response.First Git attempts to make a request, unauthenticated, which fails with a 401
response and includes
WWW-Authenticateheader(s).Next, Git invokes a credential helper which may prompt the user. If the user
approves, a credential helper can generate a token (or any auth challenge
response) to be used for that request.
For example: with a remote that supports bearer tokens from an OpenID Connect
[9] authority, a credential helper can use OpenID Connect's Discovery [10] and
Dynamic Client Registration [11] to register a client and make a request with the
correct permissions to access the remote. In this manner, a user can be
dynamically sent to the right federated identity provider for a remote without
any up-front configuration or manual processes.
Following from the principle of keeping authentication knowledge in Git to a
minimum, we modify Git to add all
WWW-Authenticatevalues to the credentialhelper call.
Git sends over standard input:
A credential helper that understands the extra
wwwauth[n]property candecide on the "best" or correct authentication scheme, generate credentials for
the request, and interact with the user.
The credential helper would then return over standard output:
Note that
WWW-Authenticatesupports multiple challenges, either in one header:or in multiple headers:
These have equivalent meaning (RFC 2616 Section 4.2 [12]). To simplify the
implementation, Git will not merge or split up any of these
WWW-Authenticateheaders, and instead pass each header line as one credential helper property.
The credential helper is responsible for splitting, merging, and otherwise
parsing these header values.
An alternative option to sending the header fields individually would be to
merge the header values in to one
key=valueproperty, for example:Future work
In the future we can further expand the protocol to allow credential helpers
decide the best authentication scheme. Today credential helpers are still only
expected to return a username/password pair to Git, meaning the other
authentication schemes that may be offered still need challenge responses sent
via a
BasicAuthorizationheader. The changes outlined above still permithelpers to select and configure an available authentication mode, but require
the remote for example to unpack a bearer token from a basic challenge.
More careful consideration is required in the handling of custom authentication
schemes which may not have a
username, or may require arbitrary additionalrequest header values be set.
For example imagine a new "FooBar" authentication scheme that is surfaced in the
following response:
With support for arbitrary authentication schemes, Git would call credential
helpers with the following over standard input:
And then an enlightened credential helper could return over standard output:
Git would be expected to attach this authorization header to the next request:
Why not SSH?
There's nothing wrong with SSH. However, Git's Smart HTTP transport is widely
used, often with OAuth Bearer tokens. Git's Smart HTTP transport sometimes
requires less client setup than SSH transport, and works in environments when
SSH ports may be blocked. As long as Git supports HTTP transport, it should
support common and popular HTTP authentication methods.
References
[0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
https://lore.kernel.org/git/pull.1352.git.1663097156.gitgitgadget@gmail.com/
[1] [PATCH 0/3] Correct credential helper discrepancies handling input
https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/
[2] Git on the Server - The Protocols
https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
[3] HTTP Authentication: Basic and Digest Access Authentication
https://datatracker.ietf.org/doc/html/rfc2617
[4] The Simple and Protected GSS-API Negotiation Mechanism
https://datatracker.ietf.org/doc/html/rfc2478
[5] Git Credentials - Custom Helpers
https://git-scm.com/docs/gitcredentials#_custom_helpers
[6] The OAuth 2.0 Authorization Framework
https://datatracker.ietf.org/doc/html/rfc6749
[7] Hypertext Transfer Protocol (HTTP/1.1): Authentication
https://datatracker.ietf.org/doc/html/rfc7235
[8] The OAuth 2.0 Authorization Framework: Bearer Token Usage
https://datatracker.ietf.org/doc/html/rfc6750
[9] OpenID Connect Core 1.0
https://openid.net/specs/openid-connect-core-1_0.html
[10] OpenID Connect Discovery 1.0
https://openid.net/specs/openid-connect-discovery-1_0.html
[11] OpenID Connect Dynamic Client Registration 1.0
https://openid.net/specs/openid-connect-registration-1_0.html
[12] Hypertext Transfer Protocol (HTTP/1.1)
https://datatracker.ietf.org/doc/html/rfc2616
Updates from RFC
Submitted first three patches as separate submission:
https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/
Various style fixes and updates to- and addition of comments.
Drop the explicit integer index in new 'array' style credential helper
attrbiutes ("key[n]=value" becomes just "key[]=value").
Added test helper; a mini HTTP server, and several tests.
Updates in v3
Split final patch that added the test-http-server in to several, easier
to review patches.
Updated wording in git-credential.txt to clarify which side of the credential
helper protocol is sending/receiving the new wwwauth and authtype attributes.
Updates in v4
authtypeattribute patches to greatlysimplify the series; auth scheme selection is punted to a future series.
This series still allows credential helpers to generate credentials and
intelligently select correct identities for a given auth challenge.
Updates in v5
Libify parts of daemon.c and share implementation with
test-http-server.Clarify
test-http-serverGit request regex pattern and auth logic comments.Use
STD*_FILENOin place of 'magic' file descriptor numbers.Use
strbuf_*functions in continuation header parsing.Use configuration file to configure auth for
test-http-serverrather thancommand-line arguments. Add ability to specify arbitrary extra headers that
is useful for testing 'malformed' server responses.
Use
st_multover unchecked multiplication in http.c curl callback functions.Fix some documentation line break issues.
Reorder some commits to bring in the tests and
test-http-serverhelper firstand, then the WWW-Authentication changes, alongside tests to cover.
Expose previously static
strvec_push_nodupfunction.Merge the two timeout args for
test-http-server(--timeoutand--init-timeout) that were a hang-over from the original daemon.c but areno longer required here.
Be more careful around continuation headers where they may be empty strings.
Add more tests to cover these header types.
Include standard trace2 tracing calls at start of
test-http-serverhelper.Updates in v6
Clarify the change to make logging optional in the
check_dead_children()function during libification of
daemon.c.Fix missing pointer dereference bugs identified in libification of child
process handling functions for
daemon.c.Add doc comments to child process handling function declarations in the
daemon-utils.hheader.Align function parameter names with variable names at callsites for libified
daemon functions.
Re-split out the
test-http-servertest helper commits in to smaller patches:error response handling, request parsing,
http-backendpass-through, simpleauthentication, arbitrary header support.
Call out auth configuration file format for
test-http-servertest helper andsupported options in commit messages, as well as a test to exercise and
demonstrate these options.
Permit
auth.tokenandauth.challengeto appear in any order; create thestruct auth_modulejust-in-time as options for that scheme are read. Thissimplifies the configuration authoring of the
test-http-servertest helper.Update tests to use
auth.allowAnoymousin the patch that introduces the newtest helper option.
Drop the
strvec_push_nodup()commit and update the implementation of HTTPrequest header line folding to use
xstrdupandstrvec_popand_pushf.Use
size_tinstead ofintincredential.cwhen iterating over thestruct strveccredential members. Also drop the not requiredconstandcast from the
full_keydefinition andfree.Replace in-tree
test-credential-helper-reply.shtest cred helper script withthe
lib-credential-helper.shreusable 'lib' test script and shell functionsto configure the helper behaviour.
Leverage
sedover thewhile read $lineloop in the test credential helperscript.
Updates in v7
Address several whitespace and arg/param list alignment issues.
Rethink the
test-http-helperworker-mode error and resultenumto be moresimple and more informative to the nature of the error.
Use
uintmax_tto store theContent-Lengthof a request in the helpertest-http-server. Maintain a bit flag to store if we received such a header.Return a "400 Bad Request" HTTP response if we fail to parse the request in
the
test-http-server.Add test case to cover request message parsing in
test-http-server.Use
size_tandALLOC_ARRAYoverintandCALLOC_ARRAYrespectively inget_auth_module.Correctly free the split
strbufs created in the header parsing loop intest-http-server.Avoid needless comparison
> 0for unsigned types.Always set optional outputs to
NULLif not present in test helper configvalue handling.
Remove an accidentally commented-out test cleanup line for one test case in
t5556.
Updates in v8
Drop custom HTTP test helper tool in favour of using a CGI shell script and
Apache; avoiding the need to implement an HTTP server.
Avoid allocations in header reading callback unless we have a header we care
about; act on the char* from libcurl directly rather than create a strbuf for
each header.
Drop st_mult overflow guarding function in curl callback functions; we're not
allocating memory based on the resulting value and just adds to potential
confusion in the future.
Updates in v9
Drop anoynmous auth tests as these cases are already covered by all other
tests that perform HTTP interactions with a remote today.
In the custom auth CGI script, avoid the empty-substitution in favour of
testing explicitly for an empty string. Also simplify some other conditional
expressions.
Avoid an allocation on each wwwauth[] credential helper key-value pair write.
Various style fixups.
Updates in v10
Style fixups.
Only consider space (SP ' ') and horizontal tab (HTAB '\t') when detecting
a header continuation line, as per the latest RFC on the matter.
Update references to old HTTP specs and formal grammars of header fields in
comments.
Rewording of commit messages to remove confusing comment about the case
sensitivity of header field names - this is not relevant with the current
iteration of the header parsing code. Also update the message around libcurl
header support to clarify that physical header lines are returned, but not
'logical' header lines.
Reword
struct credentialmember doc comment to clarify the purpose ofheader_is_last_matchis for re-folding lines of the WWW-Authenticate header.Reintroduce helpful comments in tests to show the origin of the 'magic' base64
basic auth value.
Use
grep -Fto ensure we don't do regex matching; avoid interpreting specialcharacters. Remove erronous insensitive comparison flag.
Updates in v11
Delete custom-auth.valid and .challenge explicitly in test cleanup.
Use
toloweroverstrncasecmpin implementation ofskip_iprefix_mem.Use
skip_iprefix_memto match "HTTP/" header lines.cc: Derrick Stolee derrickstolee@github.com
cc: Lessley Dennington lessleydennington@gmail.com
cc: Matthew John Cheetham mjcheetham@outlook.com
cc: M Hickford mirth.hickford@gmail.com
cc: Jeff Hostetler git@jeffhostetler.com
cc: Glen Choo chooglen@google.com
cc: Victoria Dye vdye@github.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Jeff King peff@peff.net
cc: Johannes Schindelin Johannes.Schindelin@gmx.de