diff options
author | Jacob Vosmaer <jacob@gitlab.com> | 2020-01-14 18:59:36 +0300 |
---|---|---|
committer | Jacob Vosmaer <jacob@gitlab.com> | 2020-01-14 18:59:36 +0300 |
commit | 130308e43830fe42e7afc4d7bcea0f8af4c6ba5b (patch) | |
tree | 297d75055205b7b1764ffc589e44cc6b3e94eb5b /internal | |
parent | eff812e5c76269a1be64dedc5c5b76b5fb8a207c (diff) | |
parent | 10475b579deaed643a1a19b455b22b2c472339f9 (diff) |
Merge branch 'jc-scrub-userinfo' into 'master'
Do not leak sensitive urls
Closes #1306
See merge request gitlab-org/gitaly!1710
Diffstat (limited to 'internal')
-rw-r--r-- | internal/service/repository/create_from_url.go | 38 | ||||
-rw-r--r-- | internal/service/repository/create_from_url_test.go | 82 |
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) + }) +} |