From d0308a52e7481cd9b72264dde9b0c87c5952f297 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Tue, 7 May 2019 09:11:12 -0400 Subject: [PATCH] Redact clone url during clone errors. If there was an error during cloning, we would sometimes print the clone credentials to the pull request and in the Atlantis logs. This change ensures we print the redacted clone url. We also change the redacted clone url to contain "username:" so that if users see the redacted url in logs, they'll know we were using a credentialed form and so if they try to replicate with curl they'll know to substitute the correct credentials. --- server/events/event_parser_test.go | 44 ++++++++++++++--------------- server/events/models/models.go | 10 +++++-- server/events/models/models_test.go | 8 +++--- server/events/working_dir.go | 15 ++++++---- server/events_controller_test.go | 2 +- 5 files changed, 43 insertions(+), 36 deletions(-) diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index 9d8ae71e0f..059b176937 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -47,7 +47,7 @@ func TestParseGithubRepo(t *testing.T) { Owner: "owner", FullName: "owner/repo", CloneURL: "https://github-user:github-token@github.com/owner/repo.git", - SanitizedCloneURL: Repo.GetCloneURL(), + SanitizedCloneURL: "https://github-user:@github.com/owner/repo.git", Name: "repo", VCSHost: models.VCSHost{ Hostname: "github.com", @@ -96,7 +96,7 @@ func TestParseGithubIssueCommentEvent(t *testing.T) { Owner: *comment.Repo.Owner.Login, FullName: *comment.Repo.FullName, CloneURL: "https://github-user:github-token@github.com/owner/repo.git", - SanitizedCloneURL: *comment.Repo.CloneURL, + SanitizedCloneURL: "https://github-user:@github.com/owner/repo.git", Name: "repo", VCSHost: models.VCSHost{ Hostname: "github.com", @@ -134,7 +134,7 @@ func TestParseGithubPullEvent(t *testing.T) { Owner: "owner", FullName: "owner/repo", CloneURL: "https://github-user:github-token@github.com/owner/repo.git", - SanitizedCloneURL: Repo.GetCloneURL(), + SanitizedCloneURL: "https://github-user:@github.com/owner/repo.git", Name: "repo", VCSHost: models.VCSHost{ Hostname: "github.com", @@ -252,7 +252,7 @@ func TestParseGithubPull(t *testing.T) { Owner: "owner", FullName: "owner/repo", CloneURL: "https://github-user:github-token@github.com/owner/repo.git", - SanitizedCloneURL: Repo.GetCloneURL(), + SanitizedCloneURL: "https://github-user:@github.com/owner/repo.git", Name: "repo", VCSHost: models.VCSHost{ Hostname: "github.com", @@ -287,7 +287,7 @@ func TestParseGitlabMergeEvent(t *testing.T) { expBaseRepo := models.Repo{ FullName: "lkysow/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow/atlantis-example.git", Owner: "lkysow", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow/atlantis-example.git", VCSHost: models.VCSHost{ @@ -312,7 +312,7 @@ func TestParseGitlabMergeEvent(t *testing.T) { Equals(t, models.Repo{ FullName: "sourceorg/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/sourceorg/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/sourceorg/atlantis-example.git", Owner: "sourceorg", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/sourceorg/atlantis-example.git", VCSHost: models.VCSHost{ @@ -344,7 +344,7 @@ func TestParseGitlabMergeEvent_Subgroup(t *testing.T) { expBaseRepo := models.Repo{ FullName: "lkysow-test/subgroup/sub-subgroup/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", Owner: "lkysow-test/subgroup/sub-subgroup", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", VCSHost: models.VCSHost{ @@ -369,7 +369,7 @@ func TestParseGitlabMergeEvent_Subgroup(t *testing.T) { Equals(t, models.Repo{ FullName: "lkysow-test/subgroup/sub-subgroup/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", Owner: "lkysow-test/subgroup/sub-subgroup", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", VCSHost: models.VCSHost{ @@ -440,7 +440,7 @@ func TestParseGitlabMergeRequest(t *testing.T) { repo := models.Repo{ FullName: "gitlabhq/gitlab-test", Name: "gitlab-test", - SanitizedCloneURL: "https://example.com/gitlabhq/gitlab-test.git", + SanitizedCloneURL: "https://gitlab-user:@example.com/gitlabhq/gitlab-test.git", Owner: "gitlabhq", CloneURL: "https://gitlab-user:gitlab-token@example.com/gitlabhq/gitlab-test.git", VCSHost: models.VCSHost{ @@ -479,7 +479,7 @@ func TestParseGitlabMergeRequest_Subgroup(t *testing.T) { repo := models.Repo{ FullName: "lkysow-test/subgroup/sub-subgroup/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", Owner: "lkysow-test/subgroup/sub-subgroup", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", VCSHost: models.VCSHost{ @@ -513,7 +513,7 @@ func TestParseGitlabMergeCommentEvent(t *testing.T) { Equals(t, models.Repo{ FullName: "gitlabhq/gitlab-test", Name: "gitlab-test", - SanitizedCloneURL: "https://example.com/gitlabhq/gitlab-test.git", + SanitizedCloneURL: "https://gitlab-user:@example.com/gitlabhq/gitlab-test.git", Owner: "gitlabhq", CloneURL: "https://gitlab-user:gitlab-token@example.com/gitlabhq/gitlab-test.git", VCSHost: models.VCSHost{ @@ -524,7 +524,7 @@ func TestParseGitlabMergeCommentEvent(t *testing.T) { Equals(t, models.Repo{ FullName: "gitlab-org/gitlab-test", Name: "gitlab-test", - SanitizedCloneURL: "https://example.com/gitlab-org/gitlab-test.git", + SanitizedCloneURL: "https://gitlab-user:@example.com/gitlab-org/gitlab-test.git", Owner: "gitlab-org", CloneURL: "https://gitlab-user:gitlab-token@example.com/gitlab-org/gitlab-test.git", VCSHost: models.VCSHost{ @@ -551,7 +551,7 @@ func TestParseGitlabMergeCommentEvent_Subgroup(t *testing.T) { Equals(t, models.Repo{ FullName: "lkysow-test/subgroup/sub-subgroup/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", Owner: "lkysow-test/subgroup/sub-subgroup", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", VCSHost: models.VCSHost{ @@ -562,7 +562,7 @@ func TestParseGitlabMergeCommentEvent_Subgroup(t *testing.T) { Equals(t, models.Repo{ FullName: "lkysow-test/subgroup/sub-subgroup/atlantis-example", Name: "atlantis-example", - SanitizedCloneURL: "https://gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", + SanitizedCloneURL: "https://gitlab-user:@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", Owner: "lkysow-test/subgroup/sub-subgroup", CloneURL: "https://gitlab-user:gitlab-token@gitlab.com/lkysow-test/subgroup/sub-subgroup/atlantis-example.git", VCSHost: models.VCSHost{ @@ -707,7 +707,7 @@ func TestParseBitbucketCloudCommentEvent_ValidEvent(t *testing.T) { Owner: "lkysow", Name: "atlantis-example", CloneURL: "https://bitbucket-user:bitbucket-token@bitbucket.org/lkysow/atlantis-example.git", - SanitizedCloneURL: "https://bitbucket.org/lkysow/atlantis-example.git", + SanitizedCloneURL: "https://bitbucket-user:@bitbucket.org/lkysow/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "bitbucket.org", Type: models.BitbucketCloud, @@ -729,7 +729,7 @@ func TestParseBitbucketCloudCommentEvent_ValidEvent(t *testing.T) { Owner: "lkysow-fork", Name: "atlantis-example", CloneURL: "https://bitbucket-user:bitbucket-token@bitbucket.org/lkysow-fork/atlantis-example.git", - SanitizedCloneURL: "https://bitbucket.org/lkysow-fork/atlantis-example.git", + SanitizedCloneURL: "https://bitbucket-user:@bitbucket.org/lkysow-fork/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "bitbucket.org", Type: models.BitbucketCloud, @@ -793,7 +793,7 @@ func TestParseBitbucketCloudPullEvent_ValidEvent(t *testing.T) { Owner: "lkysow", Name: "atlantis-example", CloneURL: "https://bitbucket-user:bitbucket-token@bitbucket.org/lkysow/atlantis-example.git", - SanitizedCloneURL: "https://bitbucket.org/lkysow/atlantis-example.git", + SanitizedCloneURL: "https://bitbucket-user:@bitbucket.org/lkysow/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "bitbucket.org", Type: models.BitbucketCloud, @@ -815,7 +815,7 @@ func TestParseBitbucketCloudPullEvent_ValidEvent(t *testing.T) { Owner: "lkysow-fork", Name: "atlantis-example", CloneURL: "https://bitbucket-user:bitbucket-token@bitbucket.org/lkysow-fork/atlantis-example.git", - SanitizedCloneURL: "https://bitbucket.org/lkysow-fork/atlantis-example.git", + SanitizedCloneURL: "https://bitbucket-user:@bitbucket.org/lkysow-fork/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "bitbucket.org", Type: models.BitbucketCloud, @@ -894,7 +894,7 @@ func TestParseBitbucketServerCommentEvent_ValidEvent(t *testing.T) { Owner: "atlantis", Name: "atlantis-example", CloneURL: "http://bitbucket-user:bitbucket-token@mycorp.com:7490/scm/at/atlantis-example.git", - SanitizedCloneURL: "http://mycorp.com:7490/scm/at/atlantis-example.git", + SanitizedCloneURL: "http://bitbucket-user:@mycorp.com:7490/scm/at/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "mycorp.com", Type: models.BitbucketServer, @@ -916,7 +916,7 @@ func TestParseBitbucketServerCommentEvent_ValidEvent(t *testing.T) { Owner: "atlantis-fork", Name: "atlantis-example", CloneURL: "http://bitbucket-user:bitbucket-token@mycorp.com:7490/scm/fk/atlantis-example.git", - SanitizedCloneURL: "http://mycorp.com:7490/scm/fk/atlantis-example.git", + SanitizedCloneURL: "http://bitbucket-user:@mycorp.com:7490/scm/fk/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "mycorp.com", Type: models.BitbucketServer, @@ -976,7 +976,7 @@ func TestParseBitbucketServerPullEvent_ValidEvent(t *testing.T) { Owner: "atlantis", Name: "atlantis-example", CloneURL: "http://bitbucket-user:bitbucket-token@mycorp.com:7490/scm/at/atlantis-example.git", - SanitizedCloneURL: "http://mycorp.com:7490/scm/at/atlantis-example.git", + SanitizedCloneURL: "http://bitbucket-user:@mycorp.com:7490/scm/at/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "mycorp.com", Type: models.BitbucketServer, @@ -998,7 +998,7 @@ func TestParseBitbucketServerPullEvent_ValidEvent(t *testing.T) { Owner: "atlantis-fork", Name: "atlantis-example", CloneURL: "http://bitbucket-user:bitbucket-token@mycorp.com:7490/scm/fk/atlantis-example.git", - SanitizedCloneURL: "http://mycorp.com:7490/scm/fk/atlantis-example.git", + SanitizedCloneURL: "http://bitbucket-user:@mycorp.com:7490/scm/fk/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "mycorp.com", Type: models.BitbucketServer, diff --git a/server/events/models/models.go b/server/events/models/models.go index 388907aa99..2fe503f534 100644 --- a/server/events/models/models.go +++ b/server/events/models/models.go @@ -45,8 +45,9 @@ type Repo struct { // CloneURL is the full HTTPS url for cloning with username and token string // ex. "https://username:token@github.com/atlantis/atlantis.git". CloneURL string - // SanitizedCloneURL is the full HTTPS url for cloning without the username and password. - // ex. "https://github.com/atlantis/atlantis.git". + // SanitizedCloneURL is the full HTTPS url for cloning with the password + // redacted. + // ex. "https://user:@github.com/atlantis/atlantis.git". SanitizedCloneURL string // VCSHost is where this repo is hosted. VCSHost VCSHost @@ -95,11 +96,14 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU escapedVCSUser := url.QueryEscape(vcsUser) escapedVCSToken := url.QueryEscape(vcsToken) auth := fmt.Sprintf("%s:%s@", escapedVCSUser, escapedVCSToken) + redactedAuth := fmt.Sprintf("%s:@", escapedVCSUser) // Construct clone urls with http and https auth. Need to do both // because Bitbucket supports http. authedCloneURL := strings.Replace(cloneURL, "https://", "https://"+auth, -1) authedCloneURL = strings.Replace(authedCloneURL, "http://", "http://"+auth, -1) + sanitizedCloneURL := strings.Replace(cloneURL, "https://", "https://"+redactedAuth, -1) + sanitizedCloneURL = strings.Replace(sanitizedCloneURL, "http://", "http://"+redactedAuth, -1) // Get the owner and repo names from the full name. owner, repo := SplitRepoFullName(repoFullName) @@ -120,7 +124,7 @@ func NewRepo(vcsHostType VCSHostType, repoFullName string, cloneURL string, vcsU Owner: owner, Name: repo, CloneURL: authedCloneURL, - SanitizedCloneURL: cloneURL, + SanitizedCloneURL: sanitizedCloneURL, VCSHost: VCSHost{ Type: vcsHostType, Hostname: cloneURLParsed.Hostname(), diff --git a/server/events/models/models_test.go b/server/events/models/models_test.go index fe6857cb5f..b6241ece63 100644 --- a/server/events/models/models_test.go +++ b/server/events/models/models_test.go @@ -52,7 +52,7 @@ func TestNewRepo_CloneURLBitbucketServer(t *testing.T) { Owner: "owner", Name: "repo", CloneURL: "http://u:p@mycorp.com:7990/scm/at/atlantis-example.git", - SanitizedCloneURL: "http://mycorp.com:7990/scm/at/atlantis-example.git", + SanitizedCloneURL: "http://u:@mycorp.com:7990/scm/at/atlantis-example.git", VCSHost: models.VCSHost{ Hostname: "mycorp.com", Type: models.BitbucketServer, @@ -104,7 +104,7 @@ func TestNewRepo_MissingDotGit(t *testing.T) { repo, err := models.NewRepo(models.BitbucketCloud, "owner/repo", "https://bitbucket.org/owner/repo", "u", "p") Ok(t, err) Equals(t, repo.CloneURL, "https://u:p@bitbucket.org/owner/repo.git") - Equals(t, repo.SanitizedCloneURL, "https://bitbucket.org/owner/repo.git") + Equals(t, repo.SanitizedCloneURL, "https://u:@bitbucket.org/owner/repo.git") } func TestNewRepo_HTTPAuth(t *testing.T) { @@ -116,7 +116,7 @@ func TestNewRepo_HTTPAuth(t *testing.T) { Hostname: "github.com", Type: models.Github, }, - SanitizedCloneURL: "http://github.com/owner/repo.git", + SanitizedCloneURL: "http://u:@github.com/owner/repo.git", CloneURL: "http://u:p@github.com/owner/repo.git", FullName: "owner/repo", Owner: "owner", @@ -133,7 +133,7 @@ func TestNewRepo_HTTPSAuth(t *testing.T) { Hostname: "github.com", Type: models.Github, }, - SanitizedCloneURL: "https://github.com/owner/repo.git", + SanitizedCloneURL: "https://u:@github.com/owner/repo.git", CloneURL: "https://u:p@github.com/owner/repo.git", FullName: "owner/repo", Owner: "owner", diff --git a/server/events/working_dir.go b/server/events/working_dir.go index f1b115a4d0..01bb682e45 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -179,12 +179,14 @@ func (w *FileWorkspace) forceClone(log *logging.SimpleLogger, "GIT_COMMITTER_NAME=atlantis", }...) - cmdStr := w.cmdAsSanitizedStr(cmd, p.BaseRepo, headRepo) + cmdStr := w.sanitizeGitCredentials(strings.Join(cmd.Args, " "), p.BaseRepo, headRepo) output, err := cmd.CombinedOutput() + sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo) if err != nil { - return "", errors.Wrapf(err, "running %s: %s", cmdStr, string(output)) + sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo) + return "", fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg) } - log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(string(output), "\n")) + log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n")) } return cloneDir, nil } @@ -226,8 +228,9 @@ func (w *FileWorkspace) cloneDir(r models.Repo, p models.PullRequest, workspace return filepath.Join(w.repoPullDir(r, p), workspace) } -func (w *FileWorkspace) cmdAsSanitizedStr(cmd *exec.Cmd, base models.Repo, head models.Repo) string { - cmdAsStr := strings.Join(cmd.Args, " ") - baseReplaced := strings.Replace(cmdAsStr, base.CloneURL, base.SanitizedCloneURL, -1) +// sanitizeGitCredentials replaces any git clone urls that contain credentials +// in s with the sanitized versions. +func (w *FileWorkspace) sanitizeGitCredentials(s string, base models.Repo, head models.Repo) string { + baseReplaced := strings.Replace(s, base.CloneURL, base.SanitizedCloneURL, -1) return strings.Replace(baseReplaced, head.CloneURL, head.SanitizedCloneURL, -1) } diff --git a/server/events_controller_test.go b/server/events_controller_test.go index b71be81772..34449f8eec 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -581,7 +581,7 @@ func TestPost_BBServerPullClosed(t *testing.T) { Owner: "project", Name: "repository", CloneURL: "https://bb-user:bb-token@bbserver.com/scm/proj/repository.git", - SanitizedCloneURL: "https://bbserver.com/scm/proj/repository.git", + SanitizedCloneURL: "https://bb-user:@bbserver.com/scm/proj/repository.git", VCSHost: models.VCSHost{ Hostname: "bbserver.com", Type: models.BitbucketServer,