Fix: do not log or persist credentials embedded in Terraform module remote URLs (#7201)

The remote URL of a Terraform module can embed credentials (for example
https://user:token@host/repo.git). The cache-reuse logic logged the raw URL
and wrote it to the .remote-url cache marker, which could leak those
credentials into controller logs and onto disk.

Strip the userinfo from the URL before logging it and before writing the cache
marker. The marker now stores a credential-free URL and the reuse check
compares the same stripped form, so cache reuse and re-clone-on-change behave
as before while no secret is persisted.

Signed-off-by: Ayush Kumar <65535504+roguepikachu@users.noreply.github.com>

pkg/controller/utils/capability.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
@@ -222,10 +223,10 @@ func GetTerraformConfigurationFromRemote(name, remoteURL, remotePath string, ssh
 	// tree. See GHSA-fmgp-q6jx-gg3x.
 	if !cacheMatchesRemote(cachePath, remoteURL) {
 		_ = os.RemoveAll(cachePath)
-		klog.InfoS("cloning remote Terraform module", "module", name, "url", remoteURL, "cache", cachePath)
+		klog.InfoS("cloning remote Terraform module", "module", name, "url", redactURLCredentials(remoteURL), "cache", cachePath)
 		if err = cloneTerraformModule(cachePath, remoteURL, sshPublicKey); err != nil {
 			// Do not leave a partial or oversized clone behind for the next reconcile.
-			klog.ErrorS(err, "failed to clone remote Terraform module", "module", name, "url", remoteURL)
+			klog.ErrorS(err, "failed to clone remote Terraform module", "module", name, "url", redactURLCredentials(remoteURL))
 			_ = os.RemoveAll(cachePath)
 			return "", err
 		}
@@ -247,8 +248,10 @@ func GetTerraformConfigurationFromRemote(name, remoteURL, remotePath string, ssh
 	return conf, nil
 }
 
-// cacheRemoteMarkerSuffix names the sibling file that records which remote URL a
-// cached Terraform module was cloned from, used to detect a changed URL.
+// cacheRemoteMarkerSuffix names the sibling file that records the remote a cached
+// Terraform module was cloned from, used to detect a changed remote. A
+// credential-stripped form of the URL is stored (see redactURLCredentials) so the
+// marker never persists secrets embedded in an authenticated Git URL.
 const cacheRemoteMarkerSuffix = ".remote-url"
 
 // validateModuleName rejects names that could steer the cache path (and the
@@ -262,7 +265,8 @@ func validateModuleName(name string) error {
 }
 
 // cacheMatchesRemote reports whether cachePath holds a populated clone recorded
-// as coming from remoteURL.
+// as coming from remoteURL. The marker stores a credential-stripped URL, so the
+// comparison strips credentials from remoteURL the same way before matching.
 func cacheMatchesRemote(cachePath, remoteURL string) bool {
 	entities, err := os.ReadDir(cachePath)
 	if err != nil || len(entities) == 0 {
@@ -272,16 +276,36 @@ func cacheMatchesRemote(cachePath, remoteURL string) bool {
 	if err != nil {
 		return false
 	}
-	return string(recorded) == remoteURL
+	return string(recorded) == redactURLCredentials(remoteURL)
 }
 
-// recordCacheRemote records the remote URL a freshly cloned module came from.
+// recordCacheRemote records the remote a freshly cloned module came from. It stores a
+// credential-stripped URL so the marker never persists secrets that an authenticated
+// Git URL may embed.
 func recordCacheRemote(cachePath, remoteURL string) {
-	if err := os.WriteFile(cachePath+cacheRemoteMarkerSuffix, []byte(remoteURL), 0600); err != nil {
+	if err := os.WriteFile(cachePath+cacheRemoteMarkerSuffix, []byte(redactURLCredentials(remoteURL)), 0600); err != nil {
 		klog.ErrorS(err, "failed to record Terraform module remote URL", "cache", cachePath)
 	}
 }
 
+// redactURLCredentials strips any embedded userinfo (a username, password, or token)
+// from a URL so it is safe to log and persist. Authenticated HTTPS Git URLs can carry
+// credentials in the userinfo; scp-style SSH URLs ("git@host:path") fail url.Parse but
+// authenticate with keys, so they carry no secret. The same stripped form is used for
+// logging and for the cache marker, so record and match stay consistent.
+func redactURLCredentials(raw string) string {
+	if u, err := url.Parse(raw); err == nil {
+		u.User = nil
+		return u.String()
+	}
+	// url.Parse failed (typically scp-style SSH). Strip a leading "user@" defensively
+	// in case a malformed URL still embeds credentials before the host.
+	if _, after, found := strings.Cut(raw, "@"); found {
+		return after
+	}
+	return raw
+}
+
 const (
 	// cloneTimeout bounds the network fetch of a remote Terraform module clone, so a
 	// slow or oversized attacker-supplied repository cannot hang the controller. It

pkg/controller/utils/capability_test.go

@@ -750,6 +750,54 @@ func TestCacheMatchesRemote(t *testing.T) {
 	})
 }
 
+// TestRedactURLCredentials verifies credentials embedded in a remote URL are stripped
+// before the URL is logged or written to the cache marker (GHSA-fmgp-q6jx-gg3x).
+func TestRedactURLCredentials(t *testing.T) {
+	cases := map[string]struct {
+		in   string
+		want string
+	}{
+		"https with user and password":   {"https://user:s3cr3t@github.com/org/repo.git", "https://github.com/org/repo.git"},
+		"https with token as user":       {"https://ghp_TOKEN123@github.com/org/repo.git", "https://github.com/org/repo.git"},
+		"https without credentials":      {"https://github.com/org/repo.git", "https://github.com/org/repo.git"},
+		"ssh url with user":              {"ssh://git@github.com/org/repo.git", "ssh://github.com/org/repo.git"},
+		"scp-style ssh":                  {"git@github.com:org/repo.git", "github.com:org/repo.git"},
+		"git scheme without credentials": {"git://example/repo.git", "git://example/repo.git"},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := redactURLCredentials(tc.in)
+			assert.Equal(t, tc.want, got)
+			assert.NotContains(t, got, "s3cr3t")
+			assert.NotContains(t, got, "ghp_TOKEN123")
+		})
+	}
+}
+
+// TestRecordCacheRemoteRedactsCredentials verifies the persisted cache marker never
+// contains credentials from an authenticated remote URL, while the populated cache is
+// still reused for the same repository (GHSA-fmgp-q6jx-gg3x).
+func TestRecordCacheRemoteRedactsCredentials(t *testing.T) {
+	cache := filepath.Join(t.TempDir(), "mod")
+	assert.NoError(t, os.MkdirAll(cache, 0700))
+	assert.NoError(t, os.WriteFile(filepath.Join(cache, "variables.tf"), []byte("x"), 0600))
+
+	recordCacheRemote(cache, "https://user:s3cr3t@github.com/org/repo.git")
+
+	marker, err := os.ReadFile(filepath.Clean(cache + cacheRemoteMarkerSuffix))
+	assert.NoError(t, err)
+	assert.NotContains(t, string(marker), "s3cr3t")
+	assert.NotContains(t, string(marker), "user")
+	assert.Equal(t, "https://github.com/org/repo.git", string(marker))
+
+	// The same repository, with or without credentials, still matches the recorded
+	// marker, so a populated cache is reused rather than re-cloned.
+	assert.True(t, cacheMatchesRemote(cache, "https://user:s3cr3t@github.com/org/repo.git"))
+	assert.True(t, cacheMatchesRemote(cache, "https://github.com/org/repo.git"))
+	// A different repository does not match.
+	assert.False(t, cacheMatchesRemote(cache, "https://github.com/org/other.git"))
+}
+
 // TestGetTerraformConfigurationFromRemoteInvalidatesCacheOnRejection exercises
 // the cache-reuse path with no network: a populated cache whose URL marker
 // matches skips the clone, so a poisoned cached variables.tf (a symlink escaping