* Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()
From: Junio C Hamano @ 2023-01-18 6:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, René Scharfe, Eric Sunshine
In-Reply-To: <patch-v4-08.19-1fe25bc6981-20230117T151202Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> We were leaking both the "struct strbuf" in prune_worktrees(), as well
> as the "path" we got from should_prune_worktree(). Since these were
> the only two uses of the "struct string_list" let's change it to a
> "DUP" and push these to it with "string_list_append_nodup()".
> ...
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index d34d77f8934..ba8d929d189 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -1,6 +1,8 @@
> #!/bin/sh
>
> test_description='git branch display tests'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-terminal.sh
This is wrong, isn't it?
t3203 uses --points-at, which populates filter.points_at by calling
parse_opt_object_name(). Various members of the ref-filter
structure is never freed (and there is no API helper function in
ref-filter subsystem).
Other tests that use --points-at (e.g. t6302 and t7004) are not
marked with "passes_sanitize_leak", and this one shouldn't be,
either.
With the following squashed in, the branch seems to pass, but I am
not sure which is lessor of the two evils. From the point of view
of the code maintenance, UNLEAK() to mark this singleton variable is
far cleaner to deal with than selectively running the leak checks
with the "passes_sanitize_leak" mechanism (which always feels like a
losing whack-a-mole hack).
builtin/branch.c | 1 +
1 file changed, 1 insertion(+)
diff --git c/builtin/branch.c w/builtin/branch.c
index f63fd45edb..4fe7757670 100644
--- c/builtin/branch.c
+++ w/builtin/branch.c
@@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (filter.abbrev == -1)
filter.abbrev = DEFAULT_ABBREV;
filter.ignore_case = icase;
+ UNLEAK(filter);
finalize_colopts(&colopts, -1);
if (filter.verbose) {
^ permalink raw reply related
* [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees
From: Carlo Marcelo Arenas Belón @ 2023-01-18 6:15 UTC (permalink / raw)
To: git
Cc: pclouds, Carlo Marcelo Arenas Belón, Jinwook Jeong,
Rubén Justo, Eric Sunshine
In-Reply-To: <20230116172824.93218-1-carenas@gmail.com>
Commands `git switch -C` and `git checkout -B` neglect to check whether
the provided branch is already checked out in some other worktree, as
shown by the following example:
$ git worktree list
.../foo beefb00f [main]
$ git worktree add ../other
Preparing worktree (new branch 'other')
HEAD is now at beefb00f first
$ cd ../other
$ git switch -C main
Switched to and reset branch 'main'
$ git worktree list
.../foo beefb00f [main]
.../other beefb00f [main]
Fix this problem by teaching `git switch -C` and `git checkout -B` to
check whether the branch in question is already checked out elsewhere
by expanding on the existing checks that are being used by their non
force variants.
As reflected on the tests, this will change the behaviour of those
commands when they are invoked in a worktree that has that requested
branch checked out, as that matches the logic used by branch, is safer
(assuming both commands are user facing) and can be overriden with an
existing flag.
Reported-by: Jinwook Jeong <vustthat@gmail.com>
Helped-by: Rubén Justo <rjusto@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v1
* A much better commit message
* Changes to the tests as suggested by Eric
* Changes to the logic as suggested by Rubén
builtin/checkout.c | 24 +++++++++++++++++-------
t/t2400-worktree-add.sh | 18 ++++++++++++++++--
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..58a956392b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
}
static int checkout_branch(struct checkout_opts *opts,
- struct branch_info *new_branch_info)
+ struct branch_info *new_branch_info,
+ struct branch_info *check_branch_info)
{
if (opts->pathspec.nr)
die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
if (!opts->can_switch_when_in_progress)
die_if_some_operation_in_progress();
- if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
- !opts->ignore_other_worktrees) {
+ if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
int flag;
char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
- if (head_ref &&
- (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
- die_if_checked_out(new_branch_info->path, 1);
+ if (opts->new_branch_force || (head_ref &&
+ (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))))
+ die_if_checked_out(check_branch_info->path, 1);
free(head_ref);
}
@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
struct branch_info *new_branch_info)
{
int parseopt_flags = 0;
+ struct branch_info check_branch_info = { 0 };
opts->overwrite_ignore = 1;
opts->prefix = prefix;
@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
!opts->new_branch;
int n = parse_branchname_arg(argc, argv, dwim_ok,
new_branch_info, opts, &rev);
+ check_branch_info = *new_branch_info;
argv += n;
argc -= n;
} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
opts, &rev,
opts->from_treeish);
+ check_branch_info = *new_branch_info;
if (!opts->source_tree)
die(_("reference is not a tree: %s"), opts->from_treeish);
+ } else if (opts->new_branch) {
+ struct object_id rev;
+
+ if (!get_oid_mb(opts->new_branch, &rev))
+ setup_new_branch_info_and_source_tree(&check_branch_info,
+ opts, &rev,
+ opts->new_branch);
}
if (argc) {
@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
if (opts->patch_mode || opts->pathspec.nr)
return checkout_paths(opts, new_branch_info);
else
- return checkout_branch(opts, new_branch_info);
+ return checkout_branch(opts, new_branch_info, &check_branch_info);
}
int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..a66f9bb30c 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -121,7 +121,10 @@ test_expect_success '"add" worktree creating new branch' '
test_expect_success 'die the same branch is already checked out' '
(
cd here &&
- test_must_fail git checkout newmain
+ test_must_fail git checkout newmain &&
+ test_must_fail git checkout -B newmain &&
+ test_must_fail git switch newmain &&
+ test_must_fail git switch -C newmain
)
'
@@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' '
test_expect_success 'not die on re-checking out current branch' '
(
cd there &&
- git checkout newmain
+ git checkout newmain &&
+ git switch newmain
+ )
+'
+
+test_expect_success 'but die if using force without --ignore-other-worktrees' '
+ (
+ cd there &&
+ test_must_fail git checkout -B newmain &&
+ test_must_fail git switch -C newmain &&
+ git checkout --ignore-other-worktrees -B newmain &&
+ git switch --ignore-other-worktrees -C newmain
)
'
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply related
* Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees
From: Carlo Arenas @ 2023-01-18 5:44 UTC (permalink / raw)
To: Rubén Justo; +Cc: git, vustthat, sunshine, pclouds
In-Reply-To: <45cb1b38-1284-ddc9-a2d8-0a45f10abafb@gmail.com>
On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo <rjusto@gmail.com> wrote:
> On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:
>
> > @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> > if (!opts->can_switch_when_in_progress)
> > die_if_some_operation_in_progress();
> >
> > - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> > - !opts->ignore_other_worktrees) {
> > + if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {
> > int flag;
> > char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
> > if (head_ref &&
> > - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
> > - die_if_checked_out(new_branch_info->path, 1);
> > + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> > + die_if_checked_out(check_branch_info->path, 1);
>
> What we are doing here, if I understand it correctly, is dying if the
> branch is _not checked out in the current worktree_ and _checked out in
> any other worktree_. Which is OK for normal checkout/switch.
I think the exception was added to `checkout` only, where it is
definitely needed, but switch probably should be more strict as it is
not "plumbing" and as you pointed out there is already a UI option to
override its safety.
> But IMHO with "checkout -B" we have to die if the branch is checked out
> in any other worktree, regardless of whether or not it is checked out in
> the current working tree.
I have to admit, I thought about that too, but then avoided making a
change as checkout behaviour affects a lot of other places, but in
retrospect I think that in this case it might be worth the change of
behaviour, since it is connected with the bugfix.
Before, the operation was allowed and the logic never tried to prevent
it (which is why in my first look, I thought it might have been
intentional), but once Eric pointed out it was a bug, then the obvious
conclusion would be to prevent it with the extended logic, as you
pointed out.
> Perhaps the scenario where the user has the same branch checked out in
> multiple worktrees and tries to reset in one of them, is one of those
> where we could use the "if it hurts, don't do it". But we are providing
> the "--ignore-other-worktrees" modifier, so I think we should disallow
> the "-B" if the branch is checked out in any other worktree, and let
> the user use "--ignore-other-worktrees" if he wants to go wild.
v2 includes this and AFAIK nothing is broken yet.
Carlo
PS. As explained before, tightening `switch` might be a good idea, but
it is obviously punted for this change and will be addressed later.
^ permalink raw reply
* [PATCH v6 12/12] credential: add WWW-Authenticate header to cred requests
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Add the value of the WWW-Authenticate response header to credential
requests. Credential helpers that understand and support HTTP
authentication and authorization can use this standard header (RFC 2616
Section 14.47 [1]) to generate valid credentials.
WWW-Authenticate headers can contain information pertaining to the
authority, authentication mechanism, or extra parameters/scopes that are
required.
The current I/O format for credential helpers only allows for unique
names for properties/attributes, so in order to transmit multiple header
values (with a specific order) we introduce a new convention whereby a
C-style array syntax is used in the property name to denote multiple
ordered values for the same property.
In this case we send multiple `wwwauth[]` properties where the order
that the repeated attributes appear in the conversation reflects the
order that the WWW-Authenticate headers appeared in the HTTP response.
Add a set of tests to exercise the HTTP authentication header parsing
and the interop with credential helpers. Credential helpers will receive
WWW-Authenticate information in credential requests.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
Documentation/git-credential.txt | 19 ++-
credential.c | 11 ++
t/lib-credential-helper.sh | 27 ++++
t/t5556-http-auth.sh | 245 ++++++++++++++++++++++++++++++-
4 files changed, 300 insertions(+), 2 deletions(-)
create mode 100644 t/lib-credential-helper.sh
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..50759153ef1 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -113,7 +113,13 @@ separated by an `=` (equals) sign, followed by a newline.
The key may contain any bytes except `=`, newline, or NUL. The value may
contain any bytes except newline or NUL.
-In both cases, all bytes are treated as-is (i.e., there is no quoting,
+Attributes with keys that end with C-style array brackets `[]` can have
+multiple values. Each instance of a multi-valued attribute forms an
+ordered list of values - the order of the repeated attributes defines
+the order of the values. An empty multi-valued attribute (`key[]=\n`)
+acts to clear any previous entries and reset the list.
+
+In all cases, all bytes are treated as-is (i.e., there is no quoting,
and one cannot transmit a value with newline or NUL in it). The list of
attributes is terminated by a blank line or end-of-file.
@@ -160,6 +166,17 @@ empty string.
Components which are missing from the URL (e.g., there is no
username in the example above) will be left unset.
+`wwwauth[]`::
+
+ When an HTTP response is received by Git that includes one or more
+ 'WWW-Authenticate' authentication headers, these will be passed by Git
+ to credential helpers.
++
+Each 'WWW-Authenticate' header value is passed as a multi-valued
+attribute 'wwwauth[]', where the order of the attributes is the same as
+they appear in the HTTP response. This attribute is 'one-way' from Git
+to pass additional information to credential helpers.
+
Unrecognised attributes are silently discarded.
GIT
diff --git a/credential.c b/credential.c
index 897b4679333..9f39ebc3c7e 100644
--- a/credential.c
+++ b/credential.c
@@ -263,6 +263,16 @@ static void credential_write_item(FILE *fp, const char *key, const char *value,
fprintf(fp, "%s=%s\n", key, value);
}
+static void credential_write_strvec(FILE *fp, const char *key,
+ const struct strvec *vec)
+{
+ char *full_key = xstrfmt("%s[]", key);
+ for (size_t i = 0; i < vec->nr; i++) {
+ credential_write_item(fp, full_key, vec->v[i], 0);
+ }
+ free(full_key);
+}
+
void credential_write(const struct credential *c, FILE *fp)
{
credential_write_item(fp, "protocol", c->protocol, 1);
@@ -270,6 +280,7 @@ void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0);
credential_write_item(fp, "username", c->username, 0);
credential_write_item(fp, "password", c->password, 0);
+ credential_write_strvec(fp, "wwwauth", &c->wwwauth_headers);
}
static int run_credential_helper(struct credential *c,
diff --git a/t/lib-credential-helper.sh b/t/lib-credential-helper.sh
new file mode 100644
index 00000000000..8b0e4414234
--- /dev/null
+++ b/t/lib-credential-helper.sh
@@ -0,0 +1,27 @@
+setup_credential_helper() {
+ test_expect_success 'setup credential helper' '
+ CREDENTIAL_HELPER="$TRASH_DIRECTORY/credential-helper.sh" &&
+ export CREDENTIAL_HELPER &&
+ echo $CREDENTIAL_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"
+}
diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
index e36107ea95d..79122c611a1 100755
--- a/t/t5556-http-auth.sh
+++ b/t/t5556-http-auth.sh
@@ -4,6 +4,7 @@ test_description='test http auth header and credential helper interop'
TEST_NO_CREATE_REPO=1
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-credential-helper.sh
test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
@@ -33,6 +34,8 @@ test_expect_success 'setup repos' '
git -C "$REPO_DIR" branch -M main
'
+setup_credential_helper
+
stop_http_server () {
if ! test -f "$PID_FILE"
then
@@ -92,7 +95,9 @@ start_http_server () {
per_test_cleanup () {
stop_http_server &&
- rm -f OUT.*
+ rm -f OUT.* &&
+ rm -f *.cred &&
+ rm -f auth.config
}
test_expect_success CURL 'http auth server auth config' '
@@ -152,4 +157,242 @@ test_expect_success 'http auth anonymous no challenge' '
git ls-remote $ORIGIN_URL
'
+test_expect_success 'http auth www-auth headers to credential helper basic valid' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=basic realm="example.com"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_expect_success 'http auth www-auth headers to credential helper ignore case valid' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ extraHeader = wWw-aUtHeNtIcAtE: bEaRer auThoRiTy=\"id.example.com\"
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=basic realm="example.com"
+ wwwauth[]=bEaRer auThoRiTy="id.example.com"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_expect_success 'http auth www-auth headers to credential helper continuation hdr' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = "bearer:authority=\"id.example.com\"\\n q=1\\n \\t p=0"
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
+ wwwauth[]=basic realm="example.com"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_expect_success 'http auth www-auth headers to credential helper empty continuation hdrs' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ extraheader = "WWW-Authenticate:"
+ extraheader = " "
+ extraheader = " bearer authority=\"id.example.com\""
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=basic realm="example.com"
+ wwwauth[]=bearer authority="id.example.com"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_expect_success 'http auth www-auth headers to credential helper custom schemes' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = "foobar:alg=test widget=1"
+ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+
+ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=foobar alg=test widget=1
+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
+ wwwauth[]=basic realm="example.com"
+ EOF
+
+ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
+'
+
+test_expect_success 'http auth www-auth headers to credential helper invalid' '
+ test_when_finished "per_test_cleanup" &&
+ # base64("alice:secret-passwd")
+ USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== &&
+ export USERPASS64 &&
+
+ cat >auth.config <<-EOF &&
+ [auth]
+ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
+ challenge = basic:realm=\"example.com\"
+ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
+ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=invalid-passwd
+ EOF
+
+ test_must_fail git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
+
+ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
+ wwwauth[]=basic realm="example.com"
+ EOF
+
+ expect_credential_query erase <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=invalid-passwd
+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
+ wwwauth[]=basic realm="example.com"
+ EOF
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 11/12] http: read HTTP WWW-Authenticate response headers
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Read and store the HTTP WWW-Authenticate response headers made for
a particular request.
This will allow us to pass important authentication challenge
information to credential helpers or others that would otherwise have
been lost.
According to RFC2616 Section 4.2 [1], header field names are not
case-sensitive meaning when collecting multiple values for the same
field name, we can just use the case of the first observed instance of
each field name and no normalisation is required.
libcurl only provides us with the ability to read all headers recieved
for a particular request, including any intermediate redirect requests
or proxies. The lines returned by libcurl include HTTP status lines
delinating any intermediate requests such as "HTTP/1.1 200". We use
these lines to reset the strvec of WWW-Authenticate header values as
we encounter them in order to only capture the final response headers.
The collection of all header values matching the WWW-Authenticate
header is complicated by the fact that it is legal for header fields to
be continued over multiple lines, but libcurl only gives us one line at
a time.
In the future [2] we may be able to leverage functions to read headers
from libcurl itself, but as of today we must do this ourselves.
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
credential.c | 1 +
credential.h | 15 +++++++++
http.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 110 insertions(+)
diff --git a/credential.c b/credential.c
index f6389a50684..897b4679333 100644
--- a/credential.c
+++ b/credential.c
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)
free(c->username);
free(c->password);
string_list_clear(&c->helpers, 0);
+ strvec_clear(&c->wwwauth_headers);
credential_init(c);
}
diff --git a/credential.h b/credential.h
index f430e77fea4..6f2e5bc610b 100644
--- a/credential.h
+++ b/credential.h
@@ -2,6 +2,7 @@
#define CREDENTIAL_H
#include "string-list.h"
+#include "strvec.h"
/**
* The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,19 @@ struct credential {
*/
struct string_list helpers;
+ /**
+ * A `strvec` of WWW-Authenticate header values. Each string
+ * is the value of a WWW-Authenticate header in an HTTP response,
+ * in the order they were received in the response.
+ */
+ struct strvec wwwauth_headers;
+
+ /**
+ * Internal use only. Used to keep track of split header fields
+ * in order to fold multiple lines into one value.
+ */
+ unsigned header_is_last_match:1;
+
unsigned approved:1,
configured:1,
quit:1,
@@ -130,6 +144,7 @@ struct credential {
#define CREDENTIAL_INIT { \
.helpers = STRING_LIST_INIT_DUP, \
+ .wwwauth_headers = STRVEC_INIT, \
}
/* Initialize a credential structure, setting all fields to empty. */
diff --git a/http.c b/http.c
index a2a80318bb2..595c93bc7a3 100644
--- a/http.c
+++ b/http.c
@@ -183,6 +183,98 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
return nmemb;
}
+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
+{
+ size_t size = st_mult(eltsize, nmemb);
+ struct strvec *values = &http_auth.wwwauth_headers;
+ struct strbuf buf = STRBUF_INIT;
+ const char *val;
+
+ /*
+ * Header lines may not come NULL-terminated from libcurl so we must
+ * limit all scans to the maximum length of the header line, or leverage
+ * strbufs for all operations.
+ *
+ * In addition, it is possible that header values can be split over
+ * multiple lines as per RFC 2616 (even though this has since been
+ * deprecated in RFC 7230). A continuation header field value is
+ * identified as starting with a space or horizontal tab.
+ *
+ * The formal definition of a header field as given in RFC 2616 is:
+ *
+ * message-header = field-name ":" [ field-value ]
+ * field-name = token
+ * field-value = *( field-content | LWS )
+ * field-content = <the OCTETs making up the field-value
+ * and consisting of either *TEXT or combinations
+ * of token, separators, and quoted-string>
+ */
+
+ strbuf_add(&buf, ptr, size);
+
+ /* Strip the CRLF that should be present at the end of each field */
+ strbuf_trim_trailing_newline(&buf);
+
+ /* Start of a new WWW-Authenticate header */
+ if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
+ while (isspace(*val))
+ val++;
+
+ strvec_push(values, val);
+ http_auth.header_is_last_match = 1;
+ goto exit;
+ }
+
+ /*
+ * This line could be a continuation of the previously matched header
+ * field. If this is the case then we should append this value to the
+ * end of the previously consumed value.
+ * Continuation lines start with at least one whitespace, maybe more,
+ * so we should collapse these down to a single SP (valid per the spec).
+ */
+ if (http_auth.header_is_last_match && isspace(*buf.buf)) {
+ /* Trim leading whitespace from this continuation hdr line. */
+ strbuf_ltrim(&buf);
+
+ /*
+ * At this point we should always have at least one existing
+ * value, even if it is empty. Do not bother appending the new
+ * value if this continuation header is itself empty.
+ */
+ if (!values->nr) {
+ BUG("should have at least one existing header value");
+ } else if (buf.len) {
+ char *prev = xstrdup(values->v[values->nr - 1]);
+
+ /* Join two non-empty values with a single space. */
+ const char *const sp = *prev ? " " : "";
+
+ strvec_pop(values);
+ strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
+ free(prev);
+ }
+
+ goto exit;
+ }
+
+ /* This is the start of a new header we don't care about */
+ http_auth.header_is_last_match = 0;
+
+ /*
+ * If this is a HTTP status line and not a header field, this signals
+ * a different HTTP response. libcurl writes all the output of all
+ * response headers of all responses, including redirects.
+ * We only care about the last HTTP request response's headers so clear
+ * the existing array.
+ */
+ if (istarts_with(buf.buf, "http/"))
+ strvec_clear(values);
+
+exit:
+ strbuf_release(&buf);
+ return size;
+}
+
size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
{
return nmemb;
@@ -1864,6 +1956,8 @@ static int http_request(const char *url,
fwrite_buffer);
}
+ curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
+
accept_language = http_get_accept_language_header();
if (accept_language)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 07/12] test-http-server: pass Git requests to http-backend
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
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 | 71 ++++++++++++++++++++++++
t/t5556-http-auth.sh | 107 ++++++++++++++++++++++++++++++++++++
2 files changed, 178 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 36f4a54fe6d..ae17c738259 100644
--- a/t/helper/test-http-server.c
+++ b/t/helper/test-http-server.c
@@ -291,8 +291,79 @@ 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));
+ /*
+ * This regular expression matches all dumb and smart HTTP
+ * requests that are currently in use, and defined in
+ * Documentation/gitprotocol-http.txt.
+ *
+ */
+ 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 *ok = "HTTP/1.1 200 OK\r\n";
+ struct child_process cp = CHILD_PROCESS_INIT;
+ int res;
+
+ /*
+ * Note that 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.
+ */
+ if (write(STDOUT_FILENO, ok, strlen(ok)) < 0)
+ return error(_("could not send '%s'"), ok);
+
+ 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(STDOUT_FILENO);
+ close(STDIN_FILENO);
+ return !!res;
+}
+
static enum worker_result dispatch(struct req *req)
{
+ if (is_git_request(req))
+ return do__git(req);
+
return send_http_error(STDOUT_FILENO, 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..ce1abffa6aa
--- /dev/null
+++ b/t/t5556-http-auth.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+
+test_description='test http auth header and credential helper interop'
+
+TEST_NO_CREATE_REPO=1
+. ./test-lib.sh
+
+test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
+
+# Setup a repository
+#
+REPO_DIR="$TRASH_DIRECTORY"/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="$TRASH_DIRECTORY"/pid-file.pid
+SERVER_LOG="$TRASH_DIRECTORY"/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 &&
+
+ # Attempt to read from a protected repository
+ git ls-remote $ORIGIN_URL
+'
+
+test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 10/12] http: replace unsafe size_t multiplication with st_mult
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Replace direct multiplication of two size_t parameters in curl response
stream handling callback functions with `st_mult` to guard against
overflows.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
http.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index 8a5ba3f4776..a2a80318bb2 100644
--- a/http.c
+++ b/http.c
@@ -146,7 +146,7 @@ static int http_schannel_use_ssl_cainfo;
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
- size_t size = eltsize * nmemb;
+ size_t size = st_mult(eltsize, nmemb);
struct buffer *buffer = buffer_;
if (size > buffer->buf.len - buffer->posn)
@@ -176,7 +176,7 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
- size_t size = eltsize * nmemb;
+ size_t size = st_mult(eltsize, nmemb);
struct strbuf *buffer = buffer_;
strbuf_add(buffer, ptr, size);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 09/12] test-http-server: add sending of arbitrary headers
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Add the ability to send arbitrary headers in HTTP responses from the
test-http-server. This is useful when we want to test 'malformed'
response message handling.
Add the following option to the server auth config file:
[auth]
extraHeader = [<value>]*
Each `auth.extraHeader` value will be appended to the response headers
verbatim.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
t/helper/test-http-server.c | 6 ++++++
t/t5556-http-auth.sh | 7 +++++++
2 files changed, 13 insertions(+)
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
index 691fbfb51d6..cbaee4fc0f4 100644
--- a/t/helper/test-http-server.c
+++ b/t/helper/test-http-server.c
@@ -388,6 +388,7 @@ 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 strvec extra_headers = STRVEC_INIT;
static struct auth_module *get_auth_module(const char *scheme, int create)
{
@@ -489,6 +490,9 @@ done:
string_list_append(&hdrs, challenge);
}
+ for (i = 0; i < extra_headers.nr; i++)
+ string_list_append(&hdrs, extra_headers.v[i]);
+
*wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1,
&hdrs, *wr);
}
@@ -557,6 +561,8 @@ static int read_auth_config(const char *name, const char *val, void *data)
string_list_clear(mod->tokens, 1);
} else if (!strcmp(name, "auth.allowanonymous")) {
allow_anonymous = git_config_bool(name, val);
+ } else if (!strcmp(name, "auth.extraheader")) {
+ strvec_push(&extra_headers, val);
} else {
warning("unknown auth config '%s'", name);
}
diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
index cb5562a41bf..e36107ea95d 100755
--- a/t/t5556-http-auth.sh
+++ b/t/t5556-http-auth.sh
@@ -112,6 +112,10 @@ test_expect_success CURL 'http auth server auth config' '
token = reset-tokens:the-only-valid-one
allowAnonymous = false
+
+ extraHeader = X-Extra-Header: abc
+ extraHeader = X-Extra-Header: 123
+ extraHeader = X-Another: header\twith\twhitespace!
EOF
cat >OUT.expected <<-EOF &&
@@ -119,6 +123,9 @@ test_expect_success CURL 'http auth server auth config' '
WWW-Authenticate: with-params foo="replaced" q=1
WWW-Authenticate: no-explicit-challenge
WWW-Authenticate: reset-tokens
+ X-Extra-Header: abc
+ X-Extra-Header: 123
+ X-Another: header with whitespace!
Error: 401 Unauthorized
EOF
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 08/12] test-http-server: add simple authentication
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Add simple authentication to the test-http-server test helper.
Authentication schemes and sets of valid tokens can be specified via
a configuration file (in the normal gitconfig file format).
Incoming requests are compared against the set of valid schemes and
tokens and only approved if a matching token is found, or if no auth
was provided and anonymous auth is enabled.
Configuration for auth includes a simple set of three options:
[auth]
challenge = <scheme>[:<challenge_params>]
token = <scheme>:[<token>]*
allowAnonymous = <bool>
`auth.challenge` allows you define what authentication schemes, and
optional challenge parameters the server should use. Scheme names are
unique and subsequently specified challenge parameters in the config
file will replace previously specified ones.
`auth.token` allows you to define the set of value token values for an
authentication scheme. This is a multi-var and each entry in the
config file will append to the set of valid tokens for that scheme.
Specifying an empty token value will clear the list of tokens so far for
that scheme, i.e. `token = <scheme>:`.
`auth.allowAnonymous` controls whether or not unauthenticated requests
(those without any `Authorization` headers) should succeed or not, and
trigger a 401 Unauthorized response.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
t/helper/test-http-server.c | 233 +++++++++++++++++++++++++++++++++++-
t/t5556-http-auth.sh | 43 ++++++-
2 files changed, 273 insertions(+), 3 deletions(-)
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
index ae17c738259..691fbfb51d6 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;
@@ -317,7 +319,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;
@@ -334,10 +336,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);
@@ -359,10 +367,218 @@ 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)
+{
+ int i;
+ struct auth_module *mod;
+ for (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;
+ CALLOC_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]) continue;
+
+ /* trim trailing space ' ' */
+ strbuf_setlen(split[0], split[0]->len - 1);
+
+ 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;
+ }
+ }
+
+ goto done;
+ }
+ }
+ }
+
+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);
+ }
+
+ strbuf_list_free(split);
+ 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 > 0 && p[0]->buf[p[0]->len - 1] == ':')
+ strbuf_setlen(p[0], p[0]->len - 1);
+
+ *scheme = strbuf_detach(p[0], NULL);
+
+ if (p[1])
+ *val = strbuf_detach(p[1], 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 = NULL;
+
+ 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_OK | WR_HANGUP);
@@ -621,6 +837,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 ce1abffa6aa..cb5562a41bf 100755
--- a/t/t5556-http-auth.sh
+++ b/t/t5556-http-auth.sh
@@ -95,10 +95,51 @@ per_test_cleanup () {
rm -f OUT.*
}
+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
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 06/12] test-http-server: add HTTP request parsing
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Add ability to parse HTTP requests to the test-http-server test helper.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
t/helper/test-http-server.c | 175 +++++++++++++++++++++++++++++++++++-
1 file changed, 173 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
index 6cdac223a55..36f4a54fe6d 100644
--- a/t/helper/test-http-server.c
+++ b/t/helper/test-http-server.c
@@ -83,6 +83,42 @@ enum worker_result {
WR_HANGUP = 1<<1,
};
+/*
+ * Fields from a parsed HTTP request.
+ */
+struct req {
+ struct strbuf start_line;
+
+ const char *method;
+ const char *http_version;
+
+ struct strbuf uri_path;
+ struct strbuf query_args;
+
+ struct string_list header_list;
+ const char *content_type;
+ ssize_t content_length;
+};
+
+#define REQ__INIT { \
+ .start_line = STRBUF_INIT, \
+ .uri_path = STRBUF_INIT, \
+ .query_args = STRBUF_INIT, \
+ .header_list = STRING_LIST_INIT_NODUP, \
+ .content_type = NULL, \
+ .content_length = -1 \
+ }
+
+static void req__release(struct req *req)
+{
+ strbuf_release(&req->start_line);
+
+ strbuf_release(&req->uri_path);
+ strbuf_release(&req->query_args);
+
+ string_list_clear(&req->header_list, 0);
+}
+
static enum worker_result send_http_error(
int fd,
int http_code, const char *http_code_name,
@@ -134,8 +170,136 @@ done:
return wr;
}
+/*
+ * Read the HTTP request up to the start of the optional message-body.
+ * We do this byte-by-byte because we have keep-alive turned on and
+ * cannot rely on an EOF.
+ *
+ * https://tools.ietf.org/html/rfc7230
+ *
+ * We cannot call die() here because our caller needs to properly
+ * respond to the client and/or close the socket before this
+ * child exits so that the client doesn't get a connection reset
+ * by peer error.
+ */
+static enum worker_result req__read(struct req *req, int fd)
+{
+ struct strbuf h = STRBUF_INIT;
+ struct string_list start_line_fields = STRING_LIST_INIT_DUP;
+ int nr_start_line_fields;
+ const char *uri_target;
+ const char *query;
+ char *hp;
+ const char *hv;
+
+ enum worker_result result = WR_OK;
+
+ /*
+ * Read line 0 of the request and split it into component parts:
+ *
+ * <method> SP <uri-target> SP <HTTP-version> CRLF
+ *
+ */
+ if (strbuf_getwholeline_fd(&req->start_line, fd, '\n') == EOF) {
+ result = WR_OK | WR_HANGUP;
+ goto done;
+ }
+
+ strbuf_trim_trailing_newline(&req->start_line);
+
+ nr_start_line_fields = string_list_split(&start_line_fields,
+ req->start_line.buf,
+ ' ', -1);
+ if (nr_start_line_fields != 3) {
+ logerror("could not parse request start-line '%s'",
+ req->start_line.buf);
+ result = WR_IO_ERROR;
+ goto done;
+ }
+
+ req->method = xstrdup(start_line_fields.items[0].string);
+ req->http_version = xstrdup(start_line_fields.items[2].string);
+
+ uri_target = start_line_fields.items[1].string;
+
+ if (strcmp(req->http_version, "HTTP/1.1")) {
+ logerror("unsupported version '%s' (expecting HTTP/1.1)",
+ req->http_version);
+ result = WR_IO_ERROR;
+ goto done;
+ }
+
+ query = strchr(uri_target, '?');
+
+ if (query) {
+ strbuf_add(&req->uri_path, uri_target, (query - uri_target));
+ strbuf_trim_trailing_dir_sep(&req->uri_path);
+ strbuf_addstr(&req->query_args, query + 1);
+ } else {
+ strbuf_addstr(&req->uri_path, uri_target);
+ strbuf_trim_trailing_dir_sep(&req->uri_path);
+ }
+
+ /*
+ * Read the set of HTTP headers into a string-list.
+ */
+ while (1) {
+ if (strbuf_getwholeline_fd(&h, fd, '\n') == EOF)
+ goto done;
+ strbuf_trim_trailing_newline(&h);
+
+ if (!h.len)
+ goto done; /* a blank line ends the header */
+
+ hp = strbuf_detach(&h, NULL);
+ string_list_append(&req->header_list, hp);
+
+ /* also store common request headers as struct req members */
+ if (skip_prefix(hp, "Content-Type: ", &hv)) {
+ req->content_type = hv;
+ } else if (skip_prefix(hp, "Content-Length: ", &hv)) {
+ req->content_length = strtol(hv, &hp, 10);
+ }
+ }
+
+ /*
+ * We do not attempt to read the <message-body>, if it exists.
+ * We let our caller read/chunk it in as appropriate.
+ */
+
+done:
+ string_list_clear(&start_line_fields, 0);
+
+ /*
+ * This is useful for debugging the request, but very noisy.
+ */
+ if (trace2_is_enabled()) {
+ struct string_list_item *item;
+ trace2_printf("%s: %s", TR2_CAT, req->start_line.buf);
+ trace2_printf("%s: hver: %s", TR2_CAT, req->http_version);
+ trace2_printf("%s: hmth: %s", TR2_CAT, req->method);
+ trace2_printf("%s: path: %s", TR2_CAT, req->uri_path.buf);
+ trace2_printf("%s: qury: %s", TR2_CAT, req->query_args.buf);
+ if (req->content_length >= 0)
+ trace2_printf("%s: clen: %d", TR2_CAT, req->content_length);
+ if (req->content_type)
+ trace2_printf("%s: ctyp: %s", TR2_CAT, req->content_type);
+ for_each_string_list_item(item, &req->header_list)
+ trace2_printf("%s: hdrs: %s", TR2_CAT, item->string);
+ }
+
+ return result;
+}
+
+static enum worker_result dispatch(struct req *req)
+{
+ return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL,
+ WR_OK | WR_HANGUP);
+}
+
static enum worker_result worker(void)
{
+ struct req req = REQ__INIT;
char *client_addr = getenv("REMOTE_ADDR");
char *client_port = getenv("REMOTE_PORT");
enum worker_result wr = WR_OK;
@@ -146,9 +310,16 @@ static enum worker_result worker(void)
set_keep_alive(0, logerror);
while (1) {
- wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1,
- NULL, WR_OK | WR_HANGUP);
+ req__release(&req);
+
+ alarm(timeout);
+ wr = req__read(&req, 0);
+ alarm(0);
+
+ if (wr != WR_OK)
+ break;
+ wr = dispatch(&req);
if (wr != WR_OK)
break;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 04/12] test-http-server: add stub HTTP server test helper
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Introduce a mini HTTP server helper that in the future will be enhanced
to provide a frontend for the git-http-backend, with support for
arbitrary authentication schemes.
Right now, test-http-server is a pared-down copy of the git-daemon that
always returns a 501 Not Implemented response to all callers.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
Makefile | 1 +
contrib/buildsystems/CMakeLists.txt | 11 +-
t/helper/.gitignore | 1 +
t/helper/test-http-server.c | 385 ++++++++++++++++++++++++++++
4 files changed, 396 insertions(+), 2 deletions(-)
create mode 100644 t/helper/test-http-server.c
diff --git a/Makefile b/Makefile
index 2654094dbb5..3cd61c792ac 100644
--- a/Makefile
+++ b/Makefile
@@ -865,6 +865,7 @@ TEST_BUILTINS_OBJS += test-xml-encode.o
# Do not add more tests here unless they have extra dependencies. Add
# them in TEST_BUILTINS_OBJS above.
TEST_PROGRAMS_NEED_X += test-fake-ssh
+TEST_PROGRAMS_NEED_X += test-http-server
TEST_PROGRAMS_NEED_X += test-tool
TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 2f6e0197ffa..5d949dcb16c 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -961,6 +961,9 @@ if(BUILD_TESTING)
add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)
target_link_libraries(test-fake-ssh common-main)
+add_executable(test-http-server ${CMAKE_SOURCE_DIR}/t/helper/test-http-server.c)
+target_link_libraries(test-http-server common-main)
+
#reftable-tests
parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
@@ -980,6 +983,11 @@ if(MSVC)
PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/helper)
set_target_properties(test-fake-ssh test-tool
PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/helper)
+
+ set_target_properties(test-http-server
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/helper)
+ set_target_properties(test-http-server
+ PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/helper)
endif()
#wrapper scripts
@@ -987,8 +995,7 @@ set(wrapper_scripts
git git-upload-pack git-receive-pack git-upload-archive git-shell git-remote-ext scalar)
set(wrapper_test_scripts
- test-fake-ssh test-tool)
-
+ test-http-server test-fake-ssh test-tool)
foreach(script ${wrapper_scripts})
file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME)
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 8c2ddcce95f..9aa9c752997 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,2 +1,3 @@
/test-tool
/test-fake-ssh
+/test-http-server
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
new file mode 100644
index 00000000000..11071b1dd89
--- /dev/null
+++ b/t/helper/test-http-server.c
@@ -0,0 +1,385 @@
+#include "daemon-utils.h"
+#include "config.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "string-list.h"
+#include "trace2.h"
+#include "version.h"
+#include "dir.h"
+#include "date.h"
+
+#define TR2_CAT "test-http-server"
+
+static const char *pid_file;
+static int verbose;
+static int reuseaddr;
+
+static const char test_http_auth_usage[] =
+"http-server [--verbose]\n"
+" [--timeout=<n>] [--max-connections=<n>]\n"
+" [--reuseaddr] [--pid-file=<file>]\n"
+" [--listen=<host_or_ipaddr>]* [--port=<n>]\n"
+;
+
+static unsigned int timeout;
+
+static void logreport(const char *label, const char *err, va_list params)
+{
+ struct strbuf msg = STRBUF_INIT;
+
+ strbuf_addf(&msg, "[%"PRIuMAX"] %s: ", (uintmax_t)getpid(), label);
+ strbuf_vaddf(&msg, err, params);
+ strbuf_addch(&msg, '\n');
+
+ fwrite(msg.buf, sizeof(char), msg.len, stderr);
+ fflush(stderr);
+
+ strbuf_release(&msg);
+}
+
+__attribute__((format (printf, 1, 2)))
+static void logerror(const char *err, ...)
+{
+ va_list params;
+ va_start(params, err);
+ logreport("error", err, params);
+ va_end(params);
+}
+
+__attribute__((format (printf, 1, 2)))
+static void loginfo(const char *err, ...)
+{
+ va_list params;
+ if (!verbose)
+ return;
+ va_start(params, err);
+ logreport("info", err, params);
+ va_end(params);
+}
+
+/*
+ * The code in this section is used by "worker" instances to service
+ * a single connection from a client. The worker talks to the client
+ * on 0 and 1.
+ */
+
+enum worker_result {
+ /*
+ * Operation successful.
+ * Caller *might* keep the socket open and allow keep-alive.
+ */
+ WR_OK = 0,
+
+ /*
+ * Various errors while processing the request and/or the response.
+ * Close the socket and clean up.
+ * Exit child-process with non-zero status.
+ */
+ WR_IO_ERROR = 1<<0,
+
+ /*
+ * Close the socket and clean up. Does not imply an error.
+ */
+ WR_HANGUP = 1<<1,
+};
+
+static enum worker_result worker(void)
+{
+ const char *response = "HTTP/1.1 501 Not Implemented\r\n";
+ char *client_addr = getenv("REMOTE_ADDR");
+ char *client_port = getenv("REMOTE_PORT");
+ enum worker_result wr = WR_OK;
+
+ if (client_addr)
+ loginfo("Connection from %s:%s", client_addr, client_port);
+
+ set_keep_alive(0, logerror);
+
+ while (1) {
+ if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
+ logerror("unable to write response");
+ wr = WR_IO_ERROR;
+ }
+
+ if (wr != WR_OK)
+ break;
+ }
+
+ close(STDIN_FILENO);
+ close(STDOUT_FILENO);
+
+ return !!(wr & WR_IO_ERROR);
+}
+
+static int max_connections = 32;
+
+static unsigned int live_children;
+
+static struct child *first_child;
+
+static struct strvec cld_argv = STRVEC_INIT;
+static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
+{
+ struct child_process cld = CHILD_PROCESS_INIT;
+
+ if (max_connections && live_children >= max_connections) {
+ kill_some_child(first_child);
+ sleep(1); /* give it some time to die */
+ check_dead_children(first_child, &live_children, loginfo);
+ if (live_children >= max_connections) {
+ close(incoming);
+ logerror("Too many children, dropping connection");
+ return;
+ }
+ }
+
+ if (addr->sa_family == AF_INET) {
+ char buf[128] = "";
+ struct sockaddr_in *sin_addr = (void *) addr;
+ inet_ntop(addr->sa_family, &sin_addr->sin_addr, buf, sizeof(buf));
+ strvec_pushf(&cld.env, "REMOTE_ADDR=%s", buf);
+ strvec_pushf(&cld.env, "REMOTE_PORT=%d",
+ ntohs(sin_addr->sin_port));
+#ifndef NO_IPV6
+ } else if (addr->sa_family == AF_INET6) {
+ char buf[128] = "";
+ struct sockaddr_in6 *sin6_addr = (void *) addr;
+ inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf, sizeof(buf));
+ strvec_pushf(&cld.env, "REMOTE_ADDR=[%s]", buf);
+ strvec_pushf(&cld.env, "REMOTE_PORT=%d",
+ ntohs(sin6_addr->sin6_port));
+#endif
+ }
+
+ strvec_pushv(&cld.args, cld_argv.v);
+ cld.in = incoming;
+ cld.out = dup(incoming);
+
+ if (cld.out < 0)
+ logerror("could not dup() `incoming`");
+ else if (start_command(&cld))
+ logerror("unable to fork");
+ else
+ add_child(&cld, addr, addrlen, first_child, &live_children);
+}
+
+static void child_handler(int signo)
+{
+ /*
+ * Otherwise empty handler because systemcalls will get interrupted
+ * upon signal receipt
+ * SysV needs the handler to be rearmed
+ */
+ signal(SIGCHLD, child_handler);
+}
+
+static int service_loop(struct socketlist *socklist)
+{
+ struct pollfd *pfd;
+ int i;
+
+ CALLOC_ARRAY(pfd, socklist->nr);
+
+ for (i = 0; i < socklist->nr; i++) {
+ pfd[i].fd = socklist->list[i];
+ pfd[i].events = POLLIN;
+ }
+
+ signal(SIGCHLD, child_handler);
+
+ for (;;) {
+ int i;
+ int nr_ready;
+ int timeout = (pid_file ? 100 : -1);
+
+ check_dead_children(first_child, &live_children, loginfo);
+
+ nr_ready = poll(pfd, socklist->nr, timeout);
+ if (nr_ready < 0) {
+ if (errno != EINTR) {
+ logerror("Poll failed, resuming: %s",
+ strerror(errno));
+ sleep(1);
+ }
+ continue;
+ }
+ else if (nr_ready == 0) {
+ /*
+ * If we have a pid_file, then we watch it.
+ * If someone deletes it, we shutdown the service.
+ * The shell scripts in the test suite will use this.
+ */
+ if (!pid_file || file_exists(pid_file))
+ continue;
+ goto shutdown;
+ }
+
+ for (i = 0; i < socklist->nr; i++) {
+ if (pfd[i].revents & POLLIN) {
+ union {
+ struct sockaddr sa;
+ struct sockaddr_in sai;
+#ifndef NO_IPV6
+ struct sockaddr_in6 sai6;
+#endif
+ } ss;
+ socklen_t sslen = sizeof(ss);
+ int incoming = accept(pfd[i].fd, &ss.sa, &sslen);
+ if (incoming < 0) {
+ switch (errno) {
+ case EAGAIN:
+ case EINTR:
+ case ECONNABORTED:
+ continue;
+ default:
+ die_errno("accept returned");
+ }
+ }
+ handle(incoming, &ss.sa, sslen);
+ }
+ }
+ }
+
+shutdown:
+ loginfo("Starting graceful shutdown (pid-file gone)");
+ for (i = 0; i < socklist->nr; i++)
+ close(socklist->list[i]);
+
+ return 0;
+}
+
+static int serve(struct string_list *listen_addr, int listen_port)
+{
+ struct socketlist socklist = { NULL, 0, 0 };
+
+ socksetup(listen_addr, listen_port, &socklist, reuseaddr, logerror);
+ if (socklist.nr == 0)
+ die("unable to allocate any listen sockets on port %u",
+ listen_port);
+
+ loginfo("Ready to rumble");
+
+ /*
+ * Wait to create the pid-file until we've setup the sockets
+ * and are open for business.
+ */
+ if (pid_file)
+ write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid());
+
+ return service_loop(&socklist);
+}
+
+/*
+ * This section is executed by both the primary instance and all
+ * worker instances. So, yes, each child-process re-parses the
+ * command line argument and re-discovers how it should behave.
+ */
+
+int cmd_main(int argc, const char **argv)
+{
+ int listen_port = 0;
+ struct string_list listen_addr = STRING_LIST_INIT_NODUP;
+ int worker_mode = 0;
+ int i;
+
+ trace2_cmd_name("test-http-server");
+ trace2_cmd_list_config();
+ trace2_cmd_list_env_vars();
+ setup_git_directory_gently(NULL);
+
+ for (i = 1; i < argc; i++) {
+ const char *arg = argv[i];
+ const char *v;
+
+ if (skip_prefix(arg, "--listen=", &v)) {
+ string_list_append(&listen_addr, xstrdup_tolower(v));
+ continue;
+ }
+ if (skip_prefix(arg, "--port=", &v)) {
+ char *end;
+ unsigned long n;
+ n = strtoul(v, &end, 0);
+ if (*v && !*end) {
+ listen_port = n;
+ continue;
+ }
+ }
+ if (!strcmp(arg, "--worker")) {
+ worker_mode = 1;
+ trace2_cmd_mode("worker");
+ continue;
+ }
+ if (!strcmp(arg, "--verbose")) {
+ verbose = 1;
+ continue;
+ }
+ if (skip_prefix(arg, "--timeout=", &v)) {
+ timeout = atoi(v);
+ continue;
+ }
+ if (skip_prefix(arg, "--max-connections=", &v)) {
+ max_connections = atoi(v);
+ if (max_connections < 0)
+ max_connections = 0; /* unlimited */
+ continue;
+ }
+ if (!strcmp(arg, "--reuseaddr")) {
+ reuseaddr = 1;
+ continue;
+ }
+ if (skip_prefix(arg, "--pid-file=", &v)) {
+ pid_file = v;
+ continue;
+ }
+
+ fprintf(stderr, "error: unknown argument '%s'\n", arg);
+ usage(test_http_auth_usage);
+ }
+
+ /* avoid splitting a message in the middle */
+ setvbuf(stderr, NULL, _IOFBF, 4096);
+
+ if (listen_port == 0)
+ listen_port = DEFAULT_GIT_PORT;
+
+ /*
+ * If no --listen=<addr> args are given, the setup_named_sock()
+ * code will use receive a NULL address and set INADDR_ANY.
+ * This exposes both internal and external interfaces on the
+ * port.
+ *
+ * Disallow that and default to the internal-use-only loopback
+ * address.
+ */
+ if (!listen_addr.nr)
+ string_list_append(&listen_addr, "127.0.0.1");
+
+ /*
+ * worker_mode is set in our own child process instances
+ * (that are bound to a connected socket from a client).
+ */
+ if (worker_mode)
+ return worker();
+
+ /*
+ * `cld_argv` is a bit of a clever hack. The top-level instance
+ * of test-http-server does the normal bind/listen/accept stuff.
+ * For each incoming socket, the top-level process spawns
+ * a child instance of test-http-server *WITH* the additional
+ * `--worker` argument. This causes the child to set `worker_mode`
+ * and immediately call `worker()` using the connected socket (and
+ * without the usual need for fork() or threads).
+ *
+ * The magic here is made possible because `cld_argv` is static
+ * and handle() (called by service_loop()) knows about it.
+ */
+ strvec_push(&cld_argv, argv[0]);
+ strvec_push(&cld_argv, "--worker");
+ for (i = 1; i < argc; ++i)
+ strvec_push(&cld_argv, argv[i]);
+
+ /*
+ * Setup primary instance to listen for connections.
+ */
+ return serve(&listen_addr, listen_port);
+}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 00/12] Enhance credential helper protocol to include auth headers
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham
In-Reply-To: <pull.1352.v5.git.1673475190.gitgitgadget@gmail.com>
Following from my original RFC submission [0], this submission is considered
ready for full review. This patch series is now based on top of current
master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now
separately submitted patches [1] to fix up the other credential helpers'
behaviour.
In this patch series I update the existing credential helper design in order
to allow for some new scenarios, and future evolution of auth methods that
Git hosts may wish to provide. I outline the background, summary of changes
and some challenges below.
Testing these new additions, I introduce a new test helper test-http-server
that acts as a frontend to git-http-backend; a mini HTTP server sharing code
with git-daemon, with simple authentication configurable by a config file.
Background
==========
Git uses a variety of protocols [2]: local, Smart HTTP, Dumb HTTP, SSH, and
Git. Here I focus on the Smart HTTP protocol, and attempt to enhance the
authentication capabilities of this protocol to address limitations (see
below).
The Smart HTTP protocol in Git supports a few different types of HTTP
authentication - Basic and Digest (RFC 2617) [3], and Negotiate (RFC 2478)
[4]. Git uses a extensible model where credential helpers can provide
credentials for protocols [5]. Several helpers support alternatives such as
OAuth authentication (RFC 6749) [6], but this is typically done as an
extension. For example, a helper might use basic auth and set the password
to an OAuth Bearer access token. Git uses standard input and output to
communicate with credential helpers.
After a HTTP 401 response, Git would call a credential helper with the
following over standard input:
protocol=https
host=example.com
And then a credential helper would return over standard output:
protocol=https
host=example.com
username=bob@id.example.com
password=<BEARER-TOKEN>
Git then the following request to the remote, including the standard HTTP
Authorization header (RFC 7235 Section 4.2) [7]:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: Basic base64(bob@id.example.com:<BEARER-TOKEN>)
Credential helpers are encouraged (see gitcredentials.txt) to return the
minimum information necessary.
Limitations
===========
Because this credential model was built mostly for password based
authentication systems, it's somewhat limited. In particular:
1. To generate valid credentials, additional information about the request
(or indeed the requestee and their device) may be required. For example,
OAuth is based around scopes. A scope, like "git.read", might be
required to read data from the remote. However, the remote cannot tell
the credential helper what scope is required for this request.
2. This system is not fully extensible. Each time a new type of
authentication (like OAuth Bearer) is invented, Git needs updates before
credential helpers can take advantage of it (or leverage a new
capability in libcurl).
Goals
=====
* As a user with multiple federated cloud identities:
* Reach out to a remote and have my credential helper automatically
prompt me for the correct identity.
* Allow credential helpers to differentiate between different authorities
or authentication/authorization challenge types, even from the same DNS
hostname (and without needing to use credential.useHttpPath).
* Leverage existing authentication systems built-in to many operating
systems and devices to boost security and reduce reliance on passwords.
* As a Git host and/or cloud identity provider:
* Enforce security policies (like requiring two-factor authentication)
dynamically.
* Allow integration with third party standard based identity providers in
enterprises allowing customers to have a single plane of control for
critical identities with access to source code.
Design Principles
=================
* Use the existing infrastructure. Git credential helpers are an
already-working model.
* Follow widely-adopted time-proven open standards, avoid net new ideas in
the authentication space.
* Minimize knowledge of authentication in Git; maintain modularity and
extensibility.
Proposed Changes
================
1. Teach Git to read HTTP response headers, specifically the standard
WWW-Authenticate (RFC 7235 Section 4.1) headers.
2. Teach Git to include extra information about HTTP responses that require
authentication when calling credential helpers. Specifically the
WWW-Authenticate header information.
Because the extra information forms an ordered list, and the existing
credential helper I/O format only provides for simple key=value pairs,
we introduce a new convention for transmitting an ordered list of
values. Key names that are suffixed with a C-style array syntax should
have values considered to form an order list, i.e. key[]=value, where
the order of the key=value pairs in the stream specifies the order.
For the WWW-Authenticate header values we opt to use the key wwwauth[].
Handling the WWW-Authenticate header in detail
==============================================
RFC 6750 [8] envisions that OAuth Bearer resource servers would give
responses that include WWW-Authenticate headers, for example:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
Specifically, a WWW-Authenticate header consists of a scheme and arbitrary
attributes, depending on the scheme. This pattern enables generic OAuth or
OpenID Connect [9] authorities. Note that it is possible to have several
WWW-Authenticate challenges in a response.
First Git attempts to make a request, unauthenticated, which fails with a
401 response and includes WWW-Authenticate header(s).
Next, Git invokes a credential helper which may prompt the user. If the user
approves, a credential helper can generate a token (or any auth challenge
response) to be used for that request.
For example: with a remote that supports bearer tokens from an OpenID
Connect [9] authority, a credential helper can use OpenID Connect's
Discovery [10] and Dynamic Client Registration [11] to register a client and
make a request with the correct permissions to access the remote. In this
manner, a user can be dynamically sent to the right federated identity
provider for a remote without any up-front configuration or manual
processes.
Following from the principle of keeping authentication knowledge in Git to a
minimum, we modify Git to add all WWW-Authenticate values to the credential
helper call.
Git sends over standard input:
protocol=https
host=example.com
wwwauth[]=Bearer realm="login.example", scope="git.readwrite"
wwwauth[]=Basic realm="login.example"
A credential helper that understands the extra wwwauth[n] property can
decide on the "best" or correct authentication scheme, generate credentials
for the request, and interact with the user.
The credential helper would then return over standard output:
protocol=https
host=example.com
path=foo.git
username=bob@identity.example
password=<BEARER-TOKEN>
Note that WWW-Authenticate supports multiple challenges, either in one
header:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
or in multiple headers:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"
These have equivalent meaning (RFC 2616 Section 4.2 [12]). To simplify the
implementation, Git will not merge or split up any of these WWW-Authenticate
headers, and instead pass each header line as one credential helper
property. The credential helper is responsible for splitting, merging, and
otherwise parsing these header values.
An alternative option to sending the header fields individually would be to
merge the header values in to one key=value property, for example:
...
wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
Future work
===========
In the future we can further expand the protocol to allow credential helpers
decide the best authentication scheme. Today credential helpers are still
only expected to return a username/password pair to Git, meaning the other
authentication schemes that may be offered still need challenge responses
sent via a Basic Authorization header. The changes outlined above still
permit helpers to select and configure an available authentication mode, but
require the remote for example to unpack a bearer token from a basic
challenge.
More careful consideration is required in the handling of custom
authentication schemes which may not have a username, or may require
arbitrary additional request header values be set.
For example imagine a new "FooBar" authentication scheme that is surfaced in
the following response:
HTTP/1.1 401 Unauthorized
WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"
With support for arbitrary authentication schemes, Git would call credential
helpers with the following over standard input:
protocol=https
host=example.com
wwwauth[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"
And then an enlightened credential helper could return over standard output:
protocol=https
host=example.com
authtype=FooBar
username=bob@id.example.com
password=<FooBar credential>
header[]=X-FooBar: 12345
header[]=X-FooBar-Alt: ABCDEF
Git would be expected to attach this authorization header to the next
request:
GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: FooBar <FooBar credential>
X-FooBar: 12345
X-FooBar-Alt: ABCDEF
Why not SSH?
============
There's nothing wrong with SSH. However, Git's Smart HTTP transport is
widely used, often with OAuth Bearer tokens. Git's Smart HTTP transport
sometimes requires less client setup than SSH transport, and works in
environments when SSH ports may be blocked. As long as Git supports HTTP
transport, it should support common and popular HTTP authentication methods.
References
==========
* [0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth
headers
https://lore.kernel.org/git/pull.1352.git.1663097156.gitgitgadget@gmail.com/
* [1] [PATCH 0/3] Correct credential helper discrepancies handling input
https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/
* [2] Git on the Server - The Protocols
https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
* [3] HTTP Authentication: Basic and Digest Access Authentication
https://datatracker.ietf.org/doc/html/rfc2617
* [4] The Simple and Protected GSS-API Negotiation Mechanism
https://datatracker.ietf.org/doc/html/rfc2478
* [5] Git Credentials - Custom Helpers
https://git-scm.com/docs/gitcredentials#_custom_helpers
* [6] The OAuth 2.0 Authorization Framework
https://datatracker.ietf.org/doc/html/rfc6749
* [7] Hypertext Transfer Protocol (HTTP/1.1): Authentication
https://datatracker.ietf.org/doc/html/rfc7235
* [8] The OAuth 2.0 Authorization Framework: Bearer Token Usage
https://datatracker.ietf.org/doc/html/rfc6750
* [9] OpenID Connect Core 1.0
https://openid.net/specs/openid-connect-core-1_0.html
* [10] OpenID Connect Discovery 1.0
https://openid.net/specs/openid-connect-discovery-1_0.html
* [11] OpenID Connect Dynamic Client Registration 1.0
https://openid.net/specs/openid-connect-registration-1_0.html
* [12] Hypertext Transfer Protocol (HTTP/1.1)
https://datatracker.ietf.org/doc/html/rfc2616
Updates from RFC
================
* Submitted first three patches as separate submission:
https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/
* Various style fixes and updates to- and addition of comments.
* Drop the explicit integer index in new 'array' style credential helper
attrbiutes ("key[n]=value" becomes just "key[]=value").
* Added test helper; a mini HTTP server, and several tests.
Updates in v3
=============
* Split final patch that added the test-http-server in to several, easier
to review patches.
* Updated wording in git-credential.txt to clarify which side of the
credential helper protocol is sending/receiving the new wwwauth and
authtype attributes.
Updates in v4
=============
* Drop authentication scheme selection authtype attribute patches to
greatly simplify the series; auth scheme selection is punted to a future
series. This series still allows credential helpers to generate
credentials and intelligently select correct identities for a given auth
challenge.
Updates in v5
=============
* Libify parts of daemon.c and share implementation with test-http-server.
* Clarify test-http-server Git request regex pattern and auth logic
comments.
* Use STD*_FILENO in place of 'magic' file descriptor numbers.
* Use strbuf_* functions in continuation header parsing.
* Use configuration file to configure auth for test-http-server rather than
command-line arguments. Add ability to specify arbitrary extra headers
that is useful for testing 'malformed' server responses.
* Use st_mult over unchecked multiplication in http.c curl callback
functions.
* Fix some documentation line break issues.
* Reorder some commits to bring in the tests and test-http-server helper
first and, then the WWW-Authentication changes, alongside tests to cover.
* Expose previously static strvec_push_nodup function.
* Merge the two timeout args for test-http-server (--timeout and
--init-timeout) that were a hang-over from the original daemon.c but are
no longer required here.
* Be more careful around continuation headers where they may be empty
strings. Add more tests to cover these header types.
* Include standard trace2 tracing calls at start of test-http-server
helper.
Updates in v6
=============
* Clarify the change to make logging optional in the check_dead_children()
function during libification of daemon.c.
* Fix missing pointer dereference bugs identified in libification of child
process handling functions for daemon.c.
* Add doc comments to child process handling function declarations in the
daemon-utils.h header.
* Align function parameter names with variable names at callsites for
libified daemon functions.
* Re-split out the test-http-server test helper commits in to smaller
patches: error response handling, request parsing, http-backend
pass-through, simple authentication, arbitrary header support.
* Call out auth configuration file format for test-http-server test helper
and supported options in commit messages, as well as a test to exercise
and demonstrate these options.
* Permit auth.token and auth.challenge to appear in any order; create the
struct auth_module just-in-time as options for that scheme are read. This
simplifies the configuration authoring of the test-http-server test
helper.
* Update tests to use auth.allowAnoymous in the patch that introduces the
new test helper option.
* Drop the strvec_push_nodup() commit and update the implementation of HTTP
request header line folding to use xstrdup and strvec_pop and _pushf.
* Use size_t instead of int in credential.c when iterating over the struct
strvec credential members. Also drop the not required const and cast from
the full_key definition and free.
* Replace in-tree test-credential-helper-reply.sh test cred helper script
with the lib-credential-helper.sh reusable 'lib' test script and shell
functions to configure the helper behaviour.
* Leverage sed over the while read $line loop in the test credential helper
script.
Matthew John Cheetham (12):
daemon: libify socket setup and option functions
daemon: libify child process handling functions
daemon: rename some esoteric/laboured terminology
test-http-server: add stub HTTP server test helper
test-http-server: add HTTP error response function
test-http-server: add HTTP request parsing
test-http-server: pass Git requests to http-backend
test-http-server: add simple authentication
test-http-server: add sending of arbitrary headers
http: replace unsafe size_t multiplication with st_mult
http: read HTTP WWW-Authenticate response headers
credential: add WWW-Authenticate header to cred requests
Documentation/git-credential.txt | 19 +-
Makefile | 2 +
contrib/buildsystems/CMakeLists.txt | 11 +-
credential.c | 12 +
credential.h | 15 +
daemon-utils.c | 286 +++++++++
daemon-utils.h | 55 ++
daemon.c | 306 +---------
http.c | 98 ++-
t/helper/.gitignore | 1 +
t/helper/test-http-server.c | 910 ++++++++++++++++++++++++++++
t/lib-credential-helper.sh | 27 +
t/t5556-http-auth.sh | 398 ++++++++++++
13 files changed, 1838 insertions(+), 302 deletions(-)
create mode 100644 daemon-utils.c
create mode 100644 daemon-utils.h
create mode 100644 t/helper/test-http-server.c
create mode 100644 t/lib-credential-helper.sh
create mode 100755 t/t5556-http-auth.sh
base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1352
Range-diff vs v5:
1: 74b0de14185 = 1: 74b0de14185 daemon: libify socket setup and option functions
2: bc972fc8d3d ! 2: b6ba344a671 daemon: libify child process handling functions
@@ Commit message
from the parent daemon-like process from `daemon.c` to the new shared
`daemon-utils.{c,h}` files.
+ One minor functional change is introduced to `check_dead_children()`
+ where the logging of a dead/disconnected child is now optional. With the
+ 'libification' of these functions we extract the call to `loginfo` to a
+ call to a function pointer, and guard the log message creation and
+ logging behind a `NULL` check. Callers can now skip logging by passing
+ `NULL` as the `log_fn loginfo` argument.
+ The behaviour of callers in `daemon.c` remains the same (save one extra
+ NULL check) however as a pointer to `loginfo` is always passed.
+
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
## daemon-utils.c ##
@@ daemon-utils.c: void socksetup(struct string_list *listen_addr, int listen_port,
+ struct child *newborn, **cradle;
+
+ CALLOC_ARRAY(newborn, 1);
-+ live_children++;
++ (*live_children)++;
+ memcpy(&newborn->cld, cld, sizeof(*cld));
+ memcpy(&newborn->address, addr, addrlen);
+ for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
@@ daemon-utils.c: void socksetup(struct string_list *listen_addr, int listen_port,
+
+ /* remove the child */
+ *cradle = blanket->next;
-+ live_children--;
++ (*live_children)--;
+ child_process_clear(&blanket->cld);
+ free(blanket);
+ } else
@@ daemon-utils.h: void socksetup(struct string_list *listen_addr, int listen_port,
+ struct sockaddr_storage address;
+};
+
++/*
++ * Add the child_process to the set of children and increment the number of
++ * live children.
++ */
+void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
+ struct child *firstborn, unsigned int *live_children);
+
++/*
++ * Kill the newest connection from a duplicate IP.
++ *
++ * This function should be called if the number of connections grows
++ * past the maximum number of allowed connections.
++ */
+void kill_some_child(struct child *firstborn);
+
++/*
++ * Check for children that have disconnected and remove them from the
++ * active set, decrementing the number of live children.
++ *
++ * Optionally log the child PID that disconnected by passing a loginfo
++ * function.
++ */
+void check_dead_children(struct child *firstborn, unsigned int *live_children,
+ log_fn loginfo);
+
3: 8f176d5955d ! 3: 9967401c972 daemon: rename some esoteric/laboured terminology
@@ Commit message
processes. The existing names are esoteric; stretching an analogy too
far to the point of being confusing to understand.
- Rename "firstborn" to simply "first", "newborn" to "new_cld", "blanket"
+ Rename "firstborn" to "first_child", "newborn" to "new_cld", "blanket"
to "current" and "cradle" to "ptr".
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
@@ daemon-utils.c: static int addrcmp(const struct sockaddr_storage *s1,
void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
- struct child *firstborn , unsigned int *live_children)
-+ struct child *first, unsigned int *live_children)
++ struct child *first_child, unsigned int *live_children)
{
- struct child *newborn, **cradle;
+ struct child *new_cld, **current;
- CALLOC_ARRAY(newborn, 1);
+ CALLOC_ARRAY(new_cld, 1);
- live_children++;
+ (*live_children)++;
- memcpy(&newborn->cld, cld, sizeof(*cld));
- memcpy(&newborn->address, addr, addrlen);
- for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
- if (!addrcmp(&(*cradle)->address, &newborn->address))
+ memcpy(&new_cld->cld, cld, sizeof(*cld));
+ memcpy(&new_cld->address, addr, addrlen);
-+ for (current = &first; *current; current = &(*current)->next)
++ for (current = &first_child; *current; current = &(*current)->next)
+ if (!addrcmp(&(*current)->address, &new_cld->address))
break;
- newborn->next = *cradle;
@@ daemon-utils.c: static int addrcmp(const struct sockaddr_storage *s1,
}
-void kill_some_child(struct child *firstborn)
-+void kill_some_child(struct child *first)
++void kill_some_child(struct child *first_child)
{
- const struct child *blanket, *next;
+ const struct child *current, *next;
- if (!(blanket = firstborn))
-+ if (!(current = first))
++ if (!(current = first_child))
return;
- for (; (next = blanket->next); blanket = next)
@@ daemon-utils.c: static int addrcmp(const struct sockaddr_storage *s1,
}
-void check_dead_children(struct child *firstborn, unsigned int *live_children,
-+void check_dead_children(struct child *first, unsigned int *live_children,
++void check_dead_children(struct child *first_child, unsigned int *live_children,
log_fn loginfo)
{
int status;
@@ daemon-utils.c: static int addrcmp(const struct sockaddr_storage *s1,
- for (cradle = &firstborn; (blanket = *cradle);)
- if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
+ struct child **ptr, *current;
-+ for (ptr = &first; (current = *ptr);)
++ for (ptr = &first_child; (current = *ptr);)
+ if ((pid = waitpid(current->cld.pid, &status, WNOHANG)) > 1) {
if (loginfo) {
const char *dead = "";
@@ daemon-utils.c: void check_dead_children(struct child *firstborn, unsigned int *
/* remove the child */
- *cradle = blanket->next;
+ *ptr = current->next;
- live_children--;
+ (*live_children)--;
- child_process_clear(&blanket->cld);
- free(blanket);
+ child_process_clear(¤t->cld);
@@ daemon-utils.c: void check_dead_children(struct child *firstborn, unsigned int *
## daemon-utils.h ##
@@ daemon-utils.h: struct child {
- };
-
+ * live children.
+ */
void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
- struct child *firstborn, unsigned int *live_children);
-+ struct child *first, unsigned int *live_children);
-
++ struct child *first_child, unsigned int *live_children);
+
+ /*
+ * Kill the newest connection from a duplicate IP.
+@@ daemon-utils.h: void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrl
+ * This function should be called if the number of connections grows
+ * past the maximum number of allowed connections.
+ */
-void kill_some_child(struct child *firstborn);
-+void kill_some_child(struct child *first);
-
++void kill_some_child(struct child *first_child);
+
+ /*
+ * Check for children that have disconnected and remove them from the
+@@ daemon-utils.h: void kill_some_child(struct child *firstborn);
+ * Optionally log the child PID that disconnected by passing a loginfo
+ * function.
+ */
-void check_dead_children(struct child *firstborn, unsigned int *live_children,
-+void check_dead_children(struct child *first, unsigned int *live_children,
++void check_dead_children(struct child *first_child, unsigned int *live_children,
log_fn loginfo);
#endif
4: 706fb3781bd = 4: d6e5e8825e8 test-http-server: add stub HTTP server test helper
-: ----------- > 5: 79805f042b9 test-http-server: add HTTP error response function
5: 6f66bf146b4 ! 6: 252098db219 test-http-server: add HTTP error response function
@@ Metadata
Author: Matthew John Cheetham <mjcheetham@outlook.com>
## Commit message ##
- test-http-server: add HTTP error response function
+ test-http-server: add HTTP request parsing
- Introduce a function to the test-http-server test helper to write more
- full and valid HTTP error responses, including all the standard response
- headers like `Server` and `Date`.
+ Add ability to parse HTTP requests to the test-http-server test helper.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
@@ t/helper/test-http-server.c: enum worker_result {
+ string_list_clear(&req->header_list, 0);
+}
+
-+static enum worker_result send_http_error(
-+ int fd,
-+ int http_code, const char *http_code_name,
-+ int retry_after_seconds, struct string_list *response_headers,
-+ enum worker_result wr_in)
-+{
-+ struct strbuf response_header = STRBUF_INIT;
-+ struct strbuf response_content = STRBUF_INIT;
-+ struct string_list_item *h;
-+ enum worker_result wr;
-+
-+ strbuf_addf(&response_content, "Error: %d %s\r\n",
-+ http_code, http_code_name);
-+ if (retry_after_seconds > 0)
-+ strbuf_addf(&response_content, "Retry-After: %d\r\n",
-+ retry_after_seconds);
-+
-+ strbuf_addf (&response_header, "HTTP/1.1 %d %s\r\n", http_code, http_code_name);
-+ strbuf_addstr(&response_header, "Cache-Control: private\r\n");
-+ strbuf_addstr(&response_header, "Content-Type: text/plain\r\n");
-+ strbuf_addf (&response_header, "Content-Length: %d\r\n", (int)response_content.len);
-+ if (retry_after_seconds > 0)
-+ strbuf_addf(&response_header, "Retry-After: %d\r\n", retry_after_seconds);
-+ strbuf_addf( &response_header, "Server: test-http-server/%s\r\n", git_version_string);
-+ strbuf_addf( &response_header, "Date: %s\r\n", show_date(time(NULL), 0, DATE_MODE(RFC2822)));
-+ if (response_headers)
-+ for_each_string_list_item(h, response_headers)
-+ strbuf_addf(&response_header, "%s\r\n", h->string);
-+ strbuf_addstr(&response_header, "\r\n");
-+
-+ if (write_in_full(fd, response_header.buf, response_header.len) < 0) {
-+ logerror("unable to write response header");
-+ wr = WR_IO_ERROR;
-+ goto done;
-+ }
-+
-+ if (write_in_full(fd, response_content.buf, response_content.len) < 0) {
-+ logerror("unable to write response content body");
-+ wr = WR_IO_ERROR;
-+ goto done;
-+ }
-+
-+ wr = wr_in;
-+
-+done:
-+ strbuf_release(&response_header);
-+ strbuf_release(&response_content);
-+
-+ return wr;
-+}
-+
+ static enum worker_result send_http_error(
+ int fd,
+ int http_code, const char *http_code_name,
+@@ t/helper/test-http-server.c: done:
+ return wr;
+ }
+
+/*
+ * Read the HTTP request up to the start of the optional message-body.
+ * We do this byte-by-byte because we have keep-alive turned on and
@@ t/helper/test-http-server.c: enum worker_result {
+ 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));
-+ /*
-+ * This regular expression matches all dumb and smart HTTP
-+ * requests that are currently in use, and defined in
-+ * Documentation/gitprotocol-http.txt.
-+ *
-+ */
-+ 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 *ok = "HTTP/1.1 200 OK\r\n";
-+ struct child_process cp = CHILD_PROCESS_INIT;
-+ int res;
-+
-+ /*
-+ * Note that 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.
-+ *
-+ * 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);
-+
-+ 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(STDOUT_FILENO);
-+ close(STDIN_FILENO);
-+ return !!res;
-+}
-+
+static enum worker_result dispatch(struct req *req)
+{
-+ if (is_git_request(req))
-+ return do__git(req);
-+
+ return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL,
+ WR_OK | WR_HANGUP);
+}
+
static enum worker_result worker(void)
{
-- const char *response = "HTTP/1.1 501 Not Implemented\r\n";
+ struct req req = REQ__INIT;
char *client_addr = getenv("REMOTE_ADDR");
char *client_port = getenv("REMOTE_PORT");
@@ t/helper/test-http-server.c: static enum worker_result worker(void)
set_keep_alive(0, logerror);
while (1) {
-- if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
-- logerror("unable to write response");
-- wr = WR_IO_ERROR;
-- }
+- wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1,
+- NULL, WR_OK | WR_HANGUP);
+ req__release(&req);
+
+ alarm(timeout);
@@ t/helper/test-http-server.c: static enum worker_result worker(void)
if (wr != WR_OK)
break;
}
-
- ## t/t5556-http-auth.sh (new) ##
-@@
-+#!/bin/sh
-+
-+test_description='test http auth header and credential helper interop'
-+
-+TEST_NO_CREATE_REPO=1
-+. ./test-lib.sh
-+
-+test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
-+
-+# Setup a repository
-+#
-+REPO_DIR="$TRASH_DIRECTORY"/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="$TRASH_DIRECTORY"/pid-file.pid
-+SERVER_LOG="$TRASH_DIRECTORY"/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 &&
-+
-+ # Attempt to read from a protected repository
-+ git ls-remote $ORIGIN_URL
-+'
-+
-+test_done
-: ----------- > 7: ab06ac9b965 test-http-server: pass Git requests to http-backend
6: c3c3d17a688 ! 8: a1ff55dd6e2 test-http-server: add simple authentication
@@ Commit message
tokens and only approved if a matching token is found, or if no auth
was provided and anonymous auth is enabled.
+ Configuration for auth includes a simple set of three options:
+
+ [auth]
+ challenge = <scheme>[:<challenge_params>]
+ token = <scheme>:[<token>]*
+ allowAnonymous = <bool>
+
+ `auth.challenge` allows you define what authentication schemes, and
+ optional challenge parameters the server should use. Scheme names are
+ unique and subsequently specified challenge parameters in the config
+ file will replace previously specified ones.
+
+ `auth.token` allows you to define the set of value token values for an
+ authentication scheme. This is a multi-var and each entry in the
+ config file will append to the set of valid tokens for that scheme.
+ Specifying an empty token value will clear the list of tokens so far for
+ that scheme, i.e. `token = <scheme>:`.
+
+ `auth.allowAnonymous` controls whether or not unauthenticated requests
+ (those without any `Authorization` headers) should succeed or not, and
+ trigger a 401 Unauthorized response.
+
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
## t/helper/test-http-server.c ##
@@ t/helper/test-http-server.c: static int is_git_request(struct req *req)
const char *ok = "HTTP/1.1 200 OK\r\n";
struct child_process cp = CHILD_PROCESS_INIT;
@@ t/helper/test-http-server.c: 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);
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+static struct auth_module **auth_modules = NULL;
+static size_t auth_modules_nr = 0;
+static size_t auth_modules_alloc = 0;
-+static struct strvec extra_headers = STRVEC_INIT;
+
-+static struct auth_module *create_auth_module(const char *scheme,
-+ const char *challenge)
-+{
-+ struct auth_module *mod = xmalloc(sizeof(struct auth_module));
-+ mod->scheme = xstrdup(scheme);
-+ mod->challenge_params = challenge ? xstrdup(challenge) : NULL;
-+ CALLOC_ARRAY(mod->tokens, 1);
-+ string_list_init_dup(mod->tokens);
-+ return mod;
-+}
-+
-+static struct auth_module *get_auth_module(const char *scheme)
++static struct auth_module *get_auth_module(const char *scheme, int create)
+{
+ int i;
+ struct auth_module *mod;
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ return mod;
+ }
+
-+ return NULL;
-+}
++ if (create) {
++ struct auth_module *mod = xmalloc(sizeof(struct auth_module));
++ mod->scheme = xstrdup(scheme);
++ mod->challenge_params = NULL;
++ CALLOC_ARRAY(mod->tokens, 1);
++ string_list_init_dup(mod->tokens);
+
-+static int add_auth_module(struct auth_module *mod)
-+{
-+ if (get_auth_module(mod->scheme))
-+ return error("duplicate auth scheme '%s'\n", mod->scheme);
++ ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
++ auth_modules[auth_modules_nr++] = mod;
+
-+ ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
-+ auth_modules[auth_modules_nr++] = mod;
++ return mod;
++ }
+
-+ return 0;
++ return NULL;
+}
+
+static int is_authed(struct req *req, const char **user, enum worker_result *wr)
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ /* trim trailing space ' ' */
+ strbuf_setlen(split[0], split[0]->len - 1);
+
-+ mod = get_auth_module(split[0]->buf);
++ mod = get_auth_module(split[0]->buf, 0);
+ if (mod) {
+ result = AUTH_DENY;
+
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ string_list_append(&hdrs, challenge);
+ }
+
-+ for (i = 0; i < extra_headers.nr; i++)
-+ string_list_append(&hdrs, extra_headers.v[i]);
-+
+ *wr = send_http_error(STDOUT_FILENO, 401, "Unauthorized", -1,
+ &hdrs, *wr);
+ }
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ (result == AUTH_UNKNOWN && allow_anonymous);
+}
+
-+static int split_auth_param(const char *str, char **scheme, char **val, int required_val)
++static int split_auth_param(const char *str, char **scheme, char **val)
+{
+ struct strbuf **p = strbuf_split_str(str, ':', 2);
+
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ return -1;
+
+ /* trim trailing ':' */
-+ if (p[1])
++ if (p[0]->len > 0 && p[0]->buf[p[0]->len - 1] == ':')
+ strbuf_setlen(p[0], p[0]->len - 1);
+
-+ if (required_val && !p[1])
-+ return -1;
-+
+ *scheme = strbuf_detach(p[0], NULL);
+
+ if (p[1])
@@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req)
+ struct auth_module *mod = NULL;
+
+ if (!strcmp(name, "auth.challenge")) {
-+ if (split_auth_param(val, &scheme, &challenge, 0)) {
++ if (split_auth_param(val, &scheme, &challenge)) {
+ ret = error("invalid auth challenge '%s'", val);
+ goto cleanup;
+ }
+
-+ mod = create_auth_module(scheme, challenge);
-+ if (add_auth_module(mod)) {
-+ ret = error("failed to add auth module '%s'", val);
-+ goto cleanup;
-+ }
-+ }
-+ if (!strcmp(name, "auth.token")) {
-+ if (split_auth_param(val, &scheme, &token, 1)) {
-+ ret = error("invalid auth token '%s'", val);
-+ goto cleanup;
-+ }
++ mod = get_auth_module(scheme, 1);
+
-+ mod = get_auth_module(scheme);
-+ if (!mod) {
-+ ret = error("auth scheme not defined '%s'\n", scheme);
++ /* 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;
+ }
+
-+ string_list_append(mod->tokens, token);
-+ }
-+ if (!strcmp(name, "auth.allowanonymous")) {
++ 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);
-+ }
-+ if (!strcmp(name, "auth.extraheader")) {
-+ strvec_push(&extra_headers, val);
++ } else {
++ warning("unknown auth config '%s'", name);
+ }
+
+cleanup:
@@ t/helper/test-http-server.c: int cmd_main(int argc, const char **argv)
fprintf(stderr, "error: unknown argument '%s'\n", arg);
usage(test_http_auth_usage);
+
+ ## t/t5556-http-auth.sh ##
+@@ t/t5556-http-auth.sh: per_test_cleanup () {
+ rm -f OUT.*
+ }
+
++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
-: ----------- > 9: 76125cdf239 test-http-server: add sending of arbitrary headers
7: 9c4d25945dd = 10: cc9a220ed1f http: replace unsafe size_t multiplication with st_mult
8: 65a620b08ef < -: ----------- strvec: expose strvec_push_nodup for external use
9: bcfec529d95 ! 11: bc1ac8d3eb3 http: read HTTP WWW-Authenticate response headers
@@ http.c: size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buff
+ if (!values->nr) {
+ BUG("should have at least one existing header value");
+ } else if (buf.len) {
-+ const char *prev = values->v[values->nr - 1];
-+ struct strbuf append = STRBUF_INIT;
-+ strbuf_addstr(&append, prev);
++ char *prev = xstrdup(values->v[values->nr - 1]);
+
+ /* Join two non-empty values with a single space. */
-+ if (append.len)
-+ strbuf_addch(&append, ' ');
-+
-+ strbuf_addbuf(&append, &buf);
++ const char *const sp = *prev ? " " : "";
+
+ strvec_pop(values);
-+ strvec_push_nodup(values, strbuf_detach(&append, NULL));
++ strvec_pushf(values, "%s%s%s", prev, sp, buf.buf);
++ free(prev);
+ }
+
+ goto exit;
10: af66d2d2ede ! 12: 7c8229f0b11 credential: add WWW-Authenticate header to cred requests
@@ credential.c: static void credential_write_item(FILE *fp, const char *key, const
+static void credential_write_strvec(FILE *fp, const char *key,
+ const struct strvec *vec)
+{
-+ int i = 0;
-+ const char *full_key = xstrfmt("%s[]", key);
-+ for (; i < vec->nr; i++) {
++ char *full_key = xstrfmt("%s[]", key);
++ for (size_t i = 0; i < vec->nr; i++) {
+ credential_write_item(fp, full_key, vec->v[i], 0);
+ }
-+ free((void*)full_key);
++ free(full_key);
+}
+
void credential_write(const struct credential *c, FILE *fp)
@@ credential.c: void credential_write(const struct credential *c, FILE *fp)
static int run_credential_helper(struct credential *c,
- ## t/helper/test-credential-helper-replay.sh (new) ##
+ ## t/lib-credential-helper.sh (new) ##
@@
-+cmd=$1
-+teefile=$cmd-actual.cred
-+catfile=$cmd-response.cred
-+rm -f $teefile
-+while read line;
-+do
-+ if test -z "$line"; then
-+ break;
-+ fi
-+ echo "$line" >> $teefile
-+done
-+if test "$cmd" = "get"; then
-+ cat $catfile
-+fi
++setup_credential_helper() {
++ test_expect_success 'setup credential helper' '
++ CREDENTIAL_HELPER="$TRASH_DIRECTORY/credential-helper.sh" &&
++ export CREDENTIAL_HELPER &&
++ echo $CREDENTIAL_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"
++}
## t/t5556-http-auth.sh ##
-@@ t/t5556-http-auth.sh: PID_FILE="$TRASH_DIRECTORY"/pid-file.pid
- SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log
+@@ t/t5556-http-auth.sh: test_description='test http auth header and credential helper interop'
+
+ TEST_NO_CREATE_REPO=1
+ . ./test-lib.sh
++. "$TEST_DIRECTORY"/lib-credential-helper.sh
- PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
-+CREDENTIAL_HELPER="$GIT_BUILD_DIR/t/helper/test-credential-helper-replay.sh" \
-+ && export CREDENTIAL_HELPER
+ test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
- test_expect_success 'setup repos' '
- test_create_repo "$REPO_DIR" &&
+@@ t/t5556-http-auth.sh: test_expect_success 'setup repos' '
+ git -C "$REPO_DIR" branch -M main
+ '
+
++setup_credential_helper
++
+ stop_http_server () {
+ if ! test -f "$PID_FILE"
+ then
@@ t/t5556-http-auth.sh: start_http_server () {
per_test_cleanup () {
@@ t/t5556-http-auth.sh: start_http_server () {
+ rm -f auth.config
}
- 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
+ test_expect_success CURL 'http auth server auth config' '
+@@ t/t5556-http-auth.sh: test_expect_success 'http auth anonymous no challenge' '
git ls-remote $ORIGIN_URL
'
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=basic realm="example.com"
++ username=alice
++ password=secret-passwd
+ EOF
+
-+ cat >store-expected.cred <<-EOF &&
++ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=secret-passwd
++ wwwauth[]=basic realm="example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
-+
-+ git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp store-expected.cred store-actual.cred
+'
+
+test_expect_success 'http auth www-auth headers to credential helper ignore case valid' '
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
-+ extraHeader = wWw-aUtHeNtIcAtE: bEaRer auThoRiTy=\"id.example.com\"
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
++ extraHeader = wWw-aUtHeNtIcAtE: bEaRer auThoRiTy=\"id.example.com\"
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=basic realm="example.com"
-+ wwwauth[]=bEaRer auThoRiTy="id.example.com"
++ username=alice
++ password=secret-passwd
+ EOF
+
-+ cat >store-expected.cred <<-EOF &&
++ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=secret-passwd
++ wwwauth[]=basic realm="example.com"
++ wwwauth[]=bEaRer auThoRiTy="id.example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
-+
-+ git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp store-expected.cred store-actual.cred
+'
+
+test_expect_success 'http auth www-auth headers to credential helper continuation hdr' '
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = "bearer:authority=\"id.example.com\"\\n q=1\\n \\t p=0"
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
++ challenge = "bearer:authority=\"id.example.com\"\\n q=1\\n \\t p=0"
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
-+ wwwauth[]=basic realm="example.com"
++ username=alice
++ password=secret-passwd
+ EOF
+
-+ cat >store-expected.cred <<-EOF &&
++ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=secret-passwd
++ wwwauth[]=bearer authority="id.example.com" q=1 p=0
++ wwwauth[]=basic realm="example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
-+
-+ git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp store-expected.cred store-actual.cred
+'
+
+test_expect_success 'http auth www-auth headers to credential helper empty continuation hdrs' '
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
-+ extraheader = "WWW-Authenticate:"
-+ extraheader = " "
-+ extraheader = " bearer authority=\"id.example.com\""
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
++ extraheader = "WWW-Authenticate:"
++ extraheader = " "
++ extraheader = " bearer authority=\"id.example.com\""
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=basic realm="example.com"
-+ wwwauth[]=bearer authority="id.example.com"
++ username=alice
++ password=secret-passwd
+ EOF
+
-+ cat >store-expected.cred <<-EOF &&
++ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=secret-passwd
++ wwwauth[]=basic realm="example.com"
++ wwwauth[]=bearer authority="id.example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
-+
-+ git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp store-expected.cred store-actual.cred
+'
+
+test_expect_success 'http auth www-auth headers to credential helper custom schemes' '
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = "foobar:alg=test widget=1"
-+ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
++ challenge = "foobar:alg=test widget=1"
++ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=foobar alg=test widget=1
-+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
-+ wwwauth[]=basic realm="example.com"
++ username=alice
++ password=secret-passwd
+ EOF
+
-+ cat >store-expected.cred <<-EOF &&
++ git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=secret-passwd
++ wwwauth[]=foobar alg=test widget=1
++ wwwauth[]=bearer authority="id.example.com" q=1 p=0
++ wwwauth[]=basic realm="example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query store <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=secret-passwd
+ EOF
-+
-+ git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp store-expected.cred store-actual.cred
+'
+
+test_expect_success 'http auth www-auth headers to credential helper invalid' '
@@ t/t5556-http-auth.sh: start_http_server () {
+
+ cat >auth.config <<-EOF &&
+ [auth]
-+ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
-+ challenge = basic:realm=\"example.com\"
-+ token = basic:$USERPASS64
++ challenge = "bearer:authority=\"id.example.com\" q=1 p=0"
++ challenge = basic:realm=\"example.com\"
++ token = basic:$USERPASS64
+ EOF
+
+ start_http_server --auth-config="$TRASH_DIRECTORY/auth.config" &&
+
-+ cat >get-expected.cred <<-EOF &&
++ set_credential_reply get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
-+ wwwauth[]=basic realm="example.com"
++ username=alice
++ password=invalid-passwd
+ EOF
+
-+ cat >erase-expected.cred <<-EOF &&
++ test_must_fail git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL &&
++
++ expect_credential_query get <<-EOF &&
+ protocol=http
+ host=$HOST_PORT
-+ username=alice
-+ password=invalid-passwd
+ wwwauth[]=bearer authority="id.example.com" q=1 p=0
+ wwwauth[]=basic realm="example.com"
+ EOF
+
-+ cat >get-response.cred <<-EOF &&
++ expect_credential_query erase <<-EOF
+ protocol=http
+ host=$HOST_PORT
+ username=alice
+ password=invalid-passwd
++ wwwauth[]=bearer authority="id.example.com" q=1 p=0
++ wwwauth[]=basic realm="example.com"
+ EOF
-+
-+ test_must_fail git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
-+
-+ test_cmp get-expected.cred get-actual.cred &&
-+ test_cmp erase-expected.cred erase-actual.cred
+'
+
test_done
--
gitgitgadget
^ permalink raw reply
* [PATCH v6 05/12] test-http-server: add HTTP error response function
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Introduce a function to the test-http-server test helper to write more
full and valid HTTP error responses, including all the standard response
headers like `Server` and `Date`.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
t/helper/test-http-server.c | 58 +++++++++++++++++++++++++++++++++----
1 file changed, 53 insertions(+), 5 deletions(-)
diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
index 11071b1dd89..6cdac223a55 100644
--- a/t/helper/test-http-server.c
+++ b/t/helper/test-http-server.c
@@ -83,9 +83,59 @@ enum worker_result {
WR_HANGUP = 1<<1,
};
+static enum worker_result send_http_error(
+ int fd,
+ int http_code, const char *http_code_name,
+ int retry_after_seconds, struct string_list *response_headers,
+ enum worker_result wr_in)
+{
+ struct strbuf response_header = STRBUF_INIT;
+ struct strbuf response_content = STRBUF_INIT;
+ struct string_list_item *h;
+ enum worker_result wr;
+
+ strbuf_addf(&response_content, "Error: %d %s\r\n",
+ http_code, http_code_name);
+ if (retry_after_seconds > 0)
+ strbuf_addf(&response_content, "Retry-After: %d\r\n",
+ retry_after_seconds);
+
+ strbuf_addf (&response_header, "HTTP/1.1 %d %s\r\n", http_code, http_code_name);
+ strbuf_addstr(&response_header, "Cache-Control: private\r\n");
+ strbuf_addstr(&response_header, "Content-Type: text/plain\r\n");
+ strbuf_addf (&response_header, "Content-Length: %d\r\n", (int)response_content.len);
+ if (retry_after_seconds > 0)
+ strbuf_addf(&response_header, "Retry-After: %d\r\n", retry_after_seconds);
+ strbuf_addf( &response_header, "Server: test-http-server/%s\r\n", git_version_string);
+ strbuf_addf( &response_header, "Date: %s\r\n", show_date(time(NULL), 0, DATE_MODE(RFC2822)));
+ if (response_headers)
+ for_each_string_list_item(h, response_headers)
+ strbuf_addf(&response_header, "%s\r\n", h->string);
+ strbuf_addstr(&response_header, "\r\n");
+
+ if (write_in_full(fd, response_header.buf, response_header.len) < 0) {
+ logerror("unable to write response header");
+ wr = WR_IO_ERROR;
+ goto done;
+ }
+
+ if (write_in_full(fd, response_content.buf, response_content.len) < 0) {
+ logerror("unable to write response content body");
+ wr = WR_IO_ERROR;
+ goto done;
+ }
+
+ wr = wr_in;
+
+done:
+ strbuf_release(&response_header);
+ strbuf_release(&response_content);
+
+ return wr;
+}
+
static enum worker_result worker(void)
{
- const char *response = "HTTP/1.1 501 Not Implemented\r\n";
char *client_addr = getenv("REMOTE_ADDR");
char *client_port = getenv("REMOTE_PORT");
enum worker_result wr = WR_OK;
@@ -96,10 +146,8 @@ static enum worker_result worker(void)
set_keep_alive(0, logerror);
while (1) {
- if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
- logerror("unable to write response");
- wr = WR_IO_ERROR;
- }
+ wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1,
+ NULL, WR_OK | WR_HANGUP);
if (wr != WR_OK)
break;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 03/12] daemon: rename some esoteric/laboured terminology
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Rename some of the variables and function arguments used to manage child
processes. The existing names are esoteric; stretching an analogy too
far to the point of being confusing to understand.
Rename "firstborn" to "first_child", "newborn" to "new_cld", "blanket"
to "current" and "cradle" to "ptr".
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
daemon-utils.c | 46 +++++++++++++++++++++++-----------------------
daemon-utils.h | 6 +++---
daemon.c | 10 +++++-----
3 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/daemon-utils.c b/daemon-utils.c
index 8506664b440..f23ea35ed7b 100644
--- a/daemon-utils.c
+++ b/daemon-utils.c
@@ -230,44 +230,44 @@ static int addrcmp(const struct sockaddr_storage *s1,
}
void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
- struct child *firstborn , unsigned int *live_children)
+ struct child *first_child, unsigned int *live_children)
{
- struct child *newborn, **cradle;
+ struct child *new_cld, **current;
- CALLOC_ARRAY(newborn, 1);
+ CALLOC_ARRAY(new_cld, 1);
(*live_children)++;
- memcpy(&newborn->cld, cld, sizeof(*cld));
- memcpy(&newborn->address, addr, addrlen);
- for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
- if (!addrcmp(&(*cradle)->address, &newborn->address))
+ memcpy(&new_cld->cld, cld, sizeof(*cld));
+ memcpy(&new_cld->address, addr, addrlen);
+ for (current = &first_child; *current; current = &(*current)->next)
+ if (!addrcmp(&(*current)->address, &new_cld->address))
break;
- newborn->next = *cradle;
- *cradle = newborn;
+ new_cld->next = *current;
+ *current = new_cld;
}
-void kill_some_child(struct child *firstborn)
+void kill_some_child(struct child *first_child)
{
- const struct child *blanket, *next;
+ const struct child *current, *next;
- if (!(blanket = firstborn))
+ if (!(current = first_child))
return;
- for (; (next = blanket->next); blanket = next)
- if (!addrcmp(&blanket->address, &next->address)) {
- kill(blanket->cld.pid, SIGTERM);
+ for (; (next = current->next); current = next)
+ if (!addrcmp(¤t->address, &next->address)) {
+ kill(current->cld.pid, SIGTERM);
break;
}
}
-void check_dead_children(struct child *firstborn, unsigned int *live_children,
+void check_dead_children(struct child *first_child, unsigned int *live_children,
log_fn loginfo)
{
int status;
pid_t pid;
- struct child **cradle, *blanket;
- for (cradle = &firstborn; (blanket = *cradle);)
- if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
+ struct child **ptr, *current;
+ for (ptr = &first_child; (current = *ptr);)
+ if ((pid = waitpid(current->cld.pid, &status, WNOHANG)) > 1) {
if (loginfo) {
const char *dead = "";
if (status)
@@ -277,10 +277,10 @@ void check_dead_children(struct child *firstborn, unsigned int *live_children,
}
/* remove the child */
- *cradle = blanket->next;
+ *ptr = current->next;
(*live_children)--;
- child_process_clear(&blanket->cld);
- free(blanket);
+ child_process_clear(¤t->cld);
+ free(current);
} else
- cradle = &blanket->next;
+ ptr = ¤t->next;
}
diff --git a/daemon-utils.h b/daemon-utils.h
index 97e5cae20b8..c866e9c9a4e 100644
--- a/daemon-utils.h
+++ b/daemon-utils.h
@@ -32,7 +32,7 @@ struct child {
* live children.
*/
void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
- struct child *firstborn, unsigned int *live_children);
+ struct child *first_child, unsigned int *live_children);
/*
* Kill the newest connection from a duplicate IP.
@@ -40,7 +40,7 @@ void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrl
* This function should be called if the number of connections grows
* past the maximum number of allowed connections.
*/
-void kill_some_child(struct child *firstborn);
+void kill_some_child(struct child *first_child);
/*
* Check for children that have disconnected and remove them from the
@@ -49,7 +49,7 @@ void kill_some_child(struct child *firstborn);
* Optionally log the child PID that disconnected by passing a loginfo
* function.
*/
-void check_dead_children(struct child *firstborn, unsigned int *live_children,
+void check_dead_children(struct child *first_child, unsigned int *live_children,
log_fn loginfo);
#endif
diff --git a/daemon.c b/daemon.c
index ec3b407ecbc..d3e7d81de18 100644
--- a/daemon.c
+++ b/daemon.c
@@ -789,7 +789,7 @@ static int max_connections = 32;
static unsigned int live_children;
-static struct child *firstborn;
+static struct child *first_child;
static struct strvec cld_argv = STRVEC_INIT;
static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
@@ -797,9 +797,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
struct child_process cld = CHILD_PROCESS_INIT;
if (max_connections && live_children >= max_connections) {
- kill_some_child(firstborn);
+ kill_some_child(first_child);
sleep(1); /* give it some time to die */
- check_dead_children(firstborn, &live_children, loginfo);
+ check_dead_children(first_child, &live_children, loginfo);
if (live_children >= max_connections) {
close(incoming);
logerror("Too many children, dropping connection");
@@ -832,7 +832,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
if (start_command(&cld))
logerror("unable to fork");
else
- add_child(&cld, addr, addrlen, firstborn, &live_children);
+ add_child(&cld, addr, addrlen, first_child, &live_children);
}
static void child_handler(int signo)
@@ -862,7 +862,7 @@ static int service_loop(struct socketlist *socklist)
for (;;) {
int i;
- check_dead_children(firstborn, &live_children, loginfo);
+ check_dead_children(first_child, &live_children, loginfo);
if (poll(pfd, socklist->nr, -1) < 0) {
if (errno != EINTR) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 02/12] daemon: libify child process handling functions
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Extract functions and structures for managing child processes started
from the parent daemon-like process from `daemon.c` to the new shared
`daemon-utils.{c,h}` files.
One minor functional change is introduced to `check_dead_children()`
where the logging of a dead/disconnected child is now optional. With the
'libification' of these functions we extract the call to `loginfo` to a
call to a function pointer, and guard the log message creation and
logging behind a `NULL` check. Callers can now skip logging by passing
`NULL` as the `log_fn loginfo` argument.
The behaviour of callers in `daemon.c` remains the same (save one extra
NULL check) however as a pointer to `loginfo` is always passed.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
daemon-utils.c | 77 ++++++++++++++++++++++++++++++++++++++++++
daemon-utils.h | 32 ++++++++++++++++++
daemon.c | 92 +++-----------------------------------------------
3 files changed, 114 insertions(+), 87 deletions(-)
diff --git a/daemon-utils.c b/daemon-utils.c
index b96b55962db..8506664b440 100644
--- a/daemon-utils.c
+++ b/daemon-utils.c
@@ -207,3 +207,80 @@ void socksetup(struct string_list *listen_addr, int listen_port,
}
}
}
+
+static int addrcmp(const struct sockaddr_storage *s1,
+ const struct sockaddr_storage *s2)
+{
+ const struct sockaddr *sa1 = (const struct sockaddr*) s1;
+ const struct sockaddr *sa2 = (const struct sockaddr*) s2;
+
+ if (sa1->sa_family != sa2->sa_family)
+ return sa1->sa_family - sa2->sa_family;
+ if (sa1->sa_family == AF_INET)
+ return memcmp(&((struct sockaddr_in *)s1)->sin_addr,
+ &((struct sockaddr_in *)s2)->sin_addr,
+ sizeof(struct in_addr));
+#ifndef NO_IPV6
+ if (sa1->sa_family == AF_INET6)
+ return memcmp(&((struct sockaddr_in6 *)s1)->sin6_addr,
+ &((struct sockaddr_in6 *)s2)->sin6_addr,
+ sizeof(struct in6_addr));
+#endif
+ return 0;
+}
+
+void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
+ struct child *firstborn , unsigned int *live_children)
+{
+ struct child *newborn, **cradle;
+
+ CALLOC_ARRAY(newborn, 1);
+ (*live_children)++;
+ memcpy(&newborn->cld, cld, sizeof(*cld));
+ memcpy(&newborn->address, addr, addrlen);
+ for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
+ if (!addrcmp(&(*cradle)->address, &newborn->address))
+ break;
+ newborn->next = *cradle;
+ *cradle = newborn;
+}
+
+void kill_some_child(struct child *firstborn)
+{
+ const struct child *blanket, *next;
+
+ if (!(blanket = firstborn))
+ return;
+
+ for (; (next = blanket->next); blanket = next)
+ if (!addrcmp(&blanket->address, &next->address)) {
+ kill(blanket->cld.pid, SIGTERM);
+ break;
+ }
+}
+
+void check_dead_children(struct child *firstborn, unsigned int *live_children,
+ log_fn loginfo)
+{
+ int status;
+ pid_t pid;
+
+ struct child **cradle, *blanket;
+ for (cradle = &firstborn; (blanket = *cradle);)
+ if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
+ if (loginfo) {
+ const char *dead = "";
+ if (status)
+ dead = " (with error)";
+ loginfo("[%"PRIuMAX"] Disconnected%s",
+ (uintmax_t)pid, dead);
+ }
+
+ /* remove the child */
+ *cradle = blanket->next;
+ (*live_children)--;
+ child_process_clear(&blanket->cld);
+ free(blanket);
+ } else
+ cradle = &blanket->next;
+}
diff --git a/daemon-utils.h b/daemon-utils.h
index 6710a2a6dc0..97e5cae20b8 100644
--- a/daemon-utils.h
+++ b/daemon-utils.h
@@ -2,6 +2,7 @@
#define DAEMON_UTILS_H
#include "git-compat-util.h"
+#include "run-command.h"
#include "string-list.h"
typedef void (*log_fn)(const char *msg, ...);
@@ -20,4 +21,35 @@ void socksetup(struct string_list *listen_addr, int listen_port,
struct socketlist *socklist, int reuseaddr,
log_fn logerror);
+struct child {
+ struct child *next;
+ struct child_process cld;
+ struct sockaddr_storage address;
+};
+
+/*
+ * Add the child_process to the set of children and increment the number of
+ * live children.
+ */
+void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
+ struct child *firstborn, unsigned int *live_children);
+
+/*
+ * Kill the newest connection from a duplicate IP.
+ *
+ * This function should be called if the number of connections grows
+ * past the maximum number of allowed connections.
+ */
+void kill_some_child(struct child *firstborn);
+
+/*
+ * Check for children that have disconnected and remove them from the
+ * active set, decrementing the number of live children.
+ *
+ * Optionally log the child PID that disconnected by passing a loginfo
+ * function.
+ */
+void check_dead_children(struct child *firstborn, unsigned int *live_children,
+ log_fn loginfo);
+
#endif
diff --git a/daemon.c b/daemon.c
index 1ed4e705680..ec3b407ecbc 100644
--- a/daemon.c
+++ b/daemon.c
@@ -785,93 +785,11 @@ static int execute(void)
return -1;
}
-static int addrcmp(const struct sockaddr_storage *s1,
- const struct sockaddr_storage *s2)
-{
- const struct sockaddr *sa1 = (const struct sockaddr*) s1;
- const struct sockaddr *sa2 = (const struct sockaddr*) s2;
-
- if (sa1->sa_family != sa2->sa_family)
- return sa1->sa_family - sa2->sa_family;
- if (sa1->sa_family == AF_INET)
- return memcmp(&((struct sockaddr_in *)s1)->sin_addr,
- &((struct sockaddr_in *)s2)->sin_addr,
- sizeof(struct in_addr));
-#ifndef NO_IPV6
- if (sa1->sa_family == AF_INET6)
- return memcmp(&((struct sockaddr_in6 *)s1)->sin6_addr,
- &((struct sockaddr_in6 *)s2)->sin6_addr,
- sizeof(struct in6_addr));
-#endif
- return 0;
-}
-
static int max_connections = 32;
static unsigned int live_children;
-static struct child {
- struct child *next;
- struct child_process cld;
- struct sockaddr_storage address;
-} *firstborn;
-
-static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)
-{
- struct child *newborn, **cradle;
-
- CALLOC_ARRAY(newborn, 1);
- live_children++;
- memcpy(&newborn->cld, cld, sizeof(*cld));
- memcpy(&newborn->address, addr, addrlen);
- for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
- if (!addrcmp(&(*cradle)->address, &newborn->address))
- break;
- newborn->next = *cradle;
- *cradle = newborn;
-}
-
-/*
- * This gets called if the number of connections grows
- * past "max_connections".
- *
- * We kill the newest connection from a duplicate IP.
- */
-static void kill_some_child(void)
-{
- const struct child *blanket, *next;
-
- if (!(blanket = firstborn))
- return;
-
- for (; (next = blanket->next); blanket = next)
- if (!addrcmp(&blanket->address, &next->address)) {
- kill(blanket->cld.pid, SIGTERM);
- break;
- }
-}
-
-static void check_dead_children(void)
-{
- int status;
- pid_t pid;
-
- struct child **cradle, *blanket;
- for (cradle = &firstborn; (blanket = *cradle);)
- if ((pid = waitpid(blanket->cld.pid, &status, WNOHANG)) > 1) {
- const char *dead = "";
- if (status)
- dead = " (with error)";
- loginfo("[%"PRIuMAX"] Disconnected%s", (uintmax_t)pid, dead);
-
- /* remove the child */
- *cradle = blanket->next;
- live_children--;
- child_process_clear(&blanket->cld);
- free(blanket);
- } else
- cradle = &blanket->next;
-}
+static struct child *firstborn;
static struct strvec cld_argv = STRVEC_INIT;
static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
@@ -879,9 +797,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
struct child_process cld = CHILD_PROCESS_INIT;
if (max_connections && live_children >= max_connections) {
- kill_some_child();
+ kill_some_child(firstborn);
sleep(1); /* give it some time to die */
- check_dead_children();
+ check_dead_children(firstborn, &live_children, loginfo);
if (live_children >= max_connections) {
close(incoming);
logerror("Too many children, dropping connection");
@@ -914,7 +832,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
if (start_command(&cld))
logerror("unable to fork");
else
- add_child(&cld, addr, addrlen);
+ add_child(&cld, addr, addrlen, firstborn, &live_children);
}
static void child_handler(int signo)
@@ -944,7 +862,7 @@ static int service_loop(struct socketlist *socklist)
for (;;) {
int i;
- check_dead_children();
+ check_dead_children(firstborn, &live_children, loginfo);
if (poll(pfd, socklist->nr, -1) < 0) {
if (errno != EINTR) {
--
gitgitgadget
^ permalink raw reply related
* [PATCH v6 01/12] daemon: libify socket setup and option functions
From: Matthew John Cheetham via GitGitGadget @ 2023-01-18 3:30 UTC (permalink / raw)
To: git
Cc: Derrick Stolee, Lessley Dennington, Matthew John Cheetham,
M Hickford, Jeff Hostetler, Glen Choo, Victoria Dye,
Ævar Arnfjörð Bjarmason, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <pull.1352.v6.git.1674012618.gitgitgadget@gmail.com>
From: Matthew John Cheetham <mjcheetham@outlook.com>
Extract functions for setting up listening sockets and keep-alive options
from `daemon.c` to new `daemon-utils.{c,h}` files. Remove direct
dependencies on global state by inlining the behaviour at the callsites
for all libified functions.
Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
Makefile | 1 +
daemon-utils.c | 209 +++++++++++++++++++++++++++++++++++++++++++++++
daemon-utils.h | 23 ++++++
daemon.c | 214 +------------------------------------------------
4 files changed, 237 insertions(+), 210 deletions(-)
create mode 100644 daemon-utils.c
create mode 100644 daemon-utils.h
diff --git a/Makefile b/Makefile
index b258fdbed86..2654094dbb5 100644
--- a/Makefile
+++ b/Makefile
@@ -1003,6 +1003,7 @@ LIB_OBJS += credential.o
LIB_OBJS += csum-file.o
LIB_OBJS += ctype.o
LIB_OBJS += date.o
+LIB_OBJS += daemon-utils.o
LIB_OBJS += decorate.o
LIB_OBJS += delta-islands.o
LIB_OBJS += diagnose.o
diff --git a/daemon-utils.c b/daemon-utils.c
new file mode 100644
index 00000000000..b96b55962db
--- /dev/null
+++ b/daemon-utils.c
@@ -0,0 +1,209 @@
+#include "cache.h"
+#include "daemon-utils.h"
+
+void set_keep_alive(int sockfd, log_fn logerror)
+{
+ int ka = 1;
+
+ if (setsockopt(sockfd, SOL_SOCKET, SO_KEEPALIVE, &ka, sizeof(ka)) < 0) {
+ if (errno != ENOTSOCK)
+ logerror("unable to set SO_KEEPALIVE on socket: %s",
+ strerror(errno));
+ }
+}
+
+static int set_reuse_addr(int sockfd)
+{
+ int on = 1;
+
+ return setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR,
+ &on, sizeof(on));
+}
+
+static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
+{
+#ifdef NO_IPV6
+ static char ip[INET_ADDRSTRLEN];
+#else
+ static char ip[INET6_ADDRSTRLEN];
+#endif
+
+ switch (family) {
+#ifndef NO_IPV6
+ case AF_INET6:
+ inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
+ break;
+#endif
+ case AF_INET:
+ inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
+ break;
+ default:
+ xsnprintf(ip, sizeof(ip), "<unknown>");
+ }
+ return ip;
+}
+
+#ifndef NO_IPV6
+
+static int setup_named_sock(char *listen_addr, int listen_port,
+ struct socketlist *socklist, int reuseaddr,
+ log_fn logerror)
+{
+ int socknum = 0;
+ char pbuf[NI_MAXSERV];
+ struct addrinfo hints, *ai0, *ai;
+ int gai;
+ long flags;
+
+ xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
+ hints.ai_protocol = IPPROTO_TCP;
+ hints.ai_flags = AI_PASSIVE;
+
+ gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
+ if (gai) {
+ logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));
+ return 0;
+ }
+
+ for (ai = ai0; ai; ai = ai->ai_next) {
+ int sockfd;
+
+ sockfd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
+ if (sockfd < 0)
+ continue;
+ if (sockfd >= FD_SETSIZE) {
+ logerror("Socket descriptor too large");
+ close(sockfd);
+ continue;
+ }
+
+#ifdef IPV6_V6ONLY
+ if (ai->ai_family == AF_INET6) {
+ int on = 1;
+ setsockopt(sockfd, IPPROTO_IPV6, IPV6_V6ONLY,
+ &on, sizeof(on));
+ /* Note: error is not fatal */
+ }
+#endif
+
+ if (reuseaddr && set_reuse_addr(sockfd)) {
+ logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
+ close(sockfd);
+ continue;
+ }
+
+ set_keep_alive(sockfd, logerror);
+
+ if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
+ logerror("Could not bind to %s: %s",
+ ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+ strerror(errno));
+ close(sockfd);
+ continue; /* not fatal */
+ }
+ if (listen(sockfd, 5) < 0) {
+ logerror("Could not listen to %s: %s",
+ ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
+ strerror(errno));
+ close(sockfd);
+ continue; /* not fatal */
+ }
+
+ flags = fcntl(sockfd, F_GETFD, 0);
+ if (flags >= 0)
+ fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+
+ ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+ socklist->list[socklist->nr++] = sockfd;
+ socknum++;
+ }
+
+ freeaddrinfo(ai0);
+
+ return socknum;
+}
+
+#else /* NO_IPV6 */
+
+static int setup_named_sock(char *listen_addr, int listen_port,
+ struct socketlist *socklist, int reuseaddr,
+ log_fn logerror)
+{
+ struct sockaddr_in sin;
+ int sockfd;
+ long flags;
+
+ memset(&sin, 0, sizeof sin);
+ sin.sin_family = AF_INET;
+ sin.sin_port = htons(listen_port);
+
+ if (listen_addr) {
+ /* Well, host better be an IP address here. */
+ if (inet_pton(AF_INET, listen_addr, &sin.sin_addr.s_addr) <= 0)
+ return 0;
+ } else {
+ sin.sin_addr.s_addr = htonl(INADDR_ANY);
+ }
+
+ sockfd = socket(AF_INET, SOCK_STREAM, 0);
+ if (sockfd < 0)
+ return 0;
+
+ if (reuseaddr && set_reuse_addr(sockfd)) {
+ logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
+ close(sockfd);
+ return 0;
+ }
+
+ set_keep_alive(sockfd, logerror);
+
+ if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
+ logerror("Could not bind to %s: %s",
+ ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+ strerror(errno));
+ close(sockfd);
+ return 0;
+ }
+
+ if (listen(sockfd, 5) < 0) {
+ logerror("Could not listen to %s: %s",
+ ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
+ strerror(errno));
+ close(sockfd);
+ return 0;
+ }
+
+ flags = fcntl(sockfd, F_GETFD, 0);
+ if (flags >= 0)
+ fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
+
+ ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
+ socklist->list[socklist->nr++] = sockfd;
+ return 1;
+}
+
+#endif
+
+void socksetup(struct string_list *listen_addr, int listen_port,
+ struct socketlist *socklist, int reuseaddr,
+ log_fn logerror)
+{
+ if (!listen_addr->nr)
+ setup_named_sock(NULL, listen_port, socklist, reuseaddr,
+ logerror);
+ else {
+ int i, socknum;
+ for (i = 0; i < listen_addr->nr; i++) {
+ socknum = setup_named_sock(listen_addr->items[i].string,
+ listen_port, socklist, reuseaddr,
+ logerror);
+
+ if (socknum == 0)
+ logerror("unable to allocate any listen sockets for host %s on port %u",
+ listen_addr->items[i].string, listen_port);
+ }
+ }
+}
diff --git a/daemon-utils.h b/daemon-utils.h
new file mode 100644
index 00000000000..6710a2a6dc0
--- /dev/null
+++ b/daemon-utils.h
@@ -0,0 +1,23 @@
+#ifndef DAEMON_UTILS_H
+#define DAEMON_UTILS_H
+
+#include "git-compat-util.h"
+#include "string-list.h"
+
+typedef void (*log_fn)(const char *msg, ...);
+
+struct socketlist {
+ int *list;
+ size_t nr;
+ size_t alloc;
+};
+
+/* Enable sending of keep-alive messages on the socket. */
+void set_keep_alive(int sockfd, log_fn logerror);
+
+/* Setup a number of sockets to listen on the provided addresses. */
+void socksetup(struct string_list *listen_addr, int listen_port,
+ struct socketlist *socklist, int reuseaddr,
+ log_fn logerror);
+
+#endif
diff --git a/daemon.c b/daemon.c
index 0ae7d12b5c1..1ed4e705680 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,9 +1,9 @@
#include "cache.h"
#include "config.h"
+#include "daemon-utils.h"
#include "pkt-line.h"
#include "run-command.h"
#include "strbuf.h"
-#include "string-list.h"
#ifdef NO_INITGROUPS
#define initgroups(x, y) (0) /* nothing */
@@ -737,17 +737,6 @@ static void hostinfo_clear(struct hostinfo *hi)
strbuf_release(&hi->tcp_port);
}
-static void set_keep_alive(int sockfd)
-{
- int ka = 1;
-
- if (setsockopt(sockfd, SOL_SOCKET, SO_KEEPALIVE, &ka, sizeof(ka)) < 0) {
- if (errno != ENOTSOCK)
- logerror("unable to set SO_KEEPALIVE on socket: %s",
- strerror(errno));
- }
-}
-
static int execute(void)
{
char *line = packet_buffer;
@@ -759,7 +748,7 @@ static int execute(void)
if (addr)
loginfo("Connection from %s:%s", addr, port);
- set_keep_alive(0);
+ set_keep_alive(0, logerror);
alarm(init_timeout ? init_timeout : timeout);
pktlen = packet_read(0, packet_buffer, sizeof(packet_buffer), 0);
alarm(0);
@@ -938,202 +927,6 @@ static void child_handler(int signo)
signal(SIGCHLD, child_handler);
}
-static int set_reuse_addr(int sockfd)
-{
- int on = 1;
-
- if (!reuseaddr)
- return 0;
- return setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR,
- &on, sizeof(on));
-}
-
-struct socketlist {
- int *list;
- size_t nr;
- size_t alloc;
-};
-
-static const char *ip2str(int family, struct sockaddr *sin, socklen_t len)
-{
-#ifdef NO_IPV6
- static char ip[INET_ADDRSTRLEN];
-#else
- static char ip[INET6_ADDRSTRLEN];
-#endif
-
- switch (family) {
-#ifndef NO_IPV6
- case AF_INET6:
- inet_ntop(family, &((struct sockaddr_in6*)sin)->sin6_addr, ip, len);
- break;
-#endif
- case AF_INET:
- inet_ntop(family, &((struct sockaddr_in*)sin)->sin_addr, ip, len);
- break;
- default:
- xsnprintf(ip, sizeof(ip), "<unknown>");
- }
- return ip;
-}
-
-#ifndef NO_IPV6
-
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
-{
- int socknum = 0;
- char pbuf[NI_MAXSERV];
- struct addrinfo hints, *ai0, *ai;
- int gai;
- long flags;
-
- xsnprintf(pbuf, sizeof(pbuf), "%d", listen_port);
- memset(&hints, 0, sizeof(hints));
- hints.ai_family = AF_UNSPEC;
- hints.ai_socktype = SOCK_STREAM;
- hints.ai_protocol = IPPROTO_TCP;
- hints.ai_flags = AI_PASSIVE;
-
- gai = getaddrinfo(listen_addr, pbuf, &hints, &ai0);
- if (gai) {
- logerror("getaddrinfo() for %s failed: %s", listen_addr, gai_strerror(gai));
- return 0;
- }
-
- for (ai = ai0; ai; ai = ai->ai_next) {
- int sockfd;
-
- sockfd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
- if (sockfd < 0)
- continue;
- if (sockfd >= FD_SETSIZE) {
- logerror("Socket descriptor too large");
- close(sockfd);
- continue;
- }
-
-#ifdef IPV6_V6ONLY
- if (ai->ai_family == AF_INET6) {
- int on = 1;
- setsockopt(sockfd, IPPROTO_IPV6, IPV6_V6ONLY,
- &on, sizeof(on));
- /* Note: error is not fatal */
- }
-#endif
-
- if (set_reuse_addr(sockfd)) {
- logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
- close(sockfd);
- continue;
- }
-
- set_keep_alive(sockfd);
-
- if (bind(sockfd, ai->ai_addr, ai->ai_addrlen) < 0) {
- logerror("Could not bind to %s: %s",
- ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
- strerror(errno));
- close(sockfd);
- continue; /* not fatal */
- }
- if (listen(sockfd, 5) < 0) {
- logerror("Could not listen to %s: %s",
- ip2str(ai->ai_family, ai->ai_addr, ai->ai_addrlen),
- strerror(errno));
- close(sockfd);
- continue; /* not fatal */
- }
-
- flags = fcntl(sockfd, F_GETFD, 0);
- if (flags >= 0)
- fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
-
- ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
- socklist->list[socklist->nr++] = sockfd;
- socknum++;
- }
-
- freeaddrinfo(ai0);
-
- return socknum;
-}
-
-#else /* NO_IPV6 */
-
-static int setup_named_sock(char *listen_addr, int listen_port, struct socketlist *socklist)
-{
- struct sockaddr_in sin;
- int sockfd;
- long flags;
-
- memset(&sin, 0, sizeof sin);
- sin.sin_family = AF_INET;
- sin.sin_port = htons(listen_port);
-
- if (listen_addr) {
- /* Well, host better be an IP address here. */
- if (inet_pton(AF_INET, listen_addr, &sin.sin_addr.s_addr) <= 0)
- return 0;
- } else {
- sin.sin_addr.s_addr = htonl(INADDR_ANY);
- }
-
- sockfd = socket(AF_INET, SOCK_STREAM, 0);
- if (sockfd < 0)
- return 0;
-
- if (set_reuse_addr(sockfd)) {
- logerror("Could not set SO_REUSEADDR: %s", strerror(errno));
- close(sockfd);
- return 0;
- }
-
- set_keep_alive(sockfd);
-
- if ( bind(sockfd, (struct sockaddr *)&sin, sizeof sin) < 0 ) {
- logerror("Could not bind to %s: %s",
- ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
- strerror(errno));
- close(sockfd);
- return 0;
- }
-
- if (listen(sockfd, 5) < 0) {
- logerror("Could not listen to %s: %s",
- ip2str(AF_INET, (struct sockaddr *)&sin, sizeof(sin)),
- strerror(errno));
- close(sockfd);
- return 0;
- }
-
- flags = fcntl(sockfd, F_GETFD, 0);
- if (flags >= 0)
- fcntl(sockfd, F_SETFD, flags | FD_CLOEXEC);
-
- ALLOC_GROW(socklist->list, socklist->nr + 1, socklist->alloc);
- socklist->list[socklist->nr++] = sockfd;
- return 1;
-}
-
-#endif
-
-static void socksetup(struct string_list *listen_addr, int listen_port, struct socketlist *socklist)
-{
- if (!listen_addr->nr)
- setup_named_sock(NULL, listen_port, socklist);
- else {
- int i, socknum;
- for (i = 0; i < listen_addr->nr; i++) {
- socknum = setup_named_sock(listen_addr->items[i].string,
- listen_port, socklist);
-
- if (socknum == 0)
- logerror("unable to allocate any listen sockets for host %s on port %u",
- listen_addr->items[i].string, listen_port);
- }
- }
-}
-
static int service_loop(struct socketlist *socklist)
{
struct pollfd *pfd;
@@ -1246,7 +1039,8 @@ static int serve(struct string_list *listen_addr, int listen_port,
{
struct socketlist socklist = { NULL, 0, 0 };
- socksetup(listen_addr, listen_port, &socklist);
+ socksetup(listen_addr, listen_port, &socklist, reuseaddr,
+ logerror);
if (socklist.nr == 0)
die("unable to allocate any listen sockets on port %u",
listen_port);
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] avoiding deprecated curl options
From: Ramsay Jones @ 2023-01-18 1:03 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git
In-Reply-To: <Y8YP+R/hyNr6sEFA@coredump.intra.peff.net>
On 17/01/2023 03:03, Jeff King wrote:
> On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote:
>
>> So I took a look at just dropping the deprecated bits, and it wasn't
>> _too_ bad. Here's that series. The first two I hope are obviously good.
>> The third one is _ugly_, but at least it punts on the whole "how should
>> we silence this" argument, and it takes us in the direction we'd
>> ultimately want to go.
>>
>> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT
>> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION
>> [3/3]: http: support CURLOPT_PROTOCOLS_STR
>
> In the interests of wrapping this up, here's a v2 that:
>
> - bumps the required curl version to 7.19.5 in patch 2
>
> - aims for slightly better readability in the final code of patch 3,
> versus minimizing the diff
>
I have a _slight_ preference for your v1 patches, but I don't hate
this version either! :)
Tonight, I have compile tested both v1 and v2 patches (both in 'seen')
on cygwin and linux. I would like to say I have run the tests as well,
but it seems I have disabled all the tests on cygwin (expected) *and*
on linux (most unexpected).
ie. I don't have apache installed. (I used to have apache, svn and cvs
installed to run the tests, but they just took too long to run and
caused *many* test runs to hang and leave server processes all over
the place. On cygwin, even with all of those tests skipped, it still
takes approx 5.5 hours to run the tests).
I thought I was still running the '*http*' tests on linux, but I seem
to have dropped the installation of apache at some point - oops!
I guess I should install apache tomorrow ...
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree
From: Junio C Hamano @ 2023-01-17 23:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Eric Sunshine
In-Reply-To: <eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
> We prevent some operations from being executed on a branch checked out
> in a worktree other than the current one. An example of this was
> introduced in b5cabb4 (rebase: refuse to switch to branch already
> checked out elsewhere, 2020-02-23).
>
> "find_shared_symref()" is sometimes used to find the worktree in which a
> branch is checked out. It performs its search starting with the current
> worktree.
"starting with the current" may be a correct statement of the fact,
but it is totally unclear what the relevance it has to the problem
being solved. Rather, it is unclear what problem you are solving.
Is it
- We search through the worktrees, starting with the current one,
and stop at the first one found.
- If the current branch the the current worktree is checked out in
a different worktree, we get the current worktree back.
- There are callers that want to know ONLY about other worktrees;
they check the returned value and when they see it is the current
one, they happily ignores the fact that it might be checked out
elsewhere as well.
> As we allow to have the same branch checked out in multiple worktrees
> simultaneously...
>
> $ git worktree add foo
> $ git worktree add -f bar foo
> $ git checkout --ignore-other-worktrees foo
>
> ... if the branch checked out in the current worktree is also checked
> out in another worktree, with "find_shared_symref()" we will not notice
> this "other" working tree.
It is somewhat disturbing that your solution only needs to "ignore"
the current one. Whatever problem you are seeing by the current
code not ignoring the current worktree, wouldn't we have a similar
problem if two non-current worktrees checked out the same branch?
Would it not be a problem because any non-current worktree returned
by the function triggers the "already checked-out" safety mechanism?
^ permalink raw reply
* [PATCH] fsm-listen-daarwin: combine bit operations
From: Rose via GitGitGadget @ 2023-01-17 21:25 UTC (permalink / raw)
To: git; +Cc: Rose, Seija Kijin
From: Seija Kijin <doremylover123@gmail.com>
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
fsm-listen-daarwin: combine bit operations
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1437%2FAtariDreams%2Fdarwin-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1437/AtariDreams/darwin-v1
Pull-Request: https://github.com/git/git/pull/1437
compat/fsmonitor/fsm-listen-darwin.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index 97a55a6f0a4..fccdd21d858 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -129,9 +129,9 @@ static int ef_is_root_renamed(const FSEventStreamEventFlags ef)
static int ef_is_dropped(const FSEventStreamEventFlags ef)
{
- return (ef & kFSEventStreamEventFlagMustScanSubDirs ||
- ef & kFSEventStreamEventFlagKernelDropped ||
- ef & kFSEventStreamEventFlagUserDropped);
+ return (ef & (kFSEventStreamEventFlagMustScanSubDirs |
+ kFSEventStreamEventFlagKernelDropped |
+ kFSEventStreamEventFlagUserDropped));
}
/*
base-commit: a7caae2729742fc80147bca1c02ae848cb55921a
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v5 10/10] credential: add WWW-Authenticate header to cred requests
From: Matthew John Cheetham @ 2023-01-17 21:18 UTC (permalink / raw)
To: Derrick Stolee, Matthew John Cheetham via GitGitGadget, git
Cc: Lessley Dennington, M Hickford, Jeff Hostetler, Glen Choo,
Victoria Dye, Matthew John Cheetham
In-Reply-To: <98940e93-c4c5-01ec-54b2-b6015f488ad0@github.com>
On 2023-01-12 12:41, Derrick Stolee wrote:
> On 1/11/2023 5:13 PM, Matthew John Cheetham via GitGitGadget wrote:
>
>> +static void credential_write_strvec(FILE *fp, const char *key,
>> + const struct strvec *vec)
>> +{
>> + int i = 0;
>> + const char *full_key = xstrfmt("%s[]", key);
>> + for (; i < vec->nr; i++) {
>
> style nit: use "int i;" and "for (i = 0; ..."
Thanks for pointing this out; I missed that C99 style for-loops
were allowed now. As Ævar pointed out, this should also be `size_t`
and not `int`.
>> 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" &&
>
> I see that you added auth.allowAnonymous and --auth-config options
> in Patch 6, so perhaps this test change could move to that patch.
Good point; will update on reroll.
> Thanks,
> -Stolee
^ permalink raw reply
* Re: [PATCH v5 06/10] test-http-server: add simple authentication
From: Matthew John Cheetham @ 2023-01-17 21:21 UTC (permalink / raw)
To: Victoria Dye, Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, M Hickford, Jeff Hostetler,
Glen Choo, Matthew John Cheetham
In-Reply-To: <2a0b5f3f-7ab2-bc9e-76ac-93a52b4d32d0@github.com>
On 2023-01-13 10:10, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> +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 = NULL;
>> +
>> + if (!strcmp(name, "auth.challenge")) {
>> + if (split_auth_param(val, &scheme, &challenge, 0)) {
>> + ret = error("invalid auth challenge '%s'", val);
>> + goto cleanup;
>> + }
>> +
>> + mod = create_auth_module(scheme, challenge);
>> + if (add_auth_module(mod)) {
>> + ret = error("failed to add auth module '%s'", val);
>> + goto cleanup;
>> + }
>> + }
>> + if (!strcmp(name, "auth.token")) {
>> + if (split_auth_param(val, &scheme, &token, 1)) {
>> + ret = error("invalid auth token '%s'", val);
>> + goto cleanup;
>> + }
>> +
>> + mod = get_auth_module(scheme);
>> + if (!mod) {
>> + ret = error("auth scheme not defined '%s'\n", scheme);
>> + goto cleanup;
>> + }
>> +
>> + string_list_append(mod->tokens, token);
>> + }
>
> I don't think this addresses the implicit option ordering requirement noted
> in [3]; instead of needing '--auth' before '--auth-token', this now needs
> 'auth.challenge' before 'auth.token' in the config file. While I'd prefer it
> if this could be rearranged so that the auth setup happens after all config
> parsing (so the order doesn't matter), if you want to leave it as-is please
> add a comment somewhere in this file explaining that requirement and/or add
> a note to the "auth scheme not defined" error message.
>
> [3] https://lore.kernel.org/git/2a5d6586-3d2c-8af4-12be-a5a106f966b5@github.com/
>
>> + if (!strcmp(name, "auth.allowanonymous")) {
>> + allow_anonymous = git_config_bool(name, val);
>> + }
>> + if (!strcmp(name, "auth.extraheader")) {
>> + strvec_push(&extra_headers, val);
>> + }
>
> Is it worth printing a warning if the option found isn't any of the above?
> Something like "ignoring <config option>". This is a test helper, so
> user-friendliness isn't quite as important as it is for builtins, but the
> warning might be helpful to developers trying to use it in the future.
I tried this suggestion of adding a warning, but it felt wrong. You are correct
in the first instance that it should really "just work" when specified in any
order. Watch for the next iteration where I'll make it such you can specify them
in any order :-)
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v7 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Strawbridge, Michael @ 2023-01-17 20:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Tuikov, Luben
In-Reply-To: <xmqqedrt9mmt.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:
>
>> +This hook is invoked by linkgit:git-send-email[1].
>> +
>> +It takes these command line arguments. They are,
>> +1. the name of the file which holds the contents of the email to be sent.
>> +2. The name of the file which holds the SMTP envelope and headers of the email.
>
> The previous iteration said SMTP headers, but now this talks about
> envelope. I however did not think we have direct access to any
> envelope information [*] (i.e. such a feature is necessary if we
> want to send to one set of recipients by specifying their addresses
> in the envelope, while recording different set of recipients to the
> e-mail headers' To: and Cc: list)?
>
> Side note. We can specify different sender and it gets
> passed as a command line argument "-f $sender" to the
> sendmail program, which is used in "MAIL FROM" SMTP
> envelope. But I do not think we muck with the list of
> recipients that appear in the header when we formulate "RCPT
> TO". And I do not see where you give "MAIL FROM" value in
> the input to the hook in the patch.
>
> If we say "we will give your hook information in the envelope and
> headers" to those who know the distinction between the two, they
> will inevitably say "that is great. Now how do I tell which in file
> $2 are in the envelope and which are in the header, when some of
> them are different?"
>
> We just hand the message plus the header, and let $sendmail_cmd come
> up with the envelope info using what is in the header, no? I am not
> sure we want to mention envelope as that would give readers a false
> impression that we may treat it separately from the headers.
>
> Thanks.
That's fair. I will remove the mention of envelope.
^ permalink raw reply
* Re: [PATCH v5 05/10] test-http-server: add HTTP error response function
From: Matthew John Cheetham @ 2023-01-17 21:23 UTC (permalink / raw)
To: Victoria Dye, Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, M Hickford, Jeff Hostetler,
Glen Choo, Matthew John Cheetham
In-Reply-To: <0a5f4195-600f-d099-5879-bbd7629285b2@github.com>
On 2023-01-12 12:35, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Introduce a function to the test-http-server test helper to write more
>> full and valid HTTP error responses, including all the standard response
>> headers like `Server` and `Date`.
>
> It took me a second to figure out, but this patch combines the content of
> patches 4, 5, and 6 from the last iteration. After squashing those three
> patches from v4 together locally, the range-diff is actually pretty simple
> (see below).
>
> Out of curiosity, why did you combine those patches? I don't feel strongly
> about changing it, but the smaller, incremental patches in the previous
> version were a bit easier to review.
This is my mistake. I didn't intend to do this, and agree splitting them is
easier to grok. I will restore this! My apologies! :-(
> In any case, this version addresses my feedback from [1], [2], and [3] - the
> explanatory comments are particularly helpful. Thanks!
>
> [1] https://lore.kernel.org/git/7b7d1059-cecf-744d-6927-b41963b9e5a8@github.com/
> [2] https://lore.kernel.org/git/e957d4f4-fa94-7a68-f378-38e6ed131244@github.com/
> [3] https://lore.kernel.org/git/f99c381c-1d30-7c95-6158-cecd5321dafd@github.com/
>
> Range diff v4 (patches 4-6, squashed) vs. v5 (this patch)
>
> 4: 127827637e ! 5: 6f66bf146b test-http-server: add HTTP error response function
> @@ Commit message
>
> ## t/helper/test-http-server.c ##
> @@ t/helper/test-http-server.c: enum worker_result {
> - WR_STOP_THE_MUSIC = (WR_IO_ERROR | WR_HANGUP),
> + WR_HANGUP = 1<<1,
> };
>
> +/*
> @@ t/helper/test-http-server.c: enum worker_result {
> + hp = strbuf_detach(&h, NULL);
> + string_list_append(&req->header_list, hp);
> +
> -+ /* store common request headers separately */
> ++ /* also store common request headers as struct req members */
> + if (skip_prefix(hp, "Content-Type: ", &hv)) {
> + req->content_type = hv;
> + } else if (skip_prefix(hp, "Content-Length: ", &hv)) {
> @@ t/helper/test-http-server.c: enum worker_result {
> +
> + if (!initialized) {
> + smart_http_regex = xmalloc(sizeof(*smart_http_regex));
> ++ /*
> ++ * This regular expression matches all dumb and smart HTTP
> ++ * requests that are currently in use, and defined in
> ++ * Documentation/gitprotocol-http.txt.
> ++ *
> ++ */
> + if (regcomp(smart_http_regex, "^/(HEAD|info/refs|"
> + "objects/info/[^/]+|git-(upload|receive)-pack)$",
> + REG_EXTENDED)) {
> @@ t/helper/test-http-server.c: enum worker_result {
> + !regexec(smart_http_regex, req->uri_path.buf, 0, NULL, 0);
> +}
> +
> -+static enum worker_result do__git(struct req *req, const char *user)
> ++static enum worker_result do__git(struct req *req)
> +{
> + 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)
> ++ /*
> ++ * Note that 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.
> ++ *
> ++ * 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);
> @@ t/helper/test-http-server.c: enum worker_result {
> + cp.git_cmd = 1;
> + strvec_push(&cp.args, "http-backend");
> + res = run_command(&cp);
> -+ close(1);
> -+ close(0);
> ++ close(STDOUT_FILENO);
> ++ close(STDIN_FILENO);
> + return !!res;
> +}
> +
> +static enum worker_result dispatch(struct req *req)
> +{
> + if (is_git_request(req))
> -+ return do__git(req, NULL);
> ++ return do__git(req);
> +
> -+ return send_http_error(1, 501, "Not Implemented", -1, NULL,
> ++ return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL,
> + WR_OK | WR_HANGUP);
> +}
> +
> @@ t/helper/test-http-server.c: enum worker_result {
> char *client_port = getenv("REMOTE_PORT");
> enum worker_result wr = WR_OK;
> @@ t/helper/test-http-server.c: static enum worker_result worker(void)
> - set_keep_alive(0);
> + set_keep_alive(0, logerror);
>
> while (1) {
> -- if (write_in_full(1, response, strlen(response)) < 0) {
> +- if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
> - logerror("unable to write response");
> - wr = WR_IO_ERROR;
> - }
> + req__release(&req);
> +
> -+ alarm(init_timeout ? init_timeout : timeout);
> ++ alarm(timeout);
> + wr = req__read(&req, 0);
> + alarm(0);
> +
> -+ if (wr & WR_STOP_THE_MUSIC)
> ++ if (wr != WR_OK)
> + break;
>
> + wr = dispatch(&req);
> - if (wr & WR_STOP_THE_MUSIC)
> + if (wr != WR_OK)
> break;
> }
>
> @@ t/t5556-http-auth.sh (new)
> +
> +test_description='test http auth header and credential helper interop'
> +
> ++TEST_NO_CREATE_REPO=1
> +. ./test-lib.sh
> +
> +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT
> +
> +# Setup a repository
> +#
> -+REPO_DIR="$(pwd)"/repo
> ++REPO_DIR="$TRASH_DIRECTORY"/repo
> +
> +# Setup some lookback URLs where test-http-server will be listening.
> +# We will spawn it directly inside the repo directory, so we avoid
> @@ t/t5556-http-auth.sh (new)
> +# 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
> ++PID_FILE="$TRASH_DIRECTORY"/pid-file.pid
> ++SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log
> +
> +PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH
> +
> @@ t/t5556-http-auth.sh (new)
> +
> +test_expect_success 'http auth anonymous no challenge' '
> + test_when_finished "per_test_cleanup" &&
> -+ start_http_server --allow-anonymous &&
> ++ start_http_server &&
> +
> + # Attempt to read from a protected repository
> + git ls-remote $ORIGIN_URL
>
^ permalink raw reply
* Re: [PATCH v5 03/10] daemon: rename some esoteric/laboured terminology
From: Matthew John Cheetham @ 2023-01-17 21:16 UTC (permalink / raw)
To: Victoria Dye, Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, M Hickford, Jeff Hostetler,
Glen Choo, Matthew John Cheetham
In-Reply-To: <da31180a-5beb-4f0e-667b-ddceba941e9f@github.com>
On 2023-01-12 11:44, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Rename some of the variables and function arguments used to manage child
>> processes. The existing names are esoteric; stretching an analogy too
>> far to the point of being confusing to understand.
>>
>> Rename "firstborn" to simply "first", "newborn" to "new_cld", "blanket"
>> to "current" and "cradle" to "ptr".
>
> Thanks for this, I agree that the new names make the code much easier to
> read.
>
>> diff --git a/daemon.c b/daemon.c
>> index ec3b407ecbc..d3e7d81de18 100644
>> --- a/daemon.c
>> +++ b/daemon.c
>> @@ -789,7 +789,7 @@ static int max_connections = 32;
>>
>> static unsigned int live_children;
>>
>> -static struct child *firstborn;
>> +static struct child *first_child;
>
> minor nit: you changed "firstborn" to "first" in 'daemon-utils.c' (aligning
> with the commit message), but it's "first_child" here. If you end up
> re-rolling, it would be nice to make the names consistent across both files
> (could be 'first', 'first_child', 'first_cld', or anything really).
>
Fair point. There's no technical reason to keep them named differently between
the 'libified' functions, and the actual variables for the callers.
I shall align these to `first_child` in the next iteration.
Thanks,
Matthew
^ permalink raw reply
* Re: [PATCH v5 02/10] daemon: libify child process handling functions
From: Matthew John Cheetham @ 2023-01-17 21:14 UTC (permalink / raw)
To: Victoria Dye, Matthew John Cheetham via GitGitGadget, git
Cc: Derrick Stolee, Lessley Dennington, M Hickford, Jeff Hostetler,
Glen Choo, Matthew John Cheetham
In-Reply-To: <3a8d1b66-ed06-16a3-5459-9381faa69420@github.com>
On 2023-01-12 11:35, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Extract functions and structures for managing child processes started
>> from the parent daemon-like process from `daemon.c` to the new shared
>> `daemon-utils.{c,h}` files.
>
> As with patch 1, it looks like the main changes here are changing global
> references to function arguments. Specifically, those variables are
> 'firstborn', 'live_children', and 'loginfo':
>
>> -static void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen)
>> +void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
>> + struct child *firstborn , unsigned int *live_children)
>
>> -static void kill_some_child(void)
>> +void kill_some_child(struct child *firstborn)
>
>> -static void check_dead_children(void)
>> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
>> + log_fn loginfo)
>
> Those values are provided by the callers in 'daemon.c'. The major change
> here is that 'live_children' is passed as a pointer, since its value is
> updated by difference is passing 'live_children' as a pointer, since its
> value is updated by 'check_dead_children()' and 'add_child()':
>
>> @@ -879,9 +797,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>> struct child_process cld = CHILD_PROCESS_INIT;
>>
>> if (max_connections && live_children >= max_connections) {
>> - kill_some_child();
>> + kill_some_child(firstborn);
>> sleep(1); /* give it some time to die */
>> - check_dead_children();
>> + check_dead_children(firstborn, &live_children, loginfo);
>> if (live_children >= max_connections) {
>> close(incoming);
>> logerror("Too many children, dropping connection");
>> @@ -914,7 +832,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>> if (start_command(&cld))
>> logerror("unable to fork");
>> else
>> - add_child(&cld, addr, addrlen);
>> + add_child(&cld, addr, addrlen, firstborn, &live_children);
>> }
>>
>> static void child_handler(int signo)
>> @@ -944,7 +862,7 @@ static int service_loop(struct socketlist *socklist)
>> for (;;) {
>> int i;
>>
>> - check_dead_children();
>> + check_dead_children(firstborn, &live_children, loginfo);
>>
>> if (poll(pfd, socklist->nr, -1) < 0) {
>> if (errno != EINTR) {
>
> However, I think that change to 'live_children' may have caused a bug. In
> 'check_dead_children()', you decrement the 'live_children' *pointer*. That
> changes its address, not its value:
>
>> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
>> + log_fn loginfo)
>> +{
> ...
>> + live_children--;
> ...
>> +}
>
> Same thing in 'add_child()':
>
>> +void add_child(struct child_process *cld, struct sockaddr *addr, socklen_t addrlen,
>> + struct child *firstborn , unsigned int *live_children)
>> +{
> ...
>> + live_children++;
> ...
>> +}
>
> These should be changed to '(*live_children)--' and '(*live_children)++',
> respectively.
Ah! You are correct; my bad. Will correct this in v6.
> There's also one minor functional change in 'check_dead_children()', where
> an 'if (loginfo)' check is added guarding the call to 'loginfo()':
>
>> +void check_dead_children(struct child *firstborn, unsigned int *live_children,
>> + log_fn loginfo)
>> +{
> ...
>> + if (loginfo) {
>> + const char *dead = "";
>> + if (status)
>> + dead = " (with error)";
>> + loginfo("[%"PRIuMAX"] Disconnected%s",
>> + (uintmax_t)pid, dead);
>> + }
> ...
>> +}
>
> I'm guessing this is done because a caller later in the series won't provide
> a 'loginfo', but if that's the case, it would help to note that in this
> patch's commit message.
Will call this out in the commit message in v6.
> The one other thing I noticed is that you removed the function documentation
> for 'kill_some_child()':
>
>> -/*
>> - * This gets called if the number of connections grows
>> - * past "max_connections".
>> - *
>> - * We kill the newest connection from a duplicate IP.
>> - */
>
> Is there a reason why you removed it? Otherwise, it should be added back in
> - probably in 'daemon-utils.h'?
I removed it initially as it was referencing things like `max_connections`
which no longer existed in the context of `daemon-utils.{c,h}`.
Next iteration I can restore the spirit of the comment, that this should be
called when the maximimum number of connections has been reached, in order
to kill the newest connection from a duplicate IP.
> Everything else here looks good.
Thanks,
Matthew
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox