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