diff --git a/pkg/controller/utils/capability.go b/pkg/controller/utils/capability.go index 4840b38c4..a0a460888 100644 --- a/pkg/controller/utils/capability.go +++ b/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 diff --git a/pkg/controller/utils/capability_test.go b/pkg/controller/utils/capability_test.go index d92a4f441..5d5d010ef 100644 --- a/pkg/controller/utils/capability_test.go +++ b/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