Merge branch 'security-ps-clean-creds' into 'master'

Removal of all http.*.extraHeader config values

See merge request gitlab-org/security/gitaly!16

3 files changed, 134 insertions, 0 deletions

diff --git a/changelogs/unreleased/security-ps-clean-creds.yml b/changelogs/unreleased/security-ps-clean-creds.yml
new file mode 100644
index 000000000..9d5866b3e
--- /dev/null
+++ b/changelogs/unreleased/security-ps-clean-creds.yml
@@ -0,0 +1,5 @@
+---
+title: Removal of all http.*.extraHeader config values
+merge_request:
+author:
+type: security
diff --git a/internal/gitaly/service/repository/optimize.go b/internal/gitaly/service/repository/optimize.go
index 47d9201a7..75775655e 100644
--- a/internal/gitaly/service/repository/optimize.go
+++ b/internal/gitaly/service/repository/optimize.go
@@ -1,11 +1,16 @@
 package repository
 
 import (
+	"bufio"
 	"context"
+	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
@@ -129,6 +134,12 @@ func (s *server) optimizeRepository(ctx context.Context, repository *gitalypb.Re
 		return fmt.Errorf("OptimizeRepository: remove empty refs: %w", err)
 	}
 
+	// TODO: https://gitlab.com/gitlab-org/gitaly/-/issues/3138
+	// This is a temporary code and needs to be removed once it will be run on all repositories at least once.
+	if err := s.unsetAllConfigsByRegexp(ctx, repository, "^http\\..+\\.extraHeader$"); err != nil {
+		return fmt.Errorf("OptimizeRepository: unset all configs by regexp: %w", err)
+	}
+
 	return nil
 }
 
@@ -156,3 +167,92 @@ func (s *server) validateOptimizeRepositoryRequest(in *gitalypb.OptimizeReposito
 
 	return nil
 }
+
+func (s *server) unsetAllConfigsByRegexp(ctx context.Context, repository *gitalypb.Repository, regexp string) error {
+	keys, err := getConfigKeys(ctx, repository, regexp)
+	if err != nil {
+		return fmt.Errorf("get config keys: %w", err)
+	}
+
+	if err := unsetConfigKeys(ctx, repository, keys); err != nil {
+		return fmt.Errorf("unset all keys: %w", err)
+	}
+
+	return nil
+}
+
+func getConfigKeys(ctx context.Context, repository *gitalypb.Repository, regexp string) ([]string, error) {
+	cmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{
+		Name: "config",
+		Flags: []git.Option{
+			git.Flag{Name: "--name-only"},
+			git.ValueFlag{Name: "--get-regexp", Value: regexp},
+		},
+	})
+	if err != nil {
+		return nil, fmt.Errorf("creation of 'git config': %w", err)
+	}
+
+	keys, err := parseConfigKeys(cmd)
+	if err != nil {
+		return nil, fmt.Errorf("parse config keys: %w", err)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		var termErr *exec.ExitError
+		if errors.As(err, &termErr) {
+			if termErr.ExitCode() == 1 {
+				// https://git-scm.com/docs/git-config#_description: The section or key is invalid (ret=1)
+				// This means no matching values were found.
+				return nil, nil
+			}
+		}
+		return nil, fmt.Errorf("wait for 'git config': %w", err)
+	}
+
+	return keys, nil
+}
+
+func parseConfigKeys(reader io.Reader) ([]string, error) {
+	var keys []string
+
+	scanner := bufio.NewScanner(reader)
+	for scanner.Scan() {
+		keys = append(keys, scanner.Text())
+	}
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	return keys, nil
+}
+
+func unsetConfigKeys(ctx context.Context, repository *gitalypb.Repository, names []string) error {
+	for _, name := range names {
+		if err := unsetAll(ctx, repository, name); err != nil {
+			return fmt.Errorf("unset all: %w", err)
+		}
+	}
+
+	return nil
+}
+
+func unsetAll(ctx context.Context, repository *gitalypb.Repository, name string) error {
+	if strings.TrimSpace(name) == "" {
+		return nil
+	}
+
+	cmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{
+		Name:  "config",
+		Flags: []git.Option{git.ValueFlag{Name: "--unset-all", Value: name}},
+	})
+	if err != nil {
+		return fmt.Errorf("creation of 'git config': %w", err)
+	}
+
+	if err := cmd.Wait(); err != nil {
+		return fmt.Errorf("wait for 'git config': %w", err)
+	}
+
+	return nil
+}
diff --git a/internal/gitaly/service/repository/optimize_test.go b/internal/gitaly/service/repository/optimize_test.go
index 678d57971..7c975c19d 100644
--- a/internal/gitaly/service/repository/optimize_test.go
+++ b/internal/gitaly/service/repository/optimize_test.go
@@ -1,6 +1,8 @@
 package repository
 
 import (
+	"bytes"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"testing"
@@ -53,6 +55,23 @@ func TestOptimizeRepository(t *testing.T) {
 
 	testhelper.CreateCommit(t, testRepoPath, "master", nil)
 
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "http.http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c1.git.extraHeader", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "http.http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c2.git.extraHeader", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "randomStart-http.http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c3.git.extraHeader", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "http.http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c4.git.extraHeader-randomEnd", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "hTTp.http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git.ExtrAheaDeR", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "http.http://extraHeader/extraheader/EXTRAHEADER.git.extraHeader", "Authorization: Basic secret-password")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "https.https://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git.extraHeader", "Authorization: Basic secret-password")
+	confFileData, err := ioutil.ReadFile(filepath.Join(testRepoPath, "config"))
+	require.NoError(t, err)
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c1.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c2.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c3")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c4.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://extraHeader/extraheader/EXTRAHEADER.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("https://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git")))
+
 	locator := config.NewLocator(config.Config)
 	serverSocketPath, stop := runRepoServer(t, locator)
 	defer stop()
@@ -63,6 +82,16 @@ func TestOptimizeRepository(t *testing.T) {
 	_, err = repoClient.OptimizeRepository(ctx, &gitalypb.OptimizeRepositoryRequest{Repository: testRepo})
 	require.NoError(t, err)
 
+	confFileData, err = ioutil.ReadFile(filepath.Join(testRepoPath, "config"))
+	require.NoError(t, err)
+	require.False(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c1.git")))
+	require.False(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c2.git")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c3")))
+	require.True(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c4.git")))
+	require.False(t, bytes.Contains(confFileData, []byte("http://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git")))
+	require.False(t, bytes.Contains(confFileData, []byte("http://extraHeader/extraheader/EXTRAHEADER.git.extraHeader")))
+	require.True(t, bytes.Contains(confFileData, []byte("https://localhost:51744/60631c8695bf041a808759a05de53e36a73316aacb502824fabbb0c6055637c5.git")))
+
 	require.Equal(t, getNewestPackfileModtime(t, testRepoPath), newestsPackfileTime, "there should not have been a new packfile created")
 
 	testRepo, testRepoPath, cleanupBare := testhelper.InitBareRepo(t)