diff mbox series

[5/8] credential: add WWW-Authenticate header to cred requests

Message ID 936545004b8b46cbe24d8069cfd95ae5b5f98593.1663097156.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Enhance credential helper protocol to include auth headers | expand

Commit Message

Matthew John Cheetham Sept. 13, 2022, 7:25 p.m. UTC 
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.

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.

In this case we send multiple `wwwauth[n]` properties where `n` is a
zero-indexed number, reflecting the order the WWW-Authenticate headers
appeared in the HTTP response.

[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 Documentation/git-credential.txt |  9 +++++++
 credential.c                     | 12 +++++++++
 t/lib-httpd/apache.conf          | 13 +++++++++
 t/t5551-http-fetch-smart.sh      | 46 ++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+)

Comments

Derrick Stolee Sept. 19, 2022, 4:33 p.m. UTC | #1 
On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>

> In this case we send multiple `wwwauth[n]` properties where `n` is a
> zero-indexed number, reflecting the order the WWW-Authenticate headers
> appeared in the HTTP response.
> @@ -151,6 +151,15 @@ Git understands the following attributes:
>  	were read (e.g., `url=https://example.com` would behave as if
>  	`protocol=https` and `host=example.com` had been provided). This
>  	can help callers avoid parsing URLs themselves.
> +
> +`wwwauth[n]`::
> +
> +	When an HTTP response is received that includes one or more
> +	'WWW-Authenticate' authentication headers, these can be passed to Git
> +	(and subsequent credential helpers) with these attributes.
> +	Each 'WWW-Authenticate' header value should be passed as a separate
> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> +	appear in the HTTP response.
>  +
>  Note that specifying a protocol is mandatory and if the URL
>  doesn't specify a hostname (e.g., "cert:///path/to/file") the

This "+" means that this paragraph should be connected to the previous
one, so it seems that you've inserted your new value in the middle of
the `url` key. You'll want to move yours to be after those two connected
paragraphs. Your diff hunk should look like this:

--- >8 ---

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f..127ae29be3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -160,6 +160,15 @@ empty string.
 Components which are missing from the URL (e.g., there is no
 username in the example above) will be left unset.
 
+`wwwauth[n]`::
+
+	When an HTTP response is received that includes one or more
+	'WWW-Authenticate' authentication headers, these can be passed to Git
+	(and subsequent credential helpers) with these attributes.
+	Each 'WWW-Authenticate' header value should be passed as a separate
+	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+	appear in the HTTP response.
+
 GIT
 ---
 Part of the linkgit:git[1] suite


--- >8 ---

> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 497b9b9d927..fe118d76f98 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -235,6 +235,19 @@ SSLEngine On
>  	Require valid-user
>  </LocationMatch>
>  
> +# Advertise two additional auth methods above "Basic".
> +# Neither of them actually work but serve test cases showing these
> +# additional auth headers are consumed correctly.
> +<Location /auth-wwwauth/>
> +	AuthType Basic
> +	AuthName "git-auth"
> +	AuthUserFile passwd
> +	Require valid-user
> +	SetEnvIf Authorization "^\S+" authz
> +	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
> +	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
> +</Location>
> +

This is cool that you've figured out how to make our Apache tests
add these headers! Maybe we won't need that extra test helper like
I thought (unless we want to confirm the second request sends the
right information).

> +test_expect_success 'http auth sends www-auth headers to credential helper' '
> +	write_script git-credential-tee <<-\EOF &&
> +		cmd=$1
> +		teefile=credential-$cmd
> +		if [ -f "$teefile" ]; then

I think we prefer using "test" over the braces (and linebreak
before then) like this:

		if test -n "$teefile"
		then

> +			rm $teefile
> +		fi

Alternatively, you could always run "rm -f $teefile" for
simplicity.

> +		(
> +			while read line;
> +			do
> +				if [ -z "$line" ]; then
> +					exit 0
> +				fi
> +				echo "$line" >> $teefile
> +				echo $line
> +			done
> +		) | git credential-store $cmd

Since I'm not sure, I'll ask the question: do we need the sub-shell
here, or could we pipe directly off of the "done"? Like this:

		while read line;
		do
			if [ -z "$line" ]; then
				exit 0
			fi
			echo "$line" >> $teefile
			echo $line
		done | git credential-store $cmd

> +	EOF


> +	cat >expected-get <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	wwwauth[0]=Bearer authority=https://login.example.com
> +	wwwauth[1]=FooAuth foo=bar baz=1
> +	wwwauth[2]=Basic realm="git-auth"
> +	EOF
> +
> +	cat >expected-store <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	username=user@host
> +	password=pass@host
> +	EOF
> +
> +	rm -f .git-credentials &&
> +	test_config credential.helper tee &&
> +	set_askpass user@host pass@host &&
> +	(
> +		PATH="$PWD:$PATH" &&
> +		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
> +	) &&
> +	expect_askpass both user@host &&
> +	test_cmp expected-get credential-get &&
> +	test_cmp expected-store credential-store

