| Message ID | 0a0f4fd10c8b29f327c35dadc7b17881f22b253a.1670880984.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
Matthew John Cheetham via GitGitGadget wrote: > From: Matthew John Cheetham <mjcheetham@outlook.com> > > Teach the test-http-sever test helper to forward Git requests to the > `git-http-backend`. > > Introduce a new test script t5556-http-auth.sh that spins up the test > HTTP server and attempts an `ls-remote` on the served repository, > without any authentication. > > Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> > --- > t/helper/test-http-server.c | 56 +++++++++++++++++++ > t/t5556-http-auth.sh | 105 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 161 insertions(+) > create mode 100755 t/t5556-http-auth.sh > > diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c > index 7bde678e264..9f1d6b58067 100644 > --- a/t/helper/test-http-server.c > +++ b/t/helper/test-http-server.c > @@ -305,8 +305,64 @@ done: > return result; > } > > +static int is_git_request(struct req *req) > +{ > + static regex_t *smart_http_regex; > + static int initialized; > + > + if (!initialized) { > + smart_http_regex = xmalloc(sizeof(*smart_http_regex)); > + if (regcomp(smart_http_regex, "^/(HEAD|info/refs|" > + "objects/info/[^/]+|git-(upload|receive)-pack)$", > + REG_EXTENDED)) { Could you explain the reasoning behind this regex (e.g., in a comment)? What sorts of valid/invalid requests does it represent? Is that the full set of requests that are "valid" to Git, or is it a test-specific subset? > + warning("could not compile smart HTTP regex"); > + smart_http_regex = NULL; > + } > + initialized = 1; > + } > + > + return smart_http_regex && > + !regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0); > +} > + > +static enum worker_result do__git(struct req *req, const char *user) > +{ > + const char *ok = "HTTP/1.1 200 OK\r\n"; > + struct child_process cp = CHILD_PROCESS_INIT; > + int res; > + > + if (write(1, ok, strlen(ok)) < 0) > + return error(_("could not send '%s'"), ok); Is it correct to hardcode the response status to '200 OK'? Even when 'http-backend' exits with an error? > + > + if (user) > + strvec_pushf(&cp.env, "REMOTE_USER=%s", user); I'm guessing that 'user' isn't used until a later patch? I think it might be better to not introduce that arg at all until it's needed (it'll put the usage of 'user' in context with how its value is determined), rather than hardcode it to 'NULL' for now. > + > + strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method); > + strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", > + req->uri_path.buf); > + strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1"); > + if (req->query_args.len) > + strvec_pushf(&cp.env, "QUERY_STRING=%s", > + req->query_args.buf); > + if (req->content_type) > + strvec_pushf(&cp.env, "CONTENT_TYPE=%s", > + req->content_type); > + if (req->content_length >= 0) > + strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIdMAX, > + (intmax_t)req->content_length); > + cp.git_cmd = 1; > + strvec_push(&cp.args, "http-backend"); > + res = run_command(&cp); I'm not super familiar with 'http-backend' but as long as it 1) uses the content passed into the environment to parse the request, and 2) writes the response to stdout, I think this is right. > + close(1); > + close(0); > + return !!res; > +} > + > static enum worker_result dispatch(struct req *req) > { > + if (is_git_request(req)) > + return do__git(req, NULL); > + > return send_http_error(1, 501, "Not Implemented", -1, NULL, > WR_OK | WR_HANGUP); > } > diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh > new file mode 100755 > index 00000000000..78da151f122 > --- /dev/null > +++ b/t/t5556-http-auth.sh > @@ -0,0 +1,105 @@ > +#!/bin/sh > + > +test_description='test http auth header and credential helper interop' > + > +. ./test-lib.sh > + > +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT > + > +# Setup a repository > +# > +REPO_DIR="$(pwd)"/repo nit: '$TEST_OUTPUT_DIRECTORY' instead of '$(pwd)' is more consistent with what I see in other tests. Also, if you're creating a repo in its own subdirectory ('repo'), you can set 'TEST_NO_CREATE_REPO=1' before importing './test-lib' to avoid creating a repo at the root level of the test output dir - it can help avoid potential weird/unexpected behavior as a result of being in a repo inside of another repo. > + > +# Setup some lookback URLs where test-http-server will be listening. > +# We will spawn it directly inside the repo directory, so we avoid > +# any need to configure directory mappings etc - we only serve this > +# repository from the root '/' of the server. > +# > +HOST_PORT=127.0.0.1:$GIT_TEST_HTTP_PROTOCOL_PORT > +ORIGIN_URL=http://$HOST_PORT/ > + > +# The pid-file is created by test-http-server when it starts. > +# The server will shutdown if/when we delete it (this is easier than > +# killing it by PID). > +# > +PID_FILE="$(pwd)"/pid-file.pid > +SERVER_LOG="$(pwd)"/OUT.server.log > + > +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH > + > +test_expect_success 'setup repos' ' > + test_create_repo "$REPO_DIR" && > + git -C "$REPO_DIR" branch -M main > +' > + > +stop_http_server () { > + if ! test -f "$PID_FILE" > + then > + return 0 > + fi > + # > + # The server will shutdown automatically when we delete the pid-file. > + # > + rm -f "$PID_FILE" > + # > + # Give it a few seconds to shutdown (mainly to completely release the > + # port before the next test start another instance and it attempts to > + # bind to it). > + # > + for k in 0 1 2 3 4 > + do > + if grep -q "Starting graceful shutdown" "$SERVER_LOG" > + then > + return 0 > + fi > + sleep 1 > + done > + > + echo "stop_http_server: timeout waiting for server shutdown" > + return 1 > +} > + > +start_http_server () { > + # > + # Launch our server into the background in repo_dir. > + # > + ( > + cd "$REPO_DIR" > + test-http-server --verbose \ > + --listen=127.0.0.1 \ > + --port=$GIT_TEST_HTTP_PROTOCOL_PORT \ > + --reuseaddr \ > + --pid-file="$PID_FILE" \ > + "$@" \ > + 2>"$SERVER_LOG" & > + ) > + # > + # Give it a few seconds to get started. > + # > + for k in 0 1 2 3 4 > + do > + if test -f "$PID_FILE" > + then > + return 0 > + fi > + sleep 1 > + done > + > + echo "start_http_server: timeout waiting for server startup" > + return 1 > +} These start/stop functions look good to me! > + > +per_test_cleanup () { > + stop_http_server && > + rm -f OUT.* > +} > + > +test_expect_success 'http auth anonymous no challenge' ' > + test_when_finished "per_test_cleanup" && > + start_http_server --allow-anonymous && The '--allow-anonymous' option isn't added until patch 7 [1], so the test will fail in this patch. I think the easiest way to solve that is to remove it here (although I think it's fine to leave the title "anonymous no challenge", though), then add it in patch 7. [1] https://lore.kernel.org/git/794256754c1f7d32e438dfb19a05444d423989aa.1670880984.git.gitgitgadget@gmail.com/ > + > + # Attempt to read from a protected repository > + git ls-remote $ORIGIN_URL > +' > + > +test_done
On 2022-12-14 15:20, Victoria Dye wrote: > Matthew John Cheetham via GitGitGadget wrote: >> From: Matthew John Cheetham <mjcheetham@outlook.com> >> >> Teach the test-http-sever test helper to forward Git requests to the >> `git-http-backend`. >> >> Introduce a new test script t5556-http-auth.sh that spins up the test >> HTTP server and attempts an `ls-remote` on the served repository, >> without any authentication. >> >> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> >> --- >> t/helper/test-http-server.c | 56 +++++++++++++++++++ >> t/t5556-http-auth.sh | 105 ++++++++++++++++++++++++++++++++++++ >> 2 files changed, 161 insertions(+) >> create mode 100755 t/t5556-http-auth.sh >> >> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c >> index 7bde678e264..9f1d6b58067 100644 >> --- a/t/helper/test-http-server.c >> +++ b/t/helper/test-http-server.c >> @@ -305,8 +305,64 @@ done: >> return result; >> } >> >> +static int is_git_request(struct req *req) >> +{ >> + static regex_t *smart_http_regex; >> + static int initialized; >> + >> + if (!initialized) { >> + smart_http_regex = xmalloc(sizeof(*smart_http_regex)); >> + if (regcomp(smart_http_regex, "^/(HEAD|info/refs|" >> + "objects/info/[^/]+|git-(upload|receive)-pack)$", >> + REG_EXTENDED)) { > > Could you explain the reasoning behind this regex (e.g., in a comment)? What > sorts of valid/invalid requests does it represent? Is that the full set of > requests that are "valid" to Git, or is it a test-specific subset? Explanatory comment will be added in next iteration. These are the valid Git endpoints for the dumb and smart HTTP protocols as specified in the tech docs. >> + warning("could not compile smart HTTP regex"); >> + smart_http_regex = NULL; >> + } >> + initialized = 1; >> + } >> + >> + return smart_http_regex && >> + !regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0); >> +} >> + >> +static enum worker_result do__git(struct req *req, const char *user) >> +{ >> + const char *ok = "HTTP/1.1 200 OK\r\n"; >> + struct child_process cp = CHILD_PROCESS_INIT; >> + int res; >> + >> + if (write(1, ok, strlen(ok)) < 0) >> + return error(_("could not send '%s'"), ok); > > Is it correct to hardcode the response status to '200 OK'? Even when > 'http-backend' exits with an error? We always respond with a 200 OK response even if the http-backend process exits with an error. This helper is intended only to be used to exercise the HTTP auth handling in the Git client, and specifically around authentication (not handled by http-backend). If we wanted to respond with a more 'valid' HTTP response status then we'd need to buffer the output of http-backend, wait for and grok the exit status of the process, then write the HTTP status line followed by the http-backend output. This is outside of the scope of this test helper's use at time of writing. Important auth responses (401) we are handling prior to getting to this point. The above will also be summarised in a comment on the next roll. >> + >> + if (user) >> + strvec_pushf(&cp.env, "REMOTE_USER=%s", user); > > I'm guessing that 'user' isn't used until a later patch? I think it might be > better to not introduce that arg at all until it's needed (it'll put the > usage of 'user' in context with how its value is determined), rather than > hardcode it to 'NULL' for now. Good point! >> + >> + strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method); >> + strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", >> + req->uri_path.buf); >> + strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1"); >> + if (req->query_args.len) >> + strvec_pushf(&cp.env, "QUERY_STRING=%s", >> + req->query_args.buf); >> + if (req->content_type) >> + strvec_pushf(&cp.env, "CONTENT_TYPE=%s", >> + req->content_type); >> + if (req->content_length >= 0) >> + strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIdMAX, >> + (intmax_t)req->content_length); >> + cp.git_cmd = 1; >> + strvec_push(&cp.args, "http-backend"); >> + res = run_command(&cp); > > I'm not super familiar with 'http-backend' but as long as it 1) uses the > content passed into the environment to parse the request, and 2) writes the > response to stdout, I think this is right. > >> + close(1); >> + close(0); >> + return !!res; >> +} >> + >> static enum worker_result dispatch(struct req *req) >> { >> + if (is_git_request(req)) >> + return do__git(req, NULL); >> + >> return send_http_error(1, 501, "Not Implemented", -1, NULL, >> WR_OK | WR_HANGUP); >> } >> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh >> new file mode 100755 >> index 00000000000..78da151f122 >> --- /dev/null >> +++ b/t/t5556-http-auth.sh >> @@ -0,0 +1,105 @@ >> +#!/bin/sh >> + >> +test_description='test http auth header and credential helper interop' >> + >> +. ./test-lib.sh >> + >> +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT >> + >> +# Setup a repository >> +# >> +REPO_DIR="$(pwd)"/repo > > nit: '$TEST_OUTPUT_DIRECTORY' instead of '$(pwd)' is more consistent with > what I see in other tests. I don't see this? In fact I see more usages of `$(pwd)` than your suggestion. > Also, if you're creating a repo in its own subdirectory ('repo'), you can > set 'TEST_NO_CREATE_REPO=1' before importing './test-lib' to avoid creating > a repo at the root level of the test output dir - it can help avoid > potential weird/unexpected behavior as a result of being in a repo inside of > another repo. However.. after setting `TEST_NO_CREATE_REPO=1` I was getting CI failures around a missing PWD, so my next iteration uses the `$TRASH_DIRECTORY` variable explicitly in paths instead :-) >> + >> +# Setup some lookback URLs where test-http-server will be listening. >> +# We will spawn it directly inside the repo directory, so we avoid >> +# any need to configure directory mappings etc - we only serve this >> +# repository from the root '/' of the server. >> +# >> +HOST_PORT=127.0.0.1:$GIT_TEST_HTTP_PROTOCOL_PORT >> +ORIGIN_URL=http://$HOST_PORT/ >> + >> +# The pid-file is created by test-http-server when it starts. >> +# The server will shutdown if/when we delete it (this is easier than >> +# killing it by PID). >> +# >> +PID_FILE="$(pwd)"/pid-file.pid >> +SERVER_LOG="$(pwd)"/OUT.server.log >> + >> +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH >> + >> +test_expect_success 'setup repos' ' >> + test_create_repo "$REPO_DIR" && >> + git -C "$REPO_DIR" branch -M main >> +' >> + >> +stop_http_server () { >> + if ! test -f "$PID_FILE" >> + then >> + return 0 >> + fi >> + # >> + # The server will shutdown automatically when we delete the pid-file. >> + # >> + rm -f "$PID_FILE" >> + # >> + # Give it a few seconds to shutdown (mainly to completely release the >> + # port before the next test start another instance and it attempts to >> + # bind to it). >> + # >> + for k in 0 1 2 3 4 >> + do >> + if grep -q "Starting graceful shutdown" "$SERVER_LOG" >> + then >> + return 0 >> + fi >> + sleep 1 >> + done >> + >> + echo "stop_http_server: timeout waiting for server shutdown" >> + return 1 >> +} >> + >> +start_http_server () { >> + # >> + # Launch our server into the background in repo_dir. >> + # >> + ( >> + cd "$REPO_DIR" >> + test-http-server --verbose \ >> + --listen=127.0.0.1 \ >> + --port=$GIT_TEST_HTTP_PROTOCOL_PORT \ >> + --reuseaddr \ >> + --pid-file="$PID_FILE" \ >> + "$@" \ >> + 2>"$SERVER_LOG" & >> + ) >> + # >> + # Give it a few seconds to get started. >> + # >> + for k in 0 1 2 3 4 >> + do >> + if test -f "$PID_FILE" >> + then >> + return 0 >> + fi >> + sleep 1 >> + done >> + >> + echo "start_http_server: timeout waiting for server startup" >> + return 1 >> +} > > These start/stop functions look good to me! > >> + >> +per_test_cleanup () { >> + stop_http_server && >> + rm -f OUT.* >> +} >> + >> +test_expect_success 'http auth anonymous no challenge' ' >> + test_when_finished "per_test_cleanup" && >> + start_http_server --allow-anonymous && > > The '--allow-anonymous' option isn't added until patch 7 [1], so the test > will fail in this patch. I think the easiest way to solve that is to remove > it here (although I think it's fine to leave the title "anonymous no > challenge", though), then add it in patch 7. > > [1] https://lore.kernel.org/git/794256754c1f7d32e438dfb19a05444d423989aa.1670880984.git.gitgitgadget@gmail.com/ Good catch! Will fix. >> + >> + # Attempt to read from a protected repository >> + git ls-remote $ORIGIN_URL >> +' >> + >> +test_done > Thanks, Matthew
Matthew John Cheetham wrote: > > On 2022-12-14 15:20, Victoria Dye wrote: >> nit: '$TEST_OUTPUT_DIRECTORY' instead of '$(pwd)' is more consistent with >> what I see in other tests. > > I don't see this? In fact I see more usages of `$(pwd)` than your suggestion. To be honest, I'm not sure how I missed this. '$(pwd)' *is* quite common in the tests, although it does seem to be used mostly in individual tests rather than file-level variables (although that's not universally true, e.g. using it to set 'CURR_DIR' in 't9400-diff-highlight.sh'). So, contrary to my earlier comment, this seems best left up to (your) personal preference than any concrete rule. > >> Also, if you're creating a repo in its own subdirectory ('repo'), you can >> set 'TEST_NO_CREATE_REPO=1' before importing './test-lib' to avoid creating >> a repo at the root level of the test output dir - it can help avoid >> potential weird/unexpected behavior as a result of being in a repo inside of >> another repo. > > However.. after setting `TEST_NO_CREATE_REPO=1` I was getting CI failures > around a missing PWD, so my next iteration uses the `$TRASH_DIRECTORY` variable > explicitly in paths instead :-) You're right, I was completely misreading the purpose of 'TEST_OUTPUT_DIRECTORY' in 'test-lib.sh': > if test -z "$TEST_OUTPUT_DIRECTORY" > then > # Similarly, override this to store the test-results subdir > # elsewhere > TEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY > fi "the test-results subdir" != "the test working directory". As you pointed out, '$TRASH_DIRECTORY' would be the variable to use here.
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c index 7bde678e264..9f1d6b58067 100644 --- a/t/helper/test-http-server.c +++ b/t/helper/test-http-server.c @@ -305,8 +305,64 @@ done: return result; } +static int is_git_request(struct req *req) +{ + static regex_t *smart_http_regex; + static int initialized; + + if (!initialized) { + smart_http_regex = xmalloc(sizeof(*smart_http_regex)); + if (regcomp(smart_http_regex, "^/(HEAD|info/refs|" + "objects/info/[^/]+|git-(upload|receive)-pack)$", + REG_EXTENDED)) { + warning("could not compile smart HTTP regex"); + smart_http_regex = NULL; + } + initialized = 1; + } + + return smart_http_regex && + !regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0); +} + +static enum worker_result do__git(struct req *req, const char *user) +{ + const char *ok = "HTTP/1.1 200 OK\r\n"; + struct child_process cp = CHILD_PROCESS_INIT; + int res; + + if (write(1, ok, strlen(ok)) < 0) + return error(_("could not send '%s'"), ok); + + if (user) + strvec_pushf(&cp.env, "REMOTE_USER=%s", user); + + strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method); + strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", + req->uri_path.buf); + strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1"); + if (req->query_args.len) + strvec_pushf(&cp.env, "QUERY_STRING=%s", + req->query_args.buf); + if (req->content_type) + strvec_pushf(&cp.env, "CONTENT_TYPE=%s", + req->content_type); + if (req->content_length >= 0) + strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIdMAX, + (intmax_t)req->content_length); + cp.git_cmd = 1; + strvec_push(&cp.args, "http-backend"); + res = run_command(&cp); + close(1); + close(0); + return !!res; +} + static enum worker_result dispatch(struct req *req) { + if (is_git_request(req)) + return do__git(req, NULL); + return send_http_error(1, 501, "Not Implemented", -1, NULL, WR_OK | WR_HANGUP); } diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh new file mode 100755 index 00000000000..78da151f122 --- /dev/null +++ b/t/t5556-http-auth.sh @@ -0,0 +1,105 @@ +#!/bin/sh + +test_description='test http auth header and credential helper interop' + +. ./test-lib.sh + +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT + +# Setup a repository +# +REPO_DIR="$(pwd)"/repo + +# Setup some lookback URLs where test-http-server will be listening. +# We will spawn it directly inside the repo directory, so we avoid +# any need to configure directory mappings etc - we only serve this +# repository from the root '/' of the server. +# +HOST_PORT=127.0.0.1:$GIT_TEST_HTTP_PROTOCOL_PORT +ORIGIN_URL=http://$HOST_PORT/ + +# The pid-file is created by test-http-server when it starts. +# The server will shutdown if/when we delete it (this is easier than +# killing it by PID). +# +PID_FILE="$(pwd)"/pid-file.pid +SERVER_LOG="$(pwd)"/OUT.server.log + +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH + +test_expect_success 'setup repos' ' + test_create_repo "$REPO_DIR" && + git -C "$REPO_DIR" branch -M main +' + +stop_http_server () { + if ! test -f "$PID_FILE" + then + return 0 + fi + # + # The server will shutdown automatically when we delete the pid-file. + # + rm -f "$PID_FILE" + # + # Give it a few seconds to shutdown (mainly to completely release the + # port before the next test start another instance and it attempts to + # bind to it). + # + for k in 0 1 2 3 4 + do + if grep -q "Starting graceful shutdown" "$SERVER_LOG" + then + return 0 + fi + sleep 1 + done + + echo "stop_http_server: timeout waiting for server shutdown" + return 1 +} + +start_http_server () { + # + # Launch our server into the background in repo_dir. + # + ( + cd "$REPO_DIR" + test-http-server --verbose \ + --listen=127.0.0.1 \ + --port=$GIT_TEST_HTTP_PROTOCOL_PORT \ + --reuseaddr \ + --pid-file="$PID_FILE" \ + "$@" \ + 2>"$SERVER_LOG" & + ) + # + # Give it a few seconds to get started. + # + for k in 0 1 2 3 4 + do + if test -f "$PID_FILE" + then + return 0 + fi + sleep 1 + done + + echo "start_http_server: timeout waiting for server startup" + return 1 +} + +per_test_cleanup () { + stop_http_server && + rm -f OUT.* +} + +test_expect_success 'http auth anonymous no challenge' ' + test_when_finished "per_test_cleanup" && + start_http_server --allow-anonymous && + + # Attempt to read from a protected repository + git ls-remote $ORIGIN_URL +' + +test_done