| Message ID | 20240324011301.1553072-14-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for arbitrary schemes in credentials | expand |
On Sun, 24 Mar 2024 at 01:13, brian m. carlson <sandals@crustytoothpaste.net> wrote: > > Over HTTP, NTLM and Kerberos require two rounds of authentication on the > client side. It's possible that there are custom authentication schemes > that also implement this same approach. Since these are tricky schemes > to implement and the HTTP library in use may not always handle them > gracefully on all systems, it would be helpful to allow the credential > helper to implement them instead for increased portability and > robustness. Is this design sufficiently flexible for OAuth DPoP (RFC 9449), or at least to make it work in future? OAuth 2.0 Demonstrating Proof of Possession describes "a mechanism for sender-constraining OAuth 2.0 tokens via a proof-of-possession mechanism on the application level. This mechanism allows for the detection of replay attacks with access and refresh tokens." https://www.rfc-editor.org/rfc/rfc9449.html Popular hosts GitHub, GitLab, Bitbucket and Gitea already support OAuth. OAuth DPoP "provides a general defense in depth against the impact of unanticipated token leakage". Motivated by a 2022 GitHub attack involving stolen tokens (https://github.blog/2022-04-15-security-alert-stolen-oauth-user-tokens/), some hosts are already experimenting with it. https://lore.kernel.org/git/20230128142827.17397-1-mirth.hickford@gmail.com/ In particular, the http request has to include both Authorization and DPoP headers https://www.rfc-editor.org/rfc/rfc9449.html#name-the-dpop-authentication-sch. The latter depends on timestamp and a server-optional challenge in a DPoP-Nonce header. https://www.rfc-editor.org/rfc/rfc9449.html#name-resource-server-provided-no. On Sun, 24 Mar 2024 at 01:13, brian m. carlson <sandals@crustytoothpaste.net> wrote: > > Over HTTP, NTLM and Kerberos require two rounds of authentication on the > client side. It's possible that there are custom authentication schemes > that also implement this same approach. Since these are tricky schemes > to implement and the HTTP library in use may not always handle them > gracefully on all systems, it would be helpful to allow the credential > helper to implement them instead for increased portability and > robustness. > > To allow this to happen, add a boolean flag, continue, that indicates > that instead of failing when we get a 401, we should retry another round > of authentication. However, this necessitates some changes in our > current credential code so that we can make this work. > > Keep the state[] headers between iterations, but only use them to send > to the helper and only consider the new ones we read from the credential > helper to be valid on subsequent iterations. That avoids us passing > stale data when we finally approve or reject the credential. Similarly, > clear the multistage and wwwauth[] values appropriately so that we > don't pass stale data or think we're trying a multiround response when > we're not. Remove the credential values so that we can actually fill a > second time with new responses. > > Limit the number of iterations of reauthentication we do to 3. This > means that if there's a problem, we'll terminate with an error message > instead of retrying indefinitely and not informing the user (and > possibly conducting a DoS on the server). > > In our tests, handle creating multiple response output files from our > helper so we can verify that each of the messages sent is correct. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > Documentation/git-credential.txt | 16 +++++- > builtin/credential.c | 1 + > credential.c | 32 ++++++++++-- > credential.h | 27 +++++++++- > http.c | 59 +++++++++++---------- > t/t5563-simple-http-auth.sh | 89 ++++++++++++++++++++++++++++++-- > 6 files changed, 187 insertions(+), 37 deletions(-) > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > index 6b7e017066..160dee5c6a 100644 > --- a/Documentation/git-credential.txt > +++ b/Documentation/git-credential.txt > @@ -207,6 +207,19 @@ provided on input. > This value should not be sent unless the appropriate capability (see below) is > provided on input. > > +`continue`:: > + This is a boolean value, which, if enabled, indicates that this > + authentication is a non-final part of a multistage authentication step. This > + is common in protocols such as NTLM and Kerberos, where two rounds of client > + authentication are required, and setting this flag allows the credential > + helper to implement the multistage authentication step. This flag should > + only be sent if a further stage is required; that is, if another round of > + authentication is expected. > ++ > +This value should not be sent unless the appropriate capability (see below) is > +provided on input. This attribute is 'one-way' from a credential helper to > +pass information to Git (or other programs invoking `git credential`). > + > `wwwauth[]`:: > > When an HTTP response is received by Git that includes one or more > @@ -225,7 +238,8 @@ to pass additional information to credential helpers. > + > There are two currently supported capabilities. The first is `authtype`, which > indicates that the `authtype` and `credential` values are understood. The > -second is `state`, which indicates that the `state[]` value is understood. > +second is `state`, which indicates that the `state[]` and `continue` values are > +understood. > > It is not obligatory to use the additional features just because the capability > is supported, but they should not be provided without this capability. > diff --git a/builtin/credential.c b/builtin/credential.c > index 5123dabcf1..f14d1b5ed6 100644 > --- a/builtin/credential.c > +++ b/builtin/credential.c > @@ -22,6 +22,7 @@ int cmd_credential(int argc, const char **argv, const char *prefix UNUSED) > > if (!strcmp(op, "fill")) { > credential_fill(&c, 0); > + credential_next_state(&c); > credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE); > } else if (!strcmp(op, "approve")) { > credential_approve(&c); > diff --git a/credential.c b/credential.c > index 0ca7c12895..9a08efe113 100644 > --- a/credential.c > +++ b/credential.c > @@ -31,10 +31,23 @@ void credential_clear(struct credential *c) > string_list_clear(&c->helpers, 0); > strvec_clear(&c->wwwauth_headers); > strvec_clear(&c->state_headers); > + strvec_clear(&c->state_headers_to_send); > > credential_init(c); > } > > +void credential_next_state(struct credential *c) > +{ > + strvec_clear(&c->state_headers_to_send); > + strvec_swap(&c->state_headers, &c->state_headers_to_send); > +} > + > +void credential_clear_secrets(struct credential *c) > +{ > + FREE_AND_NULL(c->password); > + FREE_AND_NULL(c->credential); > +} > + > static void credential_set_all_capabilities(struct credential *c) > { > c->capa_authtype.request_initial = 1; > @@ -295,6 +308,8 @@ int credential_read(struct credential *c, FILE *fp, int op_type) > credential_set_capability(&c->capa_authtype, op_type); > else if (!strcmp(value, "state")) > credential_set_capability(&c->capa_state, op_type); > + } else if (!strcmp(key, "continue")) { > + c->multistage = !!git_config_bool("continue", value); > } else if (!strcmp(key, "password_expiry_utc")) { > errno = 0; > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > @@ -359,8 +374,10 @@ void credential_write(const struct credential *c, FILE *fp, int op_type) > for (size_t i = 0; i < c->wwwauth_headers.nr; i++) > credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); > if (credential_has_capability(&c->capa_state, op_type)) { > - for (size_t i = 0; i < c->state_headers.nr; i++) > - credential_write_item(fp, "state[]", c->state_headers.v[i], 0); > + if (c->multistage) > + credential_write_item(fp, "continue", "1", 0); > + for (size_t i = 0; i < c->state_headers_to_send.nr; i++) > + credential_write_item(fp, "state[]", c->state_headers_to_send.v[i], 0); > } > } > > @@ -431,6 +448,9 @@ void credential_fill(struct credential *c, int all_capabilities) > if ((c->username && c->password) || c->credential) > return; > > + credential_next_state(c); > + c->multistage = 0; > + > credential_apply_config(c); > if (all_capabilities) > credential_set_all_capabilities(c); > @@ -443,8 +463,10 @@ void credential_fill(struct credential *c, int all_capabilities) > /* Reset expiry to maintain consistency */ > c->password_expiry_utc = TIME_MAX; > } > - if ((c->username && c->password) || c->credential) > + if ((c->username && c->password) || c->credential) { > + strvec_clear(&c->wwwauth_headers); > return; > + } > if (c->quit) > die("credential helper '%s' told us to quit", > c->helpers.items[i].string); > @@ -464,6 +486,8 @@ void credential_approve(struct credential *c) > if (((!c->username || !c->password) && !c->credential) || c->password_expiry_utc < time(NULL)) > return; > > + credential_next_state(c); > + > credential_apply_config(c); > > for (i = 0; i < c->helpers.nr; i++) > @@ -475,6 +499,8 @@ void credential_reject(struct credential *c) > { > int i; > > + credential_next_state(c); > + > credential_apply_config(c); > > for (i = 0; i < c->helpers.nr; i++) > diff --git a/credential.h b/credential.h > index e2021455fe..adb1fc370a 100644 > --- a/credential.h > +++ b/credential.h > @@ -143,10 +143,15 @@ struct credential { > struct strvec wwwauth_headers; > > /** > - * A `strvec` of state headers from credential helpers. > + * A `strvec` of state headers received from credential helpers. > */ > struct strvec state_headers; > > + /** > + * A `strvec` of state headers to send to credential helpers. > + */ > + struct strvec state_headers_to_send; > + > /** > * Internal use only. Keeps track of if we previously matched against a > * WWW-Authenticate header line in order to re-fold future continuation > @@ -156,6 +161,7 @@ struct credential { > > unsigned approved:1, > configured:1, > + multistage: 1, > quit:1, > use_http_path:1, > username_from_proto:1; > @@ -184,6 +190,7 @@ struct credential { > .password_expiry_utc = TIME_MAX, \ > .wwwauth_headers = STRVEC_INIT, \ > .state_headers = STRVEC_INIT, \ > + .state_headers_to_send = STRVEC_INIT, \ > } > > /* Initialize a credential structure, setting all fields to empty. */ > @@ -229,6 +236,24 @@ void credential_approve(struct credential *); > */ > void credential_reject(struct credential *); > > +/** > + * Clear the secrets in this credential, but leave other data intact. > + * > + * This is useful for resetting credentials in preparation for a subsequent > + * stage of filling. > + */ > +void credential_clear_secrets(struct credential *c); > + > +/** > + * Prepares the credential for the next iteration of the helper protocol by > + * updating the state headers to send with the ones read by the last iteration > + * of the protocol. > + * > + * Except for internal callers, this should be called exactly once between > + * reading credentials with `credential_fill` and writing them. > + */ > +void credential_next_state(struct credential *c); > + > int credential_read(struct credential *, FILE *, int); > void credential_write(const struct credential *, FILE *, int); > > diff --git a/http.c b/http.c > index f98c520924..9d373c6460 100644 > --- a/http.c > +++ b/http.c > @@ -1781,6 +1781,10 @@ static int handle_curl_result(struct slot_results *results) > else if (results->http_code == 401) { > if ((http_auth.username && http_auth.password) ||\ > (http_auth.authtype && http_auth.credential)) { > + if (http_auth.multistage) { > + credential_clear_secrets(&http_auth); > + return HTTP_REAUTH; > + } > credential_reject(&http_auth); > return HTTP_NOAUTH; > } else { > @@ -2178,6 +2182,7 @@ static int http_request_reauth(const char *url, > void *result, int target, > struct http_get_options *options) > { > + int i = 3; > int ret = http_request(url, result, target, options); > > if (ret != HTTP_OK && ret != HTTP_REAUTH) > @@ -2191,35 +2196,35 @@ static int http_request_reauth(const char *url, > } > } > > - if (ret != HTTP_REAUTH) > - return ret; > + while (ret == HTTP_REAUTH && --i) { > + /* > + * The previous request may have put cruft into our output stream; we > + * should clear it out before making our next request. > + */ > + switch (target) { > + case HTTP_REQUEST_STRBUF: > + strbuf_reset(result); > + break; > + case HTTP_REQUEST_FILE: > + if (fflush(result)) { > + error_errno("unable to flush a file"); > + return HTTP_START_FAILED; > + } > + rewind(result); > + if (ftruncate(fileno(result), 0) < 0) { > + error_errno("unable to truncate a file"); > + return HTTP_START_FAILED; > + } > + break; > + default: > + BUG("Unknown http_request target"); > + } > > - /* > - * The previous request may have put cruft into our output stream; we > - * should clear it out before making our next request. > - */ > - switch (target) { > - case HTTP_REQUEST_STRBUF: > - strbuf_reset(result); > - break; > - case HTTP_REQUEST_FILE: > - if (fflush(result)) { > - error_errno("unable to flush a file"); > - return HTTP_START_FAILED; > - } > - rewind(result); > - if (ftruncate(fileno(result), 0) < 0) { > - error_errno("unable to truncate a file"); > - return HTTP_START_FAILED; > - } > - break; > - default: > - BUG("Unknown http_request target"); > + credential_fill(&http_auth, 1); > + > + ret = http_request(url, result, target, options); > } > - > - credential_fill(&http_auth, 1); > - > - return http_request(url, result, target, options); > + return ret; > } > > int http_get_strbuf(const char *url, > diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh > index 515185ae00..5d5caa3f58 100755 > --- a/t/t5563-simple-http-auth.sh > +++ b/t/t5563-simple-http-auth.sh > @@ -21,9 +21,17 @@ test_expect_success 'setup_credential_helper' ' > CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" && > write_script "$CREDENTIAL_HELPER" <<-\EOF > cmd=$1 > - teefile=$cmd-query.cred > + teefile=$cmd-query-temp.cred > catfile=$cmd-reply.cred > sed -n -e "/^$/q" -e "p" >>$teefile > + state=$(sed -ne "s/^state\[\]=helper://p" "$teefile") > + if test -z "$state" > + then > + mv "$teefile" "$cmd-query.cred" > + else > + mv "$teefile" "$cmd-query-$state.cred" > + catfile="$cmd-reply-$state.cred" > + fi > if test "$cmd" = "get" > then > cat $catfile > @@ -32,13 +40,15 @@ test_expect_success 'setup_credential_helper' ' > ' > > set_credential_reply () { > - cat >"$TRASH_DIRECTORY/$1-reply.cred" > + local suffix="$(test -n "$2" && echo "-$2")" > + cat >"$TRASH_DIRECTORY/$1-reply$suffix.cred" > } > > expect_credential_query () { > - cat >"$TRASH_DIRECTORY/$1-expect.cred" && > - test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \ > - "$TRASH_DIRECTORY/$1-query.cred" > + local suffix="$(test -n "$2" && echo "-$2")" > + cat >"$TRASH_DIRECTORY/$1-expect$suffix.cred" && > + test_cmp "$TRASH_DIRECTORY/$1-expect$suffix.cred" \ > + "$TRASH_DIRECTORY/$1-query$suffix.cred" > } > > per_test_cleanup () { > @@ -479,4 +489,73 @@ test_expect_success 'access using bearer auth with invalid credentials' ' > EOF > ' > > +test_expect_success 'access using three-legged auth' ' > + test_when_finished "per_test_cleanup" && > + > + set_credential_reply get <<-EOF && > + capability[]=authtype > + capability[]=state > + authtype=Multistage > + credential=YS1naXQtdG9rZW4= > + state[]=helper:foobar > + continue=1 > + EOF > + > + set_credential_reply get foobar <<-EOF && > + capability[]=authtype > + capability[]=state > + authtype=Multistage > + credential=YW5vdGhlci10b2tlbg== > + state[]=helper:bazquux > + EOF > + > + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && > + id=1 creds=Multistage YS1naXQtdG9rZW4= > + id=2 creds=Multistage YW5vdGhlci10b2tlbg== > + EOF > + > + CHALLENGE="$HTTPD_ROOT_PATH/custom-auth.challenge" && > + > + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && > + id=1 status=401 response=WWW-Authenticate: Multistage challenge="456" > + id=1 status=401 response=WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 > + id=2 status=200 > + id=default response=WWW-Authenticate: Multistage challenge="123" > + id=default response=WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 > + EOF > + > + test_config_global credential.helper test-helper && > + git ls-remote "$HTTPD_URL/custom_auth/repo.git" && > + > + expect_credential_query get <<-EOF && > + capability[]=authtype > + capability[]=state > + protocol=http > + host=$HTTPD_DEST > + wwwauth[]=Multistage challenge="123" > + wwwauth[]=Bearer authorize_uri="id.example.com" p=1 q=0 > + EOF > + > + expect_credential_query get foobar <<-EOF && > + capability[]=authtype > + capability[]=state > + authtype=Multistage > + protocol=http > + host=$HTTPD_DEST > + wwwauth[]=Multistage challenge="456" > + wwwauth[]=Bearer authorize_uri="id.example.com" p=1 q=0 > + state[]=helper:foobar > + EOF > + > + expect_credential_query store bazquux <<-EOF > + capability[]=authtype > + capability[]=state > + authtype=Multistage > + credential=YW5vdGhlci10b2tlbg== > + protocol=http > + host=$HTTPD_DEST > + state[]=helper:bazquux > + EOF > +' > + > test_done
On 2024-03-28 at 08:00:00, M Hickford wrote: > Is this design sufficiently flexible for OAuth DPoP (RFC 9449), or at > least to make it work in future? > > OAuth 2.0 Demonstrating Proof of Possession describes "a mechanism for > sender-constraining OAuth 2.0 tokens via a proof-of-possession > mechanism on the application level. This mechanism allows for the > detection of replay attacks with access and refresh tokens." > https://www.rfc-editor.org/rfc/rfc9449.html > > Popular hosts GitHub, GitLab, Bitbucket and Gitea already support > OAuth. OAuth DPoP "provides a general defense in depth against the > impact of unanticipated token leakage". Motivated by a 2022 GitHub > attack involving stolen tokens > (https://github.blog/2022-04-15-security-alert-stolen-oauth-user-tokens/), > some hosts are already experimenting with it. > https://lore.kernel.org/git/20230128142827.17397-1-mirth.hickford@gmail.com/ > > In particular, the http request has to include both Authorization and > DPoP headers https://www.rfc-editor.org/rfc/rfc9449.html#name-the-dpop-authentication-sch. > The latter depends on timestamp and a server-optional challenge in a > DPoP-Nonce header. > https://www.rfc-editor.org/rfc/rfc9449.html#name-resource-server-provided-no. It will likely be sufficient with further extensions. Right now, we don't have a way to provide DPoP headers or send nonces to the client. However, there's no reason we cannot provide that functionality in the future via additional key/value pairs, in which case this design should be fine. This would have been sufficient if the OAuth working group had not added extra additional headers that other authentication mechanisms would have simply put (and, honestly, should have been put) in the WWW-Authenticate and Authorization headers, but alas, we can't change it now. Since I think this gets us at least part of the way where we need to be, I think we should be able to keep it for now and implement the extra support for DPoP later.
On Thu, 28 Mar 2024 at 21:53, brian m. carlson <sandals@crustytoothpaste.net> wrote: > > On 2024-03-28 at 08:00:00, M Hickford wrote: > > Is this design sufficiently flexible for OAuth DPoP (RFC 9449), or at > > least to make it work in future? > > > > OAuth 2.0 Demonstrating Proof of Possession describes "a mechanism for > > sender-constraining OAuth 2.0 tokens via a proof-of-possession > > mechanism on the application level. This mechanism allows for the > > detection of replay attacks with access and refresh tokens." > > https://www.rfc-editor.org/rfc/rfc9449.html > > > > Popular hosts GitHub, GitLab, Bitbucket and Gitea already support > > OAuth. OAuth DPoP "provides a general defense in depth against the > > impact of unanticipated token leakage". Motivated by a 2022 GitHub > > attack involving stolen tokens > > (https://github.blog/2022-04-15-security-alert-stolen-oauth-user-tokens/), > > some hosts are already experimenting with it. > > https://lore.kernel.org/git/20230128142827.17397-1-mirth.hickford@gmail.com/ > > > > In particular, the http request has to include both Authorization and > > DPoP headers https://www.rfc-editor.org/rfc/rfc9449.html#name-the-dpop-authentication-sch. > > The latter depends on timestamp and a server-optional challenge in a > > DPoP-Nonce header. > > https://www.rfc-editor.org/rfc/rfc9449.html#name-resource-server-provided-no. > > It will likely be sufficient with further extensions. Right now, we > don't have a way to provide DPoP headers or send nonces to the client. > However, there's no reason we cannot provide that functionality in the > future via additional key/value pairs, in which case this design should > be fine. > > This would have been sufficient if the OAuth working group had not added > extra additional headers that other authentication mechanisms would have > simply put (and, honestly, should have been put) in the WWW-Authenticate > and Authorization headers, but alas, we can't change it now. > > Since I think this gets us at least part of the way where we need to be, > I think we should be able to keep it for now and implement the extra > support for DPoP later. Fantastic, thanks for considering this. I look forward to sharing a test Git remote with OAuth DPoP when I can figure out how. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index 6b7e017066..160dee5c6a 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -207,6 +207,19 @@ provided on input. This value should not be sent unless the appropriate capability (see below) is provided on input. +`continue`:: + This is a boolean value, which, if enabled, indicates that this + authentication is a non-final part of a multistage authentication step. This + is common in protocols such as NTLM and Kerberos, where two rounds of client + authentication are required, and setting this flag allows the credential + helper to implement the multistage authentication step. This flag should + only be sent if a further stage is required; that is, if another round of + authentication is expected. ++ +This value should not be sent unless the appropriate capability (see below) is +provided on input. This attribute is 'one-way' from a credential helper to +pass information to Git (or other programs invoking `git credential`). + `wwwauth[]`:: When an HTTP response is received by Git that includes one or more @@ -225,7 +238,8 @@ to pass additional information to credential helpers. + There are two currently supported capabilities. The first is `authtype`, which indicates that the `authtype` and `credential` values are understood. The -second is `state`, which indicates that the `state[]` value is understood. +second is `state`, which indicates that the `state[]` and `continue` values are +understood. It is not obligatory to use the additional features just because the capability is supported, but they should not be provided without this capability. diff --git a/builtin/credential.c b/builtin/credential.c index 5123dabcf1..f14d1b5ed6 100644 --- a/builtin/credential.c +++ b/builtin/credential.c @@ -22,6 +22,7 @@ int cmd_credential(int argc, const char **argv, const char *prefix UNUSED) if (!strcmp(op, "fill")) { credential_fill(&c, 0); + credential_next_state(&c); credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE); } else if (!strcmp(op, "approve")) { credential_approve(&c); diff --git a/credential.c b/credential.c index 0ca7c12895..9a08efe113 100644 --- a/credential.c +++ b/credential.c @@ -31,10 +31,23 @@ void credential_clear(struct credential *c) string_list_clear(&c->helpers, 0); strvec_clear(&c->wwwauth_headers); strvec_clear(&c->state_headers); + strvec_clear(&c->state_headers_to_send); credential_init(c); } +void credential_next_state(struct credential *c) +{ + strvec_clear(&c->state_headers_to_send); + strvec_swap(&c->state_headers, &c->state_headers_to_send); +} + +void credential_clear_secrets(struct credential *c) +{ + FREE_AND_NULL(c->password); + FREE_AND_NULL(c->credential); +} + static void credential_set_all_capabilities(struct credential *c) { c->capa_authtype.request_initial = 1; @@ -295,6 +308,8 @@ int credential_read(struct credential *c, FILE *fp, int op_type) credential_set_capability(&c->capa_authtype, op_type); else if (!strcmp(value, "state")) credential_set_capability(&c->capa_state, op_type); + } else if (!strcmp(key, "continue")) { + c->multistage = !!git_config_bool("continue", value); } else if (!strcmp(key, "password_expiry_utc")) { errno = 0; c->password_expiry_utc = parse_timestamp(value, NULL, 10); @@ -359,8 +374,10 @@ void credential_write(const struct credential *c, FILE *fp, int op_type) for (size_t i = 0; i < c->wwwauth_headers.nr; i++) credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); if (credential_has_capability(&c->capa_state, op_type)) { - for (size_t i = 0; i < c->state_headers.nr; i++) - credential_write_item(fp, "state[]", c->state_headers.v[i], 0); + if (c->multistage) + credential_write_item(fp, "continue", "1", 0); + for (size_t i = 0; i < c->state_headers_to_send.nr; i++) + credential_write_item(fp, "state[]", c->state_headers_to_send.v[i], 0); } } @@ -431,6 +448,9 @@ void credential_fill(struct credential *c, int all_capabilities) if ((c->username && c->password) || c->credential) return; + credential_next_state(c); + c->multistage = 0; + credential_apply_config(c); if (all_capabilities) credential_set_all_capabilities(c); @@ -443,8 +463,10 @@ void credential_fill(struct credential *c, int all_capabilities) /* Reset expiry to maintain consistency */ c->password_expiry_utc = TIME_MAX; } - if ((c->username && c->password) || c->credential) + if ((c->username && c->password) || c->credential) { + strvec_clear(&c->wwwauth_headers); return; + } if (c->quit) die("credential helper '%s' told us to quit", c->helpers.items[i].string); @@ -464,6 +486,8 @@ void credential_approve(struct credential *c) if (((!c->username || !c->password) && !c->credential) || c->password_expiry_utc < time(NULL)) return; + credential_next_state(c); + credential_apply_config(c); for (i = 0; i < c->helpers.nr; i++) @@ -475,6 +499,8 @@ void credential_reject(struct credential *c) { int i; + credential_next_state(c); + credential_apply_config(c); for (i = 0; i < c->helpers.nr; i++) diff --git a/credential.h b/credential.h index e2021455fe..adb1fc370a 100644 --- a/credential.h +++ b/credential.h @@ -143,10 +143,15 @@ struct credential { struct strvec wwwauth_headers; /** - * A `strvec` of state headers from credential helpers. + * A `strvec` of state headers received from credential helpers. */ struct strvec state_headers; + /** + * A `strvec` of state headers to send to credential helpers. + */ + struct strvec state_headers_to_send; + /** * Internal use only. Keeps track of if we previously matched against a * WWW-Authenticate header line in order to re-fold future continuation @@ -156,6 +161,7 @@ struct credential { unsigned approved:1, configured:1, + multistage: 1, quit:1, use_http_path:1, username_from_proto:1; @@ -184,6 +190,7 @@ struct credential { .password_expiry_utc = TIME_MAX, \ .wwwauth_headers = STRVEC_INIT, \ .state_headers = STRVEC_INIT, \ + .state_headers_to_send = STRVEC_INIT, \ } /* Initialize a credential structure, setting all fields to empty. */ @@ -229,6 +236,24 @@ void credential_approve(struct credential *); */ void credential_reject(struct credential *); +/** + * Clear the secrets in this credential, but leave other data intact. + * + * This is useful for resetting credentials in preparation for a subsequent + * stage of filling. + */ +void credential_clear_secrets(struct credential *c); + +/** + * Prepares the credential for the next iteration of the helper protocol by + * updating the state headers to send with the ones read by the last iteration + * of the protocol. + * + * Except for internal callers, this should be called exactly once between + * reading credentials with `credential_fill` and writing them. + */ +void credential_next_state(struct credential *c); + int credential_read(struct credential *, FILE *, int); void credential_write(const struct credential *, FILE *, int); diff --git a/http.c b/http.c index f98c520924..9d373c6460 100644 --- a/http.c +++ b/http.c @@ -1781,6 +1781,10 @@ static int handle_curl_result(struct slot_results *results) else if (results->http_code == 401) { if ((http_auth.username && http_auth.password) ||\ (http_auth.authtype && http_auth.credential)) { + if (http_auth.multistage) { + credential_clear_secrets(&http_auth); + return HTTP_REAUTH; + } credential_reject(&http_auth); return HTTP_NOAUTH; } else { @@ -2178,6 +2182,7 @@ static int http_request_reauth(const char *url, void *result, int target, struct http_get_options *options) { + int i = 3; int ret = http_request(url, result, target, options); if (ret != HTTP_OK && ret != HTTP_REAUTH) @@ -2191,35 +2196,35 @@ static int http_request_reauth(const char *url, } } - if (ret != HTTP_REAUTH) - return ret; + while (ret == HTTP_REAUTH && --i) { + /* + * The previous request may have put cruft into our output stream; we + * should clear it out before making our next request. + */ + switch (target) { + case HTTP_REQUEST_STRBUF: + strbuf_reset(result); + break; + case HTTP_REQUEST_FILE: + if (fflush(result)) { + error_errno("unable to flush a file"); + return HTTP_START_FAILED; + } + rewind(result); + if (ftruncate(fileno(result), 0) < 0) { + error_errno("unable to truncate a file"); + return HTTP_START_FAILED; + } + break; + default: + BUG("Unknown http_request target"); + } - /* - * The previous request may have put cruft into our output stream; we - * should clear it out before making our next request. - */ - switch (target) { - case HTTP_REQUEST_STRBUF: - strbuf_reset(result); - break; - case HTTP_REQUEST_FILE: - if (fflush(result)) { - error_errno("unable to flush a file"); - return HTTP_START_FAILED; - } - rewind(result); - if (ftruncate(fileno(result), 0) < 0) { - error_errno("unable to truncate a file"); - return HTTP_START_FAILED; - } - break; - default: - BUG("Unknown http_request target"); + credential_fill(&http_auth, 1); + + ret = http_request(url, result, target, options); } - - credential_fill(&http_auth, 1); - - return http_request(url, result, target, options); + return ret; } int http_get_strbuf(const char *url, diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh index 515185ae00..5d5caa3f58 100755 --- a/t/t5563-simple-http-auth.sh +++ b/t/t5563-simple-http-auth.sh @@ -21,9 +21,17 @@ test_expect_success 'setup_credential_helper' ' CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" && write_script "$CREDENTIAL_HELPER" <<-\EOF cmd=$1 - teefile=$cmd-query.cred + teefile=$cmd-query-temp.cred catfile=$cmd-reply.cred sed -n -e "/^$/q" -e "p" >>$teefile + state=$(sed -ne "s/^state\[\]=helper://p" "$teefile") + if test -z "$state" + then + mv "$teefile" "$cmd-query.cred" + else + mv "$teefile" "$cmd-query-$state.cred" + catfile="$cmd-reply-$state.cred" + fi if test "$cmd" = "get" then cat $catfile @@ -32,13 +40,15 @@ test_expect_success 'setup_credential_helper' ' ' set_credential_reply () { - cat >"$TRASH_DIRECTORY/$1-reply.cred" + local suffix="$(test -n "$2" && echo "-$2")" + cat >"$TRASH_DIRECTORY/$1-reply$suffix.cred" } expect_credential_query () { - cat >"$TRASH_DIRECTORY/$1-expect.cred" && - test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \ - "$TRASH_DIRECTORY/$1-query.cred" + local suffix="$(test -n "$2" && echo "-$2")" + cat >"$TRASH_DIRECTORY/$1-expect$suffix.cred" && + test_cmp "$TRASH_DIRECTORY/$1-expect$suffix.cred" \ + "$TRASH_DIRECTORY/$1-query$suffix.cred" } per_test_cleanup () { @@ -479,4 +489,73 @@ test_expect_success 'access using bearer auth with invalid credentials' ' EOF ' +test_expect_success 'access using three-legged auth' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + capability[]=authtype + capability[]=state + authtype=Multistage + credential=YS1naXQtdG9rZW4= + state[]=helper:foobar + continue=1 + EOF + + set_credential_reply get foobar <<-EOF && + capability[]=authtype + capability[]=state + authtype=Multistage + credential=YW5vdGhlci10b2tlbg== + state[]=helper:bazquux + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + id=1 creds=Multistage YS1naXQtdG9rZW4= + id=2 creds=Multistage YW5vdGhlci10b2tlbg== + EOF + + CHALLENGE="$HTTPD_ROOT_PATH/custom-auth.challenge" && + + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + id=1 status=401 response=WWW-Authenticate: Multistage challenge="456" + id=1 status=401 response=WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 + id=2 status=200 + id=default response=WWW-Authenticate: Multistage challenge="123" + id=default response=WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 + EOF + + test_config_global credential.helper test-helper && + git ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + expect_credential_query get <<-EOF && + capability[]=authtype + capability[]=state + protocol=http + host=$HTTPD_DEST + wwwauth[]=Multistage challenge="123" + wwwauth[]=Bearer authorize_uri="id.example.com" p=1 q=0 + EOF + + expect_credential_query get foobar <<-EOF && + capability[]=authtype + capability[]=state + authtype=Multistage + protocol=http + host=$HTTPD_DEST + wwwauth[]=Multistage challenge="456" + wwwauth[]=Bearer authorize_uri="id.example.com" p=1 q=0 + state[]=helper:foobar + EOF + + expect_credential_query store bazquux <<-EOF + capability[]=authtype + capability[]=state + authtype=Multistage + credential=YW5vdGhlci10b2tlbg== + protocol=http + host=$HTTPD_DEST + state[]=helper:bazquux + EOF +' + test_done
Over HTTP, NTLM and Kerberos require two rounds of authentication on the client side. It's possible that there are custom authentication schemes that also implement this same approach. Since these are tricky schemes to implement and the HTTP library in use may not always handle them gracefully on all systems, it would be helpful to allow the credential helper to implement them instead for increased portability and robustness. To allow this to happen, add a boolean flag, continue, that indicates that instead of failing when we get a 401, we should retry another round of authentication. However, this necessitates some changes in our current credential code so that we can make this work. Keep the state[] headers between iterations, but only use them to send to the helper and only consider the new ones we read from the credential helper to be valid on subsequent iterations. That avoids us passing stale data when we finally approve or reject the credential. Similarly, clear the multistage and wwwauth[] values appropriately so that we don't pass stale data or think we're trying a multiround response when we're not. Remove the credential values so that we can actually fill a second time with new responses. Limit the number of iterations of reauthentication we do to 3. This means that if there's a problem, we'll terminate with an error message instead of retrying indefinitely and not informing the user (and possibly conducting a DoS on the server). In our tests, handle creating multiple response output files from our helper so we can verify that each of the messages sent is correct. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Documentation/git-credential.txt | 16 +++++- builtin/credential.c | 1 + credential.c | 32 ++++++++++-- credential.h | 27 +++++++++- http.c | 59 +++++++++++---------- t/t5563-simple-http-auth.sh | 89 ++++++++++++++++++++++++++++++-- 6 files changed, 187 insertions(+), 37 deletions(-)