quarantine: Use repo-specific quarantine directories

Quarantine directories are created in the storage's temporary directory,
where the basename of the directory will have a simple "repo" prefix
padded by a number of random bytes. With this generic prefix, it is
impossible to tell whether a given quarantine directory does belong to a
specific repository or not, so right now we'd have to assume that every
quarantine directory may belong to any repository.

This poses a problem though: `GetObjectDirectoryPath()` verifies if a
given object directory path does indeed belong to the repository it's
given. This check is quite simple at the moment: we verify whether the
object directory is located beneath the repository path. In the context
of manual object quarantine directories, which are created in the
temporary directory of a storage, this obviously falls apart given that
it's now not contained in there anymore. To fix this, we thus must also
allow object directories which are located in this temporary directory.
But because we cannot determine whether such a directory belongs to the
repository at hand, this would weaken our current protections.

To fix this, we now start to use a repository-specific prefix in the
temporary directory. This prefix is simply the first 8 bytes of the hash
of the repository's relative path, prefixed with "quarantine-". Like
this, we can tell (within a certain but small window of error) whether a
given quarantine directory belongs to a specific repository or not by
comparing the prefix with the prefix as computed from that repo's
relative path.

Note that this commit only changes the location of the quarantine
directory, but does not yet amend `GetObjectDirectoryPath()`.

3 files changed, 47 insertions, 1 deletions

diff --git a/internal/git/quarantine/quarantine.go b/internal/git/quarantine/quarantine.go
index c85862951..517c122e7 100644
--- a/internal/git/quarantine/quarantine.go
+++ b/internal/git/quarantine/quarantine.go
@@ -37,7 +37,8 @@ func New(ctx context.Context, repo *gitalypb.Repository, locator storage.Locator
 		return nil, fmt.Errorf("creating quarantine: %w", err)
 	}
 
-	quarantineDir, err := tempdir.New(ctx, repo.GetStorageName(), locator)
+	quarantineDir, err := tempdir.NewWithPrefix(ctx, repo.GetStorageName(),
+		storage.QuarantineDirectoryPrefix(repo), locator)
 	if err != nil {
 		return nil, fmt.Errorf("creating quarantine: %w", err)
 	}
diff --git a/internal/gitaly/storage/locator.go b/internal/gitaly/storage/locator.go
index 1b88b6e0e..cf83ddacc 100644
--- a/internal/gitaly/storage/locator.go
+++ b/internal/gitaly/storage/locator.go
@@ -1,7 +1,9 @@
 package storage
 
 import (
+	"crypto/sha1"
 	"errors"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -78,3 +80,16 @@ func IsGitDirectory(dir string) bool {
 
 	return true
 }
+
+// QuarantineDirectoryPrefix returns a prefix for use in the temporary directory. The prefix is
+// based on the relative repository path and will stay stable for any given repository. This allows
+// us to verify that a given quarantine object directory indeed belongs to the repository at hand.
+// Ideally, this function would directly be located in the quarantine module, but this is not
+// possible due to cyclic dependencies.
+func QuarantineDirectoryPrefix(repo repository.GitRepo) string {
+	hash := [20]byte{}
+	if repo != nil {
+		hash = sha1.Sum([]byte(repo.GetRelativePath()))
+	}
+	return fmt.Sprintf("quarantine-%x-", hash[:8])
+}
diff --git a/internal/gitaly/storage/locator_test.go b/internal/gitaly/storage/locator_test.go
index 2798077ce..80e53d131 100644
--- a/internal/gitaly/storage/locator_test.go
+++ b/internal/gitaly/storage/locator_test.go
@@ -4,6 +4,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
 func TestValidateRelativePath(t *testing.T) {
@@ -35,3 +37,31 @@ func TestValidateRelativePath(t *testing.T) {
 		})
 	}
 }
+
+func TestQuarantineDirectoryPrefix(t *testing.T) {
+	// An nil repository works alright, even if nonsensical.
+	require.Equal(t, "quarantine-0000000000000000-", QuarantineDirectoryPrefix(nil))
+
+	// A repository with only a relative path.
+	require.Equal(t, "quarantine-8843d7f92416211d-", QuarantineDirectoryPrefix(&gitalypb.Repository{
+		RelativePath: "foobar",
+	}))
+
+	// A different relative path has a different hash.
+	require.Equal(t, "quarantine-60518c1c11dc0452-", QuarantineDirectoryPrefix(&gitalypb.Repository{
+		RelativePath: "barfoo",
+	}))
+
+	// Only the relative path matters. The storage name doesn't matter either given that the
+	// temporary directory is per storage.
+	require.Equal(t, "quarantine-60518c1c11dc0452-", QuarantineDirectoryPrefix(&gitalypb.Repository{
+		StorageName:        "storage-name",
+		RelativePath:       "barfoo",
+		GitObjectDirectory: "object-directory",
+		GitAlternateObjectDirectories: []string{
+			"alternate",
+		},
+		GlRepository:  "gl-repo",
+		GlProjectPath: "gl/repo",
+	}))
+}