Merge branch '2780-disable-git-v2' into '1-14-stable'

[Gitaly v1.14] Disable git protocol v2 temporarily

See merge request gitlab/gitaly!11

16 files changed, 126 insertions, 30 deletions

diff --git a/changelogs/unreleased/2780-disable-git-v2.yml b/changelogs/unreleased/2780-disable-git-v2.yml
new file mode 100644
index 000000000..1f236be8b
--- /dev/null
+++ b/changelogs/unreleased/2780-disable-git-v2.yml
@@ -0,0 +1,5 @@
+---
+title: Disable git protocol v2 temporarily
+merge_request:
+author:
+type: security
diff --git a/internal/config/config.go b/internal/config/config.go
index 2145c2857..14bebbf3f 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -49,6 +49,17 @@ type GitlabShell struct {
 // Git contains the settings for the Git executable
 type Git struct {
 	BinPath string `toml:"bin_path"`
+
+	// ProtocolV2Enabled can be set to true to enable the newer Git protocol
+	// version. This should not be enabled until GitLab *either* stops
+	// using transfer.hideRefs for security purposes, *or* Git protocol v2
+	// respects this setting:
+	//
+	// https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/T/
+	//
+	// This is not user-configurable. Once a new Git version has been released,
+	// we can add code to enable it if the detected git binary is new enough
+	ProtocolV2Enabled bool
 }
 
 // Storage contains a single storage-shard
diff --git a/internal/git/protocol.go b/internal/git/protocol.go
index 88262ec56..922238d50 100644
--- a/internal/git/protocol.go
+++ b/internal/git/protocol.go
@@ -7,6 +7,8 @@ import (
 
 	grpc_ctxtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	"github.com/prometheus/client_golang/prometheus"
+
+	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
 const (
@@ -39,9 +41,13 @@ func AddGitProtocolEnv(ctx context.Context, req RequestWithGitProtocol, env []st
 	service, method := methodFromContext(ctx)
 
 	if req.GetGitProtocol() == ProtocolV2 {
-		env = append(env, fmt.Sprintf("GIT_PROTOCOL=%s", ProtocolV2))
+		if config.Config.Git.ProtocolV2Enabled {
+			env = append(env, fmt.Sprintf("GIT_PROTOCOL=%s", ProtocolV2))
 
-		gitProtocolRequests.WithLabelValues(service, method, "v2").Inc()
+			gitProtocolRequests.WithLabelValues(service, method, "v2").Inc()
+		} else {
+			gitProtocolRequests.WithLabelValues(service, method, "v2-rejected").Inc()
+		}
 	} else {
 		gitProtocolRequests.WithLabelValues(service, method, "v0").Inc()
 	}
diff --git a/internal/git/protocol_test.go b/internal/git/protocol_test.go
new file mode 100644
index 000000000..d3d430b95
--- /dev/null
+++ b/internal/git/protocol_test.go
@@ -0,0 +1,58 @@
+package git
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitaly/internal/config"
+)
+
+type fakeProtocolMessage struct {
+	protocol string
+}
+
+func (f fakeProtocolMessage) GetGitProtocol() string {
+	return f.protocol
+}
+
+func setGitProtocolV2Support(value bool) func() {
+	orig := config.Config.Git.ProtocolV2Enabled
+	config.Config.Git.ProtocolV2Enabled = value
+
+	return func() {
+		config.Config.Git.ProtocolV2Enabled = orig
+	}
+}
+
+func TestAddGitProtocolEnvRespectsConfigEnabled(t *testing.T) {
+	restore := setGitProtocolV2Support(true)
+	defer restore()
+
+	env := []string{"fake=value"}
+	msg := fakeProtocolMessage{protocol: "version=2"}
+	value := AddGitProtocolEnv(context.Background(), msg, env)
+
+	require.Equal(t, value, append(env, "GIT_PROTOCOL=version=2"))
+}
+
+func TestAddGitProtocolEnvWhenV2NotRequested(t *testing.T) {
+	restore := setGitProtocolV2Support(true)
+	defer restore()
+
+	env := []string{"fake=value"}
+	msg := fakeProtocolMessage{protocol: ""}
+	value := AddGitProtocolEnv(context.Background(), msg, env)
+	require.Equal(t, value, env)
+}
+
+func TestAddGitProtocolEnvRespectsConfigDisabled(t *testing.T) {
+	restore := setGitProtocolV2Support(false)
+	defer restore()
+
+	env := []string{"fake=value"}
+	msg := fakeProtocolMessage{protocol: "GIT_PROTOCOL=version=2"}
+	value := AddGitProtocolEnv(context.Background(), msg, env)
+	require.Equal(t, value, env)
+}
diff --git a/internal/service/smarthttp/.gitignore b/internal/service/smarthttp/.gitignore
new file mode 100644
index 000000000..3d3722967
--- /dev/null
+++ b/internal/service/smarthttp/.gitignore
@@ -0,0 +1 @@
+/testdata/scratch
diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index 939b56826..6683a6336 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -10,7 +10,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -54,10 +53,8 @@ func TestSuccessfulInfoRefsUploadPackWithGitConfigOptions(t *testing.T) {
 }
 
 func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
-	defer func(old string) {
-		config.Config.Git.BinPath = old
-	}(config.Config.Git.BinPath)
-	config.Config.Git.BinPath = "../../testhelper/env_git"
+	restore := testhelper.EnableGitProtocolV2Support()
+	defer restore()
 
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
@@ -148,7 +145,7 @@ func TestFailureRepoNotFoundInfoRefsReceivePack(t *testing.T) {
 
 	client, conn := newSmartHTTPClient(t, serverSocketPath)
 	defer conn.Close()
-	repo := &gitalypb.Repository{StorageName: "default", RelativePath: "testdata/data/another_repo"}
+	repo := &gitalypb.Repository{StorageName: "default", RelativePath: "testdata/scratch/another_repo"}
 	rpcRequest := &gitalypb.InfoRefsRequest{Repository: repo}
 
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index 0503b669b..ef083b67d 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -68,10 +68,8 @@ func TestSuccessfulReceivePackRequest(t *testing.T) {
 }
 
 func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
-	defer func(old string) {
-		config.Config.Git.BinPath = old
-	}(config.Config.Git.BinPath)
-	config.Config.Git.BinPath = "../../testhelper/env_git"
+	restore := testhelper.EnableGitProtocolV2Support()
+	defer restore()
 
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
diff --git a/internal/service/smarthttp/testdata/env_git b/internal/service/smarthttp/testdata/env_git
new file mode 120000
index 000000000..a01a70012
--- /dev/null
+++ b/internal/service/smarthttp/testdata/env_git
@@ -0,0 +1 @@
+../../../testhelper/env_git
\ No newline at end of file
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index b8bbb1a57..a4a496c83 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -15,7 +15,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -149,10 +148,8 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitProtocol(t *testing.T) {
-	defer func(old string) {
-		config.Config.Git.BinPath = old
-	}(config.Config.Git.BinPath)
-	config.Config.Git.BinPath = "../../testhelper/env_git"
+	restore := testhelper.EnableGitProtocolV2Support()
+	defer restore()
 
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
diff --git a/internal/service/ssh/.gitignore b/internal/service/ssh/.gitignore
index ace1063ab..3d3722967 100644
--- a/internal/service/ssh/.gitignore
+++ b/internal/service/ssh/.gitignore
@@ -1 +1 @@
-/testdata
+/testdata/scratch
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index f4237646d..d69bdb002 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -106,10 +106,8 @@ func TestReceivePackPushSuccess(t *testing.T) {
 }
 
 func TestReceivePackPushSuccessWithGitProtocol(t *testing.T) {
-	defer func(old string) {
-		config.Config.Git.BinPath = old
-	}(config.Config.Git.BinPath)
-	config.Config.Git.BinPath = "../../testhelper/env_git"
+	restore := testhelper.EnableGitProtocolV2Support()
+	defer restore()
 
 	server, serverSocketPath := runSSHServer(t)
 	defer server.Stop()
diff --git a/internal/service/ssh/testdata/env_git b/internal/service/ssh/testdata/env_git
new file mode 120000
index 000000000..a01a70012
--- /dev/null
+++ b/internal/service/ssh/testdata/env_git
@@ -0,0 +1 @@
+../../../testhelper/env_git
\ No newline at end of file
diff --git a/internal/service/ssh/testhelper_test.go b/internal/service/ssh/testhelper_test.go
index b22fd7c33..1945bf428 100644
--- a/internal/service/ssh/testhelper_test.go
+++ b/internal/service/ssh/testhelper_test.go
@@ -14,7 +14,7 @@ import (
 )
 
 const (
-	testPath     = "testdata"
+	testPath     = "testdata/scratch"
 	testRepoRoot = testPath + "/data"
 )
 
diff --git a/internal/service/ssh/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index a36c2d848..e9d2ba026 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -13,7 +13,6 @@ import (
 	"github.com/golang/protobuf/jsonpb"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -98,10 +97,8 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
-	defer func(old string) {
-		config.Config.Git.BinPath = old
-	}(config.Config.Git.BinPath)
-	config.Config.Git.BinPath = "../../testhelper/env_git"
+	restore := testhelper.EnableGitProtocolV2Support()
+	defer restore()
 
 	server, serverSocketPath := runSSHServer(t)
 	defer server.Stop()
diff --git a/internal/testhelper/git_protocol.go b/internal/testhelper/git_protocol.go
new file mode 100644
index 000000000..5619739cb
--- /dev/null
+++ b/internal/testhelper/git_protocol.go
@@ -0,0 +1,26 @@
+package testhelper
+
+import (
+	"gitlab.com/gitlab-org/gitaly/internal/config"
+)
+
+// EnableGitProtocolV2Support ensures that Git protocol v2 support is enabled,
+// and replaces the git binary in config with an `env_git` wrapper that allows
+// the protocol to be tested. It returns a function that restores the given
+// settings.
+//
+// Because we don't know how to get to that wrapper script from the current
+// working directory, callers must create a symbolic link to the `env_git` file
+// in their own `testdata` directories.
+func EnableGitProtocolV2Support() func() {
+	oldGitBinPath := config.Config.Git.BinPath
+	oldGitProtocolV2Enabled := config.Config.Git.ProtocolV2Enabled
+
+	config.Config.Git.BinPath = "testdata/env_git"
+	config.Config.Git.ProtocolV2Enabled = true
+
+	return func() {
+		config.Config.Git.BinPath = oldGitBinPath
+		config.Config.Git.ProtocolV2Enabled = oldGitProtocolV2Enabled
+	}
+}
diff --git a/internal/testhelper/hook_env.go b/internal/testhelper/hook_env.go
index b2f11449a..a8c7c3d9c 100644
--- a/internal/testhelper/hook_env.go
+++ b/internal/testhelper/hook_env.go
@@ -14,10 +14,10 @@ import (
 // environment variables get set for hooks.
 func CaptureHookEnv(t *testing.T) (hookPath string, cleanup func()) {
 	var err error
-	hooks.Override, err = filepath.Abs("testdata/hooks")
+	hooks.Override, err = filepath.Abs("testdata/scratch/hooks")
 	require.NoError(t, err)
 
-	hookOutputFile, err := filepath.Abs("testdata/hook.env")
+	hookOutputFile, err := filepath.Abs("testdata/scratch/hook.env")
 	require.NoError(t, err)
 
 	require.NoError(t, os.RemoveAll(hookOutputFile))