From: Mark Jamsek <mark@jamsek.com>
Subject: Re: gotd: remove gotsh group requirement
To: gameoftrees@openbsd.org
Date: Wed, 4 Jan 2023 20:33:20 +1100

On 23-01-03 11:12PM, Stefan Sperling wrote:
> We now require per-repository user/group access rules in gotd.conf(5),
> and enforce a per-user limit of concurrent connections to gotd(8).
> Local users should no longer be able to access repositories they should
> not have access to, or heavily DoS gotd over the unix socket.
> 
> So it should be reasonably safe to open the gotd socket up to anyone.
> The goal is to make server setup and maintenance easier.

Makes sense; and although setup is already pretty simple, I think
reducing friction wherever it makes sense is a worthy goal.

> Ok? Did I miss removing or adjusting anything?

looks good to me! ok

> diff c136f699978dbcb5baddbbbb3b8ffe593b8b2ff4 ed9245aadc25eb6eb5582c253fa17264b7439e14
> commit - c136f699978dbcb5baddbbbb3b8ffe593b8b2ff4
> commit + ed9245aadc25eb6eb5582c253fa17264b7439e14
> blob - 68f77c0970813d13f62fbcfba19833ca3a1efca1
> blob + 889d8f29f10dc0d2fd25bab36acde18ffacaa943
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -29,7 +29,6 @@
>  #include <event.h>
>  #include <limits.h>
>  #include <pwd.h>
> -#include <grp.h>
>  #include <imsg.h>
>  #include <sha1.h>
>  #include <signal.h>
> @@ -141,7 +140,7 @@ unix_socket_listen(const char *unix_socket_path, uid_t
>  	}
>  
>  	old_umask = umask(S_IXUSR|S_IXGRP|S_IWOTH|S_IROTH|S_IXOTH);
> -	mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP;
> +	mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH;
>  
>  	if (bind(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1) {
>  		log_warn("bind: %s", unix_socket_path);
> @@ -176,25 +175,6 @@ static struct group *
>  	return fd;
>  }
>  
> -static struct group *
> -match_group(gid_t *groups, int ngroups, const char *unix_group_name)
> -{
> -	struct group *gr;
> -	int i;
> -
> -	for (i = 0; i < ngroups; i++) {
> -		gr = getgrgid(groups[i]);
> -		if (gr == NULL) {
> -			log_warn("getgrgid %d", groups[i]);
> -			continue;
> -		}
> -		if (strcmp(gr->gr_name, unix_group_name) == 0)
> -			return gr;
> -	}
> -
> -	return NULL;
> -}
> -
>  static uint64_t
>  client_hash(uint32_t client_id)
>  {
> @@ -2406,10 +2386,7 @@ main(int argc, char **argv)
>  	const char *confpath = GOTD_CONF_PATH;
>  	char *argv0 = argv[0];
>  	char title[2048];
> -	gid_t groups[NGROUPS_MAX];
> -	int ngroups = NGROUPS_MAX;
>  	struct passwd *pw = NULL;
> -	struct group *gr = NULL;
>  	char *repo_path = NULL;
>  	enum gotd_procid proc_id = PROC_GOTD;
>  	struct event evsigint, evsigterm, evsighup, evsigusr1;
> @@ -2492,23 +2469,6 @@ main(int argc, char **argv)
>  		    getprogname(), pw->pw_name, getprogname());
>  	}
>  
> -	if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1)
> -		log_warnx("group membership list truncated");
> -
> -	gr = match_group(groups, ngroups, gotd.unix_group_name);
> -	if (gr == NULL) {
> -		fatalx("cannot start %s: the user running %s "
> -		    "must be a secondary member of group %s",
> -		    getprogname(), getprogname(), gotd.unix_group_name);
> -	}
> -	if (gr->gr_gid == pw->pw_gid) {
> -		fatalx("cannot start %s: the user running %s "
> -		    "must be a secondary member of group %s, but "
> -		    "%s is the user's primary group",
> -		    getprogname(), getprogname(), gotd.unix_group_name,
> -		    gotd.unix_group_name);
> -	}
> -
>  	if (proc_id == PROC_LISTEN &&
>  	    !got_path_is_absolute(gotd.unix_socket_path))
>  		fatalx("bad unix socket path \"%s\": must be an absolute path",
> @@ -2529,11 +2489,10 @@ main(int argc, char **argv)
>  		if (verbosity) {
>  			log_info("socket: %s", gotd.unix_socket_path);
>  			log_info("user: %s", pw->pw_name);
> -			log_info("secondary group: %s", gr->gr_name);
>  		}
>  
>  		fd = unix_socket_listen(gotd.unix_socket_path, pw->pw_uid,
> -		    gr->gr_gid);
> +		    pw->pw_gid);
>  		if (fd == -1) {
>  			fatal("cannot listen on unix socket %s",
>  			    gotd.unix_socket_path);
> blob - a45eb65170386548a9b8a2b7812f7309eebdc6f6
> blob + ccde19f88e7b40b792b110a9d7de24f505e67cae
> --- gotd/gotd.conf.5
> +++ gotd/gotd.conf.5
> @@ -92,19 +92,6 @@ will be used.
>  If not specified, the path
>  .Pa /var/run/gotd.sock
>  will be used.
> -.It Ic unix_group Ar group
> -Set the
> -.Ar group ,
> -defined in the
> -.Xr group 5
> -file, which is allowed to access
> -.Xr gotd 8
> -via
> -.Xr gotsh 1 .
> -The
> -.Xr gotd 8
> -user must be a secondary member of this group.
> -If not specified, the group _gotsh will be used.
>  .It Ic user Ar user
>  Set the
>  .Ar user
> @@ -195,8 +182,7 @@ configuration file.
>  .El
>  .Sh EXAMPLES
>  .Bd -literal -offset indent
> -# Default unix_group and user values:
> -unix_group _gotsh
> +# Run as the default user:
>  user _gotd
>  
>  # This repository can be accessed via ssh://user@example.com/src
> @@ -228,5 +214,4 @@ connection {
>  .Sh SEE ALSO
>  .Xr got 1 ,
>  .Xr gotsh 1 ,
> -.Xr group 5 ,
>  .Xr gotd 8
> blob - 82485863fa9ea61c5334cb5ecd4b70eac7128cb4
> blob + 266fba3b38a61177a81510a5a8753084f64b1667
> --- gotd/gotd.h
> +++ gotd/gotd.h
> @@ -17,7 +17,6 @@
>  
>  #define GOTD_UNIX_SOCKET "/var/run/gotd.sock"
>  #define GOTD_UNIX_SOCKET_BACKLOG 10
> -#define GOTD_UNIX_GROUP	"_gotsh"
>  #define GOTD_USER	"_gotd"
>  #define GOTD_CONF_PATH	"/etc/gotd.conf"
>  #define GOTD_EMPTY_PATH	"/var/empty"
> @@ -119,7 +118,6 @@ struct gotd {
>  struct gotd {
>  	pid_t pid;
>  	char unix_socket_path[PATH_MAX];
> -	char unix_group_name[32];
>  	char user_name[32];
>  	struct gotd_repolist repos;
>  	int nrepos;
> blob - d8319e4be5f8185780729a14a4a706eb136762d2
> blob + 2103f5caa2ef025a6e22f6667363ba2268844af0
> --- gotd/parse.y
> +++ gotd/parse.y
> @@ -104,7 +104,7 @@ typedef struct {
>  
>  %}
>  
> -%token	PATH ERROR ON UNIX_SOCKET UNIX_GROUP USER REPOSITORY PERMIT DENY
> +%token	PATH ERROR ON UNIX_SOCKET USER REPOSITORY PERMIT DENY
>  %token	RO RW CONNECTION LIMIT REQUEST TIMEOUT
>  
>  %token	<v.string>	STRING
> @@ -207,17 +207,6 @@ main		: UNIX_SOCKET STRING {
>  			}
>  			free($2);
>  		}
> -		| UNIX_GROUP STRING {
> -			if (strlcpy(gotd->unix_group_name, $2,
> -			    sizeof(gotd->unix_group_name)) >=
> -			    sizeof(gotd->unix_group_name)) {
> -				yyerror("%s: unix group name too long",
> -				    __func__);
> -				free($2);
> -				YYERROR;
> -			}
> -			free($2);
> -		}
>  		| USER STRING {
>  			if (strlcpy(gotd->user_name, $2,
>  			    sizeof(gotd->user_name)) >=
> @@ -372,7 +361,6 @@ lookup(char *s)
>  		{ "ro",				RO },
>  		{ "rw",				RW },
>  		{ "timeout",			TIMEOUT },
> -		{ "unix_group",			UNIX_GROUP },
>  		{ "unix_socket",		UNIX_SOCKET },
>  		{ "user",			USER },
>  	};
> @@ -715,11 +703,6 @@ parse_config(const char *filename, enum gotd_procid pr
>  		fprintf(stderr, "%s: unix socket path too long", __func__);
>  		return -1;
>  	}
> -	if (strlcpy(gotd->unix_group_name, GOTD_UNIX_GROUP,
> -	    sizeof(gotd->unix_group_name)) >= sizeof(gotd->unix_group_name)) {
> -		fprintf(stderr, "%s: unix group name too long", __func__);
> -		return -1;
> -	}
>  	if (strlcpy(gotd->user_name, GOTD_USER,
>  	    sizeof(gotd->user_name)) >= sizeof(gotd->user_name)) {
>  		fprintf(stderr, "%s: user name too long", __func__);
> blob - 436c016514b9a9589c787504047ecff6003477d9
> blob + 8e552939759f9d99f850f531a4a7041b8aaa2956
> --- gotsh/gotsh.1
> +++ gotsh/gotsh.1
> @@ -66,13 +66,6 @@ must be members of the group which has read/write perm
>  .Pp
>  Users running
>  .Nm
> -must be members of the group which has read/write permission to the
> -.Xr gotd 8
> -unix socket.
> -The group used for this purpose can be configured in
> -.Xr gotd.conf 5 .
> -Users running
> -.Nm
>  should not have access to Git repositories by means other than
>  accessing the unix socket of
>  .Xr gotd 8
> @@ -97,22 +90,24 @@ The following
>  will be used.
>  .El
>  .Sh EXAMPLES
> -The following
>  .Xr sshd_config 5
> -directives are recommended to protect the server machine and any systems
> -reachable from it via
> -.Xr ssh 1
> -forwarding features.
> -This example assumes the group called
> -.Dq _gotsh
> -has read/write access to the
> -.Xr gotd 8
> -unix socket.
> +directives such as the following are recommended to protect the server
> +machine and any systems reachable from it, especially if anonymous users
> +are allowed to connect:
>  .Bd -literal -offset indent
> -Match Group _gotsh
> +Match User anonymous
>      DisableForwarding yes
>      PermitTTY no
>  .Ed
> +.Pp
> +It can be convenient to add all relevant users to a common group, such as
> +.Dq developers ,
> +and then use this group as the Match criteria:
> +.Bd -literal -offset indent
> +Match Group developers
> +    DisableForwarding yes
> +    PermitTTY no
> +.Ed
>  .Sh SEE ALSO
>  .Xr got 1 ,
>  .Xr ssh 1 ,
> blob - 0fe6926134b97333a0a4a221f988bb8f85ab7242
> blob + c43ea128c1006bf4ed809106ce3ea7d8811bbf90
> --- regress/gotd/Makefile
> +++ regress/gotd/Makefile
> @@ -25,7 +25,6 @@ GOTD_GROUP?=gotsh
>  
>  # gotd.conf parameters
>  GOTD_USER?=got
> -GOTD_GROUP?=gotsh
>  GOTD_SOCK=${GOTD_DEVUSER_HOME}/gotd.sock
>  
>  .if "${GOT_RELEASE}" == "Yes"
> @@ -60,7 +59,6 @@ start_gotd_ro: ensure_root
>  
>  start_gotd_ro: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -71,7 +69,6 @@ start_gotd_ro_group: ensure_root
>  
>  start_gotd_ro_group: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -83,7 +80,6 @@ start_gotd_ro_denied_user: ensure_root
>  # try a permit rule followed by a deny rule; last matched rule wins
>  start_gotd_ro_denied_user: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -96,7 +92,6 @@ start_gotd_ro_denied_group: ensure_root
>  # try a permit rule followed by a deny rule; last matched rule wins
>  start_gotd_ro_denied_group: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -109,7 +104,6 @@ start_gotd_ro_bad_user: ensure_root
>  # $GOTD_DEVUSER should not equal $GOTD_USER
>  start_gotd_ro_bad_user: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -121,7 +115,6 @@ start_gotd_ro_bad_group: ensure_root
>  # $GOTD_DEVUSER should not be in group wheel
>  start_gotd_ro_bad_group: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> @@ -132,7 +125,6 @@ start_gotd_rw: ensure_root
>  
>  start_gotd_rw: ensure_root
>  	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
> -	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
>  	@echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf
>  	@echo 'repository "test-repo" {' >> $(PWD)/gotd.conf
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
> blob - 09b2d5993cc7481df50e85fbda4705700de997bd
> blob + d75faae64b46a7c347ecb77c6631207a3ded44b0
> --- regress/gotd/README
> +++ regress/gotd/README
> @@ -1,13 +1,12 @@
>  Running server regression tests requires some manual system preparation.
>  
> -Two dedicated user accounts and a group must be created. Password login
> +Two dedicated user accounts must be created. Password login
>  for these users should be disabled.
>  
> - $ doas groupadd gotsh
> - $ doas useradd -m -G gotsh got
> - $ doas useradd -m -G gotsh gotdev
> + $ doas useradd -m got
> + $ doas useradd -m gotdev
>  
> -The above user and group names correspond to defaults used by the test suite.
> +The above user names correspond to defaults used by the test suite.
>  If needed, the defaults can be overridden on by passing values for the
>  following variables to make(1): GOTD_USER, GOTD_DEVUSER, GOTD_GROUP
>  
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68