| Message ID | d362f7016d34c4803adf42a88012997c66e0bde8.1675711789.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
On Mon, Feb 06 2023, Matthew John Cheetham via GitGitGadget wrote: > From: Matthew John Cheetham <mjcheetham@outlook.com> > > 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 <mjcheetham@outlook.com> > --- > 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 > > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh > index 608949ea80b..2c49569f675 100644 > --- a/t/lib-httpd.sh > +++ b/t/lib-httpd.sh > @@ -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" > > diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf > index 0294739a77a..76335cdb24d 100644 > --- a/t/lib-httpd/apache.conf > +++ b/t/lib-httpd/apache.conf > @@ -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> > diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh > new file mode 100755 > index 00000000000..8f851aebac4 > --- /dev/null > +++ b/t/lib-httpd/nph-custom-auth.sh > @@ -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== > +# > +# 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". > +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 > +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.. > + 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" > + if test "$cmd" = "get"; then Style: We usually use "\nthen", not "; then".
Matthew John Cheetham via GitGitGadget wrote: > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh > index 608949ea80b..2c49569f675 100644 > --- a/t/lib-httpd.sh > +++ b/t/lib-httpd.sh > @@ -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" > > diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf > index 0294739a77a..76335cdb24d 100644 > --- a/t/lib-httpd/apache.conf > +++ b/t/lib-httpd/apache.conf > @@ -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 This setup (redirecting '/custom_auth/' routes to the 'nph-custom-auth.sh' script) is nice and straightforward. > <Directory ${GIT_EXEC_PATH}> > Options FollowSymlinks > </Directory> > diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh > new file mode 100755 > index 00000000000..8f851aebac4 > --- /dev/null > +++ b/t/lib-httpd/nph-custom-auth.sh > @@ -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== > +# > +# 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. [1] https://lore.kernel.org/git/230206.86fsbi5y63.gmgdl@evledraar.gmail.com/ > +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. > +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. > +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 > + > + 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!
On Wed, Feb 08 2023, Victoria Dye wrote: > Matthew John Cheetham via GitGitGadget wrote: >> 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. *nod* > [...] >> +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. > > [1] https://lore.kernel.org/git/230206.86fsbi5y63.gmgdl@evledraar.gmail.com/ 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... > [...] >> +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. 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.
On 2023-02-09 03:19, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Feb 08 2023, Victoria Dye wrote: > >> Matthew John Cheetham via GitGitGadget wrote: >>> 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. > > *nod* > >> [...] >>> +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. >> >> [1] https://lore.kernel.org/git/230206.86fsbi5y63.gmgdl@evledraar.gmail.com/ > > 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... >> [...] >>> +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. > > 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. 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. > 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.
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index 608949ea80b..2c49569f675 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -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" diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 0294739a77a..76335cdb24d 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -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> diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/nph-custom-auth.sh new file mode 100755 index 00000000000..8f851aebac4 --- /dev/null +++ b/t/lib-httpd/nph-custom-auth.sh @@ -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== +# +# 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") +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 diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh new file mode 100755 index 00000000000..004eac5d1ed --- /dev/null +++ b/t/t5563-simple-http-auth.sh @@ -0,0 +1,86 @@ +#!/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 -p "$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 + cat $catfile + fi + EOF +' + +set_credential_reply() { + cat >"$TRASH_DIRECTORY/$1-reply.cred" +} + +expect_credential_query() { + cat >"$TRASH_DIRECTORY/$1-expect.cred" && + test_cmp "$TRASH_DIRECTORY/$1-expect.cred" \ + "$TRASH_DIRECTORY/$1-query.cred" +} + +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" +' + +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 + + expect_credential_query store <<-EOF + protocol=http + host=$HTTPD_DEST + username=alice + password=secret-passwd + EOF +' + +test_done