gotd group auth is broken. It matches against the groups of
the user running gotd instead of the user which is connecting
to the socket :-) Fix this and add tests for various cases
where gotd should deny repository read access.

ok?

diff refs/heads/main refs/heads/gotd-auth-group
commit - c2a4f618fa3c9683fbb5f384b117f7e45a041122
commit + c8b61813bb25b1170687311d5deba5f694b21a93
blob - b8b141d19d580a01c1cc68f758b798a5f2947b11
blob + ce2c2df55ff1550b6443b71f5873da9002df3eac
--- gotd/auth.c
+++ gotd/auth.c
@@ -21,6 +21,7 @@
 
 #include <errno.h>
 #include <event.h>
+#include <grp.h>
 #include <limits.h>
 #include <pwd.h>
 #include <grp.h>
@@ -29,10 +30,12 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <imsg.h>
+#include <unistd.h>
 
 #include "got_error.h"
 
 #include "gotd.h"
+#include "log.h"
 #include "auth.h"
 
 static int
@@ -93,8 +96,10 @@ match_identifier(const char *identifier, gid_t *groups
 		gid_t rgid;
 		if (parsegid(identifier + 1, &rgid) == -1)
 			return 0;
+		if (rgid == egid)
+			return 1;
 		for (i = 0; i < ngroups; i++) {
-			if (rgid == groups[i] && egid == rgid)
+			if (rgid == groups[i])
 				break;
 		}
 		if (i == ngroups)
@@ -107,12 +112,22 @@ gotd_auth_check(struct gotd_access_rule_list *rules, c
 
 const struct got_error *
 gotd_auth_check(struct gotd_access_rule_list *rules, const char *repo_name,
-    gid_t *groups, int ngroups, uid_t euid, gid_t egid,
-    int required_auth)
+    uid_t euid, gid_t egid, int required_auth)
 {
 	struct gotd_access_rule *rule;
 	enum gotd_access access = GOTD_ACCESS_DENIED;
+	struct passwd *pw;
+	gid_t groups[NGROUPS_MAX];
+	int ngroups;
 
+	errno = 0; /* allow us to tell apart getpwuid does not clear */
+	pw = getpwuid(euid);
+	if (pw == NULL)
+		return got_error_from_errno("getpwuid");
+
+	if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1)
+		log_warnx("group membership list truncated");
+
 	STAILQ_FOREACH(rule, rules, entry) {
 		if (!match_identifier(rule->identifier, groups, ngroups,
 		    euid, egid))
blob - 480be955e04852bc1940c62233137deaeebcaedf
blob + 506f5e8b84ac2182f7114a5f4569b3b8d6963cc2
--- gotd/auth.h
+++ gotd/auth.h
@@ -16,4 +16,4 @@ gotd_auth_check(struct gotd_access_rule_list *rules, c
 
 const struct got_error *
 gotd_auth_check(struct gotd_access_rule_list *rules, const char *repo_name,
-    gid_t *groups, int ngroups, uid_t euid, gid_t egid, int required_auth);
+    uid_t euid, gid_t egid, int required_auth);
blob - 526949eca1f9f3357614b701fd37f29764969911
blob + c9a5e9898ab184ee0b9168d135e56c9ae529e183
--- gotd/gotd.c
+++ gotd/gotd.c
@@ -630,8 +630,7 @@ forward_list_refs_request(struct gotd_client *client, 
 		if (repo == NULL)
 			return got_error(GOT_ERR_NOT_GIT_REPO);
 		err = gotd_auth_check(&repo->rules, repo->name,
-		    gotd.groups, gotd.ngroups, client->euid, client->egid,
-		    GOTD_AUTH_READ);
+		    client->euid, client->egid, GOTD_AUTH_READ);
 		if (err)
 			return err;
 		client->repo_read = find_proc_by_repo_name(PROC_REPO_READ,
@@ -645,9 +644,8 @@ forward_list_refs_request(struct gotd_client *client, 
 		repo = find_repo_by_name(ireq.repo_name);
 		if (repo == NULL)
 			return got_error(GOT_ERR_NOT_GIT_REPO);
-		err = gotd_auth_check(&repo->rules, repo->name,
-		    gotd.groups, gotd.ngroups, client->euid, client->egid,
-		    GOTD_AUTH_READ | GOTD_AUTH_WRITE);
+		err = gotd_auth_check(&repo->rules, repo->name, client->euid,
+		    client->egid, GOTD_AUTH_READ | GOTD_AUTH_WRITE);
 		if (err)
 			return err;
 		client->repo_write = find_proc_by_repo_name(PROC_REPO_WRITE,
@@ -2097,6 +2095,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;
@@ -2167,11 +2166,10 @@ main(int argc, char **argv)
 		    getprogname(), pw->pw_name, getprogname());
 	}
 
-	if (getgrouplist(pw->pw_name, pw->pw_gid, gotd.groups, &ngroups) == -1)
+	if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1)
 		log_warnx("group membership list truncated");
-	gotd.ngroups = ngroups;
 
-	gr = match_group(gotd.groups, ngroups, gotd.unix_group_name);
+	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",
blob - fa1f5d66ce3b4c6691a931b445425921470634a2
blob + 1137c32a92e87b805d1195b4b06d153a9a53d1fa
--- gotd/gotd.h
+++ gotd/gotd.h
@@ -117,8 +117,6 @@ struct gotd {
 	struct event pause;
 	struct gotd_child_proc *procs;
 	int nprocs;
-	gid_t groups[NGROUPS_MAX];
-	int ngroups;
 };
 
 enum gotd_imsg_type {
blob - 773e9c0dfd3c2e287025d41ffed9f3749b380941
blob + cd20ee9bf8631fcd757767edf78df5f410e56161
--- regress/gotd/Makefile
+++ regress/gotd/Makefile
@@ -1,4 +1,7 @@
-REGRESS_TARGETS=test_repo_read test_repo_write test_repo_write_empty
+REGRESS_TARGETS=test_repo_read test_repo_read_group \
+	test_repo_read_denied_user test_repo_read_denied_group \
+	test_repo_read_bad_user test_repo_read_bad_group \
+	test_repo_write test_repo_write_empty
 NOOBJ=Yes
 
 .PHONY: ensure_root prepare_test_repo check_test_repo start_gotd
@@ -46,6 +49,67 @@ start_gotd_rw: ensure_root
 	@$(GOTD_TRAP); $(GOTD_START_CMD)
 	@$(GOTD_TRAP); sleep .5
 
+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
+	@echo '    permit ro :$(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
+	@echo "}" >> $(PWD)/gotd.conf
+	@$(GOTD_TRAP); $(GOTD_START_CMD)
+	@$(GOTD_TRAP); sleep .5
+
+# 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
+	@echo '    permit ro $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
+	@echo '    deny $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
+	@echo "}" >> $(PWD)/gotd.conf
+	@$(GOTD_TRAP); $(GOTD_START_CMD)
+	@$(GOTD_TRAP); sleep .5
+
+# 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
+	@echo '    permit ro $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
+	@echo '    deny :$(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
+	@echo "}" >> $(PWD)/gotd.conf
+	@$(GOTD_TRAP); $(GOTD_START_CMD)
+	@$(GOTD_TRAP); sleep .5
+
+# $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
+	@echo '    permit ro $(GOTD_USER)' >> $(PWD)/gotd.conf
+	@echo "}" >> $(PWD)/gotd.conf
+	@$(GOTD_TRAP); $(GOTD_START_CMD)
+	@$(GOTD_TRAP); sleep .5
+
+# $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
+	@echo '    permit ro :wheel' >> $(PWD)/gotd.conf
+	@echo "}" >> $(PWD)/gotd.conf
+	@$(GOTD_TRAP); $(GOTD_START_CMD)
+	@$(GOTD_TRAP); sleep .5
+
 start_gotd_rw: ensure_root
 	@echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf
 	@echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf
@@ -71,6 +135,36 @@ test_repo_write: prepare_test_repo start_gotd_rw
 	@$(GOTD_STOP_CMD) 2>/dev/null
 	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
 
+test_repo_read_group: prepare_test_repo start_gotd_ro_group
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_read.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
+test_repo_read_denied_user: prepare_test_repo start_gotd_ro_denied_user
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
+test_repo_read_denied_group: prepare_test_repo start_gotd_ro_denied_group
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
+test_repo_read_bad_user: prepare_test_repo start_gotd_ro_bad_user
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
+test_repo_read_bad_group: prepare_test_repo start_gotd_ro_bad_group
+	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
+		'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh'
+	@$(GOTD_STOP_CMD) 2>/dev/null
+	@su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh'
+
 test_repo_write: prepare_test_repo start_gotd_rw
 	@-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \
 		'env $(GOTD_TEST_ENV) sh ./repo_write.sh'
blob - /dev/null
blob + c8b4fc889c65a723539f0957fff96fda7cd7d32b (mode 644)
--- /dev/null
+++ regress/gotd/repo_read_access_denied.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+#
+# Copyright (c) 2022 Stefan Sperling <stsp@openbsd.org>
+#
+# Permission to use, copy, modify, and distribute this software for any
+# purpose with or without fee is hereby granted, provided that the above
+# copyright notice and this permission notice appear in all copies.
+#
+# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
+. ../cmdline/common.sh
+. ./common.sh
+
+test_clone_basic_access_denied() {
+	local testroot=`test_init clone_basic_access_denied 1`
+
+	cp -r ${GOTD_TEST_REPO} $testroot/repo-copy
+
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "got clone succeeded unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Verify that the clone operation failed.
+	echo 'got-fetch-pack: test-repo: Permission denied' \
+		> $testroot/stderr.expected
+	echo 'got: fetch failed' >> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+	fi
+	test_done "$testroot" "$ret"
+}
+
+test_parseargs "$@"
+run_test test_clone_basic_access_denied