Elegant check for both calls.

Thanks,
-Stolee
Matthew John Cheetham Sept. 21, 2022, 10:20 p.m. UTC | #2 
On 2022-09-19 09:33, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
>> In this case we send multiple `wwwauth[n]` properties where `n` is a
>> zero-indexed number, reflecting the order the WWW-Authenticate headers
>> appeared in the HTTP response.
>> @@ -151,6 +151,15 @@ Git understands the following attributes:
>>  	were read (e.g., `url=https://example.com` would behave as if
>>  	`protocol=https` and `host=example.com` had been provided). This
>>  	can help callers avoid parsing URLs themselves.
>> +
>> +`wwwauth[n]`::
>> +
>> +	When an HTTP response is received that includes one or more
>> +	'WWW-Authenticate' authentication headers, these can be passed to Git
>> +	(and subsequent credential helpers) with these attributes.
>> +	Each 'WWW-Authenticate' header value should be passed as a separate
>> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
>> +	appear in the HTTP response.
>>  +
>>  Note that specifying a protocol is mandatory and if the URL
>>  doesn't specify a hostname (e.g., "cert:///path/to/file") the
> 
> This "+" means that this paragraph should be connected to the previous
> one, so it seems that you've inserted your new value in the middle of
> the `url` key. You'll want to move yours to be after those two connected
> paragraphs. Your diff hunk should look like this:
> 
> --- >8 ---
> 
> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
> index f18673017f..127ae29be3 100644
> --- a/Documentation/git-credential.txt
> +++ b/Documentation/git-credential.txt
> @@ -160,6 +160,15 @@ empty string.
>  Components which are missing from the URL (e.g., there is no
>  username in the example above) will be left unset.
>  
> +`wwwauth[n]`::
> +
> +	When an HTTP response is received that includes one or more
> +	'WWW-Authenticate' authentication headers, these can be passed to Git
> +	(and subsequent credential helpers) with these attributes.
> +	Each 'WWW-Authenticate' header value should be passed as a separate
> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> +	appear in the HTTP response.
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> 
> 
> --- >8 ---

Thanks for catching!

>> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
>> index 497b9b9d927..fe118d76f98 100644
>> --- a/t/lib-httpd/apache.conf
>> +++ b/t/lib-httpd/apache.conf
>> @@ -235,6 +235,19 @@ SSLEngine On
>>  	Require valid-user
>>  </LocationMatch>
>>  
>> +# Advertise two additional auth methods above "Basic".
>> +# Neither of them actually work but serve test cases showing these
>> +# additional auth headers are consumed correctly.
>> +<Location /auth-wwwauth/>
>> +	AuthType Basic
>> +	AuthName "git-auth"
>> +	AuthUserFile passwd
>> +	Require valid-user
>> +	SetEnvIf Authorization "^\S+" authz
>> +	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
>> +	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
>> +</Location>
>> +
> 
> This is cool that you've figured out how to make our Apache tests
> add these headers! Maybe we won't need that extra test helper like
> I thought (unless we want to confirm the second request sends the
> right information).

This will exercise the new header parsing and passing the info to the helper
but will indeed not test the response. I feel like a test helper would be
beneficial still.. what I've done here doesn't feel 100% clean or complete.

>> +test_expect_success 'http auth sends www-auth headers to credential helper' '
>> +	write_script git-credential-tee <<-\EOF &&
>> +		cmd=$1
>> +		teefile=credential-$cmd
>> +		if [ -f "$teefile" ]; then
> 
> I think we prefer using "test" over the braces (and linebreak
> before then) like this:
> 
> 		if test -n "$teefile"
> 		then
> 
>> +			rm $teefile
>> +		fi
> 
> Alternatively, you could always run "rm -f $teefile" for
> simplicity.
I like simple :-)

>> +		(
>> +			while read line;
>> +			do
>> +				if [ -z "$line" ]; then
>> +					exit 0
>> +				fi
>> +				echo "$line" >> $teefile
>> +				echo $line
>> +			done
>> +		) | git credential-store $cmd
> 
> Since I'm not sure, I'll ask the question: do we need the sub-shell
> here, or could we pipe directly off of the "done"? Like this:
> 
> 		while read line;
> 		do
> 			if [ -z "$line" ]; then
> 				exit 0
> 			fi
> 			echo "$line" >> $teefile
> 			echo $line
> 		done | git credential-store $cmd

That we can.. I will update in next iteration.

>> +	EOF
> 
> 
>> +	cat >expected-get <<-EOF &&
>> +	protocol=http
>> +	host=127.0.0.1:5551
>> +	wwwauth[0]=Bearer authority=https://login.example.com
>> +	wwwauth[1]=FooAuth foo=bar baz=1
>> +	wwwauth[2]=Basic realm="git-auth"
>> +	EOF
>> +
>> +	cat >expected-store <<-EOF &&
>> +	protocol=http
>> +	host=127.0.0.1:5551
>> +	username=user@host
>> +	password=pass@host
>> +	EOF
>> +
>> +	rm -f .git-credentials &&
>> +	test_config credential.helper tee &&
>> +	set_askpass user@host pass@host &&
>> +	(
>> +		PATH="$PWD:$PATH" &&
>> +		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
>> +	) &&
>> +	expect_askpass both user@host &&
>> +	test_cmp expected-get credential-get &&
>> +	test_cmp expected-store credential-store
> 
> Elegant check for both calls.
> 
> Thanks,
> -Stolee

Thanks,
Matthew
diff mbox series

Patch

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f5..7d4a788c63d 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -151,6 +151,15 @@  Git understands the following attributes:
 	were read (e.g., `url=https://example.com` would behave as if
 	`protocol=https` and `host=example.com` had been provided). This
 	can help callers avoid parsing URLs themselves.
+
+`wwwauth[n]`::
+
+	When an HTTP response is received that includes one or more
+	'WWW-Authenticate' authentication headers, these can be passed to Git
+	(and subsequent credential helpers) with these attributes.
+	Each 'WWW-Authenticate' header value should be passed as a separate
+	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+	appear in the HTTP response.
 +
 Note that specifying a protocol is mandatory and if the URL
 doesn't specify a hostname (e.g., "cert:///path/to/file") the
diff --git a/credential.c b/credential.c
index 897b4679333..4ad40323fc7 100644
--- a/credential.c
+++ b/credential.c
@@ -263,6 +263,17 @@  static void credential_write_item(FILE *fp, const char *key, const char *value,
 	fprintf(fp, "%s=%s\n", key, value);
 }
 
+static void credential_write_strvec(FILE *fp, const char *key,
+				    const struct strvec *vec)
+{
+	int i = 0;
+	for (; i < vec->nr; i++) {
+		const char *full_key = xstrfmt("%s[%d]", key, i);
+		credential_write_item(fp, full_key, vec->v[i], 0);
+		free((void*)full_key);
+	}
+}
+
 void credential_write(const struct credential *c, FILE *fp)
 {
 	credential_write_item(fp, "protocol", c->protocol, 1);
@@ -270,6 +281,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_strvec(fp, "wwwauth", &c->wwwauth_headers);
 }
 
 static int run_credential_helper(struct credential *c,
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 497b9b9d927..fe118d76f98 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -235,6 +235,19 @@  SSLEngine On
 	Require valid-user
 </LocationMatch>
 
+# Advertise two additional auth methods above "Basic".
+# Neither of them actually work but serve test cases showing these
+# additional auth headers are consumed correctly.
+<Location /auth-wwwauth/>
+	AuthType Basic
+	AuthName "git-auth"
+	AuthUserFile passwd
+	Require valid-user
+	SetEnvIf Authorization "^\S+" authz
+	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
+	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
+</Location>
+
 RewriteCond %{QUERY_STRING} service=git-receive-pack [OR]
 RewriteCond %{REQUEST_URI} /git-receive-pack$
 RewriteRule ^/half-auth-complete/ - [E=AUTHREQUIRED:yes]
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6a38294a476..c99d8e253df 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -564,6 +564,52 @@  test_expect_success 'http auth forgets bogus credentials' '
 	expect_askpass both user@host
 '
 
+test_expect_success 'http auth sends www-auth headers to credential helper' '
+	write_script git-credential-tee <<-\EOF &&
+		cmd=$1
+		teefile=credential-$cmd
+		if [ -f "$teefile" ]; then
+			rm $teefile
+		fi
+		(
+			while read line;
+			do
+				if [ -z "$line" ]; then
+					exit 0
+				fi
+				echo "$line" >> $teefile
+				echo $line
+			done
+		) | git credential-store $cmd
+	EOF
+
+	cat >expected-get <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	wwwauth[0]=Bearer authority=https://login.example.com
+	wwwauth[1]=FooAuth foo=bar baz=1
+	wwwauth[2]=Basic realm="git-auth"
+	EOF
+
+	cat >expected-store <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	username=user@host
+	password=pass@host
+	EOF
+
+	rm -f .git-credentials &&
+	test_config credential.helper tee &&
+	set_askpass user@host pass@host &&
+	(
+		PATH="$PWD:$PATH" &&
+		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
+	) &&
+	expect_askpass both user@host &&
+	test_cmp expected-get credential-get &&
+	test_cmp expected-store credential-store
+'
+
 test_expect_success 'client falls back from v2 to v0 to match server' '
 	GIT_TRACE_PACKET=$PWD/trace \
 	GIT_TEST_PROTOCOL_VERSION=2 \