Correctly restrict Git invocations to a working directory
Context
We started observing CI failures in the nightly next
job recently. These were unable to be reproduced locally despite setting the appropriate GITALY_TESTING_GIT_BINARY
(to point to a local build of Git's next
branch) and GIT_VERSION
environment variables.
It was discovered that the failures were a result of recent upstream improvements to the git-patch-id
command (see git#315 (closed)) which causes a segfault if the command is not run within a Git repository. This manifested in CI because the Gitaly source directory is cloned and the build is made as root
(https://gitlab.com/gitlab-org/gitaly/-/blob/677e0ab263cf525c6e9557d33908cf55f0a13bd2/.gitlab-ci.yml#L44-56) while the tests/git
run as an unprivileged user. This effectively caused the safe.directory protections to ignore the Gitaly repository, and thus git-patch-id
executed outside the context of a repository and segfaulted.
Locally, the tests run within the context of your local Gitaly checkout and so git-patch-id
succeeds as usual.
The issue can be reproduced locally with the following command (which is essentially what the GetPatchID
RPC runs):
cat <<EOF | <path/to/git/next/binary> patch-id --stable
diff --git a/bar b/bar
index bdaf90f..31051f6 100644
--- a/bar
+++ b/bar
@@ -2 +2,2 @@
b
+c
EOF
where path/to/git/next/binary was a local build of 78c648537befcff421d4ad73d5ad976eb9255800
at the time of writing.
If this command is run within a Git repository it succeeds, otherwise:
[1] 91419 done bat <<<'' |
91420 segmentation fault ~/code/git/bin-wrappers/git patch-id --stable
More context is available in this Slack thread has more context.
Proposal
We have two ways of creating Git commands in Gitaly; New
and NewWithoutRepo
. The former essentially invokes Git with the --git-dir
option pointed to a specified relative path. @pks-gitlab proposes two changes:
- Spawn commands without repo in a tempdir. Restrict those via GIT_CEILING_DIRECTORIES to the tempdir.
- Spawn commands with a repo in their repo and restrict them via the envvar, as well.
We always know the exact location where Git should be executing, and escaping outside of those dirs to try and discover a Git directory in parent directories is plain wrong. So this is defense in depth, essentially, and should help both during testing, but also secure production systems.
GIT_CEILING_DIRECTORIES specifies a list of directories that Git will not traverse upwards to in order to find a repository.
For NewWithoutRepo, we should ideally use a fixed temp dir to avoid creating directories for every Git invocation.