Removal of all http.*.extraHeader config values

Because of incorrect usage of config options passed to clone
command they were stored in repository as a configuration values.
The bad thing that is stores user credentials that could be used
by anyone who has access to configuration of the repository.

With changes made the affected configuration values would be removed
from the repository config when 'OptimizeRepository' rpc will be
called. Eventually it will happen to all repositories and after this
we would be able to remove this code as not needed.

Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/3138 https://gitlab.com/gitlab-org/gitaly/-/issues/2882

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 9d0d58056..0d8520133 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"
@@ -130,6 +135,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
 }
 
@@ -157,3 +168,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)