Merge branch 'jc-scrub-userinfo' into 'master'

Do not leak sensitive urls

Closes #1306

See merge request gitlab-org/gitaly!1710

2 files changed, 108 insertions, 12 deletions

diff --git a/internal/service/repository/create_from_url.go b/internal/service/repository/create_from_url.go
index 7bfa59a2f..5843bcc15 100644
--- a/internal/service/repository/create_from_url.go
+++ b/internal/service/repository/create_from_url.go
@@ -2,9 +2,12 @@ package repository
 
 import (
 	"context"
+	"encoding/base64"
 	"fmt"
+	"net/url"
 	"os"
 
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -12,6 +15,27 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+func cloneFromURLCommand(ctx context.Context, repoURL, repositoryFullPath string) (*command.Command, error) {
+	u, err := url.Parse(repoURL)
+	if err != nil {
+		return nil, helper.ErrInternal(err)
+	}
+
+	flags := []git.Option{git.Flag{Name: "--bare"}, git.ValueFlag{Name: "-c", Value: "http.followRedirects=false"}}
+	if u.User != nil {
+		userInfo := *u.User
+		u.User = nil
+		authHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(userInfo.String())))
+		flags = append(flags, git.ValueFlag{Name: "-c", Value: fmt.Sprintf("http.%s.extraHeader=%s", u.String(), authHeader)})
+	}
+
+	return git.SafeBareCmd(ctx, nil, nil, nil, nil, nil, git.SubCmd{
+		Name:        "clone",
+		Flags:       flags,
+		PostSepArgs: []string{u.String(), repositoryFullPath},
+	})
+}
+
 func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.CreateRepositoryFromURLRequest) (*gitalypb.CreateRepositoryFromURLResponse, error) {
 	if err := validateCreateRepositoryFromURLRequest(req); err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "CreateRepositoryFromURL: %v", err)
@@ -28,19 +52,11 @@ func (s *server) CreateRepositoryFromURL(ctx context.Context, req *gitalypb.Crea
 		return nil, status.Errorf(codes.InvalidArgument, "CreateRepositoryFromURL: dest dir exists")
 	}
 
-	args := []string{
-		"-c",
-		"http.followRedirects=false",
-		"clone",
-		"--bare",
-		"--",
-		req.Url,
-		repositoryFullPath,
-	}
-	cmd, err := git.CommandWithoutRepo(ctx, args...)
+	cmd, err := cloneFromURLCommand(ctx, req.GetUrl(), repositoryFullPath)
 	if err != nil {
-		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd start: %v", err)
+		return nil, helper.ErrInternal(err)
 	}
+
 	if err := cmd.Wait(); err != nil {
 		os.RemoveAll(repositoryFullPath)
 		return nil, status.Errorf(codes.Internal, "CreateRepositoryFromURL: clone cmd wait: %v", err)
diff --git a/internal/service/repository/create_from_url_test.go b/internal/service/repository/create_from_url_test.go
index b760926e6..c07dcf2ee 100644
--- a/internal/service/repository/create_from_url_test.go
+++ b/internal/service/repository/create_from_url_test.go
@@ -1,11 +1,20 @@
 package repository
 
 import (
+	"encoding/base64"
+	"fmt"
 	"io/ioutil"
+	"net"
+	"net/http"
+	"net/http/cgi"
 	"os"
 	"path"
+	"path/filepath"
+	"strings"
 	"testing"
 
+	"gitlab.com/gitlab-org/gitaly/internal/config"
+
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -28,9 +37,17 @@ func TestSuccessfulCreateRepositoryFromURLRequest(t *testing.T) {
 		StorageName:  testhelper.DefaultStorageName,
 	}
 
+	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	user := "username123"
+	password := "password321localhost"
+	port := gitServerWithBasicAuth(t, user, password, testRepoPath)
+	url := fmt.Sprintf("http://%s:%s@localhost:%d/%s", user, password, port, filepath.Base(testRepoPath))
+
 	req := &gitalypb.CreateRepositoryFromURLRequest{
 		Repository: importedRepo,
-		Url:        "https://gitlab.com/gitlab-org/gitlab-test.git",
+		Url:        url,
 	}
 
 	_, err := client.CreateRepositoryFromURL(ctx, req)
@@ -50,6 +67,36 @@ func TestSuccessfulCreateRepositoryFromURLRequest(t *testing.T) {
 	require.NotEqual(t, 0, info.Mode()&os.ModeSymlink)
 }
 
+func TestCloneRepositoryFromUrlCommand(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	userInfo := "username:password"
+	repositoryFullPath := "full/path/to/repository"
+	url := fmt.Sprintf("https://%s@www.example.com/secretrepo.git", userInfo)
+
+	cmd, err := cloneFromURLCommand(ctx, url, repositoryFullPath)
+	require.NoError(t, err)
+
+	expectedScrubbedURL := "https://www.example.com/secretrepo.git"
+	expectedBasicAuthHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(userInfo)))
+	expectedHeader := fmt.Sprintf("http.%s.extraHeader=%s", expectedScrubbedURL, expectedBasicAuthHeader)
+
+	var urlFound, headerFound bool
+	for _, arg := range cmd.Args() {
+		require.False(t, strings.Contains(arg, userInfo), "username and password should be scrubbed")
+		if arg == expectedScrubbedURL {
+			urlFound = true
+		}
+		if arg == expectedHeader {
+			headerFound = true
+		}
+	}
+
+	require.True(t, urlFound)
+	require.True(t, headerFound)
+}
+
 func TestFailedCreateRepositoryFromURLRequestDueToExistingTarget(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
@@ -135,3 +182,36 @@ func TestPreventingRedirect(t *testing.T) {
 
 	require.Error(t, err)
 }
+
+func gitServerWithBasicAuth(t testing.TB, user, pass, repoPath string) int {
+	f, err := os.Create(filepath.Join(repoPath, "git-daemon-export-ok"))
+	require.NoError(t, err)
+	require.NoError(t, f.Close())
+
+	listener, err := net.Listen("tcp", ":0")
+	require.NoError(t, err)
+
+	s := http.Server{
+		Handler: basicAuthMiddleware(t, user, pass, &cgi.Handler{
+			Path: config.Config.Git.BinPath,
+			Dir:  "/",
+			Args: []string{"http-backend"},
+			Env: []string{
+				"GIT_PROJECT_ROOT=" + filepath.Dir(repoPath),
+			},
+		}),
+	}
+	go s.Serve(listener)
+
+	return listener.Addr().(*net.TCPAddr).Port
+}
+
+func basicAuthMiddleware(t testing.TB, user, pass string, next http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		authUser, authPass, ok := r.BasicAuth()
+		require.True(t, ok, "should contain basic auth")
+		require.Equal(t, user, authUser, "username should match")
+		require.Equal(t, pass, authPass, "password should match")
+		next.ServeHTTP(w, r)
+	})
+}