Merge branch 'pks-create-repo-from-url-leaking-credentials' into 'master'

repository: Fix clone credentials leaking via command line arguments

See merge request gitlab-org/gitaly!4668

2 files changed, 75 insertions, 57 deletions

diff --git a/internal/gitaly/service/repository/create_repository_from_url.go b/internal/gitaly/service/repository/create_repository_from_url.go
index 6b3beec01..f7fd9b853 100644
--- a/internal/gitaly/service/repository/create_repository_from_url.go
+++ b/internal/gitaly/service/repository/create_repository_from_url.go
@@ -21,13 +21,6 @@ func (s *server) cloneFromURLCommand(
 	repoURL, repoHost, repositoryFullPath, authorizationToken string, mirror bool,
 	opts ...git.CmdOpt,
 ) (*command.Command, error) {
-	u, err := url.Parse(repoURL)
-	if err != nil {
-		return nil, helper.ErrInternal(err)
-	}
-
-	var config []git.ConfigPair
-
 	cloneFlags := []git.Option{
 		git.Flag{Name: "--quiet"},
 	}
@@ -38,12 +31,18 @@ func (s *server) cloneFromURLCommand(
 		cloneFlags = append(cloneFlags, git.Flag{Name: "--bare"})
 	}
 
+	u, err := url.Parse(repoURL)
+	if err != nil {
+		return nil, helper.ErrInternal(err)
+	}
+
+	var config []git.ConfigPair
 	if u.User != nil {
-		pwd, set := u.User.Password()
+		password, hasPassword := u.User.Password()
 
 		var creds string
-		if set {
-			creds = u.User.Username() + ":" + pwd
+		if hasPassword {
+			creds = u.User.Username() + ":" + password
 		} else {
 			creds = u.User.Username()
 		}
@@ -51,11 +50,9 @@ func (s *server) cloneFromURLCommand(
 		u.User = nil
 		authHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(creds)))
 		config = append(config, git.ConfigPair{Key: "http.extraHeader", Value: authHeader})
-	} else {
-		if len(authorizationToken) > 0 {
-			authHeader := fmt.Sprintf("Authorization: %s", authorizationToken)
-			config = append(config, git.ConfigPair{Key: "http.extraHeader", Value: authHeader})
-		}
+	} else if len(authorizationToken) > 0 {
+		authHeader := fmt.Sprintf("Authorization: %s", authorizationToken)
+		config = append(config, git.ConfigPair{Key: "http.extraHeader", Value: authHeader})
 	}
 
 	if repoHost != "" {
@@ -71,7 +68,7 @@ func (s *server) cloneFromURLCommand(
 			Flags: cloneFlags,
 			Args:  []string{u.String(), repositoryFullPath},
 		},
-		append(opts, git.WithConfig(config...))...,
+		append(opts, git.WithConfigEnv(config...))...,
 	)
 }
 
