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

On 2022/11/15 16:59:18 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> This implements mandatory per-repository read/write authorization
> rules in gotd. Mandatory means that repository access will be denied
> to anyone if no access rules are configured.
> 
> This is still done in the main process. I want to get the config syntax
> and auth logic settled before starting a refactoring for privsep.
> 
> ok?

reads fine and i like the syntax, i think it's flexible enough.
ok for me.

one question and one comment inline:

> [...]
> 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?

> [...]
> 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

>  %token	<v.string>	STRING
>  %token	<v.number>	NUMBER
> @@ -221,6 +221,25 @@ repoopts1	: PATH STRING {
>  			}
>  			free($2);
>  		}
> +		| PERMIT RO STRING {
> +			if (gotd_proc_id == PROC_GOTD) {
> +				conf_new_access_rule(new_repo,
> +				    GOTD_ACCESS_PERMITTED, GOTD_AUTH_READ, $3);
> +			}
> +		}
> +		| PERMIT RW STRING {
> +			if (gotd_proc_id == PROC_GOTD) {
> +				conf_new_access_rule(new_repo,
> +				    GOTD_ACCESS_PERMITTED,
> +				    GOTD_AUTH_READ | GOTD_AUTH_WRITE, $3);
> +			}
> +		}
> +		| DENY STRING {
> +			if (gotd_proc_id == PROC_GOTD) {
> +				conf_new_access_rule(new_repo,
> +				    GOTD_ACCESS_DENIED, 0, $2);
> +			}
> +		}
>  		;
>  
>  repoopts2	: repoopts2 repoopts1 nl
> @@ -268,9 +287,13 @@ lookup(char *s)
>  {
>  	/* This has to be sorted always. */
>  	static const struct keywords keywords[] = {
> +		{ "deny",			DENY },
>  		{ "on",				ON },
>  		{ "path",			PATH },
> +		{ "permit",			PERMIT },
>  		{ "repository",			REPOSITORY },
> +		{ "ro",				RO },
> +		{ "rw",				RW },
>  		{ "unix_group",			UNIX_GROUP },
>  		{ "unix_socket",		UNIX_SOCKET },
>  		{ "user",			USER },