On 2023-01-18 03:21, Ævar Arnfjörð Bjarmason wrote:
>
> 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.
Yep!
>> + 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.
Sure.
>> + 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.
I looked at this, but this then means being more careful when looping over
different `struct auth_module`s to keep the current 'scheme' and `*mod` in
sync/together. Just feels like overkill right now.
>> +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)?
`get_auth_module` taking a scheme name as parameter is a more sensible, IMO,
than a `string_list` or `string_list_item` and an offset. Given this is a test
helper, performance also isn't a priority. Readability wins here I think.
> Also, you "split" in the loop, but...
>
>> + strbuf_list_free(split);
> ...only free() the last one here, isn't this leaking?
Yes, it is. Will fix in next iteration.
>> +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 &&
> ....)".
Sure!
> 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?
Because that's how the `strbuf_split_str` function works. The comments
in the header file even call that out. "The substrings include the
terminator". From strbuf.h:
/**
* Split str (of length slen) at the specified terminator character.
* Return a null-terminated array of pointers to strbuf objects
* holding the substrings. The substrings include the terminator,
* except for the last substring, which might be unterminated if the
* original string did not end with a terminator. [cut] ...
...
*/
struct strbuf **strbuf_split_buf(const char *str, size_t len,
int terminator, int max);
>> +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.
All of these variables need to be initialised to NULL because not all
arms of the `if-elseif` chain assign to all of these variables, but
we always `free` all of them at the function exit.
For example,
char *scheme = NULL;
char *token = NULL;
char *challenge = NULL;
...
} else if (!strcmp(name, "auth.allowanonymous")) {
allow_anonymous = git_config_bool(name, val);
} else {
...
free(scheme);
free(token);
free(challenge);
>> +
>> + 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;
>> +}
>> +
Thanks,
Matthew
