Merge remote-tracking branch 'dev/15-0-stable' into 15-0-stable15-0-stable

9 files changed, 176 insertions, 16 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e4fd320de..da5fc937b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Gitaly changelog
 
+## 15.0.5 (2022-07-28)
+
+### Security (1 change)
+
+- [Import via git protocol allows to bypass checks on repository](gitlab-org/security/gitaly@a110173b56e3bc8c7cb5db2e1e84f6cfb226c39e) ([merge request](gitlab-org/security/gitaly!55))
+
 ## 15.0.4 (2022-06-30)
 
 No changes.
diff --git a/VERSION b/VERSION
index dbe8a10a6..8fc2ffdd1 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-15.0.4
\ No newline at end of file
+15.0.5
\ No newline at end of file
diff --git a/internal/git/command_description.go b/internal/git/command_description.go
index 6ca1ce76c..0154a055b 100644
--- a/internal/git/command_description.go
+++ b/internal/git/command_description.go
@@ -54,12 +54,12 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoEndOfOptions,
 	},
 	"clone": {
-		opts: append([]GlobalOption{
+		opts: append(append([]GlobalOption{
 			// See "init" for why we set the template directory to the empty string.
 			ConfigPair{Key: "init.templateDir", Value: ""},
 			// See "fetch" for why we disable following redirects.
 			ConfigPair{Key: "http.followRedirects", Value: "false"},
-		}, packConfiguration()...),
+		}, packConfiguration()...), fsckConfiguration("fetch")...),
 	},
 	"commit": {
 		flags: 0,
diff --git a/internal/git/objectpool/clone.go b/internal/git/objectpool/clone.go
index 780afb546..a3d4661ad 100644
--- a/internal/git/objectpool/clone.go
+++ b/internal/git/objectpool/clone.go
@@ -1,7 +1,9 @@
 package objectpool
 
 import (
+	"bytes"
 	"context"
+	"fmt"
 	"os"
 	"path/filepath"
 
@@ -17,6 +19,7 @@ func (o *ObjectPool) clone(ctx context.Context, repo *localrepo.Repo) error {
 		return err
 	}
 
+	var stderr bytes.Buffer
 	cmd, err := o.gitCmdFactory.NewWithoutRepo(ctx,
 		git.SubCmd{
 			Name: "clone",
@@ -28,12 +31,17 @@ func (o *ObjectPool) clone(ctx context.Context, repo *localrepo.Repo) error {
 			Args: []string{repoPath, o.FullPath()},
 		},
 		git.WithRefTxHook(repo),
+		git.WithStderr(&stderr),
 	)
 	if err != nil {
-		return err
+		return fmt.Errorf("spawning clone: %w", err)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return fmt.Errorf("cloning to pool: %w, stderr: %q", err, stderr.String())
 	}
 
-	return cmd.Wait()
+	return nil
 }
 
 func (o *ObjectPool) removeHooksDir() error {
diff --git a/internal/git/objectpool/clone_test.go b/internal/git/objectpool/clone_test.go
index 7b6b22d7a..f4e4561aa 100644
--- a/internal/git/objectpool/clone_test.go
+++ b/internal/git/objectpool/clone_test.go
@@ -1,40 +1,88 @@
 package objectpool
 
 import (
+	"bytes"
+	"fmt"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 )
 
-func TestClone(t *testing.T) {
+func TestClone_successful(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	require.NoError(t, pool.clone(ctx, repo))
-	defer func() {
-		require.NoError(t, pool.Remove(ctx))
-	}()
 
 	require.DirExists(t, pool.FullPath())
 	require.DirExists(t, filepath.Join(pool.FullPath(), "objects"))
 }
 
-func TestCloneExistingPool(t *testing.T) {
+func TestClone_existingPool(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg, pool, repoProto := setupObjectPool(t, ctx)
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+
+	// The first time around cloning should succeed, but ...
+	require.NoError(t, pool.clone(ctx, repo))
+
+	// ... when we try to clone the same pool a second time we should get an error because the
+	// destination exists already.
+	require.EqualError(t, pool.clone(ctx, repo), fmt.Sprintf(
+		"cloning to pool: exit status 128, stderr: \"fatal: destination path '%s' already exists and is not an empty directory.\\n\"",
+		pool.FullPath(),
+	))
+}
+
+func TestClone_fsck(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cfg, pool, repoProto := setupObjectPool(t, ctx)
+
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	repoPath, err := repo.Path()
+	require.NoError(t, err)
+
+	// Write a tree into the repository that's known-broken.
+	treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+		{Content: "content", Path: "dup", Mode: "100644"},
+		{Content: "content", Path: "dup", Mode: "100644"},
+	})
 
+	gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithParents(),
+		gittest.WithBranch("main"),
+		gittest.WithTree(treeID),
+	)
+
+	// While git-clone(1) would normally complain about the broken tree we have just cloned, we
+	// don't expect the clone to fail. This is because we know that the tree is already in one
+	// of our repositories that we have locally, so raising an error now doesn't make a whole
+	// lot of sense in the first place.
+	//
+	// Note: this works because we use `git clone --local`, which only creates a copy of the
+	// repository without performing consistency checks.
 	require.NoError(t, pool.clone(ctx, repo))
-	defer func() {
-		require.NoError(t, pool.Remove(ctx))
-	}()
 
-	// re-clone on the directory
-	require.Error(t, pool.clone(ctx, repo))
+	// Verify that the broken tree is indeed in the pool repository and that it is reported as
+	// broken by git-fsck(1).
+	var stderr bytes.Buffer
+	fsckCmd := gittest.NewCommand(t, cfg, "-C", pool.FullPath(), "fsck")
+	fsckCmd.Stderr = &stderr
+
+	require.EqualError(t, fsckCmd.Run(), "exit status 1")
+	require.Equal(t, fmt.Sprintf("error in tree %s: duplicateEntries: contains duplicate file entries\n", treeID), stderr.String())
 }
diff --git a/internal/gitaly/service/repository/create_fork.go b/internal/gitaly/service/repository/create_fork.go
index f6e5f66cf..293537899 100644
--- a/internal/gitaly/service/repository/create_fork.go
+++ b/internal/gitaly/service/repository/create_fork.go
@@ -49,6 +49,12 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest
 			},
 		}, git.WithInternalFetch(&gitalypb.SSHUploadPackRequest{
 			Repository: sourceRepository,
+		}), git.WithConfig(git.ConfigPair{
+			// Disable consistency checks for fetched objects when creating a
+			// fork. We don't want to end up in a situation where it's
+			// impossible to create forks we already have anyway because we have
+			// e.g. retroactively tightened the consistency checks.
+			Key: "fetch.fsckObjects", Value: "false",
 		}), git.WithDisabledHooks())
 		if err != nil {
 			return fmt.Errorf("spawning fetch: %w", err)
diff --git a/internal/gitaly/service/repository/create_fork_test.go b/internal/gitaly/service/repository/create_fork_test.go
index 22aa1c0da..d1a5712d4 100644
--- a/internal/gitaly/service/repository/create_fork_test.go
+++ b/internal/gitaly/service/repository/create_fork_test.go
@@ -1,8 +1,10 @@
 package repository
 
 import (
+	"bytes"
 	"crypto/tls"
 	"crypto/x509"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -179,6 +181,62 @@ func TestCreateFork_refs(t *testing.T) {
 	)
 }
 
+func TestCreateFork_fsck(t *testing.T) {
+	t.Parallel()
+
+	cfg := testcfg.Build(t)
+
+	testcfg.BuildGitalyHooks(t, cfg)
+	testcfg.BuildGitalySSH(t, cfg)
+
+	client, socketPath := runRepositoryService(t, cfg, nil)
+	cfg.SocketPath = socketPath
+
+	ctx := testhelper.Context(t)
+	ctx = testhelper.MergeOutgoingMetadata(ctx, testcfg.GitalyServersMetadataFromCfg(t, cfg))
+
+	repo, repoPath := gittest.CreateRepository(ctx, t, cfg)
+
+	// Write a tree into the repository that's known-broken.
+	treeID := gittest.WriteTree(t, cfg, repoPath, []gittest.TreeEntry{
+		{Content: "content", Path: "dup", Mode: "100644"},
+		{Content: "content", Path: "dup", Mode: "100644"},
+	})
+
+	gittest.WriteCommit(t, cfg, repoPath,
+		gittest.WithParents(),
+		gittest.WithBranch("main"),
+		gittest.WithTree(treeID),
+	)
+
+	forkedRepo := &gitalypb.Repository{
+		RelativePath: gittest.NewRepositoryName(t, true),
+		StorageName:  repo.GetStorageName(),
+	}
+
+	// Create a fork from the repository with the broken tree. This should work alright: repos
+	// with preexisting broken objects that we already have on our disk anyway should not be
+	// subject to additional consistency checks. Otherwise we might end up in a situation where
+	// we retroactively tighten consistency checks for repositories such that preexisting repos
+	// wouldn't be forkable anymore.
+	_, err := client.CreateFork(ctx, &gitalypb.CreateForkRequest{
+		Repository:       forkedRepo,
+		SourceRepository: repo,
+	})
+	require.NoError(t, err)
+
+	forkedRepoPath := filepath.Join(cfg.Storages[0].Path, gittest.GetReplicaPath(ctx, t, cfg, forkedRepo))
+
+	// Verify that the broken tree is indeed in the fork and that it is reported as broken by
+	// git-fsck(1).
+	var stderr bytes.Buffer
+	fsckCmd := gittest.NewCommand(t, cfg, "-C", forkedRepoPath, "fsck")
+	fsckCmd.Stderr = &stderr
+
+	require.EqualError(t, fsckCmd.Run(), "exit status 4")
+	require.Equal(t, fmt.Sprintf("error in tree %s: duplicateEntries: contains duplicate file entries\n", treeID), stderr.String())
+}
+
 func TestCreateFork_targetExists(t *testing.T) {
 	t.Parallel()
 	ctx := testhelper.Context(t)
diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go
index 19472412f..b66e783c1 100644
--- a/internal/gitaly/service/repository/create_repository_from_url_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_url_test.go
@@ -184,6 +184,40 @@ func TestCreateRepositoryFromURL_redirect(t *testing.T) {
 	require.Contains(t, err.Error(), "The requested URL returned error: 301")
 }
 
+func TestCreateRepositoryFromURL_fsck(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg, client := setupRepositoryServiceWithoutRepo(t)
+
+	_, sourceRepoPath := gittest.CreateRepository(ctx, t, cfg)
+
+	// We're creating a new commit which has a root tree with duplicate entries. git-mktree(1)
+	// allows us to create these trees just fine, but git-fsck(1) complains.
+	gittest.WriteCommit(t, cfg, sourceRepoPath,
+		gittest.WithParents(),
+		gittest.WithBranch("main"),
+		gittest.WithTreeEntries(
+			gittest.TreeEntry{Content: "content", Path: "dup", Mode: "100644"},
+			gittest.TreeEntry{Content: "content", Path: "dup", Mode: "100644"},
+		),
+	)
+
+	targetRepoProto := &gitalypb.Repository{
+		RelativePath: gittest.NewRepositoryName(t, true),
+		StorageName:  cfg.Storages[0].Name,
+	}
+
+	_, err := client.CreateRepositoryFromURL(ctx, &gitalypb.CreateRepositoryFromURLRequest{
+		Repository: targetRepoProto,
+		Url:        "file://" + sourceRepoPath,
+	})
+	require.Error(t, err)
+	testhelper.RequireGrpcCode(t, err, codes.Internal)
+	require.Contains(t, err.Error(), "duplicateEntries: contains duplicate file entries")
+}
+
 func TestServer_CloneFromURLCommand(t *testing.T) {
 	t.Parallel()
 	ctx := testhelper.Context(t)
diff --git a/ruby/proto/gitaly/version.rb b/ruby/proto/gitaly/version.rb
index ba7e257e0..62abd2054 100644
--- a/ruby/proto/gitaly/version.rb
+++ b/ruby/proto/gitaly/version.rb
@@ -2,5 +2,5 @@
 # (https://gitlab.com/gitlab-org/release-tools/), and should not be
 # modified.
 module Gitaly
-  VERSION = '15.0.4'
+  VERSION = '15.0.5'
 end