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

From: Matthew John Cheetham <mjcheetham@outlook.com>
To: Victoria Dye <vdye@github.com>,
	Matthew John Cheetham via GitGitGadget  <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Derrick Stolee <derrickstolee@github.com>,
	Lessley Dennington <lessleydennington@gmail.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 v4 7/8] test-http-server: add simple authentication
Date: Wed, 11 Jan 2023 14:00:32 -0800	[thread overview]
Message-ID: <AS2PR03MB981593EB3382F9738D2CA3D7C0FC9@AS2PR03MB9815.eurprd03.prod.outlook.com> (raw)
In-Reply-To: <2a5d6586-3d2c-8af4-12be-a5a106f966b5@github.com>

On 2022-12-14 15:23, Victoria Dye wrote:

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

Yes, only one Authorization header *should* be passed.. but the RFCs are not very
explicit about that. The test server supports multiple, but will `ALLOW` or `DENY`
based on the first matching auth scheme (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.

Note that `AUTH_DENY` falls-through to the `AUTH_UNKNOWN` case.
The only time we *DON'T* want to output the auth challenge response headers is
when there was no challenge provided (`AUTH_UNKNOWN`) *and* we are permitting
anonymous users.

  result      | allow_anoymous | Output Challenge?
---------------------------------------------------
 AUTH_DENY    |       1        |       Yes
 AUTH_DENY    |       0        |       Yes
 AUTH_UNKNOWN |       1        |       No
 AUTH_UNKNOWN |       0        |       Yes

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

If the user is being denied by a module we should always deny access.

Admittedly, for this simple authentication scenario it's kind of silly to deny
a user who is trying to identify themselves, but permit an anoymous user.
However, if this was an authorization failure then denying a user based on their
token may be totally valid. Right now, we're only concerned about authentication
and not authorization, so I could move this check to `dispatch()` if you feel
strongly about it.

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

Yes, `--auth` needs to come first and 'setup' the module and challenge.

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

Thanks,
Matthew