Merge branch 'ssrf-fixes' into '1-59-stable'

Backports SSRF fixes for 1-59-stable

See merge request gitlab-org/gitaly!1446

8 files changed, 135 insertions, 5 deletions

diff --git a/changelogs/unreleased/ssrf-fixes.yml b/changelogs/unreleased/ssrf-fixes.yml
new file mode 100644
index 000000000..13b1e9c5f
--- /dev/null
+++ b/changelogs/unreleased/ssrf-fixes.yml
@@ -0,0 +1,5 @@
+---
+title: Do not follow redirect when cloning repo from URL
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/ssrf-fixes2.yml b/changelogs/unreleased/ssrf-fixes2.yml
new file mode 100644
index 000000000..6b595f5d5
--- /dev/null
+++ b/changelogs/unreleased/ssrf-fixes2.yml
@@ -0,0 +1,5 @@
+---
+title: Add http.followRedirects directive to `git fetch` command
+merge_request:
+author:
+type: security
diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go
index 9a67a8e67..7bfa59a2f 100644
--- a/internal/service/repository/create_from_url.go
+++ b/internal/service/repository/create_from_url.go
@@ -29,6 +29,8 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 	}
 
 	args := []string{
+		"-c",
+		"http.followRedirects=false",
 		"clone",
 		"--bare",
 		"--",
diff --git a/internal/service/repository/create_from_url_test.go b/internal/service/repository/create_from_url_test.go
index 10d3431c5..b760926e6 100644
--- a/internal/service/repository/create_from_url_test.go
+++ b/internal/service/repository/create_from_url_test.go
@@ -104,3 +104,34 @@ func TestFailedCreateRepositoryFromURLRequestDueToExistingTarget(t *testing.T) {
 		})
 	}
 }
+
+func TestPreventingRedirect(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	importedRepo := &gitalypb.Repository{
+		RelativePath: "imports/test-repo-imported.git",
+		StorageName:  testhelper.DefaultStorageName,
+	}
+
+	httpServerState, redirectingServer := StartRedirectingTestServer()
+	defer redirectingServer.Close()
+
+	req := &gitalypb.CreateRepositoryFromURLRequest{
+		Repository: importedRepo,
+		Url:        redirectingServer.URL,
+	}
+
+	_, err := client.CreateRepositoryFromURL(ctx, req)
+
+	require.True(t, httpServerState.serverVisited, "git command should make the initial HTTP request")
+	require.False(t, httpServerState.serverVisitedAfterRedirect, "git command should not follow HTTP redirection")
+
+	require.Error(t, err)
+}
diff --git a/internal/service/repository/fetch_remote_test.go b/internal/service/repository/fetch_remote_test.go
index 7001a6bba..ecdb066a8 100644
--- a/internal/service/repository/fetch_remote_test.go
+++ b/internal/service/repository/fetch_remote_test.go
@@ -188,6 +188,37 @@ func TestFetchRemoteOverHTTP(t *testing.T) {
 	}
 }
 
+func TestFetchRemoteOverHTTPWithRedirect(t *testing.T) {
+	server, serverSocketPath := runRepoServer(t)
+	defer server.Stop()
+
+	client, conn := newRepositoryClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	s := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			require.Equal(t, "/info/refs?service=git-upload-pack", r.URL.String())
+			http.Redirect(w, r, "/redirect_url", http.StatusSeeOther)
+		}),
+	)
+
+	req := &gitalypb.FetchRemoteRequest{
+		Repository:   testRepo,
+		RemoteParams: &gitalypb.Remote{Url: s.URL, Name: "geo"},
+		Timeout:      1000,
+	}
+
+	_, err := client.FetchRemote(ctx, req)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "The requested URL returned error: 303")
+}
+
 func TestFetchRemoteOverHTTPError(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
diff --git a/internal/service/repository/redirecting_test_server_test.go b/internal/service/repository/redirecting_test_server_test.go
new file mode 100644
index 000000000..8369b835d
--- /dev/null
+++ b/internal/service/repository/redirecting_test_server_test.go
@@ -0,0 +1,55 @@
+package repository
+
+import (
+	"os/exec"
+	"testing"
+
+	"net/http"
+	"net/http/httptest"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+)
+
+const redirectURL = "/redirect_url"
+
+// RedirectingTestServerState holds information about whether the server was visited and redirect was happened
+type RedirectingTestServerState struct {
+	serverVisited              bool
+	serverVisitedAfterRedirect bool
+}
+
+// StartRedirectingTestServer starts the test server with initial state
+func StartRedirectingTestServer() (*RedirectingTestServerState, *httptest.Server) {
+	state := &RedirectingTestServerState{}
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == redirectURL {
+				state.serverVisitedAfterRedirect = true
+			} else {
+				state.serverVisited = true
+				http.Redirect(w, r, redirectURL, http.StatusMovedPermanently)
+			}
+		}),
+	)
+
+	return state, server
+}
+
+func TestRedirectingServerRedirects(t *testing.T) {
+	dir, cleanup := testhelper.TempDir(t, "", t.Name())
+	defer cleanup()
+
+	httpServerState, redirectingServer := StartRedirectingTestServer()
+
+	// we only test for redirection, this command can fail after that
+	cmd := exec.Command("git", "-c", "http.followRedirects=true", "clone", "--bare", redirectingServer.URL, dir)
+	cmd.Env = append(command.GitEnv, cmd.Env...)
+	cmd.Run()
+
+	redirectingServer.Close()
+
+	require.True(t, httpServerState.serverVisited, "git command should make the initial HTTP request")
+	require.True(t, httpServerState.serverVisitedAfterRedirect, "git command should follow the HTTP redirect")
+}
diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb
index b02cdc5ec..bc3859fb4 100644
--- a/ruby/lib/gitlab/git/gitlab_projects.rb
+++ b/ruby/lib/gitlab/git/gitlab_projects.rb
@@ -120,7 +120,7 @@ module Gitlab
       private
 
       def fetch_remote_command(name, tags, prune, force)
-        %W(#{Gitlab.config.git.bin_path} fetch #{name} --quiet).tap do |cmd|
+        %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{name} --quiet).tap do |cmd|
           cmd << '--prune' if prune
           cmd << '--force' if force
           cmd << (tags ? '--tags' : '--no-tags')
diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
index 391a4b76a..85122ede6 100644
--- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
+++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb
@@ -94,8 +94,9 @@ describe Gitlab::Git::GitlabProjects do
     let(:tags) { true }
     let(:env) { { 'GIT_SSH_COMMAND' => 'foo-command bar' } }
     let(:prune) { true }
+    let(:follow_redirects) { false }
     let(:args) { { force: force, tags: tags, env: env, prune: prune } }
-    let(:cmd) { %W(#{Gitlab.config.git.bin_path} fetch #{remote_name} --quiet --prune --tags) }
+    let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --tags) }
 
     subject { gl_projects.fetch_remote(remote_name, 600, args) }
 
@@ -115,7 +116,7 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with --force' do
       let(:force) { true }
-      let(:cmd) { %W(#{Gitlab.config.git.bin_path} fetch #{remote_name} --quiet --prune --force --tags) }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --force --tags) }
 
       it 'executes the command with forced option' do
         stub_spawn(cmd, 600, tmp_repo_path, env, success: true)
@@ -126,7 +127,7 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with --no-tags' do
       let(:tags) { false }
-      let(:cmd) { %W(#{Gitlab.config.git.bin_path} fetch #{remote_name} --quiet --prune --no-tags) }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --prune --no-tags) }
 
       it 'executes the command' do
         stub_spawn(cmd, 600, tmp_repo_path, env, success: true)
@@ -137,7 +138,7 @@ describe Gitlab::Git::GitlabProjects do
 
     context 'with no prune' do
       let(:prune) { false }
-      let(:cmd) { %W(#{Gitlab.config.git.bin_path} fetch #{remote_name} --quiet --tags) }
+      let(:cmd) { %W(#{Gitlab.config.git.bin_path} -c http.followRedirects=false fetch #{remote_name} --quiet --tags) }
 
       it 'executes the command' do
         stub_spawn(cmd, 600, tmp_repo_path, env, success: true)