From: Matthew John Cheetham <mjcheetham@outlook.com>
To: Derrick Stolee <derrickstolee@github.com>,
Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Cc: Lessley Dennington <lessleydennington@gmail.com>,
Matthew John Cheetham <mjcheetham@github.com>
Subject: Re: [PATCH v2 6/6] t5556-http-auth: add test for HTTP auth hdr logic
Date: Tue, 1 Nov 2022 16:59:12 -0700 [thread overview]
Message-ID: <AS2PR03MB9815B1AA9C780D650BD88FA2C0369@AS2PR03MB9815.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <8593dd49-4d95-ed4f-b414-8170efc138d4@github.com>
On 2022-10-28 08:08, Derrick Stolee wrote:
> On 10/21/22 1:08 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
>> @@ -1500,6 +1500,8 @@ else
>> endif
>> BASIC_CFLAGS += $(CURL_CFLAGS)
>>
>> + TEST_PROGRAMS_NEED_X += test-http-server
>> +
>> REMOTE_CURL_PRIMARY = git-remote-http$X
>> REMOTE_CURL_ALIASES = git-remote-https$X git-remote-ftp$X git-remote-ftps$X
>> REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
>
> This hunk is in the "else" block of "ifdef NO_CURL",
> so this makes sense for why TEST_PROGRAMS_NEED_X is
> augmented here, away from other instances.
>
>> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
>> index 787738e6fa3..45251695ce0 100644
>> --- a/contrib/buildsystems/CMakeLists.txt
>> +++ b/contrib/buildsystems/CMakeLists.txt
>> @@ -989,6 +989,19 @@ set(wrapper_scripts
>> set(wrapper_test_scripts
>> test-fake-ssh test-tool)
>>
>> +if(CURL_FOUND)
>> + list(APPEND wrapper_test_scripts test-http-server)
>> +
>> + add_executable(test-http-server ${CMAKE_SOURCE_DIR}/t/helper/test-http-server.c)
>> + target_link_libraries(test-http-server common-main)
>> +
>> + if(MSVC)
>> + 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()
>> +endif()
>
> And this file has the pattern of many "if(CURL_FOUND)"
> blocks with isolated purposes, so it makes sense to
> have this be an isolated change instead of grouped with
> a different case.
>
>> diff --git a/t/helper/.gitignore b/t/helper/.gitignore
>> index 8c2ddcce95f..1a94ab6eed5 100644
>> --- a/t/helper/.gitignore
>> +++ b/t/helper/.gitignore
>> @@ -1,2 +1,3 @@
>> /test-tool
>> /test-fake-ssh
>> +test-http-server
>
> Should this start with a "/" like the other entries?
That it probably should! Will update.
>> diff --git a/t/helper/test-credential-helper-replay.sh b/t/helper/test-credential-helper-replay.sh
>> new file mode 100755
>> index 00000000000..03e5e63dad6
>> --- /dev/null
>> +++ b/t/helper/test-credential-helper-replay.sh
>> @@ -0,0 +1,14 @@
>> +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
>
> Should this be a helper method within another script, such
> as t/lib-credential.sh or t/lib-httpd.sh? The read over
> stdin will still work, as in this example:
>
> read_chunk() {
> while read line; do
> case "$line" in
> --) break ;;
> *) echo "$line" ;;
> esac
> done
> }
This script file is used as a credential helper that is invoked by Git.
We specify that Git should use this credential helper in the tests using
the -c option:
CREDENTIAL_HELPER="$GIT_BUILD_DIR/t/helper/test-credential-helper-replay.sh" \
&& export CREDENTIAL_HELPER
..
git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL &&
Would extracting a read_chunk() function to one of the lib-* test scripts
be worth it given we already need another entry script anyway?
What other scripts would be calling read_chunk()?
>> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
>
>> @@ -0,0 +1,1134 @@
>> +#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>] [--init-timeout=<n>] [--max-connections=<n>]\n"
>> +" [--reuseaddr] [--pid-file=<file>]\n"
>> +" [--listen=<host_or_ipaddr>]* [--port=<n>]\n"
>> +" [--anonymous-allowed]\n"
>> +" [--auth=<scheme>[:<params>] [--auth-token=<scheme>:<token>]]*\n"
>> +;
>
> These are a lot of options to implement all at once. They are probably
> simple enough, but depending on the implementation and tests, it might
> be helpful to split this patch into smaller ones that introduce these
> options along with the tests that exercise each. That will help
> verify that they are being tested properly instead of needing to track
> back and forth across the patch for each one.
I plan to split this patch in to several in a v3.
>> +
>> +/* Timeout, and initial timeout */
>> +static unsigned int timeout;
>> +static unsigned int init_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);
>> +}
>
> I wonder how much of this we need or is just a nice thing. I would
> err on the side of making things as simple as possible, but being
> able to debug this test server may be important based on your
> experience.
These are useful to debug failures. Plus they also come from my copy
from daemon.c, so didn't want to touch/delete too much from that
starting point.
>> +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));
>> + }
>> +}
>> +
>> +//////////////////////////////////////////////////////////////////
>> +// 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.
>> +//////////////////////////////////////////////////////////////////
>
> Use /* */ style comments. You can repeat the asterisks to get a
> similar visual block.
Yep!
>> +
>> +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,
>
> nit: add a whitespace line between an item and the next
> item's comment.
Sure
>> +
>> + WR_STOP_THE_MUSIC = (WR_IO_ERROR | WR_HANGUP),
>> +};
>
> (I read, but have no comments on the http-server boilerplate.)
>
>> +
>> +enum auth_result {
>> + AUTH_UNKNOWN = 0,
>> + AUTH_DENY = 1,
>> + AUTH_ALLOW = 2,
>> +};
>> +
>> +struct auth_module {
>> + const char *scheme;
>> + const char *challenge_params;
>
> Later, I notice that you set challenge_params using an
> xstrdup() so this shouldn't be const and you should
> free it in any freeing code.
One question on this suggestion.. where would be appropriate to
free said char*? We need them for the lifetime of the process,
and they never grown in number beyond initial allocation from
parsing command line args.
I could move to stack alloc these in `cmd_main` and instead pass
a pointer to the `auth_modules` and count down through every
serve/handle etc function, rather than rely on them being global?
Thoughts or preferences?
>> + 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;
>
> So, we are setting up a number of potential auth modules,
> each of which has a scheme to match a request to the module,
> and a list of tokens that would be considered worthy of the
> AUTH_ALLOW result. Otherwise, if the scheme matches but no
> token matches, we get AUTH_DENY. Finally, if no scheme matches
> we get AUTH_UNKNOWN.
>
> This concept might be worth a comment here around the data
> structures before we get into how that is implemented.
>
>> +static struct auth_module *get_auth_module(struct strbuf *scheme)
>> +{
>> + int i;
>> + struct auth_module *mod;
>> + for (i = 0; i < auth_modules_nr; i++) {
>> + mod = auth_modules[i];
>> + if (!strcasecmp(mod->scheme, scheme->buf))
>> + return mod;
>> + }
>> +
>> + return NULL;
>> +}
>
> Matching the input scheme against the list of modules.
>
> Only complaint: there is no reason that 'scheme' needs t
> be a strbuf, but could be a 'const char *' here.
True.
>> +static void add_auth_module(struct auth_module *mod)
>> +{
>> + ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
>> + auth_modules[auth_modules_nr++] = mod;
>> +}
>
> nit: this could be located earlier, next to the list
> definition, or delayed until it is needed. That would
> allow get_auth_module() to be closer to its first use.
Not sure I follow.. are you saying I should move `add_auth_module`
to earlier in the file?
>> +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;
>> +
>> + /* ask all auth modules to validate the request */
>> + 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;
>
> For each valid request header...
>
>> + // trim trailing space ' '
>> + strbuf_setlen(split[0], split[0]->len - 1);
>> +
>> + mod = get_auth_module(split[0]);
>> + if (mod) {
>
> ...get an appropriate module, if it exists...
>
>> +
>> + for_each_string_list_item(token, mod->tokens) {
>> + if (!strcmp(split[1]->buf, token->string)) {
>> + result = AUTH_ALLOW;
>> + goto done;
>> + }
>> + }
>> +
>> + if (result != AUTH_UNKNOWN)
>> + goto done;
>
> ...and report if we find a valid token.
>
> Here, it seems I was wrong in my expectation of AUTH_DENY:
> if a matching module exists but no token exists in that
> module, then we keep searching other modules.
AUTH_DENY denies a request immediately and stops searching other modules.
AUTH_ALLOW approves the request and stops looking at other modules.
AUTH_UNKNOWN means this module didn't match or 'decide' to reject, so keep
looking/asking other modules.
After reading you review, I think it may be better to change this to
more closely match your expectations (and how typical servers behave):
Return AUTH_ALLOW if we find a matching valid token for the module.
If we match a module and do NOT find a token, then return AUTH_DENY.
Otherwise return AUTH_UNKNOWN - this means the user provided some auth
mechanism we don't understand, or no auth at all.
>> + }
>> + }
>> + }
>> +
>> +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 */
>
> I'm not sure that I see a case where this is possible. Maybe
> we should have a 'result = AUTH_DENY' at the start of the
> "if (mod)" block, followed by a 'goto done' in all cases
> instead of "if (result != AUTH_UNKNOWN)"?
In this version, you're correct.. AUTH_DENY is never returned.
This tri-state response from an auth module is an oversight from an earlier
local version - sorry for the confusion here, and thanks for catching!
I will update in a v3 to match sane expectations.
>> + case AUTH_UNKNOWN:
>> + if (allow_anonymous)
>> + break;
>
> If we do not require auth, then we want to continue if there
> is no matching authentication.
>
>> + 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(1, 401, "Unauthorized", -1, &hdrs, *wr);
>
> However, here is the critical piece about how servers will
> start to act with the new WWW-Authenticate header usage in
> the Git credential helper interface. This will be critical
> in the testing for Git to retry the credential helper while
> passing these authentications schemes from the installed
> modules.
>
>> + }
>> +
>> + strbuf_list_free(split);
>> + string_list_clear(&hdrs, 0);
>> +
>> + return result == AUTH_ALLOW ||
>> + (result == AUTH_UNKNOWN && allow_anonymous);
>
> Did it work? Or did it not need to work? I'm interested to
> investigate the case that the client sent an authentication
> header that matches a module but doesn't match any tokens,
> but we allow anonymous access, anyway. Is that a 400? Or
> is that a 401?
It should probably be a 401 as the credentials are understood, but
are just 'bad'.
>> +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 we are not authed, send the 401 response.
>
>> + if (is_git_request(req))
>> + return do__git(req, user);
>
> If we are authed, then pass through to the Git response.
>
>> + return send_http_error(1, 501, "Not Implemented", -1, NULL,
>> + WR_OK | WR_HANGUP);
>
> If the Git request fails, we don't care. This is a test.
> Just pass a 500-level error and the client will barf,
> letting us know that something went wrong.
Correct assessment!
>> +static void kill_some_child(void)
>
>> +static void check_dead_children(void)
>
> These technically sound methods have unfortunate names.
> Using something like "connection" over "child" might
> alleviate some of the horror. (I initially wanted to
> suggest "subprocess" but you compare live_children to
> max_connections in the next method, so connection seemed
> appropriate.)
These are copied exactly from git-daemon, so I'd rather
avoid the churn in renaming things.
>> +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();
>> + sleep(1); /* give it some time to die */
>> + check_dead_children();
>> + if (live_children >= max_connections) {
>> + close(incoming);
>> + logerror("Too many children, dropping connection");
>> + return;
>> + }
>> + }
>
> Do we anticipate exercising concurrent requests in our
> tests? Perhaps it's not worth putting a cap on the
> connection count so we can keep the test helpers simple.
Probably not, but again.. 100% of the boilerplate here came from
the prior art in daemon.c, so didn't want to touch any of it!
I'm happy to start deleting things however if needed?
>> + 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);
>> +}
>> +
>
> I scanned the socket creation code, but my eyes were
> glazing over. I'm definitely in the camp of "if it works,
> that's enough for our tests." If we start to rely on this
> test harness in more places, we can improve any shortcomings
> as they arise.
>
>> +//////////////////////////////////////////////////////////////////
>> +// 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;
>> + struct auth_module *mod = NULL;
>> +
>> + trace2_cmd_name("test-http-server");
>> + 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, "--init-timeout=", &v)) {
>> + init_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;
>> + }
>
> ok, most of these arguments are actually about the per-connection
> subprocesses.
>
>> + if (skip_prefix(arg, "--allow-anonymous", &v)) {
>> + allow_anonymous = 1;
>> + continue;
>> + }
>
> Here is how we choose to allo anonymous access.
>
>> + if (skip_prefix(arg, "--auth=", &v)) {
>> + struct strbuf **p = strbuf_split_str(v, ':', 2);
>> +
>> + if (!p[0]) {
>> + error("invalid argument '%s'", v);
>> + usage(test_http_auth_usage);
>> + }
>> +
>> + // trim trailing ':'
>> + if (p[1])
>> + strbuf_setlen(p[0], p[0]->len - 1);
>> +
>> + if (get_auth_module(p[0])) {
>> + error("duplicate auth scheme '%s'\n", p[0]->buf);
>> + usage(test_http_auth_usage);
>> + }
>> +
>> + mod = xmalloc(sizeof(struct auth_module));
>> + mod->scheme = xstrdup(p[0]->buf);
>> + mod->challenge_params = p[1] ? xstrdup(p[1]->buf) : NULL;
>
> Here, you xstrdup() into a 'const char *', but you are really
> passing ownership so it shouldn't be conts.
Ok
>
>> + mod->tokens = xmalloc(sizeof(struct string_list));
>
> nit: this could also be "CALLOC_ARRAY(mod->tokens, 1);"
Sure!
>> + string_list_init_dup(mod->tokens);
>> +
>> + add_auth_module(mod);
>> +
>> + strbuf_list_free(p);
>> + continue;
>
> Ok, we gain the auth schemes from the command line.
>
>> + }
>> + if (skip_prefix(arg, "--auth-token=", &v)) {
>> + struct strbuf **p = strbuf_split_str(v, ':', 2);
>> + if (!p[0]) {
>> + error("invalid argument '%s'", v);
>> + usage(test_http_auth_usage);
>> + }
>> +
>> + if (!p[1]) {
>> + error("missing token value '%s'\n", v);
>> + usage(test_http_auth_usage);
>> + }
>> +
>> + // trim trailing ':'
>
> Use /* */ (Aside: I'm surprised we don't have a build option in
> DEVELOPER=1 that catches the use of these comments.)
Me too! Appologies here.
>> + strbuf_setlen(p[0], p[0]->len - 1);
>> +
>> + mod = get_auth_module(p[0]);
>> + if (!mod) {
>> + error("auth scheme not defined '%s'\n", p[0]->buf);
>> + usage(test_http_auth_usage);
>> + }
>> +
>> + string_list_append(mod->tokens, p[1]->buf);
>> + strbuf_list_free(p);
>> + continue;
>> + }
>
> And the token lists. It is important that the scheme is added
> before any token is added.
>
>> + 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);
>> +}
>
> And complete the thing with some boilerplate.
>
> This was a lot to read, and the interesting bits are all mixed in
> with the http server code, which is less interesting to what we
> are trying to accomplish. It would be beneficial to split this
> into one or two patches before we actually introduce the tests.
>
> The most important thing that I think would be helpful is to
> isolate all the authentication behavior into its own patch so
> we can see how those connections from the command-line arguments
> affect the behavior of the server responses.
>
> I think ideally we would have the following split:
>
> 1. All server boilerblate. All requests 500 not-implemented.
>
> 2. Add Git fall-through with no authentication. Add the tests
> that are intended to allow anonymous auth.
>
> 3. Add authentication data structures read from command-line,
> but not processed at all in the logic.
>
> 4. Act on the authentication data structures to alter the
> requests. Add the tests that use these authentication
> schemes.
>
> I could easily see a case for combining 1&2 as well as 3&4,
> for slightly larger but more completely-testable changes at
> every step.
I agree, and my appologies for not splitting these out.
I'll follow up with a split that should make more sense.
> From what I read, I don't think there is much to change in
> the end result of the code, but it definitely was hard to read
> the important things when surrounded by many lines of
> boilerplate.
>
>> diff --git a/t/t5556-http-auth.sh b/t/t5556-http-auth.sh
>
> I'm going to pause here and come back to the test script in
> a separate reply.
>
> Thanks,
> -Stolee
Thanks,
Matthew
next prev parent reply other threads:[~2022-11-01 23:59 UTC|newest]
Thread overview: 223+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 1/8] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 2/8] netrc: " Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
2022-09-19 16:12 ` Derrick Stolee
2022-09-21 22:48 ` Matthew John Cheetham
2022-09-13 19:25 ` [PATCH 4/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2022-09-19 16:21 ` Derrick Stolee
2022-09-21 22:24 ` Matthew John Cheetham
2022-09-26 14:13 ` Derrick Stolee
2022-09-13 19:25 ` [PATCH 5/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2022-09-19 16:33 ` Derrick Stolee
2022-09-21 22:20 ` Matthew John Cheetham
2022-09-13 19:25 ` [PATCH 6/8] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 7/8] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 8/8] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
2022-09-19 16:42 ` Derrick Stolee
2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
2022-09-19 16:44 ` Derrick Stolee
2022-09-21 22:19 ` Matthew John Cheetham
2022-09-19 23:36 ` Lessley Dennington
2022-10-21 17:07 ` [PATCH v2 0/6] " Matthew John Cheetham via GitGitGadget
2022-10-21 17:07 ` [PATCH v2 1/6] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2022-10-21 17:07 ` [PATCH v2 2/6] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2022-10-28 18:22 ` Jeff Hostetler
2022-11-01 23:07 ` Matthew John Cheetham
2022-10-21 17:08 ` [PATCH v2 3/6] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
2022-10-21 17:08 ` [PATCH v2 4/6] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
2022-10-21 17:08 ` [PATCH v2 5/6] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
2022-10-21 17:08 ` [PATCH v2 6/6] t5556-http-auth: add test for HTTP auth hdr logic Matthew John Cheetham via GitGitGadget
2022-10-28 15:08 ` Derrick Stolee
2022-10-28 19:14 ` Jeff Hostetler
2022-11-01 23:14 ` Matthew John Cheetham
2022-11-02 14:38 ` Derrick Stolee
2022-11-01 23:59 ` Matthew John Cheetham [this message]
2022-10-25 2:26 ` git-credential.txt M Hickford
2022-10-25 20:49 ` git-credential.txt Matthew John Cheetham
2022-11-02 22:09 ` [PATCH v3 00/11] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 01/11] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 02/11] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 03/11] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
2022-11-09 23:18 ` Glen Choo
2022-11-02 22:09 ` [PATCH v3 04/11] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 05/11] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
2022-11-09 23:40 ` Glen Choo
2022-12-12 21:53 ` Matthew John Cheetham
2022-11-02 22:09 ` [PATCH v3 06/11] test-http-server: add stub HTTP server test helper Matthew John Cheetham via GitGitGadget
2022-11-07 19:19 ` Derrick Stolee
2022-11-02 22:09 ` [PATCH v3 07/11] test-http-server: add HTTP error response function Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 08/11] test-http-server: add HTTP request parsing Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 09/11] test-http-server: pass Git requests to http-backend Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 10/11] test-http-server: add simple authentication Matthew John Cheetham via GitGitGadget
2022-11-02 22:09 ` [PATCH v3 11/11] t5556: add HTTP authentication tests Matthew John Cheetham via GitGitGadget
2022-11-03 19:00 ` [PATCH v3 00/11] Enhance credential helper protocol to include auth headers M Hickford
2022-12-12 22:07 ` Matthew John Cheetham
2022-11-07 19:23 ` Derrick Stolee
2022-11-09 23:06 ` Glen Choo
2022-12-12 22:03 ` Matthew John Cheetham
2022-11-28 9:40 ` Junio C Hamano
2022-12-12 21:36 ` [PATCH v4 0/8] " Matthew John Cheetham via GitGitGadget
2022-12-12 21:36 ` [PATCH v4 1/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2022-12-14 23:15 ` Victoria Dye
2023-01-11 22:09 ` Matthew John Cheetham
2022-12-15 9:27 ` Ævar Arnfjörð Bjarmason
2023-01-11 22:11 ` Matthew John Cheetham
2022-12-12 21:36 ` [PATCH v4 2/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2022-12-14 23:15 ` Victoria Dye
2023-01-11 20:37 ` Matthew John Cheetham
2022-12-12 21:36 ` [PATCH v4 3/8] test-http-server: add stub HTTP server test helper Matthew John Cheetham via GitGitGadget
2022-12-14 23:16 ` Victoria Dye
2023-01-11 20:46 ` Matthew John Cheetham
2022-12-12 21:36 ` [PATCH v4 4/8] test-http-server: add HTTP error response function Matthew John Cheetham via GitGitGadget
2022-12-14 23:17 ` Victoria Dye
2022-12-12 21:36 ` [PATCH v4 5/8] test-http-server: add HTTP request parsing Matthew John Cheetham via GitGitGadget
2022-12-14 23:18 ` Victoria Dye
2023-01-11 21:39 ` Matthew John Cheetham
2022-12-12 21:36 ` [PATCH v4 6/8] test-http-server: pass Git requests to http-backend Matthew John Cheetham via GitGitGadget
2022-12-14 23:20 ` Victoria Dye
2023-01-11 21:45 ` Matthew John Cheetham
2023-01-12 20:54 ` Victoria Dye
2022-12-12 21:36 ` [PATCH v4 7/8] test-http-server: add simple authentication Matthew John Cheetham via GitGitGadget
2022-12-14 23:23 ` Victoria Dye
2023-01-11 22:00 ` Matthew John Cheetham
2022-12-12 21:36 ` [PATCH v4 8/8] t5556: add HTTP authentication tests Matthew John Cheetham via GitGitGadget
2022-12-14 23:48 ` Victoria Dye
2022-12-15 0:21 ` Junio C Hamano
2023-01-11 22:05 ` Matthew John Cheetham
2023-01-11 22:04 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 00/10] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2023-01-11 22:13 ` [PATCH v5 01/10] daemon: libify socket setup and option functions Matthew John Cheetham via GitGitGadget
2023-01-12 19:35 ` Victoria Dye
2023-01-12 20:22 ` Derrick Stolee
2023-01-11 22:13 ` [PATCH v5 02/10] daemon: libify child process handling functions Matthew John Cheetham via GitGitGadget
2023-01-12 19:35 ` Victoria Dye
2023-01-17 21:14 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 03/10] daemon: rename some esoteric/laboured terminology Matthew John Cheetham via GitGitGadget
2023-01-12 19:44 ` Victoria Dye
2023-01-17 21:16 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 04/10] test-http-server: add stub HTTP server test helper Matthew John Cheetham via GitGitGadget
2023-01-12 19:57 ` Victoria Dye
2023-01-11 22:13 ` [PATCH v5 05/10] test-http-server: add HTTP error response function Matthew John Cheetham via GitGitGadget
2023-01-12 20:35 ` Victoria Dye
2023-01-17 21:23 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 06/10] test-http-server: add simple authentication Matthew John Cheetham via GitGitGadget
2023-01-13 18:10 ` Victoria Dye
2023-01-13 21:06 ` Junio C Hamano
2023-01-17 21:21 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 07/10] http: replace unsafe size_t multiplication with st_mult Matthew John Cheetham via GitGitGadget
2023-01-11 22:13 ` [PATCH v5 08/10] strvec: expose strvec_push_nodup for external use Matthew John Cheetham via GitGitGadget
2023-01-11 22:13 ` [PATCH v5 09/10] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-01-12 8:41 ` Ævar Arnfjörð Bjarmason
2023-01-17 21:51 ` Matthew John Cheetham
2023-01-11 22:13 ` [PATCH v5 10/10] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-01-12 8:48 ` Ævar Arnfjörð Bjarmason
2023-01-17 21:35 ` Matthew John Cheetham
2023-01-12 20:41 ` Derrick Stolee
2023-01-17 21:18 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 00/12] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 01/12] daemon: libify socket setup and option functions Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 02/12] daemon: libify child process handling functions Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 03/12] daemon: rename some esoteric/laboured terminology Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 04/12] test-http-server: add stub HTTP server test helper Matthew John Cheetham via GitGitGadget
2023-01-18 11:04 ` Ævar Arnfjörð Bjarmason
2023-01-20 22:05 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 05/12] test-http-server: add HTTP error response function Matthew John Cheetham via GitGitGadget
2023-01-18 11:07 ` Ævar Arnfjörð Bjarmason
2023-01-20 22:05 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 06/12] test-http-server: add HTTP request parsing Matthew John Cheetham via GitGitGadget
2023-01-18 11:14 ` Ævar Arnfjörð Bjarmason
2023-01-20 22:05 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 07/12] test-http-server: pass Git requests to http-backend Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 08/12] test-http-server: add simple authentication Matthew John Cheetham via GitGitGadget
2023-01-18 11:21 ` Ævar Arnfjörð Bjarmason
2023-01-20 22:05 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 09/12] test-http-server: add sending of arbitrary headers Matthew John Cheetham via GitGitGadget
2023-01-18 3:30 ` [PATCH v6 10/12] http: replace unsafe size_t multiplication with st_mult Matthew John Cheetham via GitGitGadget
2023-01-18 11:38 ` Ævar Arnfjörð Bjarmason
2023-01-18 17:28 ` Victoria Dye
2023-01-18 23:16 ` Ævar Arnfjörð Bjarmason
2023-01-18 3:30 ` [PATCH v6 11/12] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-01-18 11:42 ` Ævar Arnfjörð Bjarmason
2023-01-20 22:05 ` Matthew John Cheetham
2023-01-18 3:30 ` [PATCH v6 12/12] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 00/12] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 01/12] daemon: libify socket setup and option functions Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 02/12] daemon: libify child process handling functions Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 03/12] daemon: rename some esoteric/laboured terminology Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 04/12] test-http-server: add stub HTTP server test helper Matthew John Cheetham via GitGitGadget
2023-01-26 8:58 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 05/12] test-http-server: add HTTP error response function Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 06/12] test-http-server: add HTTP request parsing Matthew John Cheetham via GitGitGadget
2023-01-26 9:30 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 07/12] test-http-server: pass Git requests to http-backend Matthew John Cheetham via GitGitGadget
2023-01-26 9:37 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 08/12] test-http-server: add simple authentication Matthew John Cheetham via GitGitGadget
2023-01-26 10:02 ` Jeff King
2023-01-26 21:22 ` Jeff King
2023-01-26 22:27 ` Junio C Hamano
2023-01-26 20:33 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 09/12] test-http-server: add sending of arbitrary headers Matthew John Cheetham via GitGitGadget
2023-01-20 22:08 ` [PATCH v7 10/12] http: replace unsafe size_t multiplication with st_mult Matthew John Cheetham via GitGitGadget
2023-01-26 10:09 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 11/12] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-01-26 10:31 ` Jeff King
2023-02-06 19:25 ` Matthew John Cheetham
2023-02-09 13:12 ` Jeff King
2023-01-20 22:08 ` [PATCH v7 12/12] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-01-26 11:25 ` Jeff King
2023-02-06 19:18 ` Matthew John Cheetham
2023-02-09 13:08 ` Jeff King
2023-01-24 17:30 ` [PATCH v7 00/12] Enhance credential helper protocol to include auth headers Victoria Dye
2023-01-24 18:03 ` Junio C Hamano
2023-01-26 11:29 ` Jeff King
2023-01-26 16:05 ` Junio C Hamano
2023-02-02 10:14 ` Johannes Schindelin
2023-02-02 11:04 ` Ævar Arnfjörð Bjarmason
2023-02-02 13:51 ` Johannes Schindelin
2023-02-06 21:32 ` Ævar Arnfjörð Bjarmason
2023-03-27 9:05 ` Johannes Schindelin
2023-02-03 17:34 ` Jeff King
2023-03-27 9:10 ` Johannes Schindelin
2023-03-28 18:55 ` Jeff King
2023-01-28 14:28 ` M Hickford
2023-02-01 20:15 ` Matthew John Cheetham
2023-02-02 0:16 ` Jeff King
2023-02-06 19:29 ` [PATCH v8 0/3] " Matthew John Cheetham via GitGitGadget
2023-02-06 19:29 ` [PATCH v8 1/3] t5563: add tests for basic and anoymous HTTP access Matthew John Cheetham via GitGitGadget
2023-02-06 20:32 ` Ævar Arnfjörð Bjarmason
2023-02-08 20:24 ` Victoria Dye
2023-02-09 11:19 ` Ævar Arnfjörð Bjarmason
2023-02-15 19:32 ` Matthew John Cheetham
2023-02-06 19:29 ` [PATCH v8 2/3] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-02-06 20:36 ` Ævar Arnfjörð Bjarmason
2023-02-08 21:05 ` Victoria Dye
2023-02-06 19:29 ` [PATCH v8 3/3] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-02-06 20:45 ` Ævar Arnfjörð Bjarmason
2023-02-15 19:19 ` Matthew John Cheetham
2023-02-06 20:59 ` [PATCH v8 0/3] Enhance credential helper protocol to include auth headers Ævar Arnfjörð Bjarmason
2023-02-08 21:29 ` Victoria Dye
2023-02-08 21:54 ` Junio C Hamano
2023-02-15 21:34 ` [PATCH v9 " Matthew John Cheetham via GitGitGadget
2023-02-15 21:34 ` [PATCH v9 1/3] t5563: add tests for basic and anoymous HTTP access Matthew John Cheetham via GitGitGadget
2023-02-15 22:15 ` Junio C Hamano
2023-02-16 22:25 ` Matthew John Cheetham
2023-02-15 21:34 ` [PATCH v9 2/3] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-02-15 23:26 ` Junio C Hamano
2023-02-16 22:29 ` Matthew John Cheetham
2023-02-15 21:34 ` [PATCH v9 3/3] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-02-16 22:34 ` [PATCH v10 0/3] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2023-02-16 22:34 ` [PATCH v10 1/3] t5563: add tests for basic and anoymous HTTP access Matthew John Cheetham via GitGitGadget
2023-02-23 9:16 ` Jeff King
2023-02-23 9:37 ` Jeff King
2023-02-27 17:18 ` Matthew John Cheetham
2023-02-16 22:34 ` [PATCH v10 2/3] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-02-23 9:46 ` Jeff King
2023-02-23 19:49 ` Junio C Hamano
2023-02-27 17:14 ` Matthew John Cheetham
2023-02-16 22:34 ` [PATCH v10 3/3] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-02-27 17:20 ` [PATCH v11 0/3] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2023-02-27 17:20 ` [PATCH v11 1/3] t5563: add tests for basic and anoymous HTTP access Matthew John Cheetham via GitGitGadget
2023-02-27 17:20 ` [PATCH v11 2/3] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2023-02-27 17:20 ` [PATCH v11 3/3] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2023-02-27 20:27 ` [PATCH v11 0/3] Enhance credential helper protocol to include auth headers Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AS2PR03MB9815B1AA9C780D650BD88FA2C0369@AS2PR03MB9815.eurprd03.prod.outlook.com \
--to=mjcheetham@outlook.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=lessleydennington@gmail.com \
--cc=mjcheetham@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).