Re: [PATCH v7 12/12] credential: add WWW-Authenticate header to cred requests

From: Matthew John Cheetham <mjcheetham@outlook.com>
To: Jeff King <peff@peff.net>,
	Matthew John Cheetham via GitGitGadget  <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Derrick Stolee" <derrickstolee@github.com>,
	"Lessley Dennington" <lessleydennington@gmail.com>,
	"M Hickford" <mirth.hickford@gmail.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Glen Choo" <chooglen@google.com>,
	"Victoria Dye" <vdye@github.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v7 12/12] credential: add WWW-Authenticate header to cred requests
Date: Mon, 6 Feb 2023 11:18:03 -0800	[thread overview]
Message-ID: <DB9PR03MB9831A708EA98E198591F6632C0DA9@DB9PR03MB9831.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <Y9JjRfhl1H4Julv3@coredump.intra.peff.net>

On 2023-01-26 03:25, Jeff King wrote:

> On Fri, Jan 20, 2023 at 10:08:50PM +0000, Matthew John Cheetham via GitGitGadget wrote:
> 
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Add the value of the WWW-Authenticate response header to credential
>> requests. Credential helpers that understand and support HTTP
>> authentication and authorization can use this standard header (RFC 2616
>> Section 14.47 [1]) to generate valid credentials.
>>
>> WWW-Authenticate headers can contain information pertaining to the
>> authority, authentication mechanism, or extra parameters/scopes that are
>> required.
> 
> I'm definitely on board with sending these to the helpers. It does feel
> a bit weird that we don't parse them at all, and just foist that on the
> helpers.
> 
> If I understand the RFC correctly, you can have multiple challenges per
> header, but also multiple headers. So:
> 
>   WWW-Authenticate: Basic realm="foo", OtherAuth realm="bar"
>   WWW-Authenticate: YetAnotherScheme some-token

That is correct. It would be strange that server would respond with a mix
of styles, but I guess it's not forbidden.

> could be normalized as:
> 
>   www-auth-challenge=Basic realm="foo"
>   www-auth-challenge=OtherAuth realm="bar"
>   www-auth-challenge=YetAnotherScheme some-token
> 
> which saves each helper from having to do the same work. Likewise, we
> can do a _little_ more parsing to get:
> 
>   www-auth-basic=realm="foo"
>   www-auth-otherauth=realm="bar"
>   www-auth-yetanotherscheme=some-token
> 
> I don't think we can go beyond there, though, without understanding the
> syntax of individual schemes. Which is a shame, as one of the goals of
> the credential format was to let the helpers do as little as possible
> (so they can't get it wrong!). But helpers are stuck doing things like
> handling backslashed double-quotes, soaking up extra whitespace, etc.

This key format wouldn't make it obviously easier for simple helpers to
understand. Now they no longer have well-known keys but a key prefix.

My overall goal here is to have Git know less about auth, so it treats
all values as totally opaque. The only logic added is around reconstructing
folded headers, which is just HTTP and not auth specific.

> I'm not really sure what we expect to see in the real world. I guess for
> your purposes, you are working on an already-big helper that is happy to
> just get the raw values and process them according to the rfc. I'm just
> wondering if there are use cases where somebody might want to do
> something with this header, but in a quick shell script kind of way. For
> example, my credential config is still:
> 
>   [credential "https://github.com"]
>   username = peff
>   helper = "!f() { test $1 = get && echo password=$(pass ...); }; f"
> 
> That's an extreme example, but I'm wondering if there's _anything_
> useful somebody would want to do in a similar quick-and-dirty kind of
> way. For example, deciding which cred to use based on basic realm, like:
> 
>   realm=foo
>   while read line; do
>     case "$line" in
>     www-auth-basic=)
>         value=${line#*=}
> 	# oops, we're just assuming it's realm= here, and we're
> 	# not handling quotes at all. I think it could technically be
> 	# realm=foo or realm="foo"
> 	realm=${value#realm=}
> 	;;
>     esac
>   done
>   echo password=$(pass "pats-by-realm/$realm")
> 
> which could be made a lot easier if we did more parsing (e.g.,
> www-auth-basic-realm or something). I dunno. Maybe that is just opening
> up a can of worms, as we're stuffing structured data into a linearized
> key-value list. The nice thing about your proposal is that Git does not
> even have to know anything about these schemes; it's all the problem of
> the helper. My biggest fear is just that we'll want to shift that later,
> and we'll be stuck with this microformat forever.

I'm not sure there's such a continuous scale between simple and 'complex'
helpers that would mean there'd be a simple shell script generating
OAuth or DPoP credentials instead of a helper written in a higher-level
language where parsing the headers is one of the simpler challenges faced.

>> The current I/O format for credential helpers only allows for unique
>> names for properties/attributes, so in order to transmit multiple header
>> values (with a specific order) we introduce a new convention whereby a
>> C-style array syntax is used in the property name to denote multiple
>> ordered values for the same property.
> 
> I don't know if this is strictly necessary. The semantics of duplicate
> keys are not really defined anywhere, and just because the
> implementations of current readers happen to replace duplicates for the
> current set of keys doesn't mean everything has to. So you could just
> define "wwwauth" to behave differently. But I don't mind having a
> syntactic marker to indicate this new type.

I had considered another model whereby we forgo the key=value line model,
and hide another format behind the 'final' terminating new-line. However
I thought this would be even more distuptive.

> If you're at all convinced by what I said above, then we also might be
> able to get away with having unique keys anyway.
> 
>>  Documentation/git-credential.txt |  19 ++-
>>  credential.c                     |  11 ++
>>  t/lib-credential-helper.sh       |  27 ++++
>>  t/t5556-http-auth.sh             | 242 +++++++++++++++++++++++++++++++
>>  4 files changed, 298 insertions(+), 1 deletion(-)
>>  create mode 100644 t/lib-credential-helper.sh
> 
> The patch itself looks pretty reasonable to me.
> 
> One small thing I noticed:
> 
>> +	git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
> 
> As you undoubtedly figured out, the helper path is fed to the shell, so
> spaces in the trash directory are a problem. You've solved it here by
> adding a layer of double quotes, which handles spaces. But you'd run
> into problems if the absolute path that somebody is using for the test
> suite has a backslash or a double quote in it.
> 
> I don't know how careful we want to be here (or how careful we already
> are[1]), but one simple-ish solution is:
> 
>   export CREDENTIAL_HELPER
>   git -c "credential.helper=!\"\$CREDENTIAL_HELPER\"" ...
> 
> I.e., letting the inner shell expand the variable itself. Another option
> is to put the helper into $TRASH_DIRECTORY/bin and add that to the
> $PATH.
> 
> I also wondered if it was worth having setup_credential_helper() just
> stick it in $TRASH_DIRECTORY/.gitconfig so that individual tests don't
> have to keep doing that ugly "-c" invocation. Or if you really want to
> have each test enable it, perhaps have set_credential_reply() turn it on
> via test_config (which will auto-remove it at the end of the test).

Good ideas! I shall try those.

> -Peff
> 
> [1] Curious, I tried cloning git into this directory:
> 
>       mkdir '/tmp/foo/"horrible \"path\"'
> 
>     and we do indeed already fail. The first breakage I saw was recent,
>     but going further back, it looks like bin-wrappers don't correctly
>     handle this case anyway. So maybe that's evidence that nobody would
>     do something so ridiculous in practice.