diff --git a/internal/gitaly/service/repository/create_repository_from_url_test.go b/internal/gitaly/service/repository/create_repository_from_url_test.go
index 8a69b552d..5fdd93ccb 100644
--- a/internal/gitaly/service/repository/create_repository_from_url_test.go
+++ b/internal/gitaly/service/repository/create_repository_from_url_test.go
@@ -186,53 +186,74 @@ func TestCreateRepositoryFromURL_redirect(t *testing.T) {
 
 func TestServer_CloneFromURLCommand(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
-
-	var authToken string
-	userInfo := "user:pass%21%3F%40"
-	repositoryFullPath := "full/path/to/repository"
-	url := fmt.Sprintf("https://%s@192.0.2.1/secretrepo.git", userInfo)
-	host := "www.example.com"
 
 	cfg := testcfg.Build(t)
 	s := server{cfg: cfg, gitCmdFactory: gittest.NewCommandFactory(t, cfg)}
-	cmd, err := s.cloneFromURLCommand(ctx, url, host, repositoryFullPath, authToken, false, git.WithDisabledHooks())
-	require.NoError(t, err)
-
-	expectedBareFlag := "--bare"
-	expectedScrubbedURL := "https://192.0.2.1/secretrepo.git"
-	expectedBasicAuthHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte("user:pass!?@")))
-	expectedAuthHeader := fmt.Sprintf("http.extraHeader=%s", expectedBasicAuthHeader)
-	expectedHostHeader := "http.extraHeader=Host: www.example.com"
-
-	args := cmd.Args()
-	require.Contains(t, args, expectedBareFlag)
-	require.Contains(t, args, expectedScrubbedURL)
-	require.Contains(t, args, expectedAuthHeader)
-	require.Contains(t, args, expectedHostHeader)
-	require.NotContains(t, args, userInfo)
-}
-
-func TestServer_CloneFromURLCommand_withToken(t *testing.T) {
-	t.Parallel()
-	ctx := testhelper.Context(t)
-
-	repositoryFullPath := "full/path/to/repository"
-	url := "https://www.example.com/secretrepo.git"
-	authToken := "GL-Geo EhEhKSUk_385GSLnS7BI:eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJkYXRhIjoie1wic2NvcGVcIjpcInJvb3QvZ2l0bGFiLWNlXCJ9IiwianRpIjoiNmQ4ZDM1NGQtZjUxYS00MDQ5LWExZjctMjUyMjk4YmQwMTI4IiwiaWF0IjoxNjQyMDk1MzY5LCJuYmYiOjE2NDIwOTUzNjQsImV4cCI6MTY0MjA5NTk2OX0.YEpfzg8305dUqkYOiB7_dhbL0FVSaUPgpSpMuKrgNrg"
 
-	cfg := testcfg.Build(t)
-	s := server{cfg: cfg, gitCmdFactory: gittest.NewCommandFactory(t, cfg)}
-	cmd, err := s.cloneFromURLCommand(ctx, url, "", repositoryFullPath, authToken, false, git.WithDisabledHooks())
-	require.NoError(t, err)
+	user, password := "example_user", "pass%21%3F%40"
 
-	expectedScrubbedURL := "https://www.example.com/secretrepo.git"
-	expectedBasicAuthHeader := fmt.Sprintf("Authorization: %s", authToken)
-	expectedHeader := fmt.Sprintf("http.extraHeader=%s", expectedBasicAuthHeader)
+	for _, tc := range []struct {
+		desc               string
+		url                string
+		token              string
+		expectedAuthHeader string
+	}{
+		{
+			desc:  "user credentials",
+			url:   fmt.Sprintf("https://%s:%s@192.0.2.1/secretrepo.git", user, password),
+			token: "",
+			expectedAuthHeader: fmt.Sprintf(
+				"Authorization: Basic %s",
+				base64.StdEncoding.EncodeToString([]byte("example_user:pass!?@")),
+			),
+		},
+		{
+			desc:  "token",
+			url:   "https://192.0.2.1/secretrepo.git",
+			token: "some-token",
+			expectedAuthHeader: fmt.Sprintf(
+				"Authorization: %s", "some-token",
+			),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(testhelper.Context(t))
+
+			cmd, err := s.cloneFromURLCommand(
+				ctx,
+				tc.url,
+				"www.example.com",
+				"full/path/to/repository",
+				tc.token,
+				false,
+				git.WithDisabledHooks(),
+			)
+			require.NoError(t, err)
+
+			// Kill the command so that it won't leak outside of the current test
+			// context. We know that it will return an error, but we cannot quite tell
+			// what kind of error it will be because it might fail either be to the kill
+			// signal or because it failed to clone the repository.
+			cancel()
+			require.Error(t, cmd.Wait())
+
+			args := cmd.Args()
+			require.Contains(t, args, "--bare")
+			require.Contains(t, args, "https://192.0.2.1/secretrepo.git")
+			for _, arg := range args {
+				require.NotContains(t, arg, user)
+				require.NotContains(t, arg, password)
+				require.NotContains(t, arg, tc.expectedAuthHeader)
+			}
 
-	args := cmd.Args()
-	require.Contains(t, args, expectedScrubbedURL)
-	require.Contains(t, args, expectedHeader)
+			require.Subset(t, cmd.Env(), []string{
+				"GIT_CONFIG_KEY_0=http.extraHeader",
+				"GIT_CONFIG_VALUE_0=" + tc.expectedAuthHeader,
+				"GIT_CONFIG_KEY_1=http.extraHeader",
+				"GIT_CONFIG_VALUE_1=Host: www.example.com",
+			})
+		})
+	}
 }
 
 func TestServer_CloneFromURLCommand_withMirror(t *testing.T) {