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@xxxxxxxxxxxxxxxxxxx/
>
> 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.
