"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
gotd: remove gotsh group requirement
To:
gameoftrees@openbsd.org
Date:
Tue, 3 Jan 2023 23:12:03 +0100

Download raw body.

Thread 
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.

Ok? Did I miss removing or adjusting anything?

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