| Message ID | f3ccc53055acf5d5c25d0ad3eed3867ea8670e55.1676586881.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
On Thu, Feb 16, 2023 at 10:34:39PM +0000, Matthew John Cheetham via GitGitGadget wrote: > 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: diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh index ccd5f3cf82..1eadfa4bbc 100644 --- a/t/lib-httpd.sh +++ b/t/lib-httpd.sh @@ -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" diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 2aac922376..0f9083dd6c 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -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> diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/custom-auth.sh similarity index 94% rename from t/lib-httpd/nph-custom-auth.sh rename to t/lib-httpd/custom-auth.sh index f5345e775e..8bf07e9398 100755 --- a/t/lib-httpd/nph-custom-auth.sh +++ b/t/lib-httpd/custom-auth.sh @@ -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. > --- > 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
On Thu, Feb 23, 2023 at 04:16:28AM -0500, Jeff King wrote: > 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: diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh index 64d2acd032..afdf388677 100755 --- a/t/t5563-simple-http-auth.sh +++ b/t/t5563-simple-http-auth.sh @@ -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
On 2023-02-23 01:16, Jeff King wrote: > On Thu, Feb 16, 2023 at 10:34:39PM +0000, Matthew John Cheetham via GitGitGadget wrote: > >> 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 :-) > I.e., this passes your test: > > diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh > index ccd5f3cf82..1eadfa4bbc 100644 > --- a/t/lib-httpd.sh > +++ b/t/lib-httpd.sh > @@ -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" > > diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf > index 2aac922376..0f9083dd6c 100644 > --- a/t/lib-httpd/apache.conf > +++ b/t/lib-httpd/apache.conf > @@ -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> > diff --git a/t/lib-httpd/nph-custom-auth.sh b/t/lib-httpd/custom-auth.sh > similarity index 94% > rename from t/lib-httpd/nph-custom-auth.sh > rename to t/lib-httpd/custom-auth.sh > index f5345e775e..8bf07e9398 100755 > --- a/t/lib-httpd/nph-custom-auth.sh > +++ b/t/lib-httpd/custom-auth.sh > @@ -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. > >> --- >> 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. :) Oh.. that's something I missed. I just added the executable bit by habit. Will remove to match the others in lib-httpd/. > The rest of it all looks quite nice to me. > > -Peff
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..f5345e775e4 --- /dev/null +++ b/t/lib-httpd/nph-custom-auth.sh @@ -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 diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh new file mode 100755 index 00000000000..40f1b381d1b --- /dev/null +++ b/t/t5563-simple-http-auth.sh @@ -0,0 +1,82 @@ +#!/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 + 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 using basic auth' ' + test_when_finished "per_test_cleanup" && + + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + # Basic base64(alice:secret-passwd) + 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