On Sun, 24 Mar 2024 at 01:13, brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx/ 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@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> > --- > 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
