On 2023/03/28 00:03:31 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> I would like to make it easier for gotd and Git to co-exist.

That'd be awesome.

> At present it is quite easy for new users to end up in a situation
> where they configure gotd, yet end up running Git's versions of
> the git-upload-pack and git-receive-pack programs.
> This has already happened in at least one case I am aware of.
> If the user has write access to repositories it may even appear as if
> gotd was working just fine, even though it is actually Git which runs
> behind the scenes.
> 
> This patch adds a gitwrapper program which can be used in place of
> git-upload-pack and git-receive-pack. If the requested repository can
> be found in gotd.conf then gitwrapper will invoke gotsh which will
> talk to gotd. Otherwise gitwrapper will invoke native git binaries.
> 
> This idea was inspired by naddy@ who compared the current situation to
> the sendmail problem which resulted in the existence of mailwrapper(8).

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?

> The name is of couse a joke. I don't really feel comforable squatting
> on the git* namespace, but in this case it is probably harmless. We could
> change the name at any point in the future if it collides with something
> important. And the program would not actually need to be installed under
> its original name anyway, just as git-upload-pack and git-receive-pack.
> 
> For convenience, the OpenBSD ports tree could package gitwrapper
> separately from the gotd package, and have the git package depend
> on gitwrapper to provide these two programs in /usr/local/bin (the git
> package currently installs them in /usr/local/bin as well as in the
> git libexec directory; just libexec would then be enough).
> Installing the gotd package and adding repositories to gotd.conf
> would then be sufficient to make gotd work as expected.
> 
> ok?
> 
> There is also a small bug fix for lib/serve.c in there, where the size
> of the canonpath buffer does not account for a NUL terminator. This
> problem is triggerred by gitwrapper.

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

> diffstat refs/heads/main refs/heads/gitwrapper
>  M  Makefile                 |    2+  1-
>  M  README                   |    2+  0-
>  A  gitwrapper/Makefile      |   41+  0-
>  A  gitwrapper/gitwrapper.1  |  114+  0-
>  A  gitwrapper/gitwrapper.c  |  154+  0-
>  M  got-dist.txt             |    4+  0-
>  M  lib/serve.c              |    2+  2-
> 
> 7 files changed, 319 insertions(+), 3 deletions(-)
> 
> diff refs/heads/main refs/heads/gitwrapper
> commit - 5fdcbbb62bc65d9207f8fa4062b856ca904cdcc6
> commit + 5f80e2f030835cb073b057ea49be899a3e762e81
> blob - 8f2de2846e8dacea7b203a6e29ba07b05a069a13
> blob + 8d6fa8e66a64509014ab534cdc80ef36d065112e
> --- 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,6 +49,7 @@ server:
>  	${MAKE} -C gotctl
>  	${MAKE} -C gotd
>  	${MAKE} -C gotsh
> +	${MAKE} -C gitwrapper
>  
>  server-install:
>  	${MAKE} -C gotctl install
> 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

${PROG}.conf.5 looks like a copy-paste error.

> +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 + 8004ca843253b513410b2966d13a37298da5689b (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

new sentence, new line

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

Maybe /usr should be .Pa /usr ?  few times below too.

> +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

new sentence, new line :)

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

(I get that this is mailwrapper(8) with s/sendmail/git basically but
it feels a bit pedantic.  For a good reason maybe.)

> +.Sh FILES
> +Configuration for
> +.Xr gotd 8
> +is kept in
> +.Pa /etc/gotd.conf.

missing space between gotd.conf and the dot.

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

ENVIRONMENT should ideally be before FILES

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

Is this a copy-paste or intentional?

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

new sentence, new line

> +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 + 3135f87bdfa68c4442e5c84237960f6e108d785b (mode 644)
> --- /dev/null
> +++ gitwrapper/gitwrapper.c
> @@ -0,0 +1,154 @@
> +/*
> + * 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 <util.h>
> +#include <unistd.h>
> +
> +#include "got_error.h"
> +#include "got_path.h"
> +#include "got_serve.h"
> +
> +#include "gotd.h"
> +
> +#ifndef GITWRAPPER_GIT_LIBEXEC_DIR
> +#define GITWRAPPER_GIT_LIBEXEC_DIR "/usr/local/libexec/git"
> +#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);
> +}
> +
> +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 */
> +	const char *repo_path = NULL; /* as passed on the command line */
> +	char *gitcommand = NULL;
> +	struct gotd gotd;
> +	struct gotd_repo *repo = NULL;
> +	pid_t pid;
> +	int st = -1;
> +
> +#ifndef PROFILE
> +	if (pledge("stdio rpath proc exec", NULL) == -1)
> +		err(1, "pledge");
> +#endif
> +
> +	confpath = getenv("GOTD_CONF_PATH");
> +	if (confpath == NULL)
> +		confpath = GOTD_CONF_PATH;
> +
> +	if (strcmp(argv[0], GOT_SERVE_CMD_SEND) == 0 ||
> +	    strcmp(argv[0], GOT_SERVE_CMD_FETCH) == 0) {
> +		if (argc != 2)
> +			usage();
> +		command = strdup(argv[0]);
> +		if (command == NULL) {
> +			error = got_error_from_errno("strdup");
> +			goto done;
> +		}
> +		repo_path = argv[1];
> +	} 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;
> +	}
> +
> +	if (parse_config(confpath, PROC_GOTD, &gotd) == 0) {
> +		TAILQ_FOREACH(repo, &gotd.repos, entry) {
> +			if (got_path_cmp(repo->name, repo_name,
> +			    strlen(repo->name), strlen(repo_name)) == 0)
> +				break;
> +		}
> +	}
> +
> +	if (asprintf(&gitcommand, "%s/%s",
> +	    GITWRAPPER_GIT_LIBEXEC_DIR, command) == -1) {
> +		error = got_error_from_errno("asprintf");
> +		goto done;
> +	}
> +
> +	/*
> +	 * Invoke gotsh(1) if the repository was found in gotd.conf.
> +	 * Otherwise invoke native git(1) tooling which we expect to
> +	 * be found in GITWRAPPER_GIT_LIBEXEC_DIR.
> +	 */
> +	switch (pid = fork()) {
> +	case -1:
> +		goto done;
> +	case 0:
> +		if (repo) {
> +			if (execlp("gotsh", command, repo_name,
> +			    (char *)NULL) ==  -1) {

(IMHO checking exec*() return value is a bit useless, it either
succeeded (and the current program replaced) or failed, in which case
we're just interested in errno.)

> +				error = got_error_from_errno("execlp");

I'd use got_error_from_errno2 and log the executable name too, could
be useful to troubleshoot failures.

> +				goto done;
> +			 }
> +		} else {
> +			if (execl(gitcommand, gitcommand, repo_path,
> +			    (char *)NULL) == -1) {
> +				error = got_error_from_errno("execlp");
> +				goto done;
> +			 }
> +		}
> +		_exit(127);

Just wondering, was 127 picked for some specific reason?

> +	}

By the way, could we just exec instead of fork+exec?  It would leave
one less process running and also ensure that we propagate gotsh or
git-*-pack return value to who called us;

> +	while (waitpid(pid, &st, 0) == -1) {
> +		if (errno != EINTR)
> +			break;
> +	}
> +done:

this could then become an "err" target then that always returns 1,
optionally printing error->msg.

(and also drop the "proc" pledge promise)

> +	free(command);
> +	free(repo_name);
> +	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 - 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);

wooops :)

>  	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;
>  

OK for me though.