Merge branch 'pks-git-disable-http-redirects' into 'master'

git: Globally disable HTTP redirects

See merge request gitlab-org/gitaly!4131

3 files changed, 21 insertions, 7 deletions

diff --git a/internal/git/command_description.go b/internal/git/command_description.go
index 8154eed98..d4d5ec82d 100644
--- a/internal/git/command_description.go
+++ b/internal/git/command_description.go
@@ -57,6 +57,8 @@ var commandDescriptions = map[string]commandDescription{
 		opts: []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"},
 		},
 	},
 	"commit": {
@@ -92,6 +94,12 @@ var commandDescriptions = map[string]commandDescription{
 			// so. So let's disable writing commit graphs on fetches -- if it really is
 			// required, we can enable it on a case-by-case basis.
 			ConfigPair{Key: "fetch.writeCommitGraph", Value: "false"},
+
+			// By default, Git follows HTTP redirects. Because it's easy for a malicious
+			// user to set up a DNS redirect that points to a server that's internal for
+			// us and unreachable from the outside, this is dangerous. We thus have to
+			// disable redirects in all cases.
+			ConfigPair{Key: "http.followRedirects", Value: "false"},
 		}, fsckConfiguration("fetch")...),
 	},
 	"for-each-ref": {
@@ -142,6 +150,10 @@ var commandDescriptions = map[string]commandDescription{
 	},
 	"ls-remote": {
 		flags: scNoRefUpdates,
+		opts: []GlobalOption{
+			// See "fetch" for why we disable following redirects.
+			ConfigPair{Key: "http.followRedirects", Value: "false"},
+		},
 	},
 	"ls-tree": {
 		flags: scNoRefUpdates,
@@ -166,6 +178,10 @@ var commandDescriptions = map[string]commandDescription{
 	},
 	"push": {
 		flags: scNoRefUpdates,
+		opts: []GlobalOption{
+			// See "fetch" for why we disable following redirects.
+			ConfigPair{Key: "http.followRedirects", Value: "false"},
+		},
 	},
 	"receive-pack": {
 		flags: 0,
@@ -186,6 +202,10 @@ var commandDescriptions = map[string]commandDescription{
 		// While git-remote(1)'s `add` subcommand does support `--end-of-options`,
 		// `remove` doesn't.
 		flags: scNoEndOfOptions,
+		opts: []GlobalOption{
+			// See "fetch" for why we disable following redirects.
+			ConfigPair{Key: "http.followRedirects", Value: "false"},
+		},
 	},
 	"repack": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
diff --git a/internal/gitaly/service/repository/create_repository_from_url.go b/internal/gitaly/service/repository/create_repository_from_url.go
index eacaeb3c9..8166ffafd 100644
--- a/internal/gitaly/service/repository/create_repository_from_url.go
+++ b/internal/gitaly/service/repository/create_repository_from_url.go
@@ -28,9 +28,7 @@ func (s *server) cloneFromURLCommand(
 		return nil, helper.ErrInternal(err)
 	}
 
-	config := []git.ConfigPair{
-		{Key: "http.followRedirects", Value: "false"},
-	}
+	var config []git.ConfigPair
 
 	cloneFlags := []git.Option{
 		git.Flag{Name: "--bare"},
diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index 0a18c9140..81845a363 100644
--- a/internal/gitaly/service/repository/fetch_remote.go
+++ b/internal/gitaly/service/repository/fetch_remote.go
@@ -75,10 +75,6 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 		defer cancel()
 	}
 
-	opts.CommandOptions = append(opts.CommandOptions,
-		git.WithConfig(git.ConfigPair{Key: "http.followRedirects", Value: "false"}),
-	)
-
 	if err := repo.FetchRemote(ctx, remoteName, opts); err != nil {
 		if _, ok := status.FromError(err); ok {
 			// this check is used because of internal call to alternates.PathAndEnv