From cd4514bc0ca9cd14f50f73c586f35bb1ef307b1a Mon Sep 17 00:00:00 2001 From: Sameer Khan Date: Thu, 22 Oct 2026 11:08:42 +0530 Subject: [PATCH] Prevent credentials in source, cluster and diff diagnostics Sanitize HTTP source descriptions, nested request errors and target-cluster URLs without changing request authentication. Apply default Secret masking rules to tools diff, preserving the explicit masking opt-out. Add regression coverage for authenticated requests and both diff output modes. Signed-off-by: Sameer Khan --- internal/diagnostics/url.go | 34 ++++++++++++ internal/diagnostics/url_test.go | 31 +++++++++++ pkg/kapp/cmd/core/deps_factory.go | 3 +- pkg/kapp/cmd/tools/diff.go | 18 +++++-- pkg/kapp/cmd/tools/diff_sensitive_test.go | 52 +++++++++++++++++++ pkg/kapp/resources/file_sources.go | 10 ++-- .../resources/file_sources_sensitive_test.go | 35 +++++++++++++ 7 files changed, 173 insertions(+), 10 deletions(-) create mode 100644 internal/diagnostics/url.go create mode 100644 internal/diagnostics/url_test.go create mode 100644 pkg/kapp/cmd/tools/diff_sensitive_test.go create mode 100644 pkg/kapp/resources/file_sources_sensitive_test.go diff --git a/internal/diagnostics/url.go b/internal/diagnostics/url.go new file mode 100644 index 000000000..a1984accf --- /dev/null +++ b/internal/diagnostics/url.go @@ -0,0 +1,34 @@ +// Copyright 2026 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +// Package diagnostics formats endpoints without authentication values. +package diagnostics + +import ( + "errors" + "net/url" +) + +// URL returns endpoint context without authentication or query/fragment values. +// The original URL must still be used for the actual request. +func URL(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Opaque != "" { + return "[invalid URL]" + } + u.User = nil + u.RawQuery = "" + u.ForceQuery = false + u.Fragment = "" + u.RawFragment = "" + return u.String() +} + +// Error removes URL wrappers which would otherwise reintroduce the raw URL. +func Error(err error) error { + var urlErr *url.Error + if errors.As(err, &urlErr) { + return Error(urlErr.Err) + } + return err +} diff --git a/internal/diagnostics/url_test.go b/internal/diagnostics/url_test.go new file mode 100644 index 000000000..d3e674779 --- /dev/null +++ b/internal/diagnostics/url_test.go @@ -0,0 +1,31 @@ +// Copyright 2026 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package diagnostics + +import ( + "errors" + "net/url" + "testing" +) + +func TestURLPresentation(t *testing.T) { + raw := "https://user:password-canary@example.com/input" + + "?access_token=query-canary#fragment-canary" + if got := URL(raw); got != "https://example.com/input" { + t.Fatalf("unsafe URL: %s", got) + } + invalid := "https://user:password-canary@example.com:bad" + if URL(invalid) != "[invalid URL]" { + t.Fatal("invalid URL exposed") + } + nested := &url.Error{ + Op: "Get", URL: raw, + Err: &url.Error{ + Op: "proxy", URL: raw, Err: errors.New("connection refused"), + }, + } + if got := Error(nested).Error(); got != "connection refused" { + t.Fatal("nested URL leaked") + } +} diff --git a/pkg/kapp/cmd/core/deps_factory.go b/pkg/kapp/cmd/core/deps_factory.go index 7482e7eaf..1ea7a2ce6 100644 --- a/pkg/kapp/cmd/core/deps_factory.go +++ b/pkg/kapp/cmd/core/deps_factory.go @@ -8,6 +8,7 @@ import ( "fmt" "sync" + "carvel.dev/kapp/internal/diagnostics" "github.com/cppforlife/go-cli-ui/ui" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -117,7 +118,7 @@ func (f *DepsFactoryImpl) printTarget(config *rest.Config) { if len(nodesDesc) > 0 { nodesDesc = fmt.Sprintf(" (nodes: %s)", nodesDesc) } - f.ui.PrintLinef("Target cluster '%s'%s", config.Host, nodesDesc) + f.ui.PrintLinef("Target cluster '%s'%s", diagnostics.URL(config.Host), nodesDesc) }) } diff --git a/pkg/kapp/cmd/tools/diff.go b/pkg/kapp/cmd/tools/diff.go index e1700fd87..374ee1efb 100644 --- a/pkg/kapp/cmd/tools/diff.go +++ b/pkg/kapp/cmd/tools/diff.go @@ -6,13 +6,13 @@ package tools import ( "io/fs" - "github.com/cppforlife/go-cli-ui/ui" - "github.com/spf13/cobra" - ctlcap "carvel.dev/kapp/pkg/kapp/clusterapply" cmdcore "carvel.dev/kapp/pkg/kapp/cmd/core" + ctlconf "carvel.dev/kapp/pkg/kapp/config" ctldiff "carvel.dev/kapp/pkg/kapp/diff" ctlres "carvel.dev/kapp/pkg/kapp/resources" + "github.com/cppforlife/go-cli-ui/ui" + "github.com/spf13/cobra" ) type DiffOptions struct { @@ -66,8 +66,16 @@ func (o *DiffOptions) Run() error { changeViews = append(changeViews, DiffChangeView{change}) } - // TODO support adding custom config for mask rules? - ctlcap.NewChangeSetView(changeViews, nil, o.DiffFlags.ChangeSetViewOpts).Print(o.ui) + // Apply the same Secret masking defaults used by deploy diffs. + _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) + if err != nil { + return err + } + maskRules := conf.DiffMaskRules() + if !o.DiffFlags.Mask { + maskRules = nil + } + ctlcap.NewChangeSetView(changeViews, maskRules, o.DiffFlags.ChangeSetViewOpts).Print(o.ui) return nil } diff --git a/pkg/kapp/cmd/tools/diff_sensitive_test.go b/pkg/kapp/cmd/tools/diff_sensitive_test.go new file mode 100644 index 000000000..4f487cfcf --- /dev/null +++ b/pkg/kapp/cmd/tools/diff_sensitive_test.go @@ -0,0 +1,52 @@ +// Copyright 2026 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package tools + +import ( + "bytes" + "encoding/base64" + "fmt" + "strings" + "testing" + "testing/fstest" + + "github.com/cppforlife/go-cli-ui/ui" + "github.com/spf13/cobra" +) + +type diffCaptureUI struct { + ui.UI + output bytes.Buffer +} + +func (u *diffCaptureUI) PrintBlock(b []byte) { u.output.Write(b) } +func (u *diffCaptureUI) BeginLinef(f string, args ...interface{}) { fmt.Fprintf(&u.output, f, args...) } +func TestToolsDiffMasksSecrets(t *testing.T) { + for _, yamlOutput := range []bool{false, true} { + for _, mask := range []bool{false, true} { + capture := &diffCaptureUI{UI: ui.NewNoopUI()} + opts := NewDiffOptions(capture, nil) + opts.DiffFlags.SetWithPrefix("", &cobra.Command{}) + opts.DiffFlags.Changes = !yamlOutput + opts.DiffFlags.ChangesYAML = yamlOutput + opts.DiffFlags.Mask = mask + opts.DiffFlags.Summary = false + content := func(v string) []byte { + return []byte("apiVersion: v1\nkind: Secret\nmetadata:\n name: credentials\ndata:\n password: " + base64.StdEncoding.EncodeToString([]byte(v)) + "\nstringData:\n token: " + v + "\n") + } + opts.FileSystem = fstest.MapFS{"old.yml": &fstest.MapFile{Data: content("old-canary")}, "new.yml": &fstest.MapFile{Data: content("new-canary")}} + opts.FileFlags.Files = []string{"new.yml"} + opts.FileFlags2.Files = []string{"old.yml"} + if err := opts.Run(); err != nil { + t.Fatal(err) + } + for _, marker := range []string{"new-canary", base64.StdEncoding.EncodeToString([]byte("new-canary"))} { + found := strings.Contains(capture.output.String(), marker) + if found == mask { + t.Fatalf("mask=%v yaml=%v: unexpected credential visibility", mask, yamlOutput) + } + } + } + } +} diff --git a/pkg/kapp/resources/file_sources.go b/pkg/kapp/resources/file_sources.go index f57860b8b..d19168323 100644 --- a/pkg/kapp/resources/file_sources.go +++ b/pkg/kapp/resources/file_sources.go @@ -9,6 +9,8 @@ import ( "io/fs" "net/http" "os" + + "carvel.dev/kapp/internal/diagnostics" ) type FileSource interface { @@ -71,24 +73,24 @@ var _ FileSource = HTTPFileSource{} func NewHTTPFileSource(path string) HTTPFileSource { return HTTPFileSource{path, &http.Client{}} } func (s HTTPFileSource) Description() string { - return fmt.Sprintf("HTTP URL '%s'", s.url) + return fmt.Sprintf("HTTP URL '%s'", diagnostics.URL(s.url)) } func (s HTTPFileSource) Bytes() ([]byte, error) { resp, err := s.Client.Get(s.url) if err != nil { - return nil, fmt.Errorf("Requesting URL '%s': %w", s.url, err) + return nil, fmt.Errorf("Requesting URL '%s': %w", diagnostics.URL(s.url), diagnostics.Error(err)) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode > 299 { - return nil, fmt.Errorf("Requesting URL '%s': %s", s.url, resp.Status) + return nil, fmt.Errorf("Requesting URL '%s': %s", diagnostics.URL(s.url), resp.Status) } result, err := io.ReadAll(resp.Body) if err != nil { - return nil, fmt.Errorf("Reading URL '%s': %w", s.url, err) + return nil, fmt.Errorf("Reading URL '%s': %w", diagnostics.URL(s.url), diagnostics.Error(err)) } return result, nil diff --git a/pkg/kapp/resources/file_sources_sensitive_test.go b/pkg/kapp/resources/file_sources_sensitive_test.go new file mode 100644 index 000000000..57314884f --- /dev/null +++ b/pkg/kapp/resources/file_sources_sensitive_test.go @@ -0,0 +1,35 @@ +// Copyright 2026 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package resources + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHTTPSourceDiagnosticsAndAuthentication(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + _, pass, ok := r.BasicAuth() + require.True(t, ok) + require.Equal(t, "password-canary", pass) + require.Equal(t, "query-canary", r.URL.Query().Get("token")) + w.WriteHeader(http.StatusForbidden) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + raw := strings.Replace(server.URL, + "http://", "http://user:password-canary@", 1) + + "/input?token=query-canary" + source := NewHTTPFileSource(raw) + require.NotContains(t, source.Description(), "canary") + _, _ = source.Bytes() + server.Close() + _, err := source.Bytes() + require.Error(t, err) + require.NotContains(t, err.Error(), "canary") +}