Disable git protocol v2 temporarily

12 files changed, 120 insertions, 25 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/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index 7a5b8cd13..a1e08cbef 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -9,7 +9,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()
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index 3b906ec7f..c08b95584 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -77,10 +77,8 @@ func TestSuccessfulReceivePackRequestWithGitOpts(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 3c44fcd45..e8dcc8808 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -14,7 +14,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/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index 350f84bc2..46f5fe679 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -94,10 +94,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/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index a52cc99ae..9252a1fa1 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -12,7 +12,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"
 	"golang.org/x/net/context"
@@ -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
+	}
+}