Re: [PATCH v4 7/8] test-http-server: add simple authentication

[Victoria Dye <vdye@github.com>]

Matthew John Cheetham via GitGitGadget wrote:
> +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 module that matches a valid token approves 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.
> +	 */
> +	for_each_string_list_item(hdr, &req->header_list) {
> +		if (skip_iprefix(hdr->string, "Authorization: ", &v)) {

Is only one "Authorization:" header allowed? If so, adding a 'break;' at the
end of this if-statement would make that clearer. If not, what's the
expected allow/deny behavior if e.g. one header is ALLOW'd by one auth
module, and another header is DENY'd by a different auth module?

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

I think this just needs to be 'if (allow_anonymous)' - we already know
'result' is 'AUTH_UNKNOWN' once we reach this block.

> +		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);
> +	}
> +
> +	strbuf_list_free(split);
> +	string_list_clear(&hdrs, 0);
> +
> +	return result == AUTH_ALLOW ||
> +	      (result == AUTH_UNKNOWN && allow_anonymous);

So if a user is explicitly denied, even with 'allow_anonymous', this fails?
Is there a test case that uses that behavior and/or is that standard auth
behavior? Otherwise, it'd be simpler to skip the 'is_authed()' check (in
'dispatch()') altogether if 'allow_anonymous' is enabled.

> +}
> +
>  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, NULL);
> +		return do__git(req, user);
>  
>  	return send_http_error(1, 501, "Not Implemented", -1, NULL,
>  			       WR_OK | WR_HANGUP);
> @@ -854,6 +982,7 @@ int cmd_main(int argc, const char **argv)
>  	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);
> @@ -906,6 +1035,63 @@ int cmd_main(int argc, const char **argv)
>  			pid_file = v;
>  			continue;
>  		}
> +		if (skip_prefix(arg, "--allow-anonymous", &v)) {
> +			allow_anonymous = 1;
> +			continue;
> +		}
> +		if (skip_prefix(arg, "--auth=", &v)) {
...

> +		}
> +		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 ':' */
> +			strbuf_setlen(p[0], p[0]->len - 1);
> +
> +			mod = get_auth_module(p[0]->buf);
> +			if (!mod) {
> +				error("auth scheme not defined '%s'\n", p[0]->buf);
> +				usage(test_http_auth_usage);
> +			}

Does this mean that '--auth' needs to be specified before '--auth-token' to
avoid the "auth scheme not defined" error? If so, this could be made less
fragile by just setting the string value of the arg in this 'if()' block,
then processing the value after the option-parsing loop.

> +
> +			string_list_append(mod->tokens, p[1]->buf);
> +			strbuf_list_free(p);
> +			continue;
> +		}
>  
>  		fprintf(stderr, "error: unknown argument '%s'\n", arg);
>  		usage(test_http_auth_usage);

I think a test (in this patch) showing how the auth headers are handled by
this HTTP server would be really helpful in demonstrating/exercising the
intended behavior.