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
