On 2022-11-09 15:40, Glen Choo wrote:
> "Matthew John Cheetham via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
>> From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>> 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
