On 2022-11-09 15:40, Glen Choo wrote:

"Matthew John Cheetham via GitGitGadget" [off-list ref]
writes:

quoted

From: Matthew John Cheetham <redacted>

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 <redacted>
---
 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.
+

[...]

quoted

@@ -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