Skip to content

Fix misleading error message and redact credential URLs from App/PackageRepository status - #1869

Open
nikhilsagotiya wants to merge 4 commits into
carvel-dev:developfrom
nikhilsagotiya:topic/nikhilsagotiya/fix-error-message-and-secrets-redaction
Open

nikhilsagotiya wants to merge 4 commits into
carvel-dev:developfrom
nikhilsagotiya:topic/nikhilsagotiya/fix-error-message-and-secrets-redaction

Conversation

@nikhilsagotiya

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Two independent, related fixes found while auditing user-facing error/status text in kapp-controller:

  1. Error message clarity (cli/pkg/kctrl/cmd/package/available/values_schema.go, config/values-schema.yml):

    • The values-schema.yml caCerts description had a grammar error ("trusted ca's" instead of "trusted CAs"), surfaced to users via kubectl explain and the Package/PackageInstall value schema.
    • The "unsupported type" error raised while parsing a valuesSchema properties field leaked raw Go type syntax (map[string]interface{}, json.RawMessage) at a package author working in YAML/OpenAPI, not Go. Reworded to describe the expected shape in plain terms.
  2. Redact credential-bearing URLs from .status (pkg/exec/cmd_run_result.go, pkg/app/app_reconcile.go, pkg/pkgrepository/app_reconcile.go):

    • App and PackageRepository reconciliation copies subprocess (vendir/ytt/kbld/kapp) stdout, stderr, and error text verbatim into .status fields (Fetch, Template, Deploy, Inspect, Conditions[].Message, FriendlyDescription, UsefulErrorMessage).
    • If any of those tools ever emit a credential embedded in a URL (for example a GOPROXY, git, or registry URL of the form https://user:token@host), it lands verbatim in a Kubernetes resource's status — visible via kubectl get app -o yaml and any tooling/logging that reads it.
    • Added exec.RedactSecrets, which masks the userinfo component of a URL (scheme://user:pass@host → scheme://[REDACTED]@host), and applied it at every point subprocess-derived text is written into App/PackageRepository status. The regex is greedy up to the last @ before the host so a userinfo containing a literal @ (e.g. in a password) is fully masked rather than partially.
    • No control flow, exit codes, or non-text status fields changed.

Both are pure message-text/output-sanitization changes; no behavior, API, or control-flow changes.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?

Fixed a grammar error in the `caCerts` values-schema description, clarified the error message raised when a package's `valuesSchema.properties` field has an unsupported type, and ensured credential-bearing URLs (e.g. embedded in a GOPROXY/git/registry URL) in subprocess output are redacted before being written to `App`/`PackageRepository` `.status` fields.

Additional Notes for your reviewer:

  • Verified locally: go build ./..., go vet ./..., and golangci-lint run (pinned v2.12.2, matching .github/workflows/golangci-lint.yml) are clean on all touched packages (0 issues).
  • go test ./pkg/app/... ./pkg/pkgrepository/... ./pkg/exec/... was run locally; two pre-existing failures in pkg/app/pkg/pkgrepository reproduce identically on unmodified develop in this sandbox (missing vendir/kbld binaries in $PATH) and are unrelated to this change — confirmed by stashing the diff and re-running.
  • No existing test asserts the exact old message text that changed here, so no test updates were needed.
  • The RedactSecrets masking pattern (URL userinfo) covers the credential-leak shape most relevant to this codebase (GOPROXY/git/registry URLs); it does not attempt to redact arbitrary secret shapes (API keys, tokens outside a URL) since none of the current subprocess integrations are known to emit those into status text.
Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's a link to that PR
  • Code is at least as readable and maintainable as it was before this change

Additional documentation e.g., Proposal, usage docs, etc.:


…-schema type

- config/values-schema.yml: fix "trusted ca's" -> "trusted CAs" in the
  caCerts field description, surfaced to users via kubectl explain and
  Package/PackageInstall value schemas.
- cli/pkg/kctrl/cmd/package/available/values_schema.go: reword the
  "unsupported type" error raised while parsing a valuesSchema
  'properties' field. The previous message leaked Go type syntax
  (map[string]interface{}, json.RawMessage) at a package author working
  in YAML/OpenAPI, not Go; it now describes the expected shape in plain
  terms and uses %T for the actual type.

No behavior or control-flow change, message text only.

Signed-off-by: Nikhil Sagotiya <nikhil.sagotiya@broadcom.com>
Subprocess (vendir/ytt/kbld/kapp) stdout, stderr and error text were
copied verbatim into App and PackageRepository .status fields
(Fetch, Template, Deploy, Inspect, Conditions[].Message,
FriendlyDescription, UsefulErrorMessage). If any of those tools ever
emit a credential embedded in a URL (e.g. a GOPROXY, git, or registry
URL of the form https://user:token@host), it would land verbatim in a
Kubernetes resource's status, visible via `kubectl get app -o yaml`
and any tooling that reads it.

- pkg/exec/cmd_run_result.go: add RedactSecrets, which masks the
  userinfo component of a URL (scheme://user:pass@host ->
  scheme://[REDACTED]@host). The match is greedy up to the last '@'
  before the host so a userinfo containing a literal '@' (e.g. in a
  password) is fully masked rather than partially.
- pkg/app/app_reconcile.go, pkg/pkgrepository/app_reconcile.go: apply
  RedactSecrets at every point subprocess-derived text is written into
  .status (Fetch/Template/Deploy/Inspect Stdout/Stderr/Error,
  Condition.Message, FriendlyDescription, UsefulErrorMessage).

No control flow, exit codes, or non-text fields changed.

Signed-off-by: Nikhil Sagotiya <nikhil.sagotiya@broadcom.com>

@aroradaman aroradaman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Just wondering why this doesn't require a unit/integration test change.

Add comprehensive test coverage for the new RedactSecrets function,
covering:
- Basic URL credential redaction (https, http, ftp, custom schemes)
- Edge case: passwords containing @ symbols (greedy match to last @)
- Multiple URLs in a single string
- Multiline output with embedded credentials
- Special characters in passwords
- Malformed URLs and non-URL text (unchanged)
- Empty strings

All 16 test cases pass and validate the regex pattern's correctness
and the greedy-to-last-@ behavior for passwords with @ characters.

Signed-off-by: Sameer Khan <sameer.khan@broadcom.com>
@sameerforge

Copy link
Copy Markdown
Contributor

Changes look good. Just wondering why this doesn't require a unit/integration test change.

Added 16 unit tests for RedactSecrets covering all URL patterns and edge cases; the other changes are text-only.

@sameerforge
sameerforge force-pushed the topic/nikhilsagotiya/fix-error-message-and-secrets-redaction branch from c6481d6 to ff8cbd0 Compare October 7, 2026 09:10
- Fix grammar inconsistency: "can not be empty" -> "cannot be empty" (validations.go:108)
- Replace full TokenRequest object logging with specific safe field logging (token_manager.go:127)

Signed-off-by: Sameer Khan <sameer.khan@broadcom.com>
@sameerforge
sameerforge force-pushed the topic/nikhilsagotiya/fix-error-message-and-secrets-redaction branch from ff8cbd0 to ca80848 Compare October 7, 2026 10:16

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants