Merge pull request #2091 from alexcb/redact-credentials-from-remote-url-errors

Redact credentials from URLs before returning errors

source/git/gitsource.go

@@ -75,7 +75,7 @@ func (gs *gitSource) mountRemote(ctx context.Context, remote string, auth []stri
 
 	sis, err := gs.md.Search(remoteKey)
 	if err != nil {
-		return "", nil, errors.Wrapf(err, "failed to search metadata for %s", remote)
+		return "", nil, errors.Wrapf(err, "failed to search metadata for %s", redactCredentials(remote))
 	}
 
 	var remoteRef cache.MutableRef
@@ -84,19 +84,19 @@ func (gs *gitSource) mountRemote(ctx context.Context, remote string, auth []stri
 		if err != nil {
 			if errors.Is(err, cache.ErrLocked) {
 				// should never really happen as no other function should access this metadata, but lets be graceful
-				logrus.Warnf("mutable ref for %s  %s was locked: %v", remote, si.ID(), err)
+				logrus.Warnf("mutable ref for %s  %s was locked: %v", redactCredentials(remote), si.ID(), err)
 				continue
 			}
-			return "", nil, errors.Wrapf(err, "failed to get mutable ref for %s", remote)
+			return "", nil, errors.Wrapf(err, "failed to get mutable ref for %s", redactCredentials(remote))
 		}
 		break
 	}
 
 	initializeRepo := false
 	if remoteRef == nil {
-		remoteRef, err = gs.cache.New(ctx, nil, g, cache.CachePolicyRetain, cache.WithDescription(fmt.Sprintf("shared git repo for %s", remote)))
+		remoteRef, err = gs.cache.New(ctx, nil, g, cache.CachePolicyRetain, cache.WithDescription(fmt.Sprintf("shared git repo for %s", redactCredentials(remote))))
 		if err != nil {
-			return "", nil, errors.Wrapf(err, "failed to create new mutable for %s", remote)
+			return "", nil, errors.Wrapf(err, "failed to create new mutable for %s", redactCredentials(remote))
 		}
 		initializeRepo = true
 	}
@@ -348,7 +348,7 @@ func (gs *gitSourceHandler) CacheKey(ctx context.Context, g session.Group, index
 
 	buf, err := gitWithinDir(ctx, gitDir, "", sock, knownHosts, gs.auth, "ls-remote", "origin", ref)
 	if err != nil {
-		return "", nil, false, errors.Wrapf(err, "failed to fetch remote %s", remote)
+		return "", nil, false, errors.Wrapf(err, "failed to fetch remote %s", redactCredentials(remote))
 	}
 	out := buf.String()
 	idx := strings.Index(out, "\t")
@@ -450,13 +450,13 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
 			// TODO: is there a better way to do this?
 		}
 		if _, err := gitWithinDir(ctx, gitDir, "", sock, knownHosts, gs.auth, args...); err != nil {
-			return nil, errors.Wrapf(err, "failed to fetch remote %s", gs.src.Remote)
+			return nil, errors.Wrapf(err, "failed to fetch remote %s", redactCredentials(gs.src.Remote))
 		}
 	}
 
 	checkoutRef, err := gs.cache.New(ctx, nil, g, cache.WithRecordType(client.UsageRecordTypeGitCheckout), cache.WithDescription(fmt.Sprintf("git snapshot for %s#%s", gs.src.Remote, ref)))
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to create new mutable for %s", gs.src.Remote)
+		return nil, errors.Wrapf(err, "failed to create new mutable for %s", redactCredentials(gs.src.Remote))
 	}
 
 	defer func() {
@@ -509,19 +509,19 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
 		}
 		_, err = gitWithinDir(ctx, checkoutDirGit, checkoutDir, sock, knownHosts, nil, "checkout", "FETCH_HEAD")
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to checkout remote %s", gs.src.Remote)
+			return nil, errors.Wrapf(err, "failed to checkout remote %s", redactCredentials(gs.src.Remote))
 		}
 		gitDir = checkoutDirGit
 	} else {
 		_, err = gitWithinDir(ctx, gitDir, checkoutDir, sock, knownHosts, nil, "checkout", ref, "--", ".")
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to checkout remote %s", gs.src.Remote)
+			return nil, errors.Wrapf(err, "failed to checkout remote %s", redactCredentials(gs.src.Remote))
 		}
 	}
 
 	_, err = gitWithinDir(ctx, gitDir, checkoutDir, sock, knownHosts, gs.auth, "submodule", "update", "--init", "--recursive", "--depth=1")
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to update submodules for %s", gs.src.Remote)
+		return nil, errors.Wrapf(err, "failed to update submodules for %s", redactCredentials(gs.src.Remote))
 	}
 
 	if idmap := mount.IdentityMapping(); idmap != nil {

source/git/gitsource_test.go

@@ -323,6 +323,31 @@ func testMultipleRepos(t *testing.T, keepGitDir bool) {
 	require.Equal(t, "xyz\n", string(dt))
 }
 
+func TestCredentialRedaction(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Depends on unimplemented containerd bind-mount support on Windows")
+	}
+
+	t.Parallel()
+	ctx := namespaces.WithNamespace(context.Background(), "buildkit-test")
+
+	tmpdir, err := ioutil.TempDir("", "buildkit-state")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpdir)
+
+	gs := setupGitSource(t, tmpdir)
+
+	url := "https://user:keepthissecret@non-existant-host/user/private-repo.git"
+	id := &source.GitIdentifier{Remote: url}
+
+	g, err := gs.Resolve(ctx, id, nil, nil)
+	require.NoError(t, err)
+
+	_, _, _, err = g.CacheKey(ctx, nil, 0)
+	require.Error(t, err)
+	require.False(t, strings.Contains(err.Error(), "keepthissecret"))
+}
+
 func setupGitSource(t *testing.T, tmpdir string) source.Source {
 	snapshotter, err := native.NewSnapshotter(filepath.Join(tmpdir, "snapshots"))
 	assert.NoError(t, err)

source/git/redact_credentials.go

@@ -0,0 +1,28 @@
+package git
+
+import "net/url"
+
+// redactCredentials takes a URL and redacts a password from it.
+// e.g. "https://user:password@github.com/user/private-repo-failure.git" will be changed to
+// "https://user:xxxxx@github.com/user/private-repo-failure.git"
+func redactCredentials(s string) string {
+	u, err := url.Parse(s)
+	if err != nil {
+		return s // string is not a URL, just return it
+	}
+
+	return urlRedacted(u)
+}
+
+// urlRedacted comes from go but isn't available under 1.13
+func urlRedacted(u *url.URL) string {
+	if u == nil {
+		return ""
+	}
+
+	ru := *u
+	if _, has := ru.User.Password(); has {
+		ru.User = url.UserPassword(ru.User.Username(), "xxxxx")
+	}
+	return ru.String()
+}