Re: [PATCH v3 05/11] http: set specific auth scheme depending on credential

[Matthew John Cheetham <mjcheetham@outlook.com>]

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