| Message ID | b8d3e81b5534148359c7e92807cf1e2795480ddf.1674252531.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 7739ad6745bac369a69e25be1fbbf9a1518fce7d |
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
On Fri, Jan 20, 2023 at 10:08:46PM +0000, Matthew John Cheetham via GitGitGadget wrote: > +struct auth_module { > + char *scheme; > + char *challenge_params; > + struct string_list *tokens; > +}; This is a really minor nit, but: why is "tokens" a pointer? It's always initialized, so you never need or want to test it for NULL. That would make this: > + if (create) { > + struct auth_module *mod = xmalloc(sizeof(struct auth_module)); > + mod->scheme = xstrdup(scheme); > + mod->challenge_params = NULL; > + ALLOC_ARRAY(mod->tokens, 1); > + string_list_init_dup(mod->tokens); simplify to: string_list_init_dup(&mod->tokens); and one does not have to wonder why we use ALLOC_ARRAY() there, but not when allocating the module itself. :) Likewise you could skip freeing it, but since the memory is held until program end anyway, that doesn't happen either way. Certainly what you have won't behave wrong; I'd consider this more like a coding style thing. > + cat >auth.config <<-EOF && > + [auth] > + challenge = no-params > + challenge = with-params:foo=\"bar\" p=1 > + challenge = with-params:foo=\"replaced\" q=1 > + > + token = no-explicit-challenge:valid-token > + token = no-explicit-challenge:also-valid > + token = reset-tokens:these-tokens > + token = reset-tokens:will-be-reset > + token = reset-tokens: > + token = reset-tokens:the-only-valid-one > + > + allowAnonymous = false > + EOF > + > + cat >OUT.expected <<-EOF && > + WWW-Authenticate: no-params > + WWW-Authenticate: with-params foo="replaced" q=1 > + WWW-Authenticate: no-explicit-challenge > + WWW-Authenticate: reset-tokens > + > + Error: 401 Unauthorized > + EOF OK, so I think now we are getting to the interesting part of what your custom http-server does compared to something like apache. And the answer so far is: custom WWW-Authenticate lines. I think we could do that with mod_headers pretty easily. But presumably we also want to check that we are getting the correct tokens, generate a 401, etc. I suspect this could all be done as a CGI wrapping git-http-backend. You can influence the HTTP response code by sending: Status: 401 Authorization Required WWW-Authenticate: whatever you want And likewise you can see what the client sends by putting something like this in apache.conf: SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1 and then reading $HTTP_AUTHORIZATION as you like. At that point, it feels like a simple shell or perl script could then decide whether to return a 401 or not (and if not, then just exec git-http-backend to do the rest). And the scripts would be simple enough that you could have individual scripts to implement various schemes, rather than implementing this configuration scheme. You can control which script is run based on the URL; see the way we match /broken_smart/, etc, in t/lib-httpd/apache.conf. -Peff
On Fri, Jan 20, 2023 at 10:08:46PM +0000, Matthew John Cheetham via GitGitGadget wrote: > +static int split_auth_param(const char *str, char **scheme, char **val) > +{ > + struct strbuf **p = strbuf_split_str(str, ':', 2); > + > + if (!p[0]) > + return -1; > + > + /* trim trailing ':' */ > + if (p[0]->len && p[0]->buf[p[0]->len - 1] == ':') > + strbuf_setlen(p[0], p[0]->len - 1); > + > + *scheme = strbuf_detach(p[0], NULL); > + *val = p[1] ? strbuf_detach(p[1], NULL) : NULL; > + > + strbuf_list_free(p); > + return 0; > +} Oh, I forgot one more Coverity-detected problem here when reviewing last night. The early "return -1" here leaks "p" (there are no strbufs in the resulting array, but strbuf_split_str() will still have allocated the array). It needs a call to strbuf_list_free(p) there. -Peff
On Thu, Jan 26, 2023 at 05:02:27AM -0500, Jeff King wrote: > I suspect this could all be done as a CGI wrapping git-http-backend. You > can influence the HTTP response code by sending: > > Status: 401 Authorization Required > WWW-Authenticate: whatever you want > > And likewise you can see what the client sends by putting something like > this in apache.conf: > > SetEnvIf Authorization "(.*)" HTTP_AUTHORIZATION=$1 > > and then reading $HTTP_AUTHORIZATION as you like. At that point, it > feels like a simple shell or perl script could then decide whether to > return a 401 or not (and if not, then just exec git-http-backend to do > the rest). And the scripts would be simple enough that you could have > individual scripts to implement various schemes, rather than > implementing this configuration scheme. You can control which script is > run based on the URL; see the way we match /broken_smart/, etc, in > t/lib-httpd/apache.conf. And here's a minimally worked-out example of that approach. It's on top of your patches so I could use your credential-helper infrastructure in the test, but the intent is that it would replace all of the test-tool server patches and be rolled into t5556 as appropriate. --- t/lib-httpd.sh | 1 + t/lib-httpd/apache.conf | 6 ++++++ t/lib-httpd/custom-auth.sh | 18 ++++++++++++++++ t/t5563-simple-http-auth.sh | 42 +++++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+) create mode 100644 t/lib-httpd/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 608949ea80..ab255bdbc5 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 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 0294739a77..4b2256363f 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/(.*) custom-auth.sh/$1 <Directory ${GIT_EXEC_PATH}> Options FollowSymlinks </Directory> diff --git a/t/lib-httpd/custom-auth.sh b/t/lib-httpd/custom-auth.sh new file mode 100644 index 0000000000..686895ee8c --- /dev/null +++ b/t/lib-httpd/custom-auth.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +# Our acceptable auth here is hard-coded, but we could +# read it from a file provided by individual tests, etc. +# +# base64("alice:secret-passwd") +USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== + +case "$HTTP_AUTHORIZATION" in +"Basic $USERPASS64") + exec "$GIT_EXEC_PATH"/git-http-backend + ;; +*) + echo 'Status: 401 Auth Required' + echo 'WWW-Authenticate: Basic realm="whatever"' + echo + ;; +esac diff --git a/t/t5563-simple-http-auth.sh b/t/t5563-simple-http-auth.sh new file mode 100755 index 0000000000..314f9217e6 --- /dev/null +++ b/t/t5563-simple-http-auth.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +test_description='test http auth header and credential helper interop' + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh +. "$TEST_DIRECTORY"/lib-credential-helper.sh + +start_httpd + +setup_credential_helper + +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 custom auth' ' + set_credential_reply get <<-EOF && + username=alice + password=secret-passwd + EOF + + git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote \ + "$HTTPD_URL/custom_auth/repo.git" && + + expect_credential_query get <<-EOF && + protocol=http + host=$HTTPD_DEST + wwwauth[]=Basic realm="whatever" + EOF + + expect_credential_query store <<-EOF + protocol=http + host=$HTTPD_DEST + username=alice + password=secret-passwd + EOF +' + +test_done
Jeff King <peff@peff.net> writes: >> I suspect this could all be done as a CGI wrapping git-http-backend. You >> can influence the HTTP response code by sending: > ... > And here's a minimally worked-out example of that approach. It's on top > of your patches so I could use your credential-helper infrastructure in > the test, but the intent is that it would replace all of the test-tool > server patches and be rolled into t5556 as appropriate. Thanks for helping Matthew's topic move forward. I very much like seeing apache used for tests in this sample approach, like all the other http tests we do with apache, instead of a custom server that we need to ensure that it mimics the real-world use cases and we have to maintain.
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c index 4191daf3c64..72c6cca7e5c 100644 --- a/t/helper/test-http-server.c +++ b/t/helper/test-http-server.c @@ -7,6 +7,7 @@ #include "version.h" #include "dir.h" #include "date.h" +#include "config.h" #define TR2_CAT "test-http-server" @@ -19,6 +20,7 @@ static const char test_http_auth_usage[] = " [--timeout=<n>] [--max-connections=<n>]\n" " [--reuseaddr] [--pid-file=<file>]\n" " [--listen=<host_or_ipaddr>]* [--port=<n>]\n" +" [--auth-config=<file>]\n" ; static unsigned int timeout; @@ -349,7 +351,7 @@ static int is_git_request(struct req *req) !regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0); } -static enum worker_result do__git(struct req *req) +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; @@ -366,10 +368,16 @@ static enum worker_result do__git(struct req *req) * 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. + * + * The important auth responses (401) we are handling prior to getting + * to this point. */ if (write(STDOUT_FILENO, 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"); @@ -388,10 +396,217 @@ static enum worker_result do__git(struct req *req) return !!res; } +enum auth_result { + /* No auth module matches the request. */ + AUTH_UNKNOWN = 0, + + /* Auth module denied the request. */ + AUTH_DENY = 1, + + /* Auth module successfully validated the request. */ + AUTH_ALLOW = 2, +}; + +struct auth_module { + char *scheme; + char *challenge_params; + struct string_list *tokens; +}; + +static int allow_anonymous; +static struct auth_module **auth_modules = NULL; +static size_t auth_modules_nr = 0; +static size_t auth_modules_alloc = 0; + +static struct auth_module *get_auth_module(const char *scheme, int create) +{ + struct auth_module *mod; + for (size_t i = 0; i < auth_modules_nr; i++) { + mod = auth_modules[i]; + if (!strcasecmp(mod->scheme, scheme)) + return mod; + } + + if (create) { + struct auth_module *mod = xmalloc(sizeof(struct auth_module)); + mod->scheme = xstrdup(scheme); + mod->challenge_params = NULL; + ALLOC_ARRAY(mod->tokens, 1); + string_list_init_dup(mod->tokens); + + ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc); + auth_modules[auth_modules_nr++] = mod; + + return mod; + } + + return NULL; +} + +static int is_authed(struct req *req, const char **user, enum worker_result *wr) +{ + enum auth_result result = AUTH_UNKNOWN; + struct string_list hdrs = STRING_LIST_INIT_NODUP; + struct auth_module *mod; + + struct string_list_item *hdr; + struct string_list_item *token; + const char *v; + struct strbuf **split = NULL; + int i; + char *challenge; + + /* + * Check all auth modules and try to validate the request. + * The first Authorization header that matches a known auth module + * scheme will be consulted to either approve or deny the request. + * If no module is found, or if there is no valid token, then 401 error. + * Otherwise, only permit the request if anonymous auth is enabled. + * It's atypical for user agents/clients to send multiple Authorization + * headers, but not explicitly forbidden or defined. + */ + for_each_string_list_item(hdr, &req->header_list) { + if (skip_iprefix(hdr->string, "Authorization: ", &v)) { + split = strbuf_split_str(v, ' ', 2); + if (split[0] && split[1]) { + /* trim trailing space ' ' */ + strbuf_rtrim(split[0]); + + mod = get_auth_module(split[0]->buf, 0); + if (mod) { + result = AUTH_DENY; + + for_each_string_list_item(token, mod->tokens) { + if (!strcmp(split[1]->buf, token->string)) { + result = AUTH_ALLOW; + break; + } + } + + strbuf_list_free(split); + goto done; + } + } + + strbuf_list_free(split); + } + } + +done: + switch (result) { + case AUTH_ALLOW: + trace2_printf("%s: auth '%s' ALLOW", TR2_CAT, mod->scheme); + *user = "VALID_TEST_USER"; + *wr = WR_OK; + break; + + case AUTH_DENY: + trace2_printf("%s: auth '%s' DENY", TR2_CAT, mod->scheme); + /* fall-through */ + + case AUTH_UNKNOWN: + if (result != AUTH_DENY && allow_anonymous) + break; + + for (i = 0; i < auth_modules_nr; i++) { + mod = auth_modules[i]; + if (mod->challenge_params) + challenge = xstrfmt("WWW-Authenticate: %s %s", + mod->scheme, + mod->challenge_params); + else + challenge = xstrfmt("WWW-Authenticate: %s", + mod->scheme); + string_list_append(&hdrs, challenge); + } + + *wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1, + &hdrs, *wr); + } + + string_list_clear(&hdrs, 0); + + return result == AUTH_ALLOW || + (result == AUTH_UNKNOWN && allow_anonymous); +} + +static int split_auth_param(const char *str, char **scheme, char **val) +{ + struct strbuf **p = strbuf_split_str(str, ':', 2); + + if (!p[0]) + return -1; + + /* trim trailing ':' */ + if (p[0]->len && p[0]->buf[p[0]->len - 1] == ':') + strbuf_setlen(p[0], p[0]->len - 1); + + *scheme = strbuf_detach(p[0], NULL); + *val = p[1] ? strbuf_detach(p[1], NULL) : NULL; + + strbuf_list_free(p); + return 0; +} + +static int read_auth_config(const char *name, const char *val, void *data) +{ + int ret = 0; + char *scheme = NULL; + char *token = NULL; + char *challenge = NULL; + struct auth_module *mod; + + if (!strcmp(name, "auth.challenge")) { + if (split_auth_param(val, &scheme, &challenge)) { + ret = error("invalid auth challenge '%s'", val); + goto cleanup; + } + + mod = get_auth_module(scheme, 1); + + /* Replace any existing challenge parameters */ + free(mod->challenge_params); + mod->challenge_params = challenge ? xstrdup(challenge) : NULL; + } else if (!strcmp(name, "auth.token")) { + if (split_auth_param(val, &scheme, &token)) { + ret = error("invalid auth token '%s'", val); + goto cleanup; + } + + mod = get_auth_module(scheme, 1); + + /* + * Append to set of valid tokens unless an empty token value + * is provided, then clear the existing list. + */ + if (token) + string_list_append(mod->tokens, token); + else + string_list_clear(mod->tokens, 1); + } else if (!strcmp(name, "auth.allowanonymous")) { + allow_anonymous = git_config_bool(name, val); + } else { + warning("unknown auth config '%s'", name); + } + +cleanup: + free(scheme); + free(token); + free(challenge); + + return ret; +} + static enum worker_result dispatch(struct req *req) { + enum worker_result wr = WR_OK; + const char *user = NULL; + + if (!is_authed(req, &user, &wr)) + return wr; + if (is_git_request(req)) - return do__git(req); + return do__git(req, user); return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL, WR_HANGUP); @@ -655,6 +870,19 @@ int cmd_main(int argc, const char **argv) pid_file = v; continue; } + if (skip_prefix(arg, "--auth-config=", &v)) { + if (!strlen(v)) { + error("invalid argument - missing file path"); + usage(test_http_auth_usage); + } + + if (git_config_from_file(read_auth_config, v, NULL)) { + error("failed to read auth config file '%s'", v); + usage(test_http_auth_usage); + } + + continue; + } fprintf(stderr, "error: unknown argument '%s'\n", arg); usage(test_http_auth_usage); diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh index c0a47ce342b..20fd9b09aef 100755 --- a/t/t5556-http-auth.sh +++ b/t/t5556-http-auth.sh @@ -101,6 +101,7 @@ per_test_cleanup () { stop_http_server && rm -f OUT.* && rm -f IN.* && + rm -f auth.config } test_expect_success 'http auth server request parsing' ' @@ -160,11 +161,51 @@ test_expect_success 'http auth server request parsing' ' test_cmp OUT.http400 OUT.actual ' +test_expect_success CURL 'http auth server auth config' ' + test_when_finished "per_test_cleanup" && + + cat >auth.config <<-EOF && + [auth] + challenge = no-params + challenge = with-params:foo=\"bar\" p=1 + challenge = with-params:foo=\"replaced\" q=1 + + token = no-explicit-challenge:valid-token + token = no-explicit-challenge:also-valid + token = reset-tokens:these-tokens + token = reset-tokens:will-be-reset + token = reset-tokens: + token = reset-tokens:the-only-valid-one + + allowAnonymous = false + EOF + + cat >OUT.expected <<-EOF && + WWW-Authenticate: no-params + WWW-Authenticate: with-params foo="replaced" q=1 + WWW-Authenticate: no-explicit-challenge + WWW-Authenticate: reset-tokens + + Error: 401 Unauthorized + EOF + + start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" && + + curl --include $ORIGIN_URL >OUT.curl && + tr -d "\r" <OUT.curl | sed -n "/WWW-Authenticate/,\$p" >OUT.actual && + + test_cmp OUT.expected OUT.actual +' test_expect_success 'http auth anonymous no challenge' ' test_when_finished "per_test_cleanup" && - start_http_server && + cat >auth.config <<-EOF && + [auth] + allowAnonymous = true + EOF + + start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" && # Attempt to read from a protected repository git ls-remote $ORIGIN_URL