From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotd auth rules
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 15 Nov 2022 18:41:14 +0100

On Tue, Nov 15, 2022 at 06:07:13PM +0100, Omar Polo wrote:
> > blob - 6654c7116a6483e7457c581646809a9e5b09c6ae
> > blob + fa1f5d66ce3b4c6691a931b445425921470634a2
> > --- gotd/gotd.h
> > +++ gotd/gotd.h
> > @@ -54,11 +54,31 @@ struct gotd_repo {
> >  	size_t nhelpers;
> >  };
> >  
> > +enum gotd_access {
> > +	GOTD_ACCESS_PERMITTED = 1,
> > +	GOTD_ACCESS_DENIED
> > +};
> > +
> > +struct gotd_access_rule {
> > +	STAILQ_ENTRY(gotd_access_rule) entry;
> > +
> > +	enum gotd_access access;
> 
> just for curiosity, why not just using a boolean value here instead of
> an enum?  do you plan to add more values to gotd_access?

I would like to avoid relying on implicit zero-initialization for
sensitive stuff like auth. If we see a zero value it means auth
has not been set up for some reason (the code is broken), and we
will catch this problem when auth gets checked against values in
the enum. We could use an enum or #define, I don't really care which.

Perhaps I am being too paranoid because we have a deny-by-default
strategy anyway. But at least we could tell a difference between
not-initialized and 'denied' if a mistake is made during refactoring.

At some point, we could consider adding per-branch auth, in which
case the enum might be extended (or not, depending on how that gets
implemented).

> > blob - 5ccdb6be0bc1138dacf2aa6282eca1b79c9bba8f
> > blob + 6e9284e02d242f34453195f8acfce937633b6ae7
> > --- gotd/parse.y
> > +++ gotd/parse.y
> > @@ -98,7 +98,7 @@ typedef struct {
> >  
> >  %}
> >  
> > -%token	PATH ERROR ON UNIX_SOCKET UNIX_GROUP USER REPOSITORY
> > +%token	PATH ERROR ON UNIX_SOCKET UNIX_GROUP USER REPOSITORY PERMIT DENY
> 
> it's missing RO and RW
> 
>   +%token	RO RW

Nice catch, thanks! I will add these before comitting it.

Something else lacking in this patch are tests for rules matching
groups and testing multiple rules. I will add a couple more tests
later to ensure that things behave as expected.