Re: [PATCH v5 06/10] test-http-server: add simple authentication

From: Victoria Dye <vdye@github.com>
To: Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.com>,
	Matthew John Cheetham <mjcheetham@outlook.com>,
	M Hickford <mirth.hickford@gmail.com>,
	Jeff Hostetler <git@jeffhostetler.com>,
	Glen Choo <chooglen@google.com>,
	Matthew John Cheetham <mjcheetham@github.com>
Subject: Re: [PATCH v5 06/10] test-http-server: add simple authentication
Date: Fri, 13 Jan 2023 10:10:03 -0800	[thread overview]
Message-ID: <2a0b5f3f-7ab2-bc9e-76ac-93a52b4d32d0@github.com> (raw)
In-Reply-To: <c3c3d17a688963acc180e3bb7bbb4deb32a94304.1673475190.git.gitgitgadget@gmail.com>

Matthew John Cheetham via GitGitGadget wrote:
> +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 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;
> +
> +	return 0;
> +}

> +static int split_auth_param(const char *str, char **scheme, char **val, int required_val)
> +{
> +	struct strbuf **p = strbuf_split_str(str, ':', 2);
> +
> +	if (!p[0])
> +		return -1;
> +
> +	/* trim trailing ':' */
> +	if (p[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])
> +		*val = strbuf_detach(p[1], NULL);
> +
> +	strbuf_list_free(p);
> +	return 0;
> +}

There's nothing really *new* in these functions in this iteration, just code
moved from the option parsing/handling in 'cmd_main()' into dedicated
functions. Looks good!

> +	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;

I completely missed the "fall-through" comment in my last review [1], as you
kindly pointed out [2]. ;) Given that, this makes sense to me.

[1] https://lore.kernel.org/git/2a5d6586-3d2c-8af4-12be-a5a106f966b5@github.com/
[2] https://lore.kernel.org/git/AS2PR03MB981593EB3382F9738D2CA3D7C0FC9@AS2PR03MB9815.eurprd03.prod.outlook.com/

> +
> +		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);
> +		}
> +
> +		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);

The "extra_headers" configuration is new, and helps make the test server
more flexible. 

> +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.

> +
> +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);
> @@ -624,6 +853,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);