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

[Matthew John Cheetham <mjcheetham@outlook.com>]

On 2023-01-18 03:21, Ævar Arnfjörð Bjarmason wrote:

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

Yep!

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

Sure.

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

I looked at this, but this then means being more careful when looping over
different `struct auth_module`s to keep the current 'scheme' and `*mod` in
sync/together. Just feels like overkill right now.

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

`get_auth_module` taking a scheme name as parameter is a more sensible, IMO,
than a `string_list` or `string_list_item` and an offset. Given this is a test
helper, performance also isn't a priority. Readability wins here I think.

> Also, you "split" in the loop, but...
> 
>> +	strbuf_list_free(split);
> ...only free() the last one here, isn't this leaking?

Yes, it is. Will fix in next iteration.

>> +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 &&
> ....)".

Sure!

> 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?

Because that's how the `strbuf_split_str` function works. The comments
in the header file even call that out. "The substrings include the
terminator". From strbuf.h:

/**
 * Split str (of length slen) at the specified terminator character.
 * Return a null-terminated array of pointers to strbuf objects
 * holding the substrings.  The substrings include the terminator,
 * except for the last substring, which might be unterminated if the
 * original string did not end with a terminator. [cut] ...
   ...
 */
struct strbuf **strbuf_split_buf(const char *str, size_t len,
				 int terminator, int max);

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

All of these variables need to be initialised to NULL because not all
arms of the `if-elseif` chain assign to all of these variables, but
we always `free` all of them at the function exit.

For example,

char *scheme = NULL;
char *token = NULL;
char *challenge = NULL;
...
} else if (!strcmp(name, "auth.allowanonymous")) {
	allow_anonymous = git_config_bool(name, val);
} else {
...
free(scheme);
free(token);
free(challenge);

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

Thanks,
Matthew