| Message ID | 2f38427aa8db188060d153d8ece9503e1b604e91.1667426970.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. > > Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> > --- > Documentation/git-credential.txt | 12 ++++++++++++ > credential.c | 5 +++++ > credential.h | 1 + > git-curl-compat.h | 10 ++++++++++ > http.c | 24 +++++++++++++++++++++--- > 5 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > index 791a57dddfb..9069bfb2d50 100644 > --- a/Documentation/git-credential.txt > +++ b/Documentation/git-credential.txt > @@ -175,6 +175,18 @@ username in the example above) will be left unset. > attribute 'wwwauth[]', where the order of the attributes is the same as > they appear in the HTTP response. > > +`authtype`:: > + > + Indicates the type of authentication scheme that should be used by Git. > + Credential helpers may reply to a request from Git with this attribute, > + such that subsequent authenticated requests include the correct > + `Authorization` header. > + If this attribute is not present, the default value is "Basic". > + Known values include "Basic", "Digest", and "Bearer". > + If an unknown value is provided, this is taken as the authentication > + scheme for the `Authorization` header, and the `password` field is > + used as the raw unencoded authorization parameters of the same header. > + [...] > @@ -525,8 +526,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); > + } As expected, a "Bearer" authtype doesn't require passing a username to curl, but as you noted in the cover letter, credential helpers were designed with username-password authentication in mind, which raises the question of what a credential helper should do with "Bearer" credentials. e.g. it is not clear to me where the "username" comes from in the tests, e.g. +test_expect_success 'http auth www-auth headers to credential helper basic valid' ' + test_when_finished "per_test_cleanup" && + # base64("alice:secret-passwd") + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && + export USERPASS64 && + + start_http_server \ + --auth=bearer:authority=\"id.example.com\"\ q=1\ p=0 \ + --auth=basic:realm=\"example.com\" \ + --auth-token=basic:$USERPASS64 && + + cat >get-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + wwwauth[]=bearer authority="id.example.com" q=1 p=0 + wwwauth[]=basic realm="example.com" + EOF + + cat >store-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + authtype=basic + EOF + + cat >get-response.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + authtype=basic + EOF + + git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && + + test_cmp get-expected.cred get-actual.cred && + test_cmp store-expected.cred store-actual.cred +' I'm not sure how we plan to handle this. Some approaches I can see are: - We require that credential helpers set a reasonable value for "username". Presumably most credential helpers generating bearer tokens have some idea of user identity, so this might be reasonable, though it is wasteful, since we never use it in a meaningul way, e.g. I don't think Git asks the credential helper for "username=alice" and the credential helper decides to return the 'alice' credential instead of the 'bob' credential (but I could be mistaken). - We require that credential helpers set _some_ value for "username", even if it is bogus. If so, we should communicate this explicitly. - It is okay for "username" to be missing. This seems like the most elegant approach for credential helpers. I'm not sure if we're there yet with this series, e.g. http.c::handle_curl_result() reads: else if (results->http_code == 401) { if (http_auth.username && http_auth.password) { credential_reject(&http_auth); return HTTP_NOAUTH; which seems to assume both a username _and_ password. If the username is missing, we presumably don't send "erase", which might be a problem for revoked access tokens (though presumably not an issue for OIDC id tokens).
On 2022-11-09 15:40, Glen Choo wrote: > "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> 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. >> >> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> >> --- >> Documentation/git-credential.txt | 12 ++++++++++++ >> credential.c | 5 +++++ >> credential.h | 1 + >> git-curl-compat.h | 10 ++++++++++ >> http.c | 24 +++++++++++++++++++++--- >> 5 files changed, 49 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt >> index 791a57dddfb..9069bfb2d50 100644 >> --- a/Documentation/git-credential.txt >> +++ b/Documentation/git-credential.txt >> @@ -175,6 +175,18 @@ username in the example above) will be left unset. >> attribute 'wwwauth[]', where the order of the attributes is the same as >> they appear in the HTTP response. >> >> +`authtype`:: >> + >> + Indicates the type of authentication scheme that should be used by Git. >> + Credential helpers may reply to a request from Git with this attribute, >> + such that subsequent authenticated requests include the correct >> + `Authorization` header. >> + If this attribute is not present, the default value is "Basic". >> + Known values include "Basic", "Digest", and "Bearer". >> + If an unknown value is provided, this is taken as the authentication >> + scheme for the `Authorization` header, and the `password` field is >> + used as the raw unencoded authorization parameters of the same header. >> + > > [...] > >> @@ -525,8 +526,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); >> + } > > As expected, a "Bearer" authtype doesn't require passing a username to > curl, but as you noted in the cover letter, credential helpers were > designed with username-password authentication in mind, which raises the > question of what a credential helper should do with "Bearer" > credentials. > > e.g. it is not clear to me where the "username" comes from in the tests, e.g. > > +test_expect_success 'http auth www-auth headers to credential helper basic valid' ' > + test_when_finished "per_test_cleanup" && > + # base64("alice:secret-passwd") > + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && > + export USERPASS64 && > + > + start_http_server \ > + --auth=bearer:authority=\"id.example.com\"\ q=1\ p=0 \ > + --auth=basic:realm=\"example.com\" \ > + --auth-token=basic:$USERPASS64 && > + > + cat >get-expected.cred <<-EOF && > + protocol=http > + host=$HOST_PORT > + wwwauth[]=bearer authority="id.example.com" q=1 p=0 > + wwwauth[]=basic realm="example.com" > + EOF > + > + cat >store-expected.cred <<-EOF && > + protocol=http > + host=$HOST_PORT > + username=alice > + password=secret-passwd > + authtype=basic > + EOF > + > + cat >get-response.cred <<-EOF && > + protocol=http > + host=$HOST_PORT > + username=alice > + password=secret-passwd > + authtype=basic > + EOF > + > + git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && > + > + test_cmp get-expected.cred get-actual.cred && > + test_cmp store-expected.cred store-actual.cred > +' > > I'm not sure how we plan to handle this. Some approaches I can see are: > > - We require that credential helpers set a reasonable value for > "username". Presumably most credential helpers generating bearer > tokens have some idea of user identity, so this might be reasonable, > though it is wasteful, since we never use it in a meaningul way, e.g. > I don't think Git asks the credential helper for "username=alice" and > the credential helper decides to return the 'alice' credential instead > of the 'bob' credential (but I could be mistaken). > > - We require that credential helpers set _some_ value for "username", > even if it is bogus. If so, we should communicate this explicitly. > > - It is okay for "username" to be missing. This seems like the most > elegant approach for credential helpers. I'm not sure if we're there > yet with this series, e.g. http.c::handle_curl_result() reads: > > else if (results->http_code == 401) { > if (http_auth.username && http_auth.password) { > credential_reject(&http_auth); > return HTTP_NOAUTH; > > which seems to assume both a username _and_ password. If the username > is missing, we presumably don't send "erase", which might be a problem > for revoked access tokens (though presumably not an issue for OIDC id > tokens). You are correct here that a missing username here may cause some unexpected issues, and there should be more test coverage here. My recent v4 iteration has actually dropped the `authtype` patches here, and I'll pick these back up along with these concerns in a future series. Splitting this in to a future series is probably a good idea as I feel there's going to need to be several cleanup patches adjacent to the core new-feature patch, so I wouldn't want to polute this series :) Thanks! Matthew
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index 791a57dddfb..9069bfb2d50 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -175,6 +175,18 @@ username in the example above) will be left unset. attribute 'wwwauth[]', where the order of the attributes is the same as they appear in the HTTP response. +`authtype`:: + + Indicates the type of authentication scheme that should be used by Git. + Credential helpers may reply to a request from Git with this attribute, + such that subsequent authenticated requests include the correct + `Authorization` header. + If this attribute is not present, the default value is "Basic". + Known values include "Basic", "Digest", and "Bearer". + If an unknown value is provided, this is taken as the authentication + scheme for the `Authorization` header, and the `password` field is + used as the raw unencoded authorization parameters of the same header. + GIT --- Part of the linkgit:git[1] suite diff --git a/credential.c b/credential.c index 8a3ad6c0ae2..a556f9f375a 100644 --- a/credential.c +++ b/credential.c @@ -21,6 +21,7 @@ void credential_clear(struct credential *c) free(c->path); free(c->username); free(c->password); + free(c->authtype); string_list_clear(&c->helpers, 0); strvec_clear(&c->wwwauth_headers); @@ -235,6 +236,9 @@ int credential_read(struct credential *c, FILE *fp) } else if (!strcmp(key, "path")) { free(c->path); c->path = xstrdup(value); + } else if (!strcmp(key, "authtype")) { + free(c->authtype); + c->authtype = xstrdup(value); } else if (!strcmp(key, "url")) { credential_from_url(c, value); } else if (!strcmp(key, "quit")) { @@ -281,6 +285,7 @@ void credential_write(const struct credential *c, FILE *fp) credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0); + credential_write_item(fp, "authtype", c->authtype, 0); credential_write_strvec(fp, "wwwauth", &c->wwwauth_headers); } diff --git a/credential.h b/credential.h index 6f2e5bc610b..8d580b054d0 100644 --- a/credential.h +++ b/credential.h @@ -140,6 +140,7 @@ struct credential { char *protocol; char *host; char *path; + char *authtype; }; #define CREDENTIAL_INIT { \ diff --git a/git-curl-compat.h b/git-curl-compat.h index 56a83b6bbd8..839049f6dfe 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -126,4 +126,14 @@ #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS #endif +/** + * CURLAUTH_BEARER was added in 7.61.0, released in July 2018. + * However, only 7.69.0 fixes a bug where Bearer headers were not + * actually sent with reused connections on subsequent transfers + * (curl/curl@dea17b519dc1). + */ +#if LIBCURL_VERSION_NUM >= 0x074500 +#define GIT_CURL_HAVE_CURLAUTH_BEARER +#endif + #endif diff --git a/http.c b/http.c index 17b47195d22..ac620bcbf0c 100644 --- a/http.c +++ b/http.c @@ -517,7 +517,8 @@ static int curl_empty_auth_enabled(void) static void init_curl_http_auth(struct active_request_slot *slot) { - if (!http_auth.username || !*http_auth.username) { + if (!http_auth.authtype && + (!http_auth.username || !*http_auth.username)) { if (curl_empty_auth_enabled()) curl_easy_setopt(slot->curl, CURLOPT_USERPWD, ":"); return; @@ -525,8 +526,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); + } } /* *var must be free-able */