Ticket #1823 (closed defect: fixed)

Is it too nasty to require people to fork on GitHub and file PRs? It would save us a lot of time, and create a written public record of contributions. The downside is that it is slightly less easy than just pushing to contrib was...

And to be clear, by "people" I mean only "untrusted outsiders" and not core Fiji developers, same as before.

As you know, I am a big fan of requiring competent maintainers to go that extra step when it makes contributing substantially easier.

IMHO this is such a case, and I am a competent maintainer.

Just for the record: IIRC we got roughly 20 patches through the contrib branches, in addition to all of Gabriel's stuff...

I just fixed this, compiling a custom intermediary between Apache and git-http-backend, running it with the jenkins-node account as effective user. It takes all of its configuration from the environment variable GIT_PRE_RECEIVE_HOOK which has to be executable and owned by root (for security reasons). All it does is perform a couple of sanity checks before ensuring that the pre-receive hook is linked symbolically before handing off to Git.

The new Apache configuration has this additional line:

SetEnv GIT_PRE_RECEIVE_HOOK /var/www/vhosts/fiji.sc/bin/pre-receive-hook

and it now hands all smart HTTP backend stuff to /var/www/vhosts/fiji.sc/bin/contrib-http-backend/$1 instead of /usr/libexec/git-core/git-http-backend/$1.

The pre-receive hook reads like this:

#!/bin/sh

die () {
	echo "FATAL: $*" >&2
	exit 1
}

# This pre-receive hook should only limit contrib pushing via http://
test -n "$REQUEST_URI" ||
exit 0

while read oldsha1 newsha1 ref
do
	case "$ref" in
	refs/heads/contrib*)
		;; # okay
	*)
		die "Only contrib branches can be pushed ($ref)!"
		;;
	esac

	test 0000000000000000000000000000000000000000 != "$oldsha1" || continue

	basesha1="$(git merge-base "$oldsha1" "$newsha1")" &&
	test "$basesha1" = "$oldsha1" ||
	die "Only fast-forwards are allowed: $ref ($(echo $oldsha1 | cut -c 1-8)...$(echo $newsha1 | cut -c 1-8))"
done

The source code of the intermediary is written in C (because bash does not run with an effective UID, for security reasons):

#include <errno.h>
#include <limits.h>
#include <stdarg.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#include <sys/types.h>
#include <sys/stat.h>

/**
 * This program acts as a simple filter to git-http-backend to allow for
 * contrib branches.
 *
 * The basic idea is to have this program running under the effective UID of
 * the owner of the Git repositories served by Git via smart HTTP. That way, we
 * can not only write to the repository, but also ensure that a pre-receive
 * hook is installed (symlinked) to prevent bad things from happening.
 *
 * To enable this, use chown and chmod with u+s to set the effective UID, then
 * replace git-http-backend with contrib-http-backend (i.e. this program) in
 * your Apache configuration and add a couple of environment variables:
 *
 * SetEnv GIT_EXECUTABLE /path/to/git
 * SetEnv GIT_PRE_RECEIVE_HOOK /path/to/pre-receive-hook
 *
 * The script reuses the environment variable GIT_PROJECT_ROOT to ensure
 * that the specified pre-receive hook is active by adding a symbolic link.
 *
 * @author Johannes Schindelin
 */

static void __attribute__((noreturn)) die(const char *message, ...)
{
	va_list parameters;

	va_start(parameters, message);
	vfprintf(stderr, message, parameters);
	va_end(parameters);
	fputc('\n', stderr);

	exit(1);
}

static void ensure_hook(const char *repository_path)
{
	const char *hook_path = "/hooks/pre-receive";
	int hook_path_len = strlen(hook_path);

	const char *git_pre_receive_hook = getenv("GIT_PRE_RECEIVE_HOOK");
	int len;
	char *p;
	struct stat st;

	if (!git_pre_receive_hook) {
		die("GIT_PRE_RECEIVE_HOOK not set");
	}
	if (access(git_pre_receive_hook, X_OK)) {
		die("Not executable: %s", git_pre_receive_hook);
	}
	if (stat(git_pre_receive_hook, &st) || st.st_uid) {
		die("The pre-receive hook must be owned by root");
	}

	len = strlen(repository_path);
	p = malloc(len + hook_path_len + 1);
	memcpy(p, repository_path, len);
	memcpy(p + len, hook_path, hook_path_len);
	p[len + hook_path_len] = '\0';

	if (!access(p, X_OK)) {
		char buffer[PATH_MAX];

		strcpy(buffer, "(not a symlink)");
		if (!readlink(p, buffer, sizeof(buffer)) ||
				strcmp(buffer, git_pre_receive_hook)) {
			die("Invalid hook %s: %s\n", p, buffer);
		}
	} else {
		char *slash = strchr(p + len + 1, '/');

		while (slash) {
			*slash = '\0';
			if (access(p, R_OK)) {
				if (mkdir(p, 02775)) {
					die("Could not make directory %s", p);
				}
			}
			*slash = '/';
			slash = strchr(slash + 1, '/');
		}

		if (symlink(git_pre_receive_hook, p)) {
			die("Could not make symlink %s -> %s: %s",
				git_pre_receive_hook, p, strerror(errno));
		}
	}
	free(p);
}

int main(int argc, char **argv)
{
	const char *suffix = "/git-receive-pack";
	int suffix_len = strlen(suffix);

	const char *path_info = getenv("PATH_INFO");
	const char *git_root = getenv("GIT_PROJECT_ROOT");
	const char *git_executable = "/usr/bin/git";
	int len;

	/* Sanity checks */
	if (!path_info || !git_root) {
		die("Invalid environment: %s, %s", path_info, git_root);
	}

	if (!git_executable || access(git_executable, X_OK)) {
		die("Not executable: %s", git_executable);
	}

	/* Sanity checks specific to git-receive-pack */
	len = strlen(path_info);
	if (len > suffix_len && !strcmp(path_info + len - suffix_len, suffix)) {
		int git_root_len = strlen(git_root);
		char *p;
		struct stat st;

		len = git_root_len + len - suffix_len;

		p = malloc(len + 1);
		memcpy(p, git_root, git_root_len);
		memcpy(p + git_root_len, path_info, len - git_root_len);
		p[len] = '\0';
		if (stat(p, &st)) {
			die("Cannot stat %s", p);
		}
		if (geteuid() != st.st_uid && (getegid() != st.st_gid ||
				!(st.st_mode & S_IWGRP))) {
			die("Invalid ownership: %s (%d:%d)", p, geteuid(), getegid());
		}
		ensure_hook(p);
		free(p);
	}

	/* Hand off to Git's http backend */
	execl(git_executable, git_executable, "-c", "http.receivepack=true", "http-backend", NULL);
	die("Could not execute %s", git_executable);
}

An earlier version of contrib-http-backend allowed to specify the Git executable path via environment variables, too, but I became convinced that this would open a security hole: to be able to write to the repositories owned by the Jenkins user, the program must be owned by that user and run with the SUID flag set. It is only a minor concern, though, that somebody with access to the server would run this executable to modify contrib branches in Jenkins-owned repositories owned outside of the public GitWeb space.