- 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
Download raw body.
Thread
gotd: remove gotsh group requirement
Download raw body.
gotd: remove gotsh group requirement
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
gotd: remove gotsh group requirement