Re: [PATCH v6 08/12] test-http-server: add simple authentication

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:


> +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++) {

We can use "for (size_t i = 0" syntax now, let's do that here to not mix
"size_t" and "int" types needlessly.

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

Don't use CALLOC_ARRAY() if you're then going to use
string_list_init_dup() (which is good!), use ALLOC_ARRAY() instead. We
don't need to set the memory to 0, only to overwrite it entirely again.

> +		ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
> +		auth_modules[auth_modules_nr++] = mod;

I have not looked at the whole context here, but instead of:

	struct auth_module {
		char *scheme;
		char *challenge_params;
		struct string_list *tokens;
	};

Why not:

	struct auth_module {
		char *challenge_params;
		struct string_list *tokens;
	};

Then you could use a "struct string_list" for this, make the "scheme" be
the "string" member, and stick the remaining two fields in the "util",
and thus save yourself the manual memory management etc.

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

Sometimes we need a strbuf_split_str, but in this case couldn't you use
the in-place "struct string_list" variant of that instead, and just
carry a "size_t len" here for it, which you'd then pass to
get_auth_module() (which this commit adds)?

Also, you "split" in the loop, but...

> +	strbuf_list_free(split);
...only free() the last one here, isn't this leaking?

> +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] == ':')

Don't compare unsigned length fields to "> 0", just do "if (len &&
....)".

Also, maybe I'm just groggy today, but how do we have a trailing ":" if
we just split on ":", and with a limit such that...

> +	if (p[1])
> +		*val = strbuf_detach(p[1], NULL);

...we have an item after that?


> +static int read_auth_config(const char *name, const char *val, void *data)
> +{
> +	int ret = 0;
> +	char *scheme = NULL;

Don't init this to NULL, instead the split_auth_param() return value
should be trusted, the compiler will then help us catch errors, no?

> +	char *token = NULL;
> +	char *challenge = NULL;

In this case it *is* needed though, as the function will return
non-errors, but *maybe* give us the second out parameter.

For such a function though, isn't just assigning "*second_param = NULL"
at the start of it less of a "running with scissors" pattern?

> +	struct auth_module *mod = NULL;

This NULL assignment can be dropped, we assign to it below
unconditionally before using it.

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