Enhance credential helper protocol to include auth headers
Subsystems (2)
PATCH v8 1/3 t5563: add tests for basic and anoymous HTTP access
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
Add a test showing simple anoymous HTTP access to an unprotected repository, that results in no credential helper invocations. Also add a test demonstrating simple basic authentication with simple credential helper support.
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 42 +++++++++++++++++ t/t5563-simple-http-auth.sh | 86 ++++++++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
t/lib-httpd.sh +1 -0
@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +6 -0
@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
t/lib-httpd/nph-custom-auth.sh new file +42 -0
42 lines not on this page · raw message
t/t5563-simple-http-auth.sh new file +86 -0
86 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 78 lines · Matthew John Cheetham via GitGitGadget
From: Matthew John Cheetham <[email protected]>
Add a test showing simple anoymous HTTP access to an unprotected repository, that results in no credential helper invocations. Also add a test demonstrating simple basic authentication with simple credential helper support.
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
Signed-off-by Matthew John Cheetham <[email protected]>--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 42 +++++++++++++++++ t/t5563-simple-http-auth.sh | 86 ++++++++++++++++++++++++++++++++++ 4 files changed, 135 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sht/lib-httpd.sh +1
-0@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"t/lib-httpd/apache.conf +6
-0@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>t/lib-httpd/nph-custom-auth.sh new file +15
-0@@ -0,0 +1,42 @@ +#!/bin/sh + +VALID_CREDS_FILE=custom-auth.valid +CHALLENGE_FILE=custom-auth.challenge +ANONYMOUS_FILE=custom-auth.anonymous + +# +# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access. +# +# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid +# credential for the current request. Each line in the file is considered a +# valid HTTP Authorization header value. For example: +# +# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== +#
t/lib-httpd/nph-custom-auth.sh line 25
Quoted 10 lines
t/lib-httpd/nph-custom-auth.sh new file +10
-0+# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" +# + +if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \ + grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
Rather than "test -f "$f" & grep ... "$f" I think you can just use only "grep", if the file doesn't exist it'll give you an error.
If you don't want to see that error just pipe it to /dev/null, in case that's what you were trying to avoid with the "check if it exists first".
t/lib-httpd/nph-custom-auth.sh line 40
Quoted 4 lines
t/lib-httpd/nph-custom-auth.sh new file +4
-0+echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE"
Maybe the same here, i.e. just:
cat "$f" 2>/dev/null
t/lib-httpd/nph-custom-auth.sh line 31
Quoted 2 lines
t/lib-httpd/nph-custom-auth.sh new file +2
-0+test_expect_success 'setup_credential_helper' ' + mkdir -p "$TRASH_DIRECTORY/bin" &&
The "$TRASH_DIRECTORY" is already created for you, so don't use "-p", unless something went wrong here..
t/lib-httpd/nph-custom-auth.sh line 40
Quoted 9 lines
t/lib-httpd/nph-custom-auth.sh new file +9
-0+ PATH=$PATH:"$TRASH_DIRECTORY/bin" && + export PATH && + + CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" && + write_script "$CREDENTIAL_HELPER" <<-\EOF + cmd=$1 + teefile=$cmd-query.cred + catfile=$cmd-reply.cred + sed -n -e "/^$/q" -e "p" >> $teefile
Style: ">>$f", not ">> $f"
t/lib-httpd/nph-custom-auth.sh line 41
Quoted 1 line
t/lib-httpd/nph-custom-auth.sh new file +1
-0+ if test "$cmd" = "get"; then
Style: We usually use "\nthen", not "; then".
Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access
In reply to Matthew John Cheetham via GitGitGadgetQuoted 29 lines · Matthew John Cheetham via GitGitGadget
t/lib-httpd.sh +1
-0@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"t/lib-httpd/apache.conf +5
-0@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
t/lib-httpd/apache.conf ScriptAlias /broken_smart/ broken-smart-http.sh/ · line 152
Quoted 4 lines
t/lib-httpd/apache.conf +1
-0@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1
This setup (redirecting '/custom_auth/' routes to the 'nph-custom-auth.sh' script) is nice and straightforward.
Quoted 24 lines
t/lib-httpd/apache.conf +0
-0<Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>t/lib-httpd/nph-custom-auth.sh new file +15
-0@@ -0,0 +1,42 @@ +#!/bin/sh + +VALID_CREDS_FILE=custom-auth.valid +CHALLENGE_FILE=custom-auth.challenge +ANONYMOUS_FILE=custom-auth.anonymous + +# +# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access. +# +# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid +# credential for the current request. Each line in the file is considered a +# valid HTTP Authorization header value. For example: +# +# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== +#
t/lib-httpd/nph-custom-auth.sh line 25
Quoted 10 lines
t/lib-httpd/nph-custom-auth.sh new file +10
-0+# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" +# + +if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \ + grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
So there are two cases where you want to return a '200 OK' response:
1. anonymous access is allowed (indicated by $ANONYMOUS_FILE existing)
2. anonymous access is *not* allowed, 'HTTP_AUTHORIZATION' is non-empty, and it matches at least one line in $VALID_CREDS_FILE
Does the '$' at the end of "^${HTTP_AUTHORIZATION:-nopenopnope}$" need to be escaped? I'm guessing it doesn't *need* to be based on the fact that the tests are passing, but it might be safer to escape it anyway.
I see what you're going for with the "nopenopenope" substitution, but I think you could be more explicit about requiring that 'HTTP_AUTHORIZATION' is set without the need for a special invalid value fallback:
if test -f "$ANONYMOUS_FILE" || (test -n "$HTTP_AUTHORIZATION" && \
grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE")Note the addition of '-s' to 'grep' - it seems cleaner than redirecting to '/dev/null' (as Ævar suggested [1]) while achieving the same result.
t/lib-httpd/nph-custom-auth.sh line 34
Quoted 9 lines
t/lib-httpd/nph-custom-auth.sh new file +9
-0+then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend
I'm not familiar with 'exec', but a cursory look at the documentation shows that, because this replaces the current shell, it will exit with the code from 'git-http-backend', so there's no risk of continuing on to print the '401 Authorization Required' response & challenge handling.
t/lib-httpd/nph-custom-auth.sh line 41
Quoted 7 lines
t/lib-httpd/nph-custom-auth.sh new file +7
-0+fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi
In contrast to Ævar's comments in the review linked earlier, I like having the explicit 'test -f' (to sort of "self-document" that the challenge is only issued if $CHALLENGE_FILE exists). I think you're fine keeping this as-is or changing it, depending on your preference.
Quoted 29 lines
t/lib-httpd/nph-custom-auth.sh new file +29
-0+test_expect_success 'access anonymous no challenge' ' + test_when_finished "per_test_cleanup" && + touch "$HTTPD_ROOT_PATH/custom-auth.anonymous" && + git ls-remote "$HTTPD_URL/custom_auth/repo.git" +' + +test_expect_success 'access using basic auth' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + WWW-Authenticate: Basic realm="example.com" + EOF + + test_config_global credential.helper test-helper && + git ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + expect_credential_query get <<-EOF && + protocol=http + host=$HTTPD_DEST + EOF
t/lib-httpd/nph-custom-auth.sh
Quoted 10 lines
t/lib-httpd/nph-custom-auth.sh new file +10
-0+ + expect_credential_query store <<-EOF + protocol=http + host=$HTTPD_DEST + username=alice + password=secret-passwd + EOF +' + +test_done
And these tests properly exercise the custom auth handling.
While I wasn't as opposed to the custom HTTP handler as others that have commented, I do appreciate the relative simplicity of this new Apache setup and like that it's still pretty easy to test. Nice work!
Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 6 lines · Victoria Dye
Quoted 2 lines · Matthew John Cheetham via GitGitGadget
ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
+ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1This setup (redirecting '/custom_auth/' routes to the '
nph-custom-auth.sh' script) is nice and straightforward.
*nod*
Quoted 11 lines
[...]
Quoted 2 lines
t/lib-httpd/nph-custom-auth.sh new file +2
-0+if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \ + grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")[...]
if test -f "$ANONYMOUS_FILE" || (test -n "$HTTP_AUTHORIZATION" && \ grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE")Note the addition of '-s' to 'grep' - it seems cleaner than redirecting to '
/dev/null' (as Ævar suggested [1]) while achieving the same result.
I wondered if it's in POSIX, turns out it is!:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
But we don't have any existing use of it, even for things in POSIX it's often a gamble what the exact semantics are on our long tail of *nix, e.g. old AIX.
In general I'd think we could just avoid "-s" or piping to "/dev/null" here, i.e. under "-x" or whatever it's informative to know it doesn't exist from the stderr, but on second look I think both of us long track of a larger issue here...
Quoted 13 lines
[...]
t/lib-httpd/nph-custom-auth.sh line 41
Quoted 7 lines
t/lib-httpd/nph-custom-auth.sh new file +7
-0+fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fiIn contrast to Ævar's comments in the review linked earlier, I like having the explicit 'test -f' (to sort of "self-document" that the challenge is only issued if $CHALLENGE_FILE exists). I think you're fine keeping this as-is or changing it, depending on your preference.
Looking at this again I think we should just have it be unconditional here. I.e. it looks like we both assumed that this needs to be a conditional, but actually every /custom_auth/ test also sets up this "$CHALLENGE_FILE".
So this "test -f" seems to only serve the purpose of burying an error under the rug if things have already gone wrong.
But if we're making these requests why are we writing a script that handles the combination of 3 parameters, and needs to second guess things? We can just create N urls and N scripts instead. So I tried this fix-up instead:
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 7979605d344..c25e3000db0 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -141,6 +141,7 @@ prepare_httpd() {
install_script error.sh
install_script apply-one-time-perl.sh
install_script nph-custom-auth.sh
+ install_script nph-custom-auth-anon.sh
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 2aac922376c..7a63a9169c3 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -140,6 +140,7 @@ ScriptAlias /error_smart/ error-smart-http.sh/
ScriptAlias /error/ error.sh/
ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1
+ScriptAliasMatch /custom_auth_anon/(.*) nph-custom-auth-anon.sh/$1
<Directory ${GIT_EXEC_PATH}>
Options FollowSymlinks
</Directory>
diff --git a/t/lib-httpd/nph-custom-auth-anon.sh b/t/lib-httpd/nph-custom-auth-anon.sh
new file mode 100755
index 00000000000..3c7a24fed6b
--- /dev/null
+++ b/t/lib-httpd/nph-custom-auth-anon.sh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+echo 'HTTP/1.1 200 OK'
+exec "$GIT_EXEC_PATH"/git-http-backend
diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh
index 8f851aebac4..e3ee61c8c9e 100755
--- a/t/lib-httpd/nph-custom-auth.sh
+++ b/t/lib-httpd/nph-custom-auth.sh
@@ -1,28 +1,15 @@
#!/bin/sh
+set -e
+
VALID_CREDS_FILE=custom-auth.valid
-CHALLENGE_FILE=custom-auth.challenge
-ANONYMOUS_FILE=custom-auth.anonymous
-#
-# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access.
-#
# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid
# credential for the current request. Each line in the file is considered a
# valid HTTP Authorization header value. For example:
#
# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
-#
-# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers
-# in a 401 response if no valid authentication credentials were included in the
-# request. For example:
-#
-# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0
-# WWW-Authenticate: Basic realm="example.com"
-#
-
-if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \
- grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
+if grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE"
then
# Note that although git-http-backend returns a status line, it
# does so using a CGI 'Status' header. Because this script is an
@@ -31,12 +18,15 @@ then
# This is only a test script, so we don't bother to check for
# the actual status from git-http-backend and always return 200.
echo 'HTTP/1.1 200 OK'
- exec "$GIT_EXEC_PATH"/git-http-backend
+ exit 1
fi
+# Output of our challenge file as headers
+# in a 401 response if no valid authentication credentials were included in the
+# request. For example:
+#
+# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0
+# WWW-Authenticate: Basic realm="example.com"
echo 'HTTP/1.1 401 Authorization Required'
-if test -f "$CHALLENGE_FILE"
-then
- cat "$CHALLENGE_FILE"
-fi
+cat custom-auth.challenge
echo
diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh
index a7b1e5bd1af..feb8149de8f 100755
--- a/t/t5563-simple-http-auth.sh
+++ b/t/t5563-simple-http-auth.sh
@@ -47,8 +47,7 @@ test_expect_success 'setup repository' '
test_expect_success 'access anonymous no challenge' '
test_when_finished "per_test_cleanup" &&
- touch "$HTTPD_ROOT_PATH/custom-auth.anonymous" &&
- git ls-remote "$HTTPD_URL/custom_auth/repo.git"
+ git ls-remote "$HTTPD_URL/custom_auth_anon/repo.git"
'
test_expect_success 'access using basic auth' 'I think that's much better, now we just have a 2-line script to handle this "anon auth" case. Instead of creating a "custom-auth.anonymous" file to communicate how the remote end should behave, let's just communicate that by requesting a different URL, one that accepts anonymous authentication.
I did insert a deliberate bug here, or:
- exec "$GIT_EXEC_PATH"/git-http-backend + exit 1
So aside from your "exec" comment it seems both of us missed that this "exec" does nothing useful, the test will fail if we emit different headers, but it doesn't matter that we execute the git-http-backend.
Or maybe it does, but the tests aren't good enough to spot the difference.
The above is a rough WIP, I'm leaving it here for Matthew to follow-up on. I think it might benefit from being further split-up, i.e. we know which URLs we expect to fail auth, so if we just had another URL for "the auth response fails here" we'd have 3x trivial scripts with no if/else; but maybe that sucks, I didn't try it.
Re: [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 168 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines · Victoria Dye
Quoted 2 lines · Matthew John Cheetham via GitGitGadget
ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1
+ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1This setup (redirecting '/custom_auth/' routes to the '
nph-custom-auth.sh' script) is nice and straightforward.*nod*
Quoted 11 lines
[...]
Quoted 2 lines
t/lib-httpd/nph-custom-auth.sh new file +2
-0+if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \ + grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")[...]
if test -f "$ANONYMOUS_FILE" || (test -n "$HTTP_AUTHORIZATION" && \ grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE")Note the addition of '-s' to 'grep' - it seems cleaner than redirecting to '
/dev/null' (as Ævar suggested [1]) while achieving the same result.I wondered if it's in POSIX, turns out it is!:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.htmlBut we don't have any existing use of it, even for things in POSIX it's often a gamble what the exact semantics are on our long tail of *nix, e.g. old AIX.
In general I'd think we could just avoid "-s" or piping to "
/dev/null" here, i.e. under "-x" or whatever it's informative to know it doesn't exist from the stderr, but on second look I think both of us long track of a larger issue here...Quoted 13 lines
[...]
t/lib-httpd/nph-custom-auth.sh line 41
Quoted 7 lines
t/lib-httpd/nph-custom-auth.sh new file +7
-0+fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fiIn contrast to Ævar's comments in the review linked earlier, I like having the explicit 'test -f' (to sort of "self-document" that the challenge is only issued if $CHALLENGE_FILE exists). I think you're fine keeping this as-is or changing it, depending on your preference.
Looking at this again I think we should just have it be unconditional here. I.e. it looks like we both assumed that this needs to be a conditional, but actually every /custom_auth/ test also sets up this "$CHALLENGE_FILE".
So this "test -f" seems to only serve the purpose of burying an error under the rug if things have already gone wrong.
But if we're making these requests why are we writing a script that handles the combination of 3 parameters, and needs to second guess things? We can just create N urls and N scripts instead. So I tried this fix-up instead:
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 7979605d344..c25e3000db0 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -141,6 +141,7 @@ prepare_httpd() { install_script error.sh install_script apply-one-time-perl.sh install_script nph-custom-auth.sh + install_script nph-custom-auth-anon.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules" diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 2aac922376c..7a63a9169c3 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -140,6 +140,7 @@ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 +ScriptAliasMatch /custom_auth_anon/(.*) nph-custom-auth-anon.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory> diff --git a/t/lib-httpd/nph-custom-auth-anon.sh b/t/lib-httpd/nph-custom-auth-anon.sh new file mode 100755 index 00000000000..3c7a24fed6b --- /dev/null +++ b/t/lib-httpd/nph-custom-auth-anon.sh @@ -0,0 +1,4 @@ +#!/bin/sh + +echo 'HTTP/1.1 200 OK' +exec "$GIT_EXEC_PATH"/git-http-backend diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh index 8f851aebac4..e3ee61c8c9e 100755 --- a/t/lib-httpd/nph-custom-auth.sh +++ b/t/lib-httpd/nph-custom-auth.sh @@ -1,28 +1,15 @@ #!/bin/sh +set -e + VALID_CREDS_FILE=custom-auth.valid -CHALLENGE_FILE=custom-auth.challenge -ANONYMOUS_FILE=custom-auth.anonymous -# -# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access. -# # If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid # credential for the current request. Each line in the file is considered a # valid HTTP Authorization header value. For example: # # Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== -# -# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers -# in a 401 response if no valid authentication credentials were included in the -# request. For example: -# -# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 -# WWW-Authenticate: Basic realm="example.com" -# - -if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \ - grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE") +if grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE" then # Note that although git-http-backend returns a status line, it # does so using a CGI 'Status' header. Because this script is an @@ -31,12 +18,15 @@ then # This is only a test script, so we don't bother to check for # the actual status from git-http-backend and always return 200. echo 'HTTP/1.1 200 OK' - exec "$GIT_EXEC_PATH"/git-http-backend + exit 1 fi +# Output of our challenge file as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" echo 'HTTP/1.1 401 Authorization Required' -if test -f "$CHALLENGE_FILE" -then - cat "$CHALLENGE_FILE" -fi +cat custom-auth.challenge echo diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh index a7b1e5bd1af..feb8149de8f 100755 --- a/t/t5563-simple-http-auth.sh +++ b/t/t5563-simple-http-auth.sh @@ -47,8 +47,7 @@ test_expect_success 'setup repository' ' test_expect_success 'access anonymous no challenge' ' test_when_finished "per_test_cleanup" && - touch "$HTTPD_ROOT_PATH/custom-auth.anonymous" && - git ls-remote "$HTTPD_URL/custom_auth/repo.git" + git ls-remote "$HTTPD_URL/custom_auth_anon/repo.git" ' test_expect_success 'access using basic auth' '
Quoted 5 lines
I think that's much better, now we just have a 2-line script to handle this "anon auth" case. Instead of creating a "custom-auth.anonymous" file to communicate how the remote end should behave, let's just communicate that by requesting a different URL, one that accepts anonymous authentication.
Actually, we don't really need to test the anonymous auth case at all because all other tests that try accessing a remote repository over HTTP are already exercising this. See t5551-http-fetch-smart for example.. here we're performing various requests without auth.
Should we be erronously issuing credential helper challenges in these scenarios then the tests would fail with an askpass prompt.
I will drop the anon auth test and script support.
Quoted 17 lines
I did insert a deliberate bug here, or:
- exec "$GIT_EXEC_PATH"/git-http-backend + exit 1So aside from your "exec" comment it seems both of us missed that this "exec" does nothing useful, the test will fail if we emit different headers, but it doesn't matter that we execute the git-http-backend.
Or maybe it does, but the tests aren't good enough to spot the difference.
The above is a rough WIP, I'm leaving it here for Matthew to follow-up on. I think it might benefit from being further split-up, i.e. we know which URLs we expect to fail auth, so if we just had another URL for "the auth response fails here" we'd have 3x trivial scripts with no if/else; but maybe that sucks, I didn't try it.
PATCH v8 2/3 http: read HTTP WWW-Authenticate response headers
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
Read and store the HTTP WWW-Authenticate response headers made for a particular request.
This will allow us to pass important authentication challenge information to credential helpers or others that would otherwise have been lost.
According to RFC2616 Section 4.2 [1], header field names are not case-sensitive meaning when collecting multiple values for the same field name, we can just use the case of the first observed instance of each field name and no normalisation is required.
libcurl only provides us with the ability to read all headers recieved for a particular request, including any intermediate redirect requests or proxies. The lines returned by libcurl include HTTP status lines delinating any intermediate requests such as "HTTP/1.1 200". We use these lines to reset the strvec of WWW-Authenticate header values as we encounter them in order to only capture the final response headers.
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us one line at a time.
In the future [2] we may be able to leverage functions to read headers from libcurl itself, but as of today we must do this ourselves.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
--- credential.c | 1 + credential.h | 15 ++++++ git-compat-util.h | 22 +++++++++ http.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+)
credential.c +1 -0
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }
credential.h +15 -0
@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,19 @@ struct credential {*/ struct string_list helpers; +/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Used to keep track of split header fields +* in order to fold multiple lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,
@@ -130,6 +144,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */
git-compat-util.h +22 -0
@@ -1266,6 +1266,28 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); +if (len < prefix_len) +return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)){ + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +} +static inline int strtoul_ui (char const *s,int base,unsigned int *result) {unsigned long ul;
http.c +120 -0
132 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v8 2/3] http: read HTTP WWW-Authenticate response headers
git-compat-util.h static inline int skip_iprefix(const char *str, const char *prefix, · line 1278
Quoted 7 lines · Matthew John Cheetham via GitGitGadget
From: Matthew John Cheetham <[email protected]>
git-compat-util.h +5
-0+static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix);
Style nit: We typically use \n\n after the end of decls, so an extra newline here before the code.
git-compat-util.h static inline int skip_iprefix(const char *str, const char *prefix, · line 1282
Quoted 4 lines
git-compat-util.h +4
-0+if (len < prefix_len) +return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)){
Style: ")) {", not ")){".
Re: [PATCH v8 2/3] http: read HTTP WWW-Authenticate response headers
In reply to Matthew John Cheetham via GitGitGadgetQuoted 143 lines · Matthew John Cheetham via GitGitGadget
git-compat-util.h +22
-0@@ -1266,6 +1266,28 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); +if (len < prefix_len) +return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)){ + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +} +static inline int strtoul_ui (char const *s,int base,unsigned int *result) {unsigned long ul;http.c +102
-0@@ -183,6 +183,124 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)return nmemb; } +/* +* A folded header continuation line starts with at least one single whitespace +* character. It is not a continuation line if the line is *just* a newline. +* The RFC for HTTP states that CRLF is the header field line ending, but some +* servers may use LF only; we accept both. +*/ +static inline int is_hdr_continuation (const char *ptr,const size_t size) +{ +/* totally empty line or normal header */ +if (!size || !isspace (*ptr)) +return 0 ; + +/* empty line with LF line ending */ +if (size ==1 && ptr[0 ] =='\n' ) +return 0 ; + +/* empty line with CRLF line ending */ +if (size ==2 && ptr[0 ] =='\r' && ptr[1 ] =='\n' ) +return 0 ; + +return 1 ; +} + +static size_t fwrite_wwwauth (char *ptr,size_t eltsize,size_t nmemb,void *p) +{ +size_t size = eltsize * nmemb; +struct strvec *values = &http_auth.wwwauth_headers; +struct strbuf buf =STRBUF_INIT ; +const char *val; +size_t val_len; + +/* +* Header lines may not come NULL-terminated from libcurl so we must +* limit all scans to the maximum length of the header line, or leverage +* strbufs for all operations. +* +* In addition, it is possible that header values can be split over +* multiple lines as per RFC 2616 (even though this has since been +* deprecated in RFC 7230). A continuation header field value is +* identified as starting with a space or horizontal tab. +* +* The formal definition of a header field as given in RFC 2616 is: +* +* message-header = field-name ":" [ field-value ] +* field-name = token +* field-value = *( field-content | LWS ) +* field-content = <the OCTETs making up the field-value +* and consisting of either *TEXT or combinations +* of token, separators, and quoted-string> +*/ + +/* Start of a new WWW-Authenticate header */ +if (skip_iprefix_mem (ptr, size,"www-authenticate:" , &val, &val_len)) { +strbuf_add (&buf, val, val_len); + +/* +* Strip the CRLF that should be present at the end of each +* field as well as any trailing or leading whitespace from the +* value. +*/ +strbuf_trim (&buf); + +strvec_push (values, buf.buf); + http_auth.header_is_last_match =1 ; + goto exit; + } + +/* +* This line could be a continuation of the previously matched header +* field. If this is the case then we should append this value to the +* end of the previously consumed value. +*/ +if (http_auth.header_is_last_match &&is_hdr_continuation (ptr, size)) { +/* +* Trim the CRLF and any leading or trailing from this line. +*/ +strbuf_add (&buf, ptr, size); +strbuf_trim (&buf); + +/* +* At this point we should always have at least one existing +* value, even if it is empty. Do not bother appending the new +* value if this continuation header is itself empty. +*/ +if (!values->nr) { +BUG ("should have at least one existing header value" ); + }else if (buf.len) { +char *prev =xstrdup (values->v[values->nr -1 ]); + +/* Join two non-empty values with a single space. */ +const char *const sp = *prev ?" " :"" ; + +strvec_pop (values); +strvec_pushf (values,"%s%s%s" , prev, sp, buf.buf); +free (prev); + } + + goto exit; + } + +/* This is the start of a new header we don't care about */ + http_auth.header_is_last_match =0 ;
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 297
Quoted 10 lines
http.c +10
-0+ +/* +* If this is a HTTP status line and not a header field, this signals +* a different HTTP response. libcurl writes all the output of all +* response headers of all responses, including redirects. +* We only care about the last HTTP request response's headers so clear +* the existing array. +*/ +if (!strncasecmp (ptr,"http/" ,5 )) +strvec_clear (values);
I found this updated version of 'fwrite_wwwauth()' (using 'skip_iprefix_mem()', 'is_hdr_continuation()', and 'strncasecmp()') a bit easier to read than previous iterations - possibly because all the prefix-skipping is done before adding to 'buf', so 'buf' represents *only* the line's header value (possibly with leading/trailing whitespace, which is trimmed). Plus, avoiding unnecessary allocations is always nice.
Quoted 5 lines
http.c +5
-0+ +exit: +strbuf_release (&buf); +return size; +}
PATCH v8 3/3 credential: add WWW-Authenticate header to cred requests
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
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[] properties where the order that the repeated attributes appear in the conversation reflects the order that the WWW-Authenticate headers appeared in the HTTP response.
Add a set of tests to exercise the HTTP authentication header parsing and the interop with credential helpers. Credential helpers will receive WWW-Authenticate information in credential requests.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
--- Documentation/git-credential.txt | 19 ++- credential.c | 11 ++ t/t5563-simple-http-auth.sh | 236 +++++++++++++++++++++++++++++++ 3 files changed, 265 insertions(+), 1 deletion(-)
Documentation/git-credential.txt +18 -1
@@ -113,7 +113,13 @@ separated by an `=` (equals) sign, followed by a newline. The key may contain any bytes except `=`, newline, or NUL. The value may contain any bytes except newline or NUL. -In both cases, all bytes are treated as-is (i.e., there is no quoting, +Attributes with keys that end with C-style array brackets `[]` can have +multiple values. Each instance of a multi-valued attribute forms an +ordered list of values - the order of the repeated attributes defines +the order of the values. An empty multi-valued attribute (`key[]=\n`) +acts to clear any previous entries and reset the list. + +In all cases, all bytes are treated as-is (i.e., there is no quoting, and one cannot transmit a value with newline or NUL in it). The list of attributes is terminated by a blank line or end-of-file.
@@ -160,6 +166,17 @@ empty string. Components which are missing from the URL (e.g., there is no username in the example above) will be left unset. +`wwwauth[]`:: + + When an HTTP response is received by Git that includes one or more + 'WWW-Authenticate' authentication headers, these will be passed by Git + to credential helpers. ++ +Each 'WWW-Authenticate' header value is passed as a multi-valued +attribute 'wwwauth[]', where the order of the attributes is the same as +they appear in the HTTP response. This attribute is 'one-way' from Git +to pass additional information to credential helpers. + Unrecognised attributes are silently discarded. GIT
credential.c +11 -0
@@ -263,6 +263,16 @@ 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) +{ +char *full_key =xstrfmt ("%s[]" , key); +for (size_t i =0 ; i < vec->nr; i++) { +credential_write_item (fp, full_key, vec->v[i],0 ); + } +free (full_key); +} +void credential_write (const struct credential *c,FILE *fp) {credential_write_item (fp,"protocol" , c->protocol,1 );
@@ -270,6 +280,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,
t/t5563-simple-http-auth.sh +236 -0
242 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v8 3/3] credential: add WWW-Authenticate header to cred requests
credential.c static void credential_write_item(FILE *fp, const char *key, const char *value, · line 269
Quoted 10 lines · Matthew John Cheetham via GitGitGadget
From: Matthew John Cheetham <[email protected]>
credential.c +4
-0@@ -263,6 +263,16 @@ 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) +{ +char *full_key =xstrfmt ("%s[]" , key);
FWIW you could avoid this allocation if you just renamed the current "credential_write_item()" to "credential_write_fmt()", and had it take a format instead of its current hardcoded "%s=%s\n".
Then you could have two wrappers, credential_write_item() and credential_write_items() (instead of "strvec"), the first passing "%s=%s\n", the other "%s[]=%s\n".
Just a thought.
credential.c static void credential_write_item(FILE *fp, const char *key, const char *value, · line 271
Quoted 2 lines
credential.c +2
-0+for (size_t i =0 ; i < vec->nr; i++) { +credential_write_item (fp, full_key, vec->v[i],0 );
The {} here can be dropped in any case.
Re: [PATCH v8 3/3] credential: add WWW-Authenticate header to cred requests
Quoted 18 lines · Ævar Arnfjörð Bjarmason
credential.c static void credential_write_item(FILE *fp, const char *key, const char *value, · line 269
Quoted 10 lines · Matthew John Cheetham via GitGitGadget
From: Matthew John Cheetham <[email protected]>
credential.c +4
-0@@ -263,6 +263,16 @@ 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) +{ +char *full_key =xstrfmt ("%s[]" , key);FWIW you could avoid this allocation if you just renamed the current "
credential_write_item()" to "credential_write_fmt()", and had it take a format instead of its current hardcoded "%s=%s\n".
Quoted 5 lines
Then you could have two wrappers,
credential_write_item()andcredential_write_items()(instead of "strvec"), the first passing "%s=%s\n", the other "%s[]=%s\n".Just a thought.
Given this is the only items/strvec that I'm writing out, just inlining the for-loop and calling credential_write_item directly with the key as "wwwauth[]" would avoid the allocation without needing to make the write_item function more complicated.
-static void credential_write_strvec(FILE *fp, const char *key,
- const struct strvec *vec)
-{
- char *full_key = xstrfmt("%s[]", key);
- for (size_t i = 0; i < vec->nr; i++) {
- credential_write_item(fp, full_key, vec->v[i], 0);
- }
- free(full_key);
-}
-
void credential_write(const struct credential *c, FILE *fp)
{
credential_write_item(fp, "protocol", c->protocol, 1);
@@ -280,7 +270,10 @@ 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);
+ for (size_t i = 0; i < c->wwwauth_headers.nr; i++)
+ credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i],
+ 0);
}
Quoted 5 lines
credential.c static void credential_write_item(FILE *fp, const char *key, const char *value, · line 271
Quoted 2 lines
credential.c +2
-0+for (size_t i =0 ; i < vec->nr; i++) { +credential_write_item (fp, full_key, vec->v[i],0 );The {} here can be dropped in any case.
Quoted 6 lines · Matthew John Cheetham via GitGitGadget
Updates in v8
=============* Drop custom HTTP test helper tool in favour of using a CGI shell script and Apache; avoiding the need to implement an HTTP server.
Quoted 7 lines
* Avoid allocations in header reading callback unless we have a header we care about; act on the char* from libcurl directly rather than create a strbuf for each header.
* Drop
st_multoverflow guarding function in curl callback functions; we're not allocating memory based on the resulting value and just adds to potential confusion in the future.
I just had some nit-y and other trivial comments spotted on a read-through, but with the caveat that I'm not too familiar with the credential infrastructure this looks good to me.
I'm rather neutral on the whole question of whether we eventually ship a httpd in-tree, but I think the v7 to v8 clearly demonstrates that whatever we do there, this topic is much improved by having that question un-tangled from the credential improvements here.
Thanks!
Quoted 6 lines · Matthew John Cheetham via GitGitGadget
Updates in v8
=============* Drop custom HTTP test helper tool in favour of using a CGI shell script and Apache; avoiding the need to implement an HTTP server.
Quoted 8 lines
* Avoid allocations in header reading callback unless we have a header we care about; act on the char* from libcurl directly rather than create a strbuf for each header.
* Drop
st_multoverflow guarding function in curl callback functions; we're not allocating memory based on the resulting value and just adds to potential confusion in the future.
The core functionality change in this series (passing through 'WWW-Authenticate' headers to credential requests) didn't change much between v7 and v8, and what was updated was a net improvement [1]. Otherwise, the new test infrastructure is concise and made for an easy re-review; I had one small suggestion for patch 1 [2], but it's not critical enough on its own to hold this series up. I think this could probably be merged to 'next' as-is, but a final re-roll with some of the minor fixups suggested in other reviews wouldn't hurt either.
Thanks again for the time & effort you've put into perfecting these patches!
[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
"Matthew John Cheetham via GitGitGadget" <[email protected]>
Quoted 8 lines · writes:
In this patch series I update the existing credential helper design in order to allow for some new scenarios, and future evolution of auth methods that Git hosts may wish to provide. I outline the background, summary of changes and some challenges below.
Testing these new additions, I use a small CGI shell script that acts as a frontend to git-http-backend; simple authentication is configurable by files.
Thanks for an update, and thanks Victoria and Ævar for your prompt reviews.
Queued on 'seen', anticipating a small finishing touches before merging to 'next'.
v9 cover Enhance credential helper protocol to include auth headers
In reply to Matthew John Cheetham via GitGitGadgetFollowing from my original RFC submission [0], this submission is considered ready for full review. This patch series is now based on top of current master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now separately submitted patches [1] to fix up the other credential helpers' behaviour.
In this patch series I update the existing credential helper design in order to allow for some new scenarios, and future evolution of auth methods that Git hosts may wish to provide. I outline the background, summary of changes and some challenges below.
Testing these new additions, I use a small CGI shell script that acts as a frontend to git-http-backend; simple authentication is configurable by files.
Background
==========
Git uses a variety of protocols [2]: local, Smart HTTP, Dumb HTTP, SSH, and Git. Here I focus on the Smart HTTP protocol, and attempt to enhance the authentication capabilities of this protocol to address limitations (see below).
The Smart HTTP protocol in Git supports a few different types of HTTP authentication - Basic and Digest (RFC 2617) [3], and Negotiate (RFC 2478) [4]. Git uses a extensible model where credential helpers can provide credentials for protocols [5]. Several helpers support alternatives such as OAuth authentication (RFC 6749) [6], but this is typically done as an extension. For example, a helper might use basic auth and set the password to an OAuth Bearer access token. Git uses standard input and output to communicate with credential helpers.
After a HTTP 401 response, Git would call a credential helper with the following over standard input:
protocol=https
host=example.com
And then a credential helper would return over standard output:
protocol=https
host=example.com
[email protected]
password=<BEARER-TOKEN>
Git then the following request to the remote, including the standard HTTP Authorization header (RFC 7235 Section 4.2) [7]:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: Basic base64([email protected]:<BEARER-TOKEN>)
Credential helpers are encouraged (see gitcredentials.txt) to return the minimum information necessary.
Limitations
===========
Because this credential model was built mostly for password based authentication systems, it's somewhat limited. In particular:
1. To generate valid credentials, additional information about the request (or indeed the requestee and their device) may be required. For example, OAuth is based around scopes. A scope, like "git.read", might be required to read data from the remote. However, the remote cannot tell the credential helper what scope is required for this request.
2. This system is not fully extensible. Each time a new type of authentication (like OAuth Bearer) is invented, Git needs updates before credential helpers can take advantage of it (or leverage a new capability in libcurl).
Goals
=====
* As a user with multiple federated cloud identities:
* Reach out to a remote and have my credential helper automatically prompt me for the correct identity.
* Allow credential helpers to differentiate between different authorities or authentication/authorization challenge types, even from the same DNS hostname (and without needing to use credential.useHttpPath).
* Leverage existing authentication systems built-in to many operating systems and devices to boost security and reduce reliance on passwords.
* As a Git host and/or cloud identity provider:
* Enforce security policies (like requiring two-factor authentication) dynamically.
* Allow integration with third party standard based identity providers in enterprises allowing customers to have a single plane of control for critical identities with access to source code.
Design Principles
=================
* Use the existing infrastructure. Git credential helpers are an already-working model.
* Follow widely-adopted time-proven open standards, avoid net new ideas in the authentication space.
* Minimize knowledge of authentication in Git; maintain modularity and extensibility.
Proposed Changes
================
1. Teach Git to read HTTP response headers, specifically the standard WWW-Authenticate (RFC 7235 Section 4.1) headers.
2. Teach Git to include extra information about HTTP responses that require authentication when calling credential helpers. Specifically the WWW-Authenticate header information.
Because the extra information forms an ordered list, and the existing credential helper I/O format only provides for simple key=value pairs, we introduce a new convention for transmitting an ordered list of values. Key names that are suffixed with a C-style array syntax should have values considered to form an order list, i.e. key[]=value, where the order of the key=value pairs in the stream specifies the order.
For the WWW-Authenticate header values we opt to use the key wwwauth[].
Handling the WWW-Authenticate header in detail
==============================================
RFC 6750 [8] envisions that OAuth Bearer resource servers would give responses that include WWW-Authenticate headers, for example:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
Specifically, a WWW-Authenticate header consists of a scheme and arbitrary attributes, depending on the scheme. This pattern enables generic OAuth or OpenID Connect [9] authorities. Note that it is possible to have several WWW-Authenticate challenges in a response.
First Git attempts to make a request, unauthenticated, which fails with a 401 response and includes WWW-Authenticate header(s).
Next, Git invokes a credential helper which may prompt the user. If the user approves, a credential helper can generate a token (or any auth challenge response) to be used for that request.
For example: with a remote that supports bearer tokens from an OpenID Connect [9] authority, a credential helper can use OpenID Connect's Discovery [10] and Dynamic Client Registration [11] to register a client and make a request with the correct permissions to access the remote. In this manner, a user can be dynamically sent to the right federated identity provider for a remote without any up-front configuration or manual processes.
Following from the principle of keeping authentication knowledge in Git to a minimum, we modify Git to add all WWW-Authenticate values to the credential helper call.
Git sends over standard input:
protocol=https
host=example.com
wwwauth[]=Bearer realm="login.example", scope="git.readwrite" wwwauth[]=Basic realm="login.example"
A credential helper that understands the extra wwwauth[n] property can decide on the "best" or correct authentication scheme, generate credentials for the request, and interact with the user.
The credential helper would then return over standard output:
protocol=https
host=example.com
path=foo.git
[email protected]
password=<BEARER-TOKEN>
Note that WWW-Authenticate supports multiple challenges, either in one header:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
or in multiple headers:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
These have equivalent meaning (RFC 2616 Section 4.2 [12]). To simplify the implementation, Git will not merge or split up any of these WWW-Authenticate headers, and instead pass each header line as one credential helper property. The credential helper is responsible for splitting, merging, and otherwise parsing these header values.
An alternative option to sending the header fields individually would be to merge the header values in to one key=value property, for example:
...
wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
Future work
===========
In the future we can further expand the protocol to allow credential helpers decide the best authentication scheme. Today credential helpers are still only expected to return a username/password pair to Git, meaning the other authentication schemes that may be offered still need challenge responses sent via a Basic Authorization header. The changes outlined above still permit helpers to select and configure an available authentication mode, but require the remote for example to unpack a bearer token from a basic challenge.
More careful consideration is required in the handling of custom authentication schemes which may not have a username, or may require arbitrary additional request header values be set.
For example imagine a new "FooBar" authentication scheme that is surfaced in the following response:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"
With support for arbitrary authentication schemes, Git would call credential helpers with the following over standard input:
protocol=https
host=example.com
wwwauth[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"
And then an enlightened credential helper could return over standard output:
protocol=https
host=example.com
authtype=FooBar
[email protected]
password=<FooBar credential>
header[]=X-FooBar: 12345
header[]=X-FooBar-Alt: ABCDEF
Git would be expected to attach this authorization header to the next request:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: FooBar <FooBar credential>
X-FooBar: 12345
X-FooBar-Alt: ABCDEF
Why not SSH?
============
There's nothing wrong with SSH. However, Git's Smart HTTP transport is widely used, often with OAuth Bearer tokens. Git's Smart HTTP transport sometimes requires less client setup than SSH transport, and works in environments when SSH ports may be blocked. As long as Git supports HTTP transport, it should support common and popular HTTP authentication methods.
References
==========
* [0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
https://lore.kernel.org/git/[email protected]/
* [1] [PATCH 0/3] Correct credential helper discrepancies handling input
https://lore.kernel.org/git/[email protected]/
* [2] Git on the Server - The Protocols
https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
* [3] HTTP Authentication: Basic and Digest Access Authentication
https://datatracker.ietf.org/doc/html/rfc2617
* [4] The Simple and Protected GSS-API Negotiation Mechanism
https://datatracker.ietf.org/doc/html/rfc2478
* [5] Git Credentials - Custom Helpers
https://git-scm.com/docs/gitcredentials#_custom_helpers
* [6] The OAuth 2.0 Authorization Framework
https://datatracker.ietf.org/doc/html/rfc6749
* [7] Hypertext Transfer Protocol (HTTP/1.1): Authentication
https://datatracker.ietf.org/doc/html/rfc7235
* [8] The OAuth 2.0 Authorization Framework: Bearer Token Usage
https://datatracker.ietf.org/doc/html/rfc6750
* [9] OpenID Connect Core 1.0
https://openid.net/specs/openid-connect-core-1_0.html
* [10] OpenID Connect Discovery 1.0
https://openid.net/specs/openid-connect-discovery-1_0.html
* [11] OpenID Connect Dynamic Client Registration 1.0
https://openid.net/specs/openid-connect-registration-1_0.html
* [12] Hypertext Transfer Protocol (HTTP/1.1)
https://datatracker.ietf.org/doc/html/rfc2616
Updates from RFC
================
* Submitted first three patches as separate submission:
https://lore.kernel.org/git/[email protected]/
* Various style fixes and updates to- and addition of comments.
* Drop the explicit integer index in new 'array' style credential helper attrbiutes ("key[n]=value" becomes just "key[]=value").
* Added test helper; a mini HTTP server, and several tests.
Updates in v3
=============
* Split final patch that added the test-http-server in to several, easier to review patches.
* Updated wording in git-credential.txt to clarify which side of the credential helper protocol is sending/receiving the new wwwauth and authtype attributes.
Updates in v4
=============
* Drop authentication scheme selection authtype attribute patches to greatly simplify the series; auth scheme selection is punted to a future series. This series still allows credential helpers to generate credentials and intelligently select correct identities for a given auth challenge.
Updates in v5
=============
* Libify parts of daemon.c and share implementation with test-http-server.
* Clarify test-http-server Git request regex pattern and auth logic comments.
* Use STD*_FILENO in place of 'magic' file descriptor numbers.
* Use strbuf_* functions in continuation header parsing.
* Use configuration file to configure auth for test-http-server rather than command-line arguments. Add ability to specify arbitrary extra headers that is useful for testing 'malformed' server responses.
* Use st_mult over unchecked multiplication in http.c curl callback functions.
* Fix some documentation line break issues.
* Reorder some commits to bring in the tests and test-http-server helper first and, then the WWW-Authentication changes, alongside tests to cover.
* Expose previously static strvec_push_nodup function.
* Merge the two timeout args for test-http-server (--timeout and --init-timeout) that were a hang-over from the original daemon.c but are no longer required here.
* Be more careful around continuation headers where they may be empty strings. Add more tests to cover these header types.
* Include standard trace2 tracing calls at start of test-http-server helper.
Updates in v6
=============
* Clarify the change to make logging optional in the check_dead_children() function during libification of daemon.c.
* Fix missing pointer dereference bugs identified in libification of child process handling functions for daemon.c.
* Add doc comments to child process handling function declarations in the daemon-utils.h header.
* Align function parameter names with variable names at callsites for libified daemon functions.
* Re-split out the test-http-server test helper commits in to smaller
patches: error response handling, request parsing, http-backend pass-through, simple authentication, arbitrary header support.
* Call out auth configuration file format for test-http-server test helper and supported options in commit messages, as well as a test to exercise and demonstrate these options.
* Permit auth.token and auth.challenge to appear in any order; create the struct auth_module just-in-time as options for that scheme are read. This simplifies the configuration authoring of the test-http-server test helper.
* Update tests to use auth.allowAnoymous in the patch that introduces the new test helper option.
* Drop the strvec_push_nodup() commit and update the implementation of HTTP request header line folding to use xstrdup and strvec_pop and _pushf.
* Use size_t instead of int in credential.c when iterating over the struct strvec credential members. Also drop the not required const and cast from the full_key definition and free.
* Replace in-tree test-credential-helper-reply.sh test cred helper script with the lib-credential-helper.sh reusable 'lib' test script and shell functions to configure the helper behaviour.
* Leverage sed over the while read $line loop in the test credential helper script.
Updates in v7
=============
* Address several whitespace and arg/param list alignment issues.
* Rethink the test-http-helper worker-mode error and result enum to be more simple and more informative to the nature of the error.
* Use uintmax_t to store the Content-Length of a request in the helper test-http-server. Maintain a bit flag to store if we received such a header.
* Return a "400 Bad Request" HTTP response if we fail to parse the request in the test-http-server.
* Add test case to cover request message parsing in test-http-server.
* Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in get_auth_module.
* Correctly free the split strbufs created in the header parsing loop in test-http-server.
* Avoid needless comparison > 0 for unsigned types.
* Always set optional outputs to NULL if not present in test helper config value handling.
* Remove an accidentally commented-out test cleanup line for one test case in t5556.
Updates in v8
=============
* Drop custom HTTP test helper tool in favour of using a CGI shell script and Apache; avoiding the need to implement an HTTP server.
* Avoid allocations in header reading callback unless we have a header we care about; act on the char* from libcurl directly rather than create a strbuf for each header.
* Drop st_mult overflow guarding function in curl callback functions; we're not allocating memory based on the resulting value and just adds to potential confusion in the future.
Updates in v9
=============
* Drop anoynmous auth tests as these cases are already covered by all other tests that perform HTTP interactions with a remote today.
* In the custom auth CGI script, avoid the empty-substitution in favour of testing explicitly for an empty string. Also simplify some other conditional expressions.
* Avoid an allocation on each wwwauth[] credential helper key-value pair write.
* Various style fixups.
Matthew John Cheetham (3):
t5563: add tests for basic and anoymous HTTP access
http: read HTTP WWW-Authenticate response headers
credential: add WWW-Authenticate header to cred requests
Documentation/git-credential.txt | 19 +- credential.c | 4 + credential.h | 15 ++ git-compat-util.h | 23 +++ http.c | 120 ++++++++++++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 + t/lib-httpd/nph-custom-auth.sh | 39 ++++ t/t5563-simple-http-auth.sh | 317 +++++++++++++++++++++++++++++++ 9 files changed, 543 insertions(+), 1 deletion(-) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v9
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v9
Pull-Request: https://github.com/gitgitgadget/git/pull/1352
Range-diff vs v8:
1: d362f7016d3 ! 1: 05449ec892b t5563: add tests for basic and anoymous HTTP access
@@ t/lib-httpd/nph-custom-auth.sh (new)
+
+VALID_CREDS_FILE=custom-auth.valid
+CHALLENGE_FILE=custom-auth.challenge
-+ANONYMOUS_FILE=custom-auth.anonymous
+
+#
-+# If $ANONYMOUS_FILE exists in $HTTPD_ROOT_PATH, allow anonymous access.
-+#
+# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid
+# credential for the current request. Each line in the file is considered a
+# valid HTTP Authorization header value. For example:
@@ t/lib-httpd/nph-custom-auth.sh (new)
+# WWW-Authenticate: Basic realm="example.com"
+#
+
-+if test -f "$ANONYMOUS_FILE" || (test -f "$VALID_CREDS_FILE" && \
-+ grep -qi "^${HTTP_AUTHORIZATION:-nopenopnope}$" "$VALID_CREDS_FILE")
++if test -n "$HTTP_AUTHORIZATION" && \
++ grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE"
+then
+ # Note that although git-http-backend returns a status line, it
+ # does so using a CGI 'Status' header. Because this script is an
@@ t/t5563-simple-http-auth.sh (new)
+start_httpd
+
+test_expect_success 'setup_credential_helper' '
-+ mkdir -p "$TRASH_DIRECTORY/bin" &&
++ mkdir "$TRASH_DIRECTORY/bin" &&
+ PATH=$PATH:"$TRASH_DIRECTORY/bin" &&
+ export PATH &&
+
@@ t/t5563-simple-http-auth.sh (new)
+ cmd=$1
+ teefile=$cmd-query.cred
+ catfile=$cmd-reply.cred
-+ sed -n -e "/^$/q" -e "p" >> $teefile
-+ if test "$cmd" = "get"; then
++ sed -n -e "/^$/q" -e "p" >>$teefile
++ if test "$cmd" = "get"
++ then
+ cat $catfile
+ fi
+ EOF
@@ t/t5563-simple-http-auth.sh (new)
+ git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
+'
+
-+test_expect_success 'access anonymous no challenge' '
-+ test_when_finished "per_test_cleanup" &&
-+ touch "$HTTPD_ROOT_PATH/custom-auth.anonymous" &&
-+ git ls-remote "$HTTPD_URL/custom_auth/repo.git"
-+'
-+
+test_expect_success 'access using basic auth' '
+ test_when_finished "per_test_cleanup" &&
+
2: cd9a02ba94e ! 2: f3f28e508c1 http: read HTTP WWW-Authenticate response headers
@@ git-compat-util.h: static inline int skip_iprefix(const char *str, const char *p
+ const char **out, size_t *outlen)
+{
+ size_t prefix_len = strlen(prefix);
++
+ if (len < prefix_len)
+ return 0;
+
-+ if (!strncasecmp(buf, prefix, prefix_len)){
++ if (!strncasecmp(buf, prefix, prefix_len)) {
+ *out = buf + prefix_len;
+ *outlen = len - prefix_len;
+ return 1;
3: 149aedf5501 ! 3: eba58c0d08d credential: add WWW-Authenticate header to cred requests
@@ Documentation/git-credential.txt: empty string.
GIT
## credential.c ##
-@@ credential.c: 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)
-+{
-+ char *full_key = xstrfmt("%s[]", key);
-+ for (size_t i = 0; i < vec->nr; i++) {
-+ credential_write_item(fp, full_key, vec->v[i], 0);
-+ }
-+ free(full_key);
-+}
-+
- void credential_write(const struct credential *c, FILE *fp)
- {
- credential_write_item(fp, "protocol", c->protocol, 1);
@@ credential.c: 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);
++ for (size_t i = 0; i < c->wwwauth_headers.nr; i++)
++ credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i],
++ 0);
}
static int run_credential_helper(struct credential *c,Signature · 1 line
gitgitgadget
PATCH v9 1/3 t5563: add tests for basic and anoymous HTTP access
From: Matthew John Cheetham <[email protected]>
Add a test showing simple anoymous HTTP access to an unprotected repository, that results in no credential helper invocations. Also add a test demonstrating simple basic authentication with simple credential helper support.
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++ t/t5563-simple-http-auth.sh | 81 ++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
t/lib-httpd.sh +1 -0
@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +6 -0
@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
t/lib-httpd/nph-custom-auth.sh new file +39 -0
@@ -0,0 +1,39 @@ +#!/bin/sh + +VALID_CREDS_FILE=custom-auth.valid +CHALLENGE_FILE=custom-auth.challenge + +# +# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid +# credential for the current request. Each line in the file is considered a +# valid HTTP Authorization header value. For example: +# +# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== +# +# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" +# + +if test -n "$HTTP_AUTHORIZATION" && \ + grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE" +then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend +fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi +echo
t/t5563-simple-http-auth.sh new file +81 -0
81 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v9 1/3] t5563: add tests for basic and anoymous HTTP access
"Matthew John Cheetham via GitGitGadget" <[email protected]>
t/lib-httpd/nph-custom-auth.sh line 22
Quoted 2 lines · writes:
t/lib-httpd/nph-custom-auth.sh new file +2
-0+if test -n "$HTTP_AUTHORIZATION" && \ + grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE"
Do we require a regexp match (and worry about metacharacters in HTTP_AUTHORIZATION variable), or would we want to use "grep -F -x" here to force match with the entire line?
t/lib-httpd/nph-custom-auth.sh line 32
Quoted 10 lines
t/lib-httpd/nph-custom-auth.sh new file +10
-0+then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend +fi
OK. That's the successful auth case. Otherwise ...
t/lib-httpd/nph-custom-auth.sh line 39
Quoted 6 lines
t/lib-httpd/nph-custom-auth.sh new file +6
-0+echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi +echo
OK. We'll just give a challenge.
Quoted 24 lines
t/t5563-simple-http-auth.sh new file +18
-0@@ -0,0 +1,81 @@ +#!/bin/sh + +test_description='test http auth header and credential helper interop' + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh + +start_httpd + +test_expect_success 'setup_credential_helper' ' + mkdir "$TRASH_DIRECTORY/bin" && + PATH=$PATH:"$TRASH_DIRECTORY/bin" && + export PATH && + + CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" && + write_script "$CREDENTIAL_HELPER" <<-\EOF + cmd=$1 + teefile=$cmd-query.cred
t/t5563-simple-http-auth.sh line 28
Quoted 10 lines
t/t5563-simple-http-auth.sh new file +10
-0+ catfile=$cmd-reply.cred + sed -n -e "/^$/q" -e "p" >>$teefile + if test "$cmd" = "get" + then + cat $catfile + fi + EOF +' + +set_credential_reply() {
Style. Have SP before "()" as well as after.
t/t5563-simple-http-auth.sh line 32
Quoted 4 lines
t/t5563-simple-http-auth.sh new file +4
-0+ cat >"$TRASH_DIRECTORY/$1-reply.cred" +} + +expect_credential_query() {
Style. Have SP before "()" as well as after.
Quoted 5 lines
t/t5563-simple-http-auth.sh new file +5
-0+ cat >"$TRASH_DIRECTORY/$1-expect.cred" && + test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \ + "$TRASH_DIRECTORY/$1-query.cred" +} +
t/t5563-simple-http-auth.sh line 47
Quoted 10 lines
t/t5563-simple-http-auth.sh new file +10
-0+per_test_cleanup () { + rm -f *.cred && + rm -f "$HTTPD_ROOT_PATH"/custom-auth.* +} + +test_expect_success 'setup repository' ' + test_commit foo && + git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" +'
OK.
t/t5563-simple-http-auth.sh line 59
Quoted 11 lines
t/t5563-simple-http-auth.sh new file +11
-0+test_expect_success 'access using basic auth' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== + EOF
Perhaps we want to note that this matches the "alice:secret-passwd" we prepared earlier?
t/t5563-simple-http-auth.sh line 63
Quoted 3 lines
t/t5563-simple-http-auth.sh new file +3
-0+ cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + WWW-Authenticate: Basic realm="example.com" + EOF
OK.
Quoted 5 lines
t/t5563-simple-http-auth.sh new file +5
-0+ test_config_global credential.helper test-helper && + git ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + expect_credential_query get <<-EOF && + protocol=http
t/t5563-simple-http-auth.sh line 79
Quoted 10 lines
t/t5563-simple-http-auth.sh new file +10
-0+ host=$HTTPD_DEST + EOF + + expect_credential_query store <<-EOF + protocol=http + host=$HTTPD_DEST + username=alice + password=secret-passwd + EOF +'
OK.
Quoted 1 line
t/t5563-simple-http-auth.sh new file +1
-0+test_done
Re: [PATCH v9 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 9 lines · Junio C Hamano
"Matthew John Cheetham via GitGitGadget" <[email protected]>
t/lib-httpd/nph-custom-auth.sh line 22
Quoted 2 lines · writes:
t/lib-httpd/nph-custom-auth.sh new file +2
-0+if test -n "$HTTP_AUTHORIZATION" && \ + grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE"Do we require a regexp match (and worry about metacharacters in
HTTP_AUTHORIZATIONvariable), or would we want to use "grep -F -x" here to force match with the entire line?
You're right. We don't need a regex match here. Will fix.
Quoted 50 lines
t/lib-httpd/nph-custom-auth.sh line 32
Quoted 10 lines
t/lib-httpd/nph-custom-auth.sh new file +10
-0+then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend +fiOK. That's the successful auth case. Otherwise ...
t/lib-httpd/nph-custom-auth.sh line 39
Quoted 6 lines
t/lib-httpd/nph-custom-auth.sh new file +6
-0+echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi +echoOK. We'll just give a challenge.
Quoted 28 lines
t/t5563-simple-http-auth.sh new file +22
-0@@ -0,0 +1,81 @@ +#!/bin/sh + +test_description='test http auth header and credential helper interop' + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh + +start_httpd + +test_expect_success 'setup_credential_helper' ' + mkdir "$TRASH_DIRECTORY/bin" && + PATH=$PATH:"$TRASH_DIRECTORY/bin" && + export PATH && + + CREDENTIAL_HELPER="$TRASH_DIRECTORY/bin/git-credential-test-helper" && + write_script "$CREDENTIAL_HELPER" <<-\EOF + cmd=$1 + teefile=$cmd-query.cred + catfile=$cmd-reply.cred + sed -n -e "/^$/q" -e "p" >>$teefile + if test "$cmd" = "get" + then
Quoted 8 lines
t/t5563-simple-http-auth.sh line 28
Quoted 6 lines
t/t5563-simple-http-auth.sh new file +6
-0+ cat $catfile + fi + EOF +' + +set_credential_reply() {Style. Have SP before "()" as well as after.
Will fix.
Quoted 6 lines
t/t5563-simple-http-auth.sh line 32
Quoted 4 lines
t/t5563-simple-http-auth.sh new file +4
-0+ cat >"$TRASH_DIRECTORY/$1-reply.cred" +} + +expect_credential_query() {Style. Have SP before "()" as well as after.
Ditto.
Quoted 18 lines
Quoted 5 lines
t/t5563-simple-http-auth.sh new file +5
-0+ cat >"$TRASH_DIRECTORY/$1-expect.cred" && + test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \ + "$TRASH_DIRECTORY/$1-query.cred" +} +t/t5563-simple-http-auth.sh line 47
Quoted 10 lines
t/t5563-simple-http-auth.sh new file +10
-0+per_test_cleanup () { + rm -f *.cred && + rm -f "$HTTPD_ROOT_PATH"/custom-auth.* +} + +test_expect_success 'setup repository' ' + test_commit foo && + git init --bare "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git push --mirror "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" +'OK.
Quoted 14 lines
t/t5563-simple-http-auth.sh line 59
Quoted 11 lines
t/t5563-simple-http-auth.sh new file +11
-0+test_expect_success 'access using basic auth' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF && + Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== + EOFPerhaps we want to note that this matches the "alice:secret-passwd" we prepared earlier?
Good point. It's helpful. Will re-introduce.
Quoted 25 lines
t/t5563-simple-http-auth.sh line 63
Quoted 3 lines
t/t5563-simple-http-auth.sh new file +3
-0+ cat >"$HTTPD_ROOT_PATH/custom-auth.challenge" <<-EOF && + WWW-Authenticate: Basic realm="example.com" + EOFOK.
Quoted 5 lines
t/t5563-simple-http-auth.sh new file +5
-0+ test_config_global credential.helper test-helper && + git ls-remote "$HTTPD_URL/custom_auth/repo.git" && + + expect_credential_query get <<-EOF && + protocol=httpt/t5563-simple-http-auth.sh line 79
Quoted 10 lines
t/t5563-simple-http-auth.sh new file +10
-0+ host=$HTTPD_DEST + EOF + + expect_credential_query store <<-EOF + protocol=http + host=$HTTPD_DEST + username=alice + password=secret-passwd + EOF +'OK.
Quoted 1 line
t/t5563-simple-http-auth.sh new file +1
-0+test_done
PATCH v9 2/3 http: read HTTP WWW-Authenticate response headers
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
Read and store the HTTP WWW-Authenticate response headers made for a particular request.
This will allow us to pass important authentication challenge information to credential helpers or others that would otherwise have been lost.
According to RFC2616 Section 4.2 [1], header field names are not case-sensitive meaning when collecting multiple values for the same field name, we can just use the case of the first observed instance of each field name and no normalisation is required.
libcurl only provides us with the ability to read all headers recieved for a particular request, including any intermediate redirect requests or proxies. The lines returned by libcurl include HTTP status lines delinating any intermediate requests such as "HTTP/1.1 200". We use these lines to reset the strvec of WWW-Authenticate header values as we encounter them in order to only capture the final response headers.
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us one line at a time.
In the future [2] we may be able to leverage functions to read headers from libcurl itself, but as of today we must do this ourselves.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
--- credential.c | 1 + credential.h | 15 ++++++ git-compat-util.h | 23 +++++++++ http.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 159 insertions(+)
credential.c +1 -0
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }
credential.h +15 -0
@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,19 @@ struct credential {*/ struct string_list helpers; +/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Used to keep track of split header fields +* in order to fold multiple lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,
@@ -130,6 +144,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */
git-compat-util.h +23 -0
@@ -1266,6 +1266,29 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); + +if (len < prefix_len) +return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +} +static inline int strtoul_ui (char const *s,int base,unsigned int *result) {unsigned long ul;
http.c +120 -0
132 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v9 2/3] http: read HTTP WWW-Authenticate response headers
"Matthew John Cheetham via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
According to RFC2616 Section 4.2 [1], header field names are not case-sensitive meaning when collecting multiple values for the same field name, we can just use the case of the first observed instance of each field name and no normalisation is required.
If the names are not case-sensitive, you can choose to first downcase the names you see, and use that consistently, and the result would still be valid. IOW, "not case-sensitive" does not at all mean you have to use the first observed instance without normalization. You are allowed to choose such an implementation, but "not case-sensitive" is not a justification to choose such an implementation among possible implementation that would be allowed under the rule.
Quoted 4 lines
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us one line at a time.
Saying "one physical line" at a time may make it clear what you are pointing out as a weak point in the interface libcURL gives us (I think you are getting at "if they handled header folding for us and fed us one logical line at a time, it would have been nicer").
Quoted 23 lines
credential.c +1
-0@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }credential.h +1
-0@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and @@ -115,6 +116,19 @@ struct credential {*/ struct string_list helpers;
credential.h struct credential { · line 127
Quoted 10 lines
credential.h +9
-0+/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Used to keep track of split header fields
The technical term for what you call "split header" here seems to be "line folding" (RFC 7230, which deprecates it).
Quoted 35 lines
credential.h +5
-0+* in order to fold multiple lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,@@ -130,6 +144,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */ git-compat-util.h +12
-0@@ -1266,6 +1266,29 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); + +if (len < prefix_len)
git-compat-util.h static inline int skip_iprefix(const char *str, const char *prefix, · line 1290
Quoted 10 lines
git-compat-util.h +10
-0+return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +}
OK.
Quoted 5 lines
http.c +0
-0
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 191
Quoted 9 lines
http.c +6
-0@@ -183,6 +183,124 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)return nmemb; } +/* +* A folded header continuation line starts with at least one single whitespace +* character. It is not a continuation line if the line is *just* a newline. +* The RFC for HTTP states that CRLF is the header field line ending, but some +* servers may use LF only; we accept both. +*/
Nice.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 196
Quoted 5 lines
http.c +5
-0+static inline int is_hdr_continuation (const char *ptr,const size_t size) +{ +/* totally empty line or normal header */ +if (!size || !isspace (*ptr)) +return 0 ;
obs-fold (RFC7230) begins the next line with SP or HTAB, but isspace() allows not just SP and HT but also CR and LF. So this is a bit pessimistic but rejects what is not a folded continuation line reliably.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 200
Quoted 3 lines
http.c +3
-0+/* empty line with LF line ending */ +if (size ==1 && ptr[0 ] =='\n' ) +return 0 ;
And this is a blank line after the headers, with LF (not conforming but is OK).
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 204
Quoted 3 lines
http.c +3
-0+/* empty line with CRLF line ending */ +if (size ==2 && ptr[0 ] =='\r' && ptr[1 ] =='\n' ) +return 0 ;
And this is another form of a blank line after the headers, with CRLF.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 204
Quoted 2 lines
http.c +2
-0+return 1 ; +}
After rejecting the above two "blank", it is a folded continuation line. OK.
I wonder if
static inline int ... () {
return (size && (*ptr == ' ' || *ptr == '\t'));
}sufficient and easier to grok, though.
Quoted 32 lines
http.c +32
-0+static size_t fwrite_wwwauth (char *ptr,size_t eltsize,size_t nmemb,void *p) +{ +size_t size = eltsize * nmemb; +struct strvec *values = &http_auth.wwwauth_headers; +struct strbuf buf =STRBUF_INIT ; +const char *val; +size_t val_len; + +/* +* Header lines may not come NULL-terminated from libcurl so we must +* limit all scans to the maximum length of the header line, or leverage +* strbufs for all operations. +* +* In addition, it is possible that header values can be split over +* multiple lines as per RFC 2616 (even though this has since been +* deprecated in RFC 7230). A continuation header field value is +* identified as starting with a space or horizontal tab. +* +* The formal definition of a header field as given in RFC 2616 is: +* +* message-header = field-name ":" [ field-value ] +* field-name = token +* field-value = *( field-content | LWS ) +* field-content = <the OCTETs making up the field-value +* and consisting of either *TEXT or combinations +* of token, separators, and quoted-string> +*/ + +/* Start of a new WWW-Authenticate header */ +if (skip_iprefix_mem (ptr, size,"www-authenticate:" , &val, &val_len)) { +strbuf_add (&buf, val, val_len); +
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 250
Quoted 10 lines
http.c +10
-0+/* +* Strip the CRLF that should be present at the end of each +* field as well as any trailing or leading whitespace from the +* value. +*/ +strbuf_trim (&buf); + +strvec_push (values, buf.buf); + http_auth.header_is_last_match =1 ; + goto exit;
OK. We remember that we have seen the beginning of a header we are interested in (so that we can append if it is a continuation we see next). Good.
Quoted 11 lines
http.c +11
-0+ } + +/* +* This line could be a continuation of the previously matched header +* field. If this is the case then we should append this value to the +* end of the previously consumed value. +*/ +if (http_auth.header_is_last_match &&is_hdr_continuation (ptr, size)) { +/* +* Trim the CRLF and any leading or trailing from this line. +*/
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 271
Quoted 10 lines
http.c +10
-0+strbuf_add (&buf, ptr, size); +strbuf_trim (&buf); + +/* +* At this point we should always have at least one existing +* value, even if it is empty. Do not bother appending the new +* value if this continuation header is itself empty. +*/ +if (!values->nr) { +BUG ("should have at least one existing header value" );
OK, we should set _is_last_match to true only after we recorded the header that might see a continuation, so it would be a bug if we didn't have anything there. Good.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 283
Quoted 12 lines
http.c +12
-0+ }else if (buf.len) { +char *prev =xstrdup (values->v[values->nr -1 ]); + +/* Join two non-empty values with a single space. */ +const char *const sp = *prev ?" " :"" ; + +strvec_pop (values); +strvec_pushf (values,"%s%s%s" , prev, sp, buf.buf); +free (prev); + } + + goto exit;
Good that we are prepared to see a logical line split over more than two lines (i.e. by not toggling _is_last_match off prematurely here).
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 287
Quoted 4 lines
http.c +4
-0+ } + +/* This is the start of a new header we don't care about */ + http_auth.header_is_last_match =0 ;
Or what we just saw and ignored could be a continuation line of a header we ignored. The comment is slightly misleading.
Other than that, looking good.
Thanks.
Re: [PATCH v9 2/3] http: read HTTP WWW-Authenticate response headers
Quoted 8 lines · Junio C Hamano
"Matthew John Cheetham via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
According to RFC2616 Section 4.2 [1], header field names are not case-sensitive meaning when collecting multiple values for the same field name, we can just use the case of the first observed instance of each field name and no normalisation is required.
Quoted 8 lines
If the names are not case-sensitive, you can choose to first downcase the names you see, and use that consistently, and the result would still be valid. IOW, "not case-sensitive" does not at all mean you have to use the first observed instance without normalization. You are allowed to choose such an implementation, but "not case-sensitive" is not a justification to choose such an implementation among possible implementation that would be allowed under the rule.
Re-reading this paragraph, it doens't really need to even be here. This was an artefact of a time when I was storing all headers, including keys and values. Since we're only interested now in the WWW-Authenticate header _values_, there's no need to call out this out. Will drop this paragraph.
Quoted 9 lines
Quoted 4 lines
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us one line at a time.
Saying "one physical line" at a time may make it clear what you are pointing out as a weak point in the interface libcURL gives us (I think you are getting at "if they handled header folding for us and fed us one logical line at a time, it would have been nicer").
Logical header fields vs physical header lines is useful and clearer terminology - I will update the commit message to reflect in the next iteration. Thanks!
Quoted 133 lines
Quoted 23 lines
credential.c +1
-0@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }credential.h +1
-0@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and @@ -115,6 +116,19 @@ struct credential {*/ struct string_list helpers;credential.h struct credential { · line 127
Quoted 10 lines
credential.h +9
-0+/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Used to keep track of split header fields The technical term for what you call "split header" here seems to be "line folding" (RFC 7230, which deprecates it).
Quoted 35 lines
credential.h +5
-0+* in order to fold multiple lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,@@ -130,6 +144,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */ git-compat-util.h +12
-0@@ -1266,6 +1266,29 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); + +if (len < prefix_len)git-compat-util.h static inline int skip_iprefix(const char *str, const char *prefix, · line 1290
Quoted 10 lines
git-compat-util.h +10
-0+return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +}OK.
Quoted 5 lines
http.c +0
-0http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 191
Quoted 9 lines
http.c +6
-0@@ -183,6 +183,124 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)return nmemb; } +/* +* A folded header continuation line starts with at least one single whitespace +* character. It is not a continuation line if the line is *just* a newline. +* The RFC for HTTP states that CRLF is the header field line ending, but some +* servers may use LF only; we accept both. +*/ Nice.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 196
Quoted 5 lines
http.c +5
-0+static inline int is_hdr_continuation (const char *ptr,const size_t size) +{ +/* totally empty line or normal header */ +if (!size || !isspace (*ptr)) +return 0 ;obs-fold (RFC7230) begins the next line with SP or HTAB, but
isspace()allows not just SP and HT but also CR and LF. So this is a bit pessimistic but rejects what is not a folded continuation line reliably.http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 200
Quoted 3 lines
http.c +3
-0+/* empty line with LF line ending */ +if (size ==1 && ptr[0 ] =='\n' ) +return 0 ;And this is a blank line after the headers, with LF (not conforming but is OK).
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 204
Quoted 3 lines
http.c +3
-0+/* empty line with CRLF line ending */ +if (size ==2 && ptr[0 ] =='\r' && ptr[1 ] =='\n' ) +return 0 ;And this is another form of a blank line after the headers, with CRLF.
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 204
Quoted 2 lines
http.c +2
-0+return 1 ; +}After rejecting the above two "blank", it is a folded continuation line. OK.
Quoted 7 lines
I wonder if
static inline int ... () { return (size && (*ptr == ' ' || *ptr == '\t')); }sufficient and easier to grok, though.
You're correct. This implementation is 'more correct' and easier to grok.
Quoted 89 lines
Quoted 32 lines
http.c +32
-0+static size_t fwrite_wwwauth (char *ptr,size_t eltsize,size_t nmemb,void *p) +{ +size_t size = eltsize * nmemb; +struct strvec *values = &http_auth.wwwauth_headers; +struct strbuf buf =STRBUF_INIT ; +const char *val; +size_t val_len; + +/* +* Header lines may not come NULL-terminated from libcurl so we must +* limit all scans to the maximum length of the header line, or leverage +* strbufs for all operations. +* +* In addition, it is possible that header values can be split over +* multiple lines as per RFC 2616 (even though this has since been +* deprecated in RFC 7230). A continuation header field value is +* identified as starting with a space or horizontal tab. +* +* The formal definition of a header field as given in RFC 2616 is: +* +* message-header = field-name ":" [ field-value ] +* field-name = token +* field-value = *( field-content | LWS ) +* field-content = <the OCTETs making up the field-value +* and consisting of either *TEXT or combinations +* of token, separators, and quoted-string> +*/ + +/* Start of a new WWW-Authenticate header */ +if (skip_iprefix_mem (ptr, size,"www-authenticate:" , &val, &val_len)) { +strbuf_add (&buf, val, val_len); +http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 250
Quoted 10 lines
http.c +10
-0+/* +* Strip the CRLF that should be present at the end of each +* field as well as any trailing or leading whitespace from the +* value. +*/ +strbuf_trim (&buf); + +strvec_push (values, buf.buf); + http_auth.header_is_last_match =1 ; + goto exit;OK. We remember that we have seen the beginning of a header we are interested in (so that we can append if it is a continuation we see next). Good.
Quoted 11 lines
http.c +11
-0+ } + +/* +* This line could be a continuation of the previously matched header +* field. If this is the case then we should append this value to the +* end of the previously consumed value. +*/ +if (http_auth.header_is_last_match &&is_hdr_continuation (ptr, size)) { +/* +* Trim the CRLF and any leading or trailing from this line. +*/ http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 271
Quoted 10 lines
http.c +10
-0+strbuf_add (&buf, ptr, size); +strbuf_trim (&buf); + +/* +* At this point we should always have at least one existing +* value, even if it is empty. Do not bother appending the new +* value if this continuation header is itself empty. +*/ +if (!values->nr) { +BUG ("should have at least one existing header value" );OK, we should set
_is_last_matchto true only after we recorded the header that might see a continuation, so it would be a bug if we didn't have anything there. Good.http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 283
Quoted 12 lines
http.c +12
-0+ }else if (buf.len) { +char *prev =xstrdup (values->v[values->nr -1 ]); + +/* Join two non-empty values with a single space. */ +const char *const sp = *prev ?" " :"" ; + +strvec_pop (values); +strvec_pushf (values,"%s%s%s" , prev, sp, buf.buf); +free (prev); + } + + goto exit;Good that we are prepared to see a logical line split over more than two lines (i.e. by not toggling
_is_last_matchoff prematurely here).
Quoted 7 lines
http.c size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) · line 287
Quoted 4 lines
http.c +4
-0+ } + +/* This is the start of a new header we don't care about */ + http_auth.header_is_last_match =0 ;Or what we just saw and ignored could be a continuation line of a header we ignored. The comment is slightly misleading.
I'll try and reword this to make it more accurate - we have determined this line is not a continuation of the previous WWW-Authenticate header.
Quoted 3 lines
Other than that, looking good.
Thanks.
PATCH v9 3/3 credential: add WWW-Authenticate header to cred requests
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
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[] properties where the order that the repeated attributes appear in the conversation reflects the order that the WWW-Authenticate headers appeared in the HTTP response.
Add a set of tests to exercise the HTTP authentication header parsing and the interop with credential helpers. Credential helpers will receive WWW-Authenticate information in credential requests.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
--- Documentation/git-credential.txt | 19 ++- credential.c | 3 + t/t5563-simple-http-auth.sh | 236 +++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+), 1 deletion(-)
Documentation/git-credential.txt +18 -1
@@ -113,7 +113,13 @@ separated by an `=` (equals) sign, followed by a newline. The key may contain any bytes except `=`, newline, or NUL. The value may contain any bytes except newline or NUL. -In both cases, all bytes are treated as-is (i.e., there is no quoting, +Attributes with keys that end with C-style array brackets `[]` can have +multiple values. Each instance of a multi-valued attribute forms an +ordered list of values - the order of the repeated attributes defines +the order of the values. An empty multi-valued attribute (`key[]=\n`) +acts to clear any previous entries and reset the list. + +In all cases, all bytes are treated as-is (i.e., there is no quoting, and one cannot transmit a value with newline or NUL in it). The list of attributes is terminated by a blank line or end-of-file.
@@ -160,6 +166,17 @@ empty string. Components which are missing from the URL (e.g., there is no username in the example above) will be left unset. +`wwwauth[]`:: + + When an HTTP response is received by Git that includes one or more + 'WWW-Authenticate' authentication headers, these will be passed by Git + to credential helpers. ++ +Each 'WWW-Authenticate' header value is passed as a multi-valued +attribute 'wwwauth[]', where the order of the attributes is the same as +they appear in the HTTP response. This attribute is 'one-way' from Git +to pass additional information to credential helpers. + Unrecognised attributes are silently discarded. GIT
credential.c +3 -0
@@ -270,6 +270,9 @@ 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 ); +for (size_t i =0 ; i < c->wwwauth_headers.nr; i++) +credential_write_item (fp,"wwwauth[]" , c->wwwauth_headers.v[i], +0 ); } static int run_credential_helper(struct credential *c,
t/t5563-simple-http-auth.sh +236 -0
242 lines not on this page · raw message
Signature · 1 line
gitgitgadget
v10 cover Enhance credential helper protocol to include auth headers
In reply to Matthew John Cheetham via GitGitGadgetFollowing from my original RFC submission [0], this submission is considered ready for full review. This patch series is now based on top of current master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now separately submitted patches [1] to fix up the other credential helpers' behaviour.
In this patch series I update the existing credential helper design in order to allow for some new scenarios, and future evolution of auth methods that Git hosts may wish to provide. I outline the background, summary of changes and some challenges below.
Testing these new additions, I use a small CGI shell script that acts as a frontend to git-http-backend; simple authentication is configurable by files.
Background
==========
Git uses a variety of protocols [2]: local, Smart HTTP, Dumb HTTP, SSH, and Git. Here I focus on the Smart HTTP protocol, and attempt to enhance the authentication capabilities of this protocol to address limitations (see below).
The Smart HTTP protocol in Git supports a few different types of HTTP authentication - Basic and Digest (RFC 2617) [3], and Negotiate (RFC 2478) [4]. Git uses a extensible model where credential helpers can provide credentials for protocols [5]. Several helpers support alternatives such as OAuth authentication (RFC 6749) [6], but this is typically done as an extension. For example, a helper might use basic auth and set the password to an OAuth Bearer access token. Git uses standard input and output to communicate with credential helpers.
After a HTTP 401 response, Git would call a credential helper with the following over standard input:
protocol=https
host=example.com
And then a credential helper would return over standard output:
protocol=https
host=example.com
[email protected]
password=<BEARER-TOKEN>
Git then the following request to the remote, including the standard HTTP Authorization header (RFC 7235 Section 4.2) [7]:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: Basic base64([email protected]:<BEARER-TOKEN>)
Credential helpers are encouraged (see gitcredentials.txt) to return the minimum information necessary.
Limitations
===========
Because this credential model was built mostly for password based authentication systems, it's somewhat limited. In particular:
1. To generate valid credentials, additional information about the request (or indeed the requestee and their device) may be required. For example, OAuth is based around scopes. A scope, like "git.read", might be required to read data from the remote. However, the remote cannot tell the credential helper what scope is required for this request.
2. This system is not fully extensible. Each time a new type of authentication (like OAuth Bearer) is invented, Git needs updates before credential helpers can take advantage of it (or leverage a new capability in libcurl).
Goals
=====
* As a user with multiple federated cloud identities:
* Reach out to a remote and have my credential helper automatically prompt me for the correct identity.
* Allow credential helpers to differentiate between different authorities or authentication/authorization challenge types, even from the same DNS hostname (and without needing to use credential.useHttpPath).
* Leverage existing authentication systems built-in to many operating systems and devices to boost security and reduce reliance on passwords.
* As a Git host and/or cloud identity provider:
* Enforce security policies (like requiring two-factor authentication) dynamically.
* Allow integration with third party standard based identity providers in enterprises allowing customers to have a single plane of control for critical identities with access to source code.
Design Principles
=================
* Use the existing infrastructure. Git credential helpers are an already-working model.
* Follow widely-adopted time-proven open standards, avoid net new ideas in the authentication space.
* Minimize knowledge of authentication in Git; maintain modularity and extensibility.
Proposed Changes
================
1. Teach Git to read HTTP response headers, specifically the standard WWW-Authenticate (RFC 7235 Section 4.1) headers.
2. Teach Git to include extra information about HTTP responses that require authentication when calling credential helpers. Specifically the WWW-Authenticate header information.
Because the extra information forms an ordered list, and the existing credential helper I/O format only provides for simple key=value pairs, we introduce a new convention for transmitting an ordered list of values. Key names that are suffixed with a C-style array syntax should have values considered to form an order list, i.e. key[]=value, where the order of the key=value pairs in the stream specifies the order.
For the WWW-Authenticate header values we opt to use the key wwwauth[].
Handling the WWW-Authenticate header in detail
==============================================
RFC 6750 [8] envisions that OAuth Bearer resource servers would give responses that include WWW-Authenticate headers, for example:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
Specifically, a WWW-Authenticate header consists of a scheme and arbitrary attributes, depending on the scheme. This pattern enables generic OAuth or OpenID Connect [9] authorities. Note that it is possible to have several WWW-Authenticate challenges in a response.
First Git attempts to make a request, unauthenticated, which fails with a 401 response and includes WWW-Authenticate header(s).
Next, Git invokes a credential helper which may prompt the user. If the user approves, a credential helper can generate a token (or any auth challenge response) to be used for that request.
For example: with a remote that supports bearer tokens from an OpenID Connect [9] authority, a credential helper can use OpenID Connect's Discovery [10] and Dynamic Client Registration [11] to register a client and make a request with the correct permissions to access the remote. In this manner, a user can be dynamically sent to the right federated identity provider for a remote without any up-front configuration or manual processes.
Following from the principle of keeping authentication knowledge in Git to a minimum, we modify Git to add all WWW-Authenticate values to the credential helper call.
Git sends over standard input:
protocol=https
host=example.com
wwwauth[]=Bearer realm="login.example", scope="git.readwrite" wwwauth[]=Basic realm="login.example"
A credential helper that understands the extra wwwauth[n] property can decide on the "best" or correct authentication scheme, generate credentials for the request, and interact with the user.
The credential helper would then return over standard output:
protocol=https
host=example.com
path=foo.git
[email protected]
password=<BEARER-TOKEN>
Note that WWW-Authenticate supports multiple challenges, either in one header:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
or in multiple headers:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
These have equivalent meaning (RFC 2616 Section 4.2 [12]). To simplify the implementation, Git will not merge or split up any of these WWW-Authenticate headers, and instead pass each header line as one credential helper property. The credential helper is responsible for splitting, merging, and otherwise parsing these header values.
An alternative option to sending the header fields individually would be to merge the header values in to one key=value property, for example:
...
wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
Future work
===========
In the future we can further expand the protocol to allow credential helpers decide the best authentication scheme. Today credential helpers are still only expected to return a username/password pair to Git, meaning the other authentication schemes that may be offered still need challenge responses sent via a Basic Authorization header. The changes outlined above still permit helpers to select and configure an available authentication mode, but require the remote for example to unpack a bearer token from a basic challenge.
More careful consideration is required in the handling of custom authentication schemes which may not have a username, or may require arbitrary additional request header values be set.
For example imagine a new "FooBar" authentication scheme that is surfaced in the following response:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"
With support for arbitrary authentication schemes, Git would call credential helpers with the following over standard input:
protocol=https
host=example.com
wwwauth[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"
And then an enlightened credential helper could return over standard output:
protocol=https
host=example.com
authtype=FooBar
[email protected]
password=<FooBar credential>
header[]=X-FooBar: 12345
header[]=X-FooBar-Alt: ABCDEF
Git would be expected to attach this authorization header to the next request:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: FooBar <FooBar credential>
X-FooBar: 12345
X-FooBar-Alt: ABCDEF
Why not SSH?
============
There's nothing wrong with SSH. However, Git's Smart HTTP transport is widely used, often with OAuth Bearer tokens. Git's Smart HTTP transport sometimes requires less client setup than SSH transport, and works in environments when SSH ports may be blocked. As long as Git supports HTTP transport, it should support common and popular HTTP authentication methods.
References
==========
* [0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
https://lore.kernel.org/git/[email protected]/
* [1] [PATCH 0/3] Correct credential helper discrepancies handling input
https://lore.kernel.org/git/[email protected]/
* [2] Git on the Server - The Protocols
https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
* [3] HTTP Authentication: Basic and Digest Access Authentication
https://datatracker.ietf.org/doc/html/rfc2617
* [4] The Simple and Protected GSS-API Negotiation Mechanism
https://datatracker.ietf.org/doc/html/rfc2478
* [5] Git Credentials - Custom Helpers
https://git-scm.com/docs/gitcredentials#_custom_helpers
* [6] The OAuth 2.0 Authorization Framework
https://datatracker.ietf.org/doc/html/rfc6749
* [7] Hypertext Transfer Protocol (HTTP/1.1): Authentication
https://datatracker.ietf.org/doc/html/rfc7235
* [8] The OAuth 2.0 Authorization Framework: Bearer Token Usage
https://datatracker.ietf.org/doc/html/rfc6750
* [9] OpenID Connect Core 1.0
https://openid.net/specs/openid-connect-core-1_0.html
* [10] OpenID Connect Discovery 1.0
https://openid.net/specs/openid-connect-discovery-1_0.html
* [11] OpenID Connect Dynamic Client Registration 1.0
https://openid.net/specs/openid-connect-registration-1_0.html
* [12] Hypertext Transfer Protocol (HTTP/1.1)
https://datatracker.ietf.org/doc/html/rfc2616
Updates from RFC
================
* Submitted first three patches as separate submission:
https://lore.kernel.org/git/[email protected]/
* Various style fixes and updates to- and addition of comments.
* Drop the explicit integer index in new 'array' style credential helper attrbiutes ("key[n]=value" becomes just "key[]=value").
* Added test helper; a mini HTTP server, and several tests.
Updates in v3
=============
* Split final patch that added the test-http-server in to several, easier to review patches.
* Updated wording in git-credential.txt to clarify which side of the credential helper protocol is sending/receiving the new wwwauth and authtype attributes.
Updates in v4
=============
* Drop authentication scheme selection authtype attribute patches to greatly simplify the series; auth scheme selection is punted to a future series. This series still allows credential helpers to generate credentials and intelligently select correct identities for a given auth challenge.
Updates in v5
=============
* Libify parts of daemon.c and share implementation with test-http-server.
* Clarify test-http-server Git request regex pattern and auth logic comments.
* Use STD*_FILENO in place of 'magic' file descriptor numbers.
* Use strbuf_* functions in continuation header parsing.
* Use configuration file to configure auth for test-http-server rather than command-line arguments. Add ability to specify arbitrary extra headers that is useful for testing 'malformed' server responses.
* Use st_mult over unchecked multiplication in http.c curl callback functions.
* Fix some documentation line break issues.
* Reorder some commits to bring in the tests and test-http-server helper first and, then the WWW-Authentication changes, alongside tests to cover.
* Expose previously static strvec_push_nodup function.
* Merge the two timeout args for test-http-server (--timeout and --init-timeout) that were a hang-over from the original daemon.c but are no longer required here.
* Be more careful around continuation headers where they may be empty strings. Add more tests to cover these header types.
* Include standard trace2 tracing calls at start of test-http-server helper.
Updates in v6
=============
* Clarify the change to make logging optional in the check_dead_children() function during libification of daemon.c.
* Fix missing pointer dereference bugs identified in libification of child process handling functions for daemon.c.
* Add doc comments to child process handling function declarations in the daemon-utils.h header.
* Align function parameter names with variable names at callsites for libified daemon functions.
* Re-split out the test-http-server test helper commits in to smaller
patches: error response handling, request parsing, http-backend pass-through, simple authentication, arbitrary header support.
* Call out auth configuration file format for test-http-server test helper and supported options in commit messages, as well as a test to exercise and demonstrate these options.
* Permit auth.token and auth.challenge to appear in any order; create the struct auth_module just-in-time as options for that scheme are read. This simplifies the configuration authoring of the test-http-server test helper.
* Update tests to use auth.allowAnoymous in the patch that introduces the new test helper option.
* Drop the strvec_push_nodup() commit and update the implementation of HTTP request header line folding to use xstrdup and strvec_pop and _pushf.
* Use size_t instead of int in credential.c when iterating over the struct strvec credential members. Also drop the not required const and cast from the full_key definition and free.
* Replace in-tree test-credential-helper-reply.sh test cred helper script with the lib-credential-helper.sh reusable 'lib' test script and shell functions to configure the helper behaviour.
* Leverage sed over the while read $line loop in the test credential helper script.
Updates in v7
=============
* Address several whitespace and arg/param list alignment issues.
* Rethink the test-http-helper worker-mode error and result enum to be more simple and more informative to the nature of the error.
* Use uintmax_t to store the Content-Length of a request in the helper test-http-server. Maintain a bit flag to store if we received such a header.
* Return a "400 Bad Request" HTTP response if we fail to parse the request in the test-http-server.
* Add test case to cover request message parsing in test-http-server.
* Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in get_auth_module.
* Correctly free the split strbufs created in the header parsing loop in test-http-server.
* Avoid needless comparison > 0 for unsigned types.
* Always set optional outputs to NULL if not present in test helper config value handling.
* Remove an accidentally commented-out test cleanup line for one test case in t5556.
Updates in v8
=============
* Drop custom HTTP test helper tool in favour of using a CGI shell script and Apache; avoiding the need to implement an HTTP server.
* Avoid allocations in header reading callback unless we have a header we care about; act on the char* from libcurl directly rather than create a strbuf for each header.
* Drop st_mult overflow guarding function in curl callback functions; we're not allocating memory based on the resulting value and just adds to potential confusion in the future.
Updates in v9
=============
* Drop anoynmous auth tests as these cases are already covered by all other tests that perform HTTP interactions with a remote today.
* In the custom auth CGI script, avoid the empty-substitution in favour of testing explicitly for an empty string. Also simplify some other conditional expressions.
* Avoid an allocation on each wwwauth[] credential helper key-value pair write.
* Various style fixups.
Updates in v10
==============
* Style fixups.
* Only consider space (SP ' ') and horizontal tab (HTAB '\t') when detecting a header continuation line, as per the latest RFC on the matter.
* Update references to old HTTP specs and formal grammars of header fields in comments.
* Rewording of commit messages to remove confusing comment about the case sensitivity of header field names - this is not relevant with the current iteration of the header parsing code. Also update the message around libcurl header support to clarify that physical header lines are returned, but not 'logical' header lines.
* Reword struct credential member doc comment to clarify the purpose of header_is_last_match is for re-folding lines of the WWW-Authenticate header.
* Reintroduce helpful comments in tests to show the origin of the 'magic' base64 basic auth value.
* Use grep -F to ensure we don't do regex matching; avoid interpreting special characters. Remove erronous insensitive comparison flag.
Matthew John Cheetham (3):
t5563: add tests for basic and anoymous HTTP access
http: read HTTP WWW-Authenticate response headers
credential: add WWW-Authenticate header to cred requests
Documentation/git-credential.txt | 19 +- credential.c | 4 + credential.h | 16 ++ git-compat-util.h | 23 +++ http.c | 111 +++++++++++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 + t/lib-httpd/nph-custom-auth.sh | 39 ++++ t/t5563-simple-http-auth.sh | 324 +++++++++++++++++++++++++++++++ 9 files changed, 542 insertions(+), 1 deletion(-) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v10
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v10
Pull-Request: https://github.com/gitgitgadget/git/pull/1352
Range-diff vs v9:
1: 05449ec892b ! 1: f3ccc53055a t5563: add tests for basic and anoymous HTTP access
@@ t/lib-httpd/nph-custom-auth.sh (new)
+#
+
+if test -n "$HTTP_AUTHORIZATION" && \
-+ grep -qsi "^${HTTP_AUTHORIZATION}\$" "$VALID_CREDS_FILE"
++ grep -Fqsx "${HTTP_AUTHORIZATION}" "$VALID_CREDS_FILE"
+then
+ # Note that although git-http-backend returns a status line, it
+ # does so using a CGI 'Status' header. Because this script is an
@@ t/t5563-simple-http-auth.sh (new)
+ EOF
+'
+
-+set_credential_reply() {
++set_credential_reply () {
+ cat >"$TRASH_DIRECTORY/$1-reply.cred"
+}
+
-+expect_credential_query() {
++expect_credential_query () {
+ cat >"$TRASH_DIRECTORY/$1-expect.cred" &&
+ test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \
+ "$TRASH_DIRECTORY/$1-query.cred"
@@ t/t5563-simple-http-auth.sh (new)
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF 2: f3f28e508c1 ! 2: 703ac15222f http: read HTTP WWW-Authenticate response headers
@@ Commit message
information to credential helpers or others that would otherwise have been lost.
- According to RFC2616 Section 4.2 [1], header field names are not
- case-sensitive meaning when collecting multiple values for the same
- field name, we can just use the case of the first observed instance of
- each field name and no normalisation is required.
-
libcurl only provides us with the ability to read all headers recieved for a particular request, including any intermediate redirect requests or proxies. The lines returned by libcurl include HTTP status lines
@@ Commit message
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to
- be continued over multiple lines, but libcurl only gives us one line at
- a time.
+ be continued over multiple lines, but libcurl only gives us each
+ physical line a time, not each logical header. This line folding feature
+ is deprecated in RFC 7230 [1] but older servers may still emit them, so
+ we need to handle them.
In the future [2] we may be able to leverage functions to read headers from libcurl itself, but as of today we must do this ourselves.
- [1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
+ [1] https://www.rfc-editor.org/rfc/rfc7230#section-3.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
@@ credential.h: struct credential {
+ struct strvec wwwauth_headers;
+
+ /**
-+ * Internal use only. Used to keep track of split header fields
-+ * in order to fold multiple lines into one value.
++ * Internal use only. Keeps track of if we previously matched against a
++ * WWW-Authenticate header line in order to re-fold future continuation
++ * lines into one value.
+ */
+ unsigned header_is_last_match:1;
+
@@ http.c: size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buff
}
+/*
-+ * A folded header continuation line starts with at least one single whitespace -+ * character. It is not a continuation line if the line is *just* a newline. -+ * The RFC for HTTP states that CRLF is the header field line ending, but some -+ * servers may use LF only; we accept both.
++ * A folded header continuation line starts with any number of spaces or ++ * horizontal tab characters (SP or HTAB) as per RFC 7230 section 3.2. ++ * It is not a continuation line if the line starts with any other character.
+ */
+static inline int is_hdr_continuation(const char *ptr, const size_t size) +{
-+ /* totally empty line or normal header */
-+ if (!size || !isspace(*ptr))
-+ return 0;
-+
-+ /* empty line with LF line ending */
-+ if (size == 1 && ptr[0] == '\n')
-+ return 0;
-+
-+ /* empty line with CRLF line ending */
-+ if (size == 2 && ptr[0] == '\r' && ptr[1] == '\n')
-+ return 0;
-+
-+ return 1;
++ return size && (*ptr == ' ' || *ptr == '\t');
+}
+
+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
@@ http.c: size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buff
+ * strbufs for all operations.
+ *
+ * In addition, it is possible that header values can be split over
-+ * multiple lines as per RFC 2616 (even though this has since been
-+ * deprecated in RFC 7230). A continuation header field value is
-+ * identified as starting with a space or horizontal tab.
++ * multiple lines as per RFC 7230. 'Line folding' has been deprecated
++ * but older servers may still emit them. A continuation header field
++ * value is identified as starting with a space or horizontal tab.
++ *
++ * The formal definition of a header field as given in RFC 7230 is:
++ *
++ * header-field = field-name ":" OWS field-value OWS
+ *
-+ * The formal definition of a header field as given in RFC 2616 is:
++ * field-name = token
++ * field-value = *( field-content / obs-fold )
++ * field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
++ * field-vchar = VCHAR / obs-text
+ *
-+ * message-header = field-name ":" [ field-value ]
-+ * field-name = token
-+ * field-value = *( field-content | LWS )
-+ * field-content = <the OCTETs making up the field-value
-+ * and consisting of either *TEXT or combinations
-+ * of token, separators, and quoted-string>
++ * obs-fold = CRLF 1*( SP / HTAB )
++ * ; obsolete line folding
++ * ; see Section 3.2.4
+ */
+
+ /* Start of a new WWW-Authenticate header */
@@ http.c: size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buff
+ goto exit;
+ }
+
-+ /* This is the start of a new header we don't care about */
++ /* Not a continuation of a previously matched auth header line. */
+ http_auth.header_is_last_match = 0;
+
+ /*
3: eba58c0d08d ! 3: 186da54fd3b credential: add WWW-Authenticate header to cred requests
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=wrong-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOF
@@ t/t5563-simple-http-auth.sh: test_expect_success 'access using basic auth' '
+ password=secret-passwd
+ EOF
+
++ # Basic base64(alice:secret-passwd)
+ cat >"$HTTPD_ROOT_PATH/custom-auth.valid" <<-EOF &&
+ Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA==
+ EOFSignature · 1 line
gitgitgadget
PATCH v10 1/3 t5563: add tests for basic and anoymous HTTP access
From: Matthew John Cheetham <[email protected]>
Add a test showing simple anoymous HTTP access to an unprotected repository, that results in no credential helper invocations. Also add a test demonstrating simple basic authentication with simple credential helper support.
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++ t/t5563-simple-http-auth.sh | 82 ++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
t/lib-httpd.sh +1 -0
@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +6 -0
@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
t/lib-httpd/nph-custom-auth.sh new file +39 -0
@@ -0,0 +1,39 @@ +#!/bin/sh + +VALID_CREDS_FILE=custom-auth.valid +CHALLENGE_FILE=custom-auth.challenge + +# +# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid +# credential for the current request. Each line in the file is considered a +# valid HTTP Authorization header value. For example: +# +# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== +# +# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" +# + +if test -n "$HTTP_AUTHORIZATION" && \ + grep -Fqsx "${HTTP_AUTHORIZATION}" "$VALID_CREDS_FILE" +then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend +fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi +echo
t/t5563-simple-http-auth.sh new file +82 -0
82 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v10 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 3 lines · Matthew John Cheetham via GitGitGadget
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
Hmm, today I learned about NPH scripts.
Obviously it works here, but I have to wonder: is there a reason we need this? AFAICT the only thing we do is set the HTTP response code, which could also be done with a Status: header.
I.e., this passes your test:
t/lib-httpd.sh +1 -1
@@ -140,7 +140,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh - install_script nph-custom-auth.sh + install_script custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +1 -1
@@ -139,7 +139,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 -ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 +ScriptAliasMatch /custom_auth/(.*) custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
t/lib-httpd/custom-auth.sh renamed from t/lib-httpd/nph-custom-auth.sh +1 -2
@@ -27,11 +27,10 @@ then # status line. # This is only a test script, so we don't bother to check for # the actual status from git-http-backend and always return 200. - echo 'HTTP/1.1 200 OK' exec "$GIT_EXEC_PATH"/git-http-backend fi -echo 'HTTP/1.1 401 Authorization Required' +echo 'Status: 401' if test -f "$CHALLENGE_FILE" then cat "$CHALLENGE_FILE"
The other, more invisible thing happening behind the scenes is that Apache isn't adding any of its usual headers. But I don't know of any that would interfere with our goal of doing auth here. Is there some feature you're planning where it would?
I think you could argue that it's mostly a matter of personal preference and doesn't matter much either way. But all things being equal, I'd usually go with the thing that is simpler and closer to the rest of the system (e.g., I think you kill the ability of http-backend to return a non-200 status, though I doubt it matters much in practice).
So I dunno. We are on v10 and this is arguably a nit. Mostly I'm just curious what led you in this direction in the first place.
Quoted 7 lines
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++ t/t5563-simple-http-auth.sh | 82 ++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh
Most of the other scripts here don't have an execute bit. They get one when they're copied by instal_script in lib-httpd.sh. The exception is error.sh, but I don't think there's any good reason for it. So probably not a big deal either way, but another nit. :)
The rest of it all looks quite nice to me.
-Peff
Re: [PATCH v10 1/3] t5563: add tests for basic and anoymous HTTP access
Quoted 7 lines · Jeff King
Hmm, today I learned about NPH scripts.
Obviously it works here, but I have to wonder: is there a reason we need this? AFAICT the only thing we do is set the HTTP response code, which could also be done with a Status: header.
I.e., this passes your test:
Having looked at patch 3 now, this also needs:
t/t5563-simple-http-auth.sh +1 -1
@@ -37,7 +37,7 @@ expect_credential_query () { per_test_cleanup () { rm -f *.cred && - rm -f "$HTTPD_ROOT_PATH"/custom-auth.* + rm -f "$HTTPD_ROOT_PATH"/custom-auth.valid "$HTTPD_ROOT_PATH"/custom-auth.challenge } test_expect_success 'setup repository' '
or comedy ensues. But more importantly, realized why you want to use NPH here. Apache will happily munge:
WWW-Authenticate: foo
WWW-Authenticate: bar
into:
WWW-Authenticate: foo, bar
and you want to stress the parser with specific syntactic forms. So that makes sense, and I agree NPH is the right solution here.
I think you did try to say this in the commit message as:
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
but I was too dense to realize quite what that meant. :)
-Peff
Re: [PATCH v10 1/3] t5563: add tests for basic and anoymous HTTP access
In reply to Jeff KingQuoted 11 lines · Jeff King
Quoted 3 lines · Matthew John Cheetham via GitGitGadget
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
Hmm, today I learned about NPH scripts.
Obviously it works here, but I have to wonder: is there a reason we need this? AFAICT the only thing we do is set the HTTP response code, which could also be done with a Status: header.
Yep - I think you realised why in a later email. It's because Apache is doing some CGI -> HTTP header normalisation, but we want to control the exact byte output of WWW-Authenticate headers for exercising the new code :-)
Quoted 72 lines
I.e., this passes your test:
t/lib-httpd.sh +1
-1@@ -140,7 +140,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh - install_script nph-custom-auth.sh + install_script custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"t/lib-httpd/apache.conf +1
-1@@ -139,7 +139,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 -ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 +ScriptAliasMatch /custom_auth/(.*) custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>t/lib-httpd/custom-auth.sh renamed from t/lib-httpd/nph-custom-auth.sh +1
-2@@ -27,11 +27,10 @@ then # status line. # This is only a test script, so we don't bother to check for # the actual status from git-http-backend and always return 200. - echo 'HTTP/1.1 200 OK' exec "$GIT_EXEC_PATH"/git-http-backend fi -echo 'HTTP/1.1 401 Authorization Required' +echo 'Status: 401' if test -f "$CHALLENGE_FILE" then cat "$CHALLENGE_FILE"The other, more invisible thing happening behind the scenes is that Apache isn't adding any of its usual headers. But I don't know of any that would interfere with our goal of doing auth here. Is there some feature you're planning where it would?
I think you could argue that it's mostly a matter of personal preference and doesn't matter much either way. But all things being equal, I'd usually go with the thing that is simpler and closer to the rest of the system (e.g., I think you kill the ability of http-backend to return a non-200 status, though I doubt it matters much in practice).
So I dunno. We are on v10 and this is arguably a nit. Mostly I'm just curious what led you in this direction in the first place.
Quoted 7 lines
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++ t/t5563-simple-http-auth.sh | 82 ++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+) create mode 100755 t/lib-httpd/nph-custom-auth.sh
Quoted 4 lines
Most of the other scripts here don't have an execute bit. They get one when they're copied by
instal_scriptinlib-httpd.sh. The exception iserror.sh, but I don't think there's any good reason for it. So probably not a big deal either way, but another nit. :)
Oh.. that's something I missed. I just added the executable bit by habit. Will remove to match the others in lib-httpd/.
Quoted 3 lines
The rest of it all looks quite nice to me.
-Peff
PATCH v10 2/3 http: read HTTP WWW-Authenticate response headers
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
Read and store the HTTP WWW-Authenticate response headers made for a particular request.
This will allow us to pass important authentication challenge information to credential helpers or others that would otherwise have been lost.
libcurl only provides us with the ability to read all headers recieved for a particular request, including any intermediate redirect requests or proxies. The lines returned by libcurl include HTTP status lines delinating any intermediate requests such as "HTTP/1.1 200". We use these lines to reset the strvec of WWW-Authenticate header values as we encounter them in order to only capture the final response headers.
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us each physical line a time, not each logical header. This line folding feature is deprecated in RFC 7230 [1] but older servers may still emit them, so we need to handle them.
In the future [2] we may be able to leverage functions to read headers from libcurl itself, but as of today we must do this ourselves.
[1] https://www.rfc-editor.org/rfc/rfc7230#section-3.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
--- credential.c | 1 + credential.h | 16 +++++++ git-compat-util.h | 23 ++++++++++ http.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 151 insertions(+)
credential.c +1 -0
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }
credential.h +16 -0
@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,20 @@ struct credential {*/ struct string_list helpers; +/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Keeps track of if we previously matched against a +* WWW-Authenticate header line in order to re-fold future continuation +* lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,
@@ -130,6 +145,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */
git-compat-util.h +23 -0
@@ -1266,6 +1266,29 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +size_t prefix_len =strlen (prefix); + +if (len < prefix_len) +return 0 ; + +if (!strncasecmp (buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; +return 1 ; + } + +return 0 ; +} +static inline int strtoul_ui (char const *s,int base,unsigned int *result) {unsigned long ul;
http.c +111 -0
123 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v10 2/3] http: read HTTP WWW-Authenticate response headers
Quoted 12 lines · Matthew John Cheetham via GitGitGadget
(quoted diff) +12
-0+/* + * Like skip_prefix_mem, but compare case-insensitively. Note that the + * comparison is done via tolower(), so it is strictly ASCII (no multi-byte + * characters or locale-specific conversions). + */ +static inline int skip_iprefix_mem(const char *buf, size_t len, + const char *prefix, + const char **out, size_t *outlen) +{ + size_t prefix_len = strlen(prefix); + + if (len < prefix_len)
Review
Quoted 10 lines
(quoted diff) +10
-0+ return 0; + + if (!strncasecmp(buf, prefix, prefix_len)) { + *out = buf + prefix_len; + *outlen = len - prefix_len; + return 1; + } + + return 0; +}
The comment at the top of the function seems out of date. It's using strncasecmp(), so it probably would be locale-dependent. I think that's probably OK, but we should probably fix the comment.
Alternatively, you could copy the tolower() loop from skip_iprefix(). Something like:
git-compat-util.h +7 -11
@@ -1296,17 +1296,13 @@ static inline int skip_iprefix_mem(const char *buf, size_t len,const char *prefix, const char **out, size_t *outlen) { - size_t prefix_len = strlen(prefix); - -if (len < prefix_len) -return 0 ; - -if (!strncasecmp (buf, prefix, prefix_len)) { - *out = buf + prefix_len; - *outlen = len - prefix_len; -return 1 ; - } - + do { + if (!*prefix) { + *out = buf; + *outlen = len; +return 1 ; + } + } while (len-- > 0 && tolower(*buf++) == tolower(*prefix++));return 0 ; }
looks right to me, though only lightly tested (via t5563). I'm happy with either implementation.
Review
Quoted 11 lines
(quoted diff) +10
-0+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p) + /* + * If this is a HTTP status line and not a header field, this signals + * a different HTTP response. libcurl writes all the output of all + * response headers of all responses, including redirects. + * We only care about the last HTTP request response's headers so clear + * the existing array. + */ + if (!strncasecmp(ptr, "http/", 5)) + strvec_clear(values);
Since "ptr" isn't NUL terminated, using strncasecmp() may walk off the end. I think you'd need to check that there are five bytes. You could even use skip_iprefix_mem(), though of course we'd throw away the output values. (For strings there is also istarts_with(), but I don't think we have a "mem" equivalent).
The rest of the parsing looks good to me.
-Peff
Re: [PATCH v10 2/3] http: read HTTP WWW-Authenticate response headers
Quoted 33 lines · Jeff King
Alternatively, you could copy the
tolower()loop fromskip_iprefix(). Something like:git-compat-util.h +7
-11@@ -1296,17 +1296,13 @@ static inline int skip_iprefix_mem(const char *buf, size_t len,const char *prefix, const char **out, size_t *outlen) { - size_t prefix_len = strlen(prefix); - -if (len < prefix_len) -return 0 ; - -if (!strncasecmp (buf, prefix, prefix_len)) { - *out = buf + prefix_len; - *outlen = len - prefix_len; -return 1 ; - } - + do { + if (!*prefix) { + *out = buf; + *outlen = len; +return 1 ; + } + } while (len-- > 0 && tolower(*buf++) == tolower(*prefix++));return 0 ; }
Quoted 2 lines
looks right to me, though only lightly tested (via t5563). I'm happy with either implementation.
Yeah, the alternative version looks clearer to me.
Quoted 9 lines
Quoted 9 lines
(quoted diff) +8
-0+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p) + /* + * If this is a HTTP status line and not a header field, this signals + * a different HTTP response. libcurl writes all the output of all + * response headers of all responses, including redirects. + * We only care about the last HTTP request response's headers so clear + * the existing array. + */
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!strncasecmp(ptr, "http/", 5)) + strvec_clear(values);Since "ptr" isn't NUL terminated, using
strncasecmp()may walk off the end. I think you'd need to check that there are five bytes. You could even useskip_iprefix_mem(), though of course we'd throw away the output values. (For strings there is alsoistarts_with(), but I don't think we have a "mem" equivalent).
Yuck, thank you very much for carefully reading. I missed this one when I queued it.
Quoted 1 line
The rest of the parsing looks good to me.
Thanks.
Re: [PATCH v10 2/3] http: read HTTP WWW-Authenticate response headers
Quoted 35 lines · Junio C Hamano
Quoted 33 lines · Jeff King
Alternatively, you could copy the
tolower()loop fromskip_iprefix(). Something like:git-compat-util.h +7
-11@@ -1296,17 +1296,13 @@ static inline int skip_iprefix_mem(const char *buf, size_t len,const char *prefix, const char **out, size_t *outlen) { - size_t prefix_len = strlen(prefix); - -if (len < prefix_len) -return 0 ; - -if (!strncasecmp (buf, prefix, prefix_len)) { - *out = buf + prefix_len; - *outlen = len - prefix_len; -return 1 ; - } - + do { + if (!*prefix) { + *out = buf; + *outlen = len; +return 1 ; + } + } while (len-- > 0 && tolower(*buf++) == tolower(*prefix++));return 0 ; }
Quoted 4 lines
Quoted 2 lines
looks right to me, though only lightly tested (via t5563). I'm happy with either implementation.
Yeah, the alternative version looks clearer to me.
Will update - thanks!
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
(quoted diff) +8
-0+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p) + /* + * If this is a HTTP status line and not a header field, this signals + * a different HTTP response. libcurl writes all the output of all + * response headers of all responses, including redirects. + * We only care about the last HTTP request response's headers so clear + * the existing array. + */Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!strncasecmp(ptr, "http/", 5)) + strvec_clear(values);Since "ptr" isn't NUL terminated, using
strncasecmp()may walk off the end. I think you'd need to check that there are five bytes. You could even useskip_iprefix_mem(), though of course we'd throw away the output values. (For strings there is alsoistarts_with(), but I don't think we have a "mem" equivalent).
Quoted 2 lines
Yuck, thank you very much for carefully reading. I missed this one when I queued it.
Oops! Will update in a v11
Quoted 3 lines
Quoted 1 line
The rest of the parsing looks good to me.
Thanks.
PATCH v10 3/3 credential: add WWW-Authenticate header to cred requests
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
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[] properties where the order that the repeated attributes appear in the conversation reflects the order that the WWW-Authenticate headers appeared in the HTTP response.
Add a set of tests to exercise the HTTP authentication header parsing and the interop with credential helpers. Credential helpers will receive WWW-Authenticate information in credential requests.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
--- Documentation/git-credential.txt | 19 ++- credential.c | 3 + t/t5563-simple-http-auth.sh | 242 +++++++++++++++++++++++++++++++ 3 files changed, 263 insertions(+), 1 deletion(-)
Documentation/git-credential.txt +18 -1
@@ -113,7 +113,13 @@ separated by an `=` (equals) sign, followed by a newline. The key may contain any bytes except `=`, newline, or NUL. The value may contain any bytes except newline or NUL. -In both cases, all bytes are treated as-is (i.e., there is no quoting, +Attributes with keys that end with C-style array brackets `[]` can have +multiple values. Each instance of a multi-valued attribute forms an +ordered list of values - the order of the repeated attributes defines +the order of the values. An empty multi-valued attribute (`key[]=\n`) +acts to clear any previous entries and reset the list. + +In all cases, all bytes are treated as-is (i.e., there is no quoting, and one cannot transmit a value with newline or NUL in it). The list of attributes is terminated by a blank line or end-of-file.
@@ -160,6 +166,17 @@ empty string. Components which are missing from the URL (e.g., there is no username in the example above) will be left unset. +`wwwauth[]`:: + + When an HTTP response is received by Git that includes one or more + 'WWW-Authenticate' authentication headers, these will be passed by Git + to credential helpers. ++ +Each 'WWW-Authenticate' header value is passed as a multi-valued +attribute 'wwwauth[]', where the order of the attributes is the same as +they appear in the HTTP response. This attribute is 'one-way' from Git +to pass additional information to credential helpers. + Unrecognised attributes are silently discarded. GIT
credential.c +3 -0
@@ -270,6 +270,9 @@ 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 ); +for (size_t i =0 ; i < c->wwwauth_headers.nr; i++) +credential_write_item (fp,"wwwauth[]" , c->wwwauth_headers.v[i], +0 ); } static int run_credential_helper(struct credential *c,
t/t5563-simple-http-auth.sh +242 -0
248 lines not on this page · raw message
Signature · 1 line
gitgitgadget
v11 cover Enhance credential helper protocol to include auth headers
In reply to Matthew John Cheetham via GitGitGadgetFollowing from my original RFC submission [0], this submission is considered ready for full review. This patch series is now based on top of current master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now separately submitted patches [1] to fix up the other credential helpers' behaviour.
In this patch series I update the existing credential helper design in order to allow for some new scenarios, and future evolution of auth methods that Git hosts may wish to provide. I outline the background, summary of changes and some challenges below.
Testing these new additions, I use a small CGI shell script that acts as a frontend to git-http-backend; simple authentication is configurable by files.
Background
==========
Git uses a variety of protocols [2]: local, Smart HTTP, Dumb HTTP, SSH, and Git. Here I focus on the Smart HTTP protocol, and attempt to enhance the authentication capabilities of this protocol to address limitations (see below).
The Smart HTTP protocol in Git supports a few different types of HTTP authentication - Basic and Digest (RFC 2617) [3], and Negotiate (RFC 2478) [4]. Git uses a extensible model where credential helpers can provide credentials for protocols [5]. Several helpers support alternatives such as OAuth authentication (RFC 6749) [6], but this is typically done as an extension. For example, a helper might use basic auth and set the password to an OAuth Bearer access token. Git uses standard input and output to communicate with credential helpers.
After a HTTP 401 response, Git would call a credential helper with the following over standard input:
protocol=https
host=example.com
And then a credential helper would return over standard output:
protocol=https
host=example.com
[email protected]
password=<BEARER-TOKEN>
Git then the following request to the remote, including the standard HTTP Authorization header (RFC 7235 Section 4.2) [7]:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: Basic base64([email protected]:<BEARER-TOKEN>)
Credential helpers are encouraged (see gitcredentials.txt) to return the minimum information necessary.
Limitations
===========
Because this credential model was built mostly for password based authentication systems, it's somewhat limited. In particular:
1. To generate valid credentials, additional information about the request (or indeed the requestee and their device) may be required. For example, OAuth is based around scopes. A scope, like "git.read", might be required to read data from the remote. However, the remote cannot tell the credential helper what scope is required for this request.
2. This system is not fully extensible. Each time a new type of authentication (like OAuth Bearer) is invented, Git needs updates before credential helpers can take advantage of it (or leverage a new capability in libcurl).
Goals
=====
* As a user with multiple federated cloud identities:
* Reach out to a remote and have my credential helper automatically prompt me for the correct identity.
* Allow credential helpers to differentiate between different authorities or authentication/authorization challenge types, even from the same DNS hostname (and without needing to use credential.useHttpPath).
* Leverage existing authentication systems built-in to many operating systems and devices to boost security and reduce reliance on passwords.
* As a Git host and/or cloud identity provider:
* Enforce security policies (like requiring two-factor authentication) dynamically.
* Allow integration with third party standard based identity providers in enterprises allowing customers to have a single plane of control for critical identities with access to source code.
Design Principles
=================
* Use the existing infrastructure. Git credential helpers are an already-working model.
* Follow widely-adopted time-proven open standards, avoid net new ideas in the authentication space.
* Minimize knowledge of authentication in Git; maintain modularity and extensibility.
Proposed Changes
================
1. Teach Git to read HTTP response headers, specifically the standard WWW-Authenticate (RFC 7235 Section 4.1) headers.
2. Teach Git to include extra information about HTTP responses that require authentication when calling credential helpers. Specifically the WWW-Authenticate header information.
Because the extra information forms an ordered list, and the existing credential helper I/O format only provides for simple key=value pairs, we introduce a new convention for transmitting an ordered list of values. Key names that are suffixed with a C-style array syntax should have values considered to form an order list, i.e. key[]=value, where the order of the key=value pairs in the stream specifies the order.
For the WWW-Authenticate header values we opt to use the key wwwauth[].
Handling the WWW-Authenticate header in detail
==============================================
RFC 6750 [8] envisions that OAuth Bearer resource servers would give responses that include WWW-Authenticate headers, for example:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
Specifically, a WWW-Authenticate header consists of a scheme and arbitrary attributes, depending on the scheme. This pattern enables generic OAuth or OpenID Connect [9] authorities. Note that it is possible to have several WWW-Authenticate challenges in a response.
First Git attempts to make a request, unauthenticated, which fails with a 401 response and includes WWW-Authenticate header(s).
Next, Git invokes a credential helper which may prompt the user. If the user approves, a credential helper can generate a token (or any auth challenge response) to be used for that request.
For example: with a remote that supports bearer tokens from an OpenID Connect [9] authority, a credential helper can use OpenID Connect's Discovery [10] and Dynamic Client Registration [11] to register a client and make a request with the correct permissions to access the remote. In this manner, a user can be dynamically sent to the right federated identity provider for a remote without any up-front configuration or manual processes.
Following from the principle of keeping authentication knowledge in Git to a minimum, we modify Git to add all WWW-Authenticate values to the credential helper call.
Git sends over standard input:
protocol=https
host=example.com
wwwauth[]=Bearer realm="login.example", scope="git.readwrite" wwwauth[]=Basic realm="login.example"
A credential helper that understands the extra wwwauth[n] property can decide on the "best" or correct authentication scheme, generate credentials for the request, and interact with the user.
The credential helper would then return over standard output:
protocol=https
host=example.com
path=foo.git
[email protected]
password=<BEARER-TOKEN>
Note that WWW-Authenticate supports multiple challenges, either in one header:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
or in multiple headers:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
These have equivalent meaning (RFC 2616 Section 4.2 [12]). To simplify the implementation, Git will not merge or split up any of these WWW-Authenticate headers, and instead pass each header line as one credential helper property. The credential helper is responsible for splitting, merging, and otherwise parsing these header values.
An alternative option to sending the header fields individually would be to merge the header values in to one key=value property, for example:
...
wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
Future work
===========
In the future we can further expand the protocol to allow credential helpers decide the best authentication scheme. Today credential helpers are still only expected to return a username/password pair to Git, meaning the other authentication schemes that may be offered still need challenge responses sent via a Basic Authorization header. The changes outlined above still permit helpers to select and configure an available authentication mode, but require the remote for example to unpack a bearer token from a basic challenge.
More careful consideration is required in the handling of custom authentication schemes which may not have a username, or may require arbitrary additional request header values be set.
For example imagine a new "FooBar" authentication scheme that is surfaced in the following response:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"
With support for arbitrary authentication schemes, Git would call credential helpers with the following over standard input:
protocol=https
host=example.com
wwwauth[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"
And then an enlightened credential helper could return over standard output:
protocol=https
host=example.com
authtype=FooBar
[email protected]
password=<FooBar credential>
header[]=X-FooBar: 12345
header[]=X-FooBar-Alt: ABCDEF
Git would be expected to attach this authorization header to the next request:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: FooBar <FooBar credential>
X-FooBar: 12345
X-FooBar-Alt: ABCDEF
Why not SSH?
============
There's nothing wrong with SSH. However, Git's Smart HTTP transport is widely used, often with OAuth Bearer tokens. Git's Smart HTTP transport sometimes requires less client setup than SSH transport, and works in environments when SSH ports may be blocked. As long as Git supports HTTP transport, it should support common and popular HTTP authentication methods.
References
==========
* [0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
https://lore.kernel.org/git/[email protected]/
* [1] [PATCH 0/3] Correct credential helper discrepancies handling input
https://lore.kernel.org/git/[email protected]/
* [2] Git on the Server - The Protocols
https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
* [3] HTTP Authentication: Basic and Digest Access Authentication
https://datatracker.ietf.org/doc/html/rfc2617
* [4] The Simple and Protected GSS-API Negotiation Mechanism
https://datatracker.ietf.org/doc/html/rfc2478
* [5] Git Credentials - Custom Helpers
https://git-scm.com/docs/gitcredentials#_custom_helpers
* [6] The OAuth 2.0 Authorization Framework
https://datatracker.ietf.org/doc/html/rfc6749
* [7] Hypertext Transfer Protocol (HTTP/1.1): Authentication
https://datatracker.ietf.org/doc/html/rfc7235
* [8] The OAuth 2.0 Authorization Framework: Bearer Token Usage
https://datatracker.ietf.org/doc/html/rfc6750
* [9] OpenID Connect Core 1.0
https://openid.net/specs/openid-connect-core-1_0.html
* [10] OpenID Connect Discovery 1.0
https://openid.net/specs/openid-connect-discovery-1_0.html
* [11] OpenID Connect Dynamic Client Registration 1.0
https://openid.net/specs/openid-connect-registration-1_0.html
* [12] Hypertext Transfer Protocol (HTTP/1.1)
https://datatracker.ietf.org/doc/html/rfc2616
Updates from RFC
================
* Submitted first three patches as separate submission:
https://lore.kernel.org/git/[email protected]/
* Various style fixes and updates to- and addition of comments.
* Drop the explicit integer index in new 'array' style credential helper attrbiutes ("key[n]=value" becomes just "key[]=value").
* Added test helper; a mini HTTP server, and several tests.
Updates in v3
=============
* Split final patch that added the test-http-server in to several, easier to review patches.
* Updated wording in git-credential.txt to clarify which side of the credential helper protocol is sending/receiving the new wwwauth and authtype attributes.
Updates in v4
=============
* Drop authentication scheme selection authtype attribute patches to greatly simplify the series; auth scheme selection is punted to a future series. This series still allows credential helpers to generate credentials and intelligently select correct identities for a given auth challenge.
Updates in v5
=============
* Libify parts of daemon.c and share implementation with test-http-server.
* Clarify test-http-server Git request regex pattern and auth logic comments.
* Use STD*_FILENO in place of 'magic' file descriptor numbers.
* Use strbuf_* functions in continuation header parsing.
* Use configuration file to configure auth for test-http-server rather than command-line arguments. Add ability to specify arbitrary extra headers that is useful for testing 'malformed' server responses.
* Use st_mult over unchecked multiplication in http.c curl callback functions.
* Fix some documentation line break issues.
* Reorder some commits to bring in the tests and test-http-server helper first and, then the WWW-Authentication changes, alongside tests to cover.
* Expose previously static strvec_push_nodup function.
* Merge the two timeout args for test-http-server (--timeout and --init-timeout) that were a hang-over from the original daemon.c but are no longer required here.
* Be more careful around continuation headers where they may be empty strings. Add more tests to cover these header types.
* Include standard trace2 tracing calls at start of test-http-server helper.
Updates in v6
=============
* Clarify the change to make logging optional in the check_dead_children() function during libification of daemon.c.
* Fix missing pointer dereference bugs identified in libification of child process handling functions for daemon.c.
* Add doc comments to child process handling function declarations in the daemon-utils.h header.
* Align function parameter names with variable names at callsites for libified daemon functions.
* Re-split out the test-http-server test helper commits in to smaller
patches: error response handling, request parsing, http-backend pass-through, simple authentication, arbitrary header support.
* Call out auth configuration file format for test-http-server test helper and supported options in commit messages, as well as a test to exercise and demonstrate these options.
* Permit auth.token and auth.challenge to appear in any order; create the struct auth_module just-in-time as options for that scheme are read. This simplifies the configuration authoring of the test-http-server test helper.
* Update tests to use auth.allowAnoymous in the patch that introduces the new test helper option.
* Drop the strvec_push_nodup() commit and update the implementation of HTTP request header line folding to use xstrdup and strvec_pop and _pushf.
* Use size_t instead of int in credential.c when iterating over the struct strvec credential members. Also drop the not required const and cast from the full_key definition and free.
* Replace in-tree test-credential-helper-reply.sh test cred helper script with the lib-credential-helper.sh reusable 'lib' test script and shell functions to configure the helper behaviour.
* Leverage sed over the while read $line loop in the test credential helper script.
Updates in v7
=============
* Address several whitespace and arg/param list alignment issues.
* Rethink the test-http-helper worker-mode error and result enum to be more simple and more informative to the nature of the error.
* Use uintmax_t to store the Content-Length of a request in the helper test-http-server. Maintain a bit flag to store if we received such a header.
* Return a "400 Bad Request" HTTP response if we fail to parse the request in the test-http-server.
* Add test case to cover request message parsing in test-http-server.
* Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in get_auth_module.
* Correctly free the split strbufs created in the header parsing loop in test-http-server.
* Avoid needless comparison > 0 for unsigned types.
* Always set optional outputs to NULL if not present in test helper config value handling.
* Remove an accidentally commented-out test cleanup line for one test case in t5556.
Updates in v8
=============
* Drop custom HTTP test helper tool in favour of using a CGI shell script and Apache; avoiding the need to implement an HTTP server.
* Avoid allocations in header reading callback unless we have a header we care about; act on the char* from libcurl directly rather than create a strbuf for each header.
* Drop st_mult overflow guarding function in curl callback functions; we're not allocating memory based on the resulting value and just adds to potential confusion in the future.
Updates in v9
=============
* Drop anoynmous auth tests as these cases are already covered by all other tests that perform HTTP interactions with a remote today.
* In the custom auth CGI script, avoid the empty-substitution in favour of testing explicitly for an empty string. Also simplify some other conditional expressions.
* Avoid an allocation on each wwwauth[] credential helper key-value pair write.
* Various style fixups.
Updates in v10
==============
* Style fixups.
* Only consider space (SP ' ') and horizontal tab (HTAB '\t') when detecting a header continuation line, as per the latest RFC on the matter.
* Update references to old HTTP specs and formal grammars of header fields in comments.
* Rewording of commit messages to remove confusing comment about the case sensitivity of header field names - this is not relevant with the current iteration of the header parsing code. Also update the message around libcurl header support to clarify that physical header lines are returned, but not 'logical' header lines.
* Reword struct credential member doc comment to clarify the purpose of header_is_last_match is for re-folding lines of the WWW-Authenticate header.
* Reintroduce helpful comments in tests to show the origin of the 'magic' base64 basic auth value.
* Use grep -F to ensure we don't do regex matching; avoid interpreting special characters. Remove erronous insensitive comparison flag.
Updates in v11
==============
* Delete custom-auth.valid and .challenge explicitly in test cleanup.
* Use tolower over strncasecmp in implementation of skip_iprefix_mem.
* Use skip_iprefix_mem to match "HTTP/" header lines.
Matthew John Cheetham (3):
t5563: add tests for basic and anoymous HTTP access
http: read HTTP WWW-Authenticate response headers
credential: add WWW-Authenticate header to cred requests
Documentation/git-credential.txt | 19 +- credential.c | 4 + credential.h | 16 ++ git-compat-util.h | 19 ++ http.c | 111 +++++++++++ t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 + t/lib-httpd/nph-custom-auth.sh | 39 ++++ t/t5563-simple-http-auth.sh | 325 +++++++++++++++++++++++++++++++ 9 files changed, 539 insertions(+), 1 deletion(-) create mode 100644 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v11
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v11
Pull-Request: https://github.com/gitgitgadget/git/pull/1352
Range-diff vs v10:
1: f3ccc53055a ! 1: f7b234c4038 t5563: add tests for basic and anoymous HTTP access
@@ t/t5563-simple-http-auth.sh (new)
+
+per_test_cleanup () {
+ rm -f *.cred &&
-+ rm -f "$HTTPD_ROOT_PATH"/custom-auth.*
++ rm -f "$HTTPD_ROOT_PATH"/custom-auth.valid \
++ "$HTTPD_ROOT_PATH"/custom-auth.challenge
+}
+
+test_expect_success 'setup repository' '
2: 703ac15222f ! 2: 3cca9ea0736 http: read HTTP WWW-Authenticate response headers
@@ git-compat-util.h: static inline int skip_iprefix(const char *str, const char *p
+ const char *prefix,
+ const char **out, size_t *outlen)
+{
-+ size_t prefix_len = strlen(prefix);
-+
-+ if (len < prefix_len)
-+ return 0;
-+
-+ if (!strncasecmp(buf, prefix, prefix_len)) {
-+ *out = buf + prefix_len;
-+ *outlen = len - prefix_len;
-+ return 1;
-+ }
-+
++ do {
++ if (!*prefix) {
++ *out = buf;
++ *outlen = len;
++ return 1;
++ }
++ } while (len-- > 0 && tolower(*buf++) == tolower(*prefix++));
+ return 0;
+}
+
@@ http.c: size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buff
+ * We only care about the last HTTP request response's headers so clear
+ * the existing array.
+ */
-+ if (!strncasecmp(ptr, "http/", 5))
++ if (skip_iprefix_mem(ptr, size, "http/", &val, &val_len))
+ strvec_clear(values);
+
+exit: 3: 186da54fd3b = 3: b774acf3896 credential: add WWW-Authenticate header to cred requests
Signature · 1 line
gitgitgadget
PATCH v11 1/3 t5563: add tests for basic and anoymous HTTP access
From: Matthew John Cheetham <[email protected]>
Add a test showing simple anoymous HTTP access to an unprotected repository, that results in no credential helper invocations. Also add a test demonstrating simple basic authentication with simple credential helper support.
Leverage a no-parsed headers (NPH) CGI script so that we can directly control the HTTP responses to simulate a multitude of good, bad and ugly remote server implementations around auth.
--- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 +++ t/lib-httpd/nph-custom-auth.sh | 39 ++++++++++++++++ t/t5563-simple-http-auth.sh | 83 ++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+) create mode 100644 t/lib-httpd/nph-custom-auth.sh create mode 100755 t/t5563-simple-http-auth.sh
t/lib-httpd.sh +1 -0
@@ -137,6 +137,7 @@ prepare_httpd() { install_script error-smart-http.sh install_script error.sh install_script apply-one-time-perl.sh + install_script nph-custom-auth.sh ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
t/lib-httpd/apache.conf +6 -0
@@ -135,6 +135,11 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_HTTP_EXPORT_ALL SetEnv GIT_PROTOCOL </LocationMatch> +<LocationMatch /custom_auth/> + SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} + SetEnv GIT_HTTP_EXPORT_ALL + CGIPassAuth on +</LocationMatch> ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/ ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/ ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
@@ -144,6 +149,7 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error_smart/ error-smart-http.sh/ ScriptAlias /error/ error.sh/ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1 +ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory>
t/lib-httpd/nph-custom-auth.sh new file +39 -0
@@ -0,0 +1,39 @@ +#!/bin/sh + +VALID_CREDS_FILE=custom-auth.valid +CHALLENGE_FILE=custom-auth.challenge + +# +# If $VALID_CREDS_FILE exists in $HTTPD_ROOT_PATH, consider each line as a valid +# credential for the current request. Each line in the file is considered a +# valid HTTP Authorization header value. For example: +# +# Basic YWxpY2U6c2VjcmV0LXBhc3N3ZA== +# +# If $CHALLENGE_FILE exists in $HTTPD_ROOT_PATH, output the contents as headers +# in a 401 response if no valid authentication credentials were included in the +# request. For example: +# +# WWW-Authenticate: Bearer authorize_uri="id.example.com" p=1 q=0 +# WWW-Authenticate: Basic realm="example.com" +# + +if test -n "$HTTP_AUTHORIZATION" && \ + grep -Fqsx "${HTTP_AUTHORIZATION}" "$VALID_CREDS_FILE" +then + # Note that although git-http-backend returns a status line, it + # does so using a CGI 'Status' header. Because this script is an + # No Parsed Headers (NPH) script, we must return a real HTTP + # status line. + # This is only a test script, so we don't bother to check for + # the actual status from git-http-backend and always return 200. + echo 'HTTP/1.1 200 OK' + exec "$GIT_EXEC_PATH"/git-http-backend +fi + +echo 'HTTP/1.1 401 Authorization Required' +if test -f "$CHALLENGE_FILE" +then + cat "$CHALLENGE_FILE" +fi +echo
t/t5563-simple-http-auth.sh new file +83 -0
83 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v11 2/3 http: read HTTP WWW-Authenticate response headers
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
Read and store the HTTP WWW-Authenticate response headers made for a particular request.
This will allow us to pass important authentication challenge information to credential helpers or others that would otherwise have been lost.
libcurl only provides us with the ability to read all headers recieved for a particular request, including any intermediate redirect requests or proxies. The lines returned by libcurl include HTTP status lines delinating any intermediate requests such as "HTTP/1.1 200". We use these lines to reset the strvec of WWW-Authenticate header values as we encounter them in order to only capture the final response headers.
The collection of all header values matching the WWW-Authenticate header is complicated by the fact that it is legal for header fields to be continued over multiple lines, but libcurl only gives us each physical line a time, not each logical header. This line folding feature is deprecated in RFC 7230 [1] but older servers may still emit them, so we need to handle them.
In the future [2] we may be able to leverage functions to read headers from libcurl itself, but as of today we must do this ourselves.
[1] https://www.rfc-editor.org/rfc/rfc7230#section-3.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
--- credential.c | 1 + credential.h | 16 +++++++ git-compat-util.h | 19 ++++++++ http.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+)
credential.c +1 -0
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)free (c->username);free (c->password);string_list_clear (&c->helpers,0 ); +strvec_clear (&c->wwwauth_headers);credential_init (c); }
credential.h +16 -0
@@ -2,6 +2,7 @@#define CREDENTIAL_H #include "string-list.h" +#include "strvec.h" /** * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,20 @@ struct credential {*/ struct string_list helpers; +/** +* A `strvec` of WWW-Authenticate header values. Each string +* is the value of a WWW-Authenticate header in an HTTP response, +* in the order they were received in the response. +*/ +struct strvec wwwauth_headers; + +/** +* Internal use only. Keeps track of if we previously matched against a +* WWW-Authenticate header line in order to re-fold future continuation +* lines into one value. +*/ + unsigned header_is_last_match:1; + unsigned approved:1, configured:1, quit:1,
@@ -130,6 +145,7 @@ struct credential {#define CREDENTIAL_INIT { \ .helpers = STRING_LIST_INIT_DUP, \ + .wwwauth_headers = STRVEC_INIT, \ }/* Initialize a credential structure, setting all fields to empty. */
git-compat-util.h +19 -0
@@ -1266,6 +1266,25 @@ static inline int skip_iprefix(const char *str, const char *prefix,return 0 ; } +/* +* Like skip_prefix_mem, but compare case-insensitively. Note that the +* comparison is done via tolower(), so it is strictly ASCII (no multi-byte +* characters or locale-specific conversions). +*/ +static inline int skip_iprefix_mem (const char *buf,size_t len, +const char *prefix, +const char **out,size_t *outlen) +{ +do { +if (!*prefix) { + *out = buf; + *outlen = len; +return 1 ; + } + }while (len-- >0 &&tolower (*buf++) ==tolower (*prefix++)); +return 0 ; +} +static inline int strtoul_ui (char const *s,int base,unsigned int *result) {unsigned long ul;
http.c +111 -0
123 lines not on this page · raw message
Signature · 1 line
gitgitgadget
PATCH v11 3/3 credential: add WWW-Authenticate header to cred requests
In reply to Matthew John Cheetham via GitGitGadgetFrom: Matthew John Cheetham <[email protected]>
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[] properties where the order that the repeated attributes appear in the conversation reflects the order that the WWW-Authenticate headers appeared in the HTTP response.
Add a set of tests to exercise the HTTP authentication header parsing and the interop with credential helpers. Credential helpers will receive WWW-Authenticate information in credential requests.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
--- Documentation/git-credential.txt | 19 ++- credential.c | 3 + t/t5563-simple-http-auth.sh | 242 +++++++++++++++++++++++++++++++ 3 files changed, 263 insertions(+), 1 deletion(-)
Documentation/git-credential.txt +18 -1
@@ -113,7 +113,13 @@ separated by an `=` (equals) sign, followed by a newline. The key may contain any bytes except `=`, newline, or NUL. The value may contain any bytes except newline or NUL. -In both cases, all bytes are treated as-is (i.e., there is no quoting, +Attributes with keys that end with C-style array brackets `[]` can have +multiple values. Each instance of a multi-valued attribute forms an +ordered list of values - the order of the repeated attributes defines +the order of the values. An empty multi-valued attribute (`key[]=\n`) +acts to clear any previous entries and reset the list. + +In all cases, all bytes are treated as-is (i.e., there is no quoting, and one cannot transmit a value with newline or NUL in it). The list of attributes is terminated by a blank line or end-of-file.
@@ -160,6 +166,17 @@ empty string. Components which are missing from the URL (e.g., there is no username in the example above) will be left unset. +`wwwauth[]`:: + + When an HTTP response is received by Git that includes one or more + 'WWW-Authenticate' authentication headers, these will be passed by Git + to credential helpers. ++ +Each 'WWW-Authenticate' header value is passed as a multi-valued +attribute 'wwwauth[]', where the order of the attributes is the same as +they appear in the HTTP response. This attribute is 'one-way' from Git +to pass additional information to credential helpers. + Unrecognised attributes are silently discarded. GIT
credential.c +3 -0
@@ -270,6 +270,9 @@ 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 ); +for (size_t i =0 ; i < c->wwwauth_headers.nr; i++) +credential_write_item (fp,"wwwauth[]" , c->wwwauth_headers.v[i], +0 ); } static int run_credential_helper(struct credential *c,
t/t5563-simple-http-auth.sh +242 -0
248 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Quoted 8 lines · Matthew John Cheetham via GitGitGadget
Updates in v11
==============* Delete custom-auth.valid and .challenge explicitly in test cleanup.
* Use tolower over strncasecmp in implementation of
skip_iprefix_mem.* Use
skip_iprefix_memto match "HTTP/" header lines.
Thanks, I looked over all three changes and the whole thing looks good to me. The first one isn't strictly necessary if we're not renaming the script, but I agree that it is probably worth being a bit more strict when deleting in $HTTPD_ROOT_PATH.
-Peff