On Tue, Mar 28, 2023 at 09:37:39AM +0200, Omar Polo wrote:
> I get the idea, but this doesn't seem to be free from
> misunderstanding.  One can still end up using git-receive-pack because
> they forgot to add one repository stanza to gotd.conf.  Maybe we
> should aim for a more strict approach (or at least by default) where
> it either uses gotd or git-*-pack unconditionally?

That would probably not really help us in practice, unfortunately.
It would put parts of the problem onto users again. The approach
I implemented seems like the most practical solution to me.

There will always be mixed uses cases. Users might have git repositories
somewhere in their home directory which have nothing to do with a gotd
setup running on the same machine. By design, gotd is unable to serve
such repositories, which don't belong to the gotd user ID and aren't
subject to access rules listed in gotd.conf.

If a repository is not mentioned in gotd.conf then gotd and gotsh
should not be expected to take control of it. But once a repository
is mentioned in gotd.conf it is a reasonable expectation that access
via gotsh will just work without having to uninstall the git package
first or having to play tricks with PATH.

The only downside I see is that repo names in gotd.conf could in
theory collide with user-created paths in the filesystem.
But that is an edge case and should not be a big problem when it
happens and the solution should be obvious. The user would simply
have to move their repository to a different path in order to use
native Git tooling.

> few comments below, but it's ok for me as-is (can fix those in-tree.)

Thanks! I have already made some changes before reading your feedback.
Some of these changes happen to address feedback already. I will get
the rest dealt with in follow-up commits.

This improved version has been tested more thoroughly and fixes
a few bugs such that gitwrapper actually works as advertised.
It also makes use of unveil(2) to restrict the set of programs
which can be run (even though a user who ends up running this
tool probably has shell access anyway).

Regarding exec without fork, I tried this initially but gitwrapper
always ended up with SIGABRT in exec. Not sure why. But forking is
not a huge deal.

diffstat refs/heads/main refs/heads/gitwrapper
 M  Makefile                 |    3+   1-
 M  README                   |    2+   0-
 A  gitwrapper/Makefile      |   41+   0-
 A  gitwrapper/gitwrapper.1  |  114+   0-
 A  gitwrapper/gitwrapper.c  |  226+   0-
 M  got-dist.txt             |    4+   0-
 M  gotd/gotd.c              |    5+  22-
 M  gotd/gotd.h              |    1+   0-
 M  gotd/parse.y             |   18+   0-
 M  lib/serve.c              |    2+   2-

10 files changed, 416 insertions(+), 25 deletions(-)

diff refs/heads/main refs/heads/gitwrapper
commit - fdd6701019b668502be2d00e042b3f9932a78f36
commit + 3d0842e54963b38e79f73c8bb04880324952eea1
blob - 8f2de2846e8dacea7b203a6e29ba07b05a069a13
blob + 86e0c721884cc636b289ca3fd5d77a75bedc9f8f
--- Makefile
+++ Makefile
@@ -7,7 +7,7 @@ SUBDIR += gotwebd gotd gotsh gotctl template
 .endif
 
 .if make(clean) || make(obj) || make(release)
-SUBDIR += gotwebd gotd gotsh gotctl template
+SUBDIR += gotwebd gotd gotsh gotctl template gitwrapper
 .endif
 
 .if make(tags) || make(cleandir)
@@ -49,11 +49,13 @@ server:
 	${MAKE} -C gotctl
 	${MAKE} -C gotd
 	${MAKE} -C gotsh
+	${MAKE} -C gitwrapper
 
 server-install:
 	${MAKE} -C gotctl install
 	${MAKE} -C gotd install
 	${MAKE} -C gotsh install
+	${MAKE} -C gitwrapper install
 
 server-regress:
 	${MAKE} -C regress/gotd
blob - a67c022cd57805a8d11d93379be5d9306df2ec48
blob + 2328859beb94e97d832f30c6cbb5eb26ac6d3b0e
--- README
+++ README
@@ -73,6 +73,7 @@ This will install the following commands:
  gotd, the repository server program
  gotctl, the server control utility
  gotsh, the login shell for users accessing the server via the network
+ gitwrapper, like mailwrapper(8) but for git-upload-pack and git-receive-pack
 
 See the following manual page files for information about server setup:
 
@@ -80,6 +81,7 @@ See the following manual page files for information ab
  $ man -l gotd/gotd.conf.5
  $ man -l gotctl/gotctl.8
  $ man -l gotsh/gotsh.1
+ $ man -l gitwrapper/gitwrapper.1
 
 See regress/gotd/README for information about running the server test suite.
 
blob - /dev/null
blob + cd56f97718ffa1b0dc5eabec3e2e6048cbb763b7 (mode 644)
--- /dev/null
+++ gitwrapper/Makefile
@@ -0,0 +1,41 @@
+.PATH:${.CURDIR}/../lib
+.PATH:${.CURDIR}/../gotd
+
+.include "../got-version.mk"
+
+.if ${GOT_RELEASE} == "Yes"
+BINDIR ?=	${PREFIX}/bin
+.endif
+
+PROG=		gitwrapper
+SRCS=		gitwrapper.c parse.y log.c serve.c auth.c listen.c pkt.c \
+		error.c gitproto.c hash.c reference.c object.c object_parse.c \
+		path.c object_idset.c object_create.c inflate.c opentemp.c \
+		lockfile.c repository.c gotconfig.c pack.c bloom.c buf.c \
+		object_cache.c privsep_stub.c pollfd.c imsg.c \
+		reference_parse.c object_open_io.c sigs.c deflate.c \
+		read_gotconfig.c read_gitconfig.c delta_cache.c delta.c \
+		murmurhash2.c date.c gitconfig.c
+
+CLEANFILES = parse.h
+
+MAN =		${PROG}.conf.5 ${PROG}.1
+
+CPPFLAGS = -I${.CURDIR}/../include -I${.CURDIR}/../lib -I${.CURDIR}/../gotd
+
+.if defined(PROFILE)
+LDADD = -lutil_p -lz_p -lm_p -lc_p -levent_p
+.else
+LDADD = -lutil -lz -lm -levent
+.endif
+DPADD = ${LIBZ} ${LIBUTIL}
+
+.if ${GOT_RELEASE} != "Yes"
+NOMAN = Yes
+.endif
+
+realinstall:
+	${INSTALL} ${INSTALL_COPY} -o ${BINOWN} -g ${BINGRP} \
+	-m ${BINMODE} ${PROG} ${BINDIR}/${PROG}
+
+.include <bsd.prog.mk>
blob - /dev/null
blob + 1abe622b5804d7eb31b472b5d727cb086f7c8c7e (mode 644)
--- /dev/null
+++ gitwrapper/gitwrapper.1
@@ -0,0 +1,114 @@
+.\"
+.\" Copyright (c) 2023 Stefan Sperling
+.\"
+.\" 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.
+.\"
+.Dd $Mdocdate$
+.Dt GITWRAPPER 1
+.Os
+.Sh NAME
+.Nm gitwrapper
+.Nd invoke an appropriate Git repository server
+.Sh SYNOPSIS
+.Nm Fl c Sq Cm git-receive-pack Ar repository-path
+.Nm Fl c Sq Cm git-upload-pack Ar repository-path
+.Sh DESCRIPTION
+At one time, the only Git repository server software easily available
+was built into
+.Xr git-upload-pack 1
+and
+.Xr git-receive-pack 1
+which are part of the
+.Xr git 1
+suite.
+As a result of this, most Git client implementations had the path and
+calling conventions expected by
+.Xr git 1
+compiled in.
+.Pp
+Times have changed, however.  On a modern system, the administrator may
+wish to use one of several available Git repository servers, such as
+.Xr gotd 8 .
+.Pp
+It would be difficult to modify all Git client software typically available
+on a system, so most of the authors of alternative Git servers have written
+their programs so that they use the same calling conventions as
+.Xr git-upload-pack 1
+and
+.Xr git-receive-pack 1
+and may be put into place in their stead.
+.Pp
+Although having drop-in replacements for
+.Xr git-upload-pack 1
+and
+.Xr git-receive-pack 1
+helps in installing alternative Git servers, it essentially makes the
+configuration of the system depend on hard installing new programs in /usr.
+This leads to configuration problems for many administrators, since they may
+wish to install a new Git server without altering the system provided /usr.
+(This may be, for example, to avoid having upgrade problems when a new
+version of the system is installed over the old.)  They may also have a
+shared /usr among several machines, and may wish to avoid placing implicit
+configuration information in a read-only /usr.
+.Pp
+The
+.Nm
+program is designed to replace
+.Xr git-upload-pack 1
+and
+.Xr git-receive-pack 1
+and to invoke an appropriate Git server based on configuration information
+placed in
+.Xr gotd.conf 5 .
+This permits the administrator to configure which Git server is to be
+invoked on the system at run-time.
+Git repositories which are listed in
+.Xr gotd.conf 5
+and exist on the filesystem will be served by
+.Xr gotsh 1 .
+Any other Git repositories will be served by
+.Xr git-upload-pack 1
+and
+.Xr git-receive-pack 1 .
+.Sh FILES
+Configuration for
+.Xr gotd 8
+is kept in
+.Pa /etc/gotd.conf.
+.Pp
+.Pa git-upload-pack
+and
+.Pa git-receive-pack
+are typically set up as a symlink to
+.Nm
+which is not usually invoked on its own.
+.Sh ENVIRONMENT
+.Bl -tag -width GOTD_CONF_PATH
+.It Ev GOTD_CONF_PATH
+Set the path to the configuration file for
+.Xr gotd 8 .
+If not specified, the default path
+.Pa /etc/gotd.conf
+will be used.
+.El
+.Sh SEE ALSO
+.Xr got 1 ,
+.Xr gotd.conf 5 ,
+.Xr gotd 8 ,
+.Xr mailwrapper 8
+.Sh AUTHORS
+.An Stefan Sperling Aq Mt stsp@openbsd.org
+.Sh BUGS
+The entire reason this program exists is a crock. Instead, a command for
+invoking a Git server should be standardized or the Git protocol should
+be changed to make the path to the program discoverable by Git clients.
blob - /dev/null
blob + a5d0bcb2233ff202330b3c197e216e3300f272f4 (mode 644)
--- /dev/null
+++ gitwrapper/gitwrapper.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright (c) 2023 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.
+ */
+
+/*
+ * Resolve path namespace conflicts for git-upload-pack and git-receive-pack.
+ */
+
+#include <sys/queue.h>
+#include <sys/types.h>
+#include <sys/uio.h>
+#include <sys/wait.h>
+
+#include <err.h>
+#include <errno.h>
+#include <event.h>
+#include <imsg.h>
+#include <limits.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sha1.h>
+#include <sha2.h>
+#include <syslog.h>
+#include <util.h>
+#include <unistd.h>
+
+#include "got_error.h"
+#include "got_path.h"
+#include "got_serve.h"
+
+#include "gotd.h"
+#include "log.h"
+
+#ifndef GITWRAPPER_GIT_LIBEXEC_DIR
+#define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/local/libexec/git"
+#endif
+
+#ifndef GITWRAPPER_MY_SERVER_PROG
+#define GITWRAPPER_MY_SERVER_PROG "gotsh"
+#endif
+
+
+__dead static void
+usage(void)
+{
+	fprintf(stderr, "usage: %s -c '%s|%s repository-path'\n",
+	    getprogname(), GOT_SERVE_CMD_SEND, GOT_SERVE_CMD_FETCH);
+	exit(1);
+}
+
+/*
+ * Unveil the specific programs we want to start and hide everything else.
+ * This is important to limit the impact of our "exec" pledge.
+ */
+static const struct got_error *
+apply_unveil(const char *myserver)
+{
+	const char *fetchcmd = GITWRAPPER_GIT_LIBEXEC_DIR "/" \
+		GOT_SERVE_CMD_FETCH;
+	const char *sendcmd = GITWRAPPER_GIT_LIBEXEC_DIR "/" \
+		GOT_SERVE_CMD_SEND;
+
+#ifdef PROFILE
+	if (unveil("gmon.out", "rwc") != 0)
+		return got_error_from_errno2("unveil", "gmon.out");
+#endif
+	if (unveil(fetchcmd, "x") != 0)
+		return got_error_from_errno2("unveil", fetchcmd);
+
+	if (unveil(sendcmd, "x") != 0)
+		return got_error_from_errno2("unveil", sendcmd);
+
+	if (myserver && unveil(myserver, "x") != 0)
+		return got_error_from_errno2("unveil", myserver);
+
+	if (unveil(NULL, NULL) != 0)
+		return got_error_from_errno("unveil");
+
+	return NULL;
+}
+
+int
+main(int argc, char *argv[])
+{
+	const struct got_error *error;
+	const char *confpath = NULL;
+	char *command = NULL, *repo_name = NULL; /* for matching gotd.conf */
+	char *myserver = NULL;
+	const char *repo_path = NULL; /* as passed on the command line */
+	const char *relpath;
+	char *gitcommand = NULL;
+	struct gotd gotd;
+	struct gotd_repo *repo = NULL;
+	pid_t pid;
+	int st = -1;
+
+	log_init(1, LOG_USER); /* Log to stderr. */
+
+#ifndef PROFILE
+	if (pledge("stdio rpath proc exec unveil", NULL) == -1)
+		err(1, "pledge");
+#endif
+
+	/*
+	 * Look up our own server program in PATH so we can unveil(2) it.
+	 * This call only errors out upon memory allocation failure.
+	 * If the program cannot be found then myserver will be set to NULL.
+	 */
+	error = got_path_find_prog(&myserver, GITWRAPPER_MY_SERVER_PROG);
+	if (error)
+		goto done;
+
+	/*
+	 * Run parse_config() before unveil(2) because parse_config()
+	 * checks whether repository paths exist on disk.
+	 * Parsing errors and warnings will be logged to stderr.
+	 * Upon failure we will run Git's native tooling so do not
+	 * bother checking for errors here.
+	 */
+	confpath = getenv("GOTD_CONF_PATH");
+	if (confpath == NULL)
+		confpath = GOTD_CONF_PATH;
+	parse_config(confpath, PROC_GOTD, &gotd);
+
+	error = apply_unveil(myserver);
+	if (error)
+		goto done;
+
+#ifndef PROFILE
+	if (pledge("stdio proc exec", NULL) == -1)
+		err(1, "pledge");
+#endif
+
+	if (strcmp(getprogname(), GOT_SERVE_CMD_SEND) == 0 ||
+	    strcmp(getprogname(), GOT_SERVE_CMD_FETCH) == 0) {
+		if (argc != 2)
+			usage();
+		command = strdup(getprogname());
+		if (command == NULL) {
+			error = got_error_from_errno("strdup");
+			goto done;
+		}
+		repo_path = argv[1];
+		relpath = argv[1];
+		while (relpath[0] == '/')
+			relpath++;
+		repo_name = strdup(relpath);
+		if (repo_name == NULL) {
+			error = got_error_from_errno("strdup");
+			goto done;
+		}
+	} else {
+		if (argc != 3 || strcmp(argv[1], "-c") != 0)
+			usage();
+		repo_path = argv[2];
+		error = got_serve_parse_command(&command, &repo_name,
+		    repo_path);
+		if (error && error->code == GOT_ERR_BAD_PACKET)
+			usage();
+		if (error)
+			goto done;
+	}
+
+	repo = gotd_find_repo_by_name(repo_name, &gotd);
+
+	/*
+	 * Invoke our custom Git server if it was found in PATH and
+	 * if the repository was found in gotd.conf.
+	 * Otherwise invoke native git(1) tooling.
+	 */
+	switch (pid = fork()) {
+	case -1:
+		goto done;
+	case 0:
+		if (repo && myserver) {
+			if (execl(myserver, command, repo_name,
+			    (char *)NULL) ==  -1) {
+				error = got_error_from_errno2("execl",
+				    myserver);
+				goto done;
+			 }
+		} else {
+			if (asprintf(&gitcommand, "%s/%s",
+			    GITWRAPPER_GIT_LIBEXEC_DIR, command) == -1) {
+				error = got_error_from_errno("asprintf");
+				goto done;
+			}
+			if (execl(gitcommand, gitcommand, repo_path,
+			    (char *)NULL) == -1) {
+				error = got_error_from_errno2("execl",
+				    gitcommand);
+				goto done;
+			 }
+		}
+		_exit(127);
+	}
+
+	while (waitpid(pid, &st, 0) == -1) {
+		if (errno != EINTR)
+			break;
+	}
+done:
+	free(command);
+	free(repo_name);
+	free(myserver);
+	free(gitcommand);
+	if (error) {
+		fprintf(stderr, "%s: %s\n", getprogname(), error->msg);
+		return 1;
+	}
+
+	return 0;
+}
blob - 0779cab1856784fdd06081ce2ddf025599ae207a
blob + e030b87f2810cd0261799b0a15ef3e38beeb6ae0
--- got-dist.txt
+++ got-dist.txt
@@ -5,6 +5,10 @@
 /Makefile.inc
 /README
 /TODO
+/gitwrapper
+/gitwrapper/Makefile
+/gitwrapper/gitwrapper.1
+/gitwrapper/gitwrapper.c
 /got
 /got-version.mk
 /got/Makefile
blob - 86f2b6013f0c53e31f8adbc89cd954b062c2ee0b
blob + c7a273204feec2eff188ea2e2530aa3c3d833253
--- gotd/gotd.c
+++ gotd/gotd.c
@@ -499,24 +499,6 @@ static struct gotd_repo *
 	return NULL;
 }
 
-static struct gotd_repo *
-find_repo_by_name(const char *repo_name)
-{
-	struct gotd_repo *repo;
-	size_t namelen;
-
-	TAILQ_FOREACH(repo, &gotd.repos, entry) {
-		namelen = strlen(repo->name);
-		if (strncmp(repo->name, repo_name, namelen) != 0)
-			continue;
-		if (repo_name[namelen] == '\0' ||
-		    strcmp(&repo_name[namelen], ".git") == 0)
-			return repo;
-	}
-
-	return NULL;
-}
-
 static const struct got_error *
 start_client_authentication(struct gotd_client *client, struct imsg *imsg)
 {
@@ -541,7 +523,7 @@ start_client_authentication(struct gotd_client *client
 		err = ensure_client_is_not_writing(client);
 		if (err)
 			return err;
-		repo = find_repo_by_name(ireq.repo_name);
+		repo = gotd_find_repo_by_name(ireq.repo_name, &gotd);
 		if (repo == NULL)
 			return got_error(GOT_ERR_NOT_GIT_REPO);
 		err = start_auth_child(client, GOTD_AUTH_READ, repo,
@@ -553,7 +535,7 @@ start_client_authentication(struct gotd_client *client
 		err = ensure_client_is_not_reading(client);
 		if (err)
 			return err;
-		repo = find_repo_by_name(ireq.repo_name);
+		repo = gotd_find_repo_by_name(ireq.repo_name, &gotd);
 		if (repo == NULL)
 			return got_error(GOT_ERR_NOT_GIT_REPO);
 		err = start_auth_child(client,
@@ -1142,7 +1124,7 @@ gotd_dispatch_auth_child(int fd, short event, void *ar
 		goto done;
 	}
 
-	repo = find_repo_by_name(client->auth->repo_name);
+	repo = gotd_find_repo_by_name(client->auth->repo_name, &gotd);
 	if (repo == NULL) {
 		err = got_error(GOT_ERR_NOT_GIT_REPO);
 		goto done;
@@ -1292,8 +1274,9 @@ gotd_dispatch_client_session(int fd, short event, void
 
 		if (do_start_repo_child) {
 			struct gotd_repo *repo;
+			const char *name = client->session->repo_name;
 
-			repo = find_repo_by_name(client->session->repo_name);
+			repo = gotd_find_repo_by_name(name, &gotd);
 			if (repo != NULL) {
 				enum gotd_procid proc_type;
 
blob - e22197aef200b4b37d02bf22b05649f1af0a55a4
blob + 63cffc677d842269d09d3f07a3cc61bdf1e6c28b
--- gotd/gotd.h
+++ gotd/gotd.h
@@ -447,6 +447,7 @@ int parse_config(const char *, enum gotd_procid, struc
 };
 
 int parse_config(const char *, enum gotd_procid, struct gotd *);
+struct gotd_repo *gotd_find_repo_by_name(const char *, struct gotd *);
 
 /* imsg.c */
 const struct got_error *gotd_imsg_flush(struct imsgbuf *);
blob - 7c1d70c3d90da2de044cf00d5ed56f0b3efae0c3
blob + 9a5b91d273ade58fbf2195742c6bf0866eb7ac69
--- gotd/parse.y
+++ gotd/parse.y
@@ -892,3 +892,21 @@ symget(const char *nam)
 	}
 	return (NULL);
 }
+
+struct gotd_repo *
+gotd_find_repo_by_name(const char *repo_name, struct gotd *gotd)
+{
+	struct gotd_repo *repo;
+	size_t namelen;
+
+	TAILQ_FOREACH(repo, &gotd->repos, entry) {
+		namelen = strlen(repo->name);
+		if (strncmp(repo->name, repo_name, namelen) != 0)
+			continue;
+		if (repo_name[namelen] == '\0' ||
+		    strcmp(&repo_name[namelen], ".git") == 0)
+			return repo;
+	}
+
+	return NULL;
+}
blob - 14220632c56b0614a6dab68fe22981fd134183c8
blob + 8ef2f8a5f604c4672e7ef11adbba4c6af455fd18
--- lib/serve.c
+++ lib/serve.c
@@ -130,12 +130,12 @@ got_serve_parse_command(char **command, char **repo_pa
 		goto done;
 	}
 	pathlen = strlen(abspath);
-	canonpath = malloc(pathlen);
+	canonpath = malloc(pathlen + 1);
 	if (canonpath == NULL) {
 		err = got_error_from_errno("malloc");
 		goto done;
 	}
-	err = got_canonpath(abspath, canonpath, pathlen);
+	err = got_canonpath(abspath, canonpath, pathlen + 1);
 	if (err)
 		goto done;