Re: [PATCH v6 08/12] test-http-server: add simple authentication
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 18, 2023, 11:21 UTC
- Message-ID
- <230118.867cxkxece.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <a1ff55dd6e25aa39f14b494f482720edf7d1eabd.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
Show 5 quoted lines
> +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.
Show 6 quoted lines
> + 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.
Show 42 quoted lines
> +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?
Show 9 quoted lines
> +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.
Show 42 quoted lines
> +
> + 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;
> +}
> +