Repository navigation
fix(output): redact sensitive fields under json/pretty formats and debug logging - #95
noamloewenstern wants to merge 10 commits into
Conversation
|
Pushed three follow-up commits after re-reviewing my own work adversarially — building each edge case as a failing test rather than reasoning about it. The PR description is updated to match. Summary of what that found: The redactor was reshaping output it only meant to pass through. By rebuilding every value it walked, it also reimplemented parts of
The fix was to stop rebuilding what doesn't need rebuilding: walk only types that can contain a
The debug-log key set had no automated link to the tags it mirrors. A One deliberate non-fix, documented in the PR body: Full suite still green except the pre-existing |
|
Any update? |
JSONFormatter and PrettyFormatter marshaled structs directly, ignoring the `sensitive:"true"` struct tag that TableFormatter already honors. Any command using --format json/pretty printed real secrets (instance tokens, SSH private keys, webhook secrets, DB env values, etc.) regardless of --show-sensitive. Add a shared reflection-based redactor (internal/output/redact.go) that deep-walks structs, pointers, slices, and maps, replacing sensitive fields with SensitiveOverlay unless ShowSensitive is set, and wire it into both formatters. TableFormatter's own per-field check now reuses the same isSensitiveField helper instead of duplicating the tag lookup. Struct output is rendered through an order-preserving map so existing JSON key ordering is unaffected.
The debug HTTP logger only masked the literal JSON key "token" before printing request/response bodies to stderr. Every other sensitive field (SSH private/public keys, webhook secrets, OAuth client secrets, DB/service env values, server IP/user, etc.) was printed in full whenever --debug was used, independent of --format or --show-sensitive. Extend the key set to cover every JSON field name backing a `sensitive:"true"` struct tag in internal/models and internal/config. This layer decodes arbitrary JSON (request bodies can be untyped maps, not just tagged structs), so it can't walk Go struct tags like internal/output's formatters do — a name-based set is the equivalent for that shape.
…leak `context get` renders the real config.Instance (including its token), unlike `context list` which already had a narrow point-fix stripping Token entirely. It was still fully exposed under --format json/pretty until the internal/output redaction fix landed. Add a regression test so this specific command stays covered.
The reflection-based redactor rebuilt every value it walked, so it also reimplemented parts of encoding/json it never meant to change. Four divergences, each reproduced against json.Marshal of the same value: - []byte rendered as an array of numbers instead of base64. - Maps rendered through the insertion-ordered map used for structs, which exposed Go's randomized map iteration order where encoding/json sorts keys. Live on `server provider <p> regions --format json`, whose models.ProviderOption is a map[string]any: key order changed per run. - Embedded struct fields were nested under the embedded type's name instead of promoted into the parent object, or dropped entirely when the embedded type is unexported. - A type implementing json.Marshaler short-circuited the walk, so a sensitive field reachable only through it was printed in full. - A self-referential value recursed until the goroutine stack overflowed, which is fatal and unrecoverable, where encoding/json returns an error. Fix the shape by not rebuilding what doesn't need rebuilding: walk only types that can contain a `sensitive:"true"` field (containsSensitive, memoized) and hand everything else back untouched for encoding/json to marshal natively. That makes non-sensitive output identical by construction rather than by reimplementation, and it removes the need for the json.Marshaler special case, since types with no sensitive fields are already passed through. Within a walked subtree: promote embedded struct fields, render maps as plain maps so encoding/json sorts their keys, redact through a custom MarshalJSON rather than deferring to it, and guard pointer cycles. Note that SensitiveOverlay is a string, so a redacted non-string field (models.Server.Port) renders as a JSON string. Documented on redactSensitive; --show-sensitive returns the correctly typed value.
models.Database carries ten password fields (postgres, mysql, mariadb,
mongo, redis, keydb, clickhouse, dragonfly) that had `table:"-"` but no
`sensitive:"true"`. The tag-driven redaction added for json/pretty output
cannot see an untagged field, so `database get`/`database list` printed
them in plaintext under --format json and --format pretty:
{"uuid":"...","name":"db","postgres_password":"<real password>"}
They were likewise absent from the debug logger's key set, so --debug
printed them too. Tag all ten and add their JSON names to
sensitiveDebugLogKeys.
Only the Database model is tagged here; the create/update request types
carry the same field names and are covered on the debug path by the key
set, which is the only path that renders them.
…y set sensitiveDebugLogKeys is a hand-maintained list of JSON field names with no automated link to the `sensitive:"true"` tags it mirrors. The debug logger works on untyped JSON bodies, so it cannot discover new tags on its own: a tag added to a model next month would silently keep being logged in full, with nothing failing. Parse internal/models and internal/config, collect every tagged field's JSON name, and assert the set covers them. Verified by deleting one key and watching it name the exact field: Database.RedisPassword is tagged sensitive:"true" but its JSON name "redis_password" is missing from sensitiveDebugLogKeys in client.go
…debug server registry list masked usernames only in the table path (ServerRegistryRows). For json/pretty the raw []ServerRegistry went to the formatter, so the username printed in full regardless of --show-sensitive. Tag the field sensitive:"true" and add username to the debug-log key set so the tag/key-set consistency test keeps passing.
Tag-driven redaction cannot see secrets that arrive as map[string]any
(notification, instance email and server subsystem settings, audit
metadata/changes) or are typed `any`, so json/pretty output and --debug
bodies printed them in full.
- add internal/redact.IsCredentialKey, a name matcher for untyped data
- json/pretty: mask non-empty string values under credential-named keys
inside walked maps
- --debug: apply the same matcher on top of the tagged-field key set, so
`notification update --json '{"telegram_token":...}'` no longer logs it
- keep absent sensitive pointers as null instead of "********" so an
unset credential is not reported as set
- tag AuditEvent.ActorEmail and IPAddress like Team.Email and Server.IP
- README: json/pretty now mask audit actor email/IP and registry usernames
without -s; --debug always masks
Replace the deprecated parser.ParseDir in the debug key consistency test and use assert.Contains in the context get test. The key-order assertion in redact_test keeps assert.Equal on purpose: order is part of the contract.
--debug now masks credentials only: tokens, passwords, secrets, private keys and env var values. IP, user, port, email, username, env var names, public keys and scripts stay readable, since masking them made server and env troubleshooting hard without hiding anything secret. The key consistency test now requires every `sensitive:"true"` JSON name to be in either the masked set or a new, explicit debugVisibleSensitiveKeys set. json/pretty output shows unset sensitive values (null, "", 0) as they are instead of "********", so an empty field no longer looks like a secret. redact.IsCredentialKey no longer matches public_key.
c73e3e1 to
5ef6292
Compare
|
Rebased onto the current The only conflict was in the debug logger, where
Notification, email settings and server subsystem commands print
Unset values ( It merges cleanly now. If it's too much to review in one go I can split it. The smallest piece is the |
5ef6292 to
d410a9a
Compare
Fixes #94.
Only the table formatter reads the
sensitive:"true"tag. json and pretty marshal straight throughencoding/json, socontext get --format jsonprints the stored API token even without-s. Private keys, webhook secrets, OAuth client secrets, env var values and database passwords come out the same way.--debugmasks the keystokenandpasswordin request and response logs and nothing else.What I changed
json and pretty output now swap tagged fields for
********unless you pass-s. The code is ininternal/output/redact.go. It only walks types that can hold a tagged field and hands everything else toencoding/jsonuntouched, so other output stays byte-for-byte the same. Unset values (null,"",0) print as they are, so an empty field doesn't look like a secret.Tags don't help with
map[string]any, and a few commands print those: notification settings, instance email settings, server log drain and sentinel settings, and auditmetadataandchanges.internal/redact.IsCredentialKeyhandles them by key name. It masks non-empty string values under keys liketelegram_token,smtp_password,resend_api_keyordiscord_webhook_url, and skips keys that only point at a credential (*_uuid,*_id,*_name,*_enabled) as well aspublic_key.--debugnow masks credentials in request and response bodies: tokens, passwords, secrets, private keys, env var values and the database passwords. IPs, users, ports, emails, usernames, env var names and public keys stay readable, because masking them made server and env debugging harder and hid nothing secret. This is always on, with or without-s.The debug logger sees decoded JSON and can't read tags. So
internal/api/sensitive_keys_test.goparsesinternal/modelsandinternal/configand fails if a tagged JSON name is in neithersensitiveDebugLogKeys(masked) nordebugVisibleSensitiveKeys(shown on purpose). A new tag can't slip through unnoticed.On the models: the ten password fields on
models.Databaseare tagged now.ServerRegistry.Usernameis tagged too, sincemainmasks it only in the table rows and json/pretty printed it.AuditEvent.ActorEmailandAuditEvent.IPAddressare tagged likeTeam.EmailandServer.IP. The README says the same.Worth knowing
models.Server.Portshows as the string"********"under json and pretty, because the overlay is a string.-sgives the number back.cmd/cloudtoken,cmd/s3andcmd/cloudinithave their own redaction, which is redundant now. I left it alone to keep the diff smaller.messageorerrorkey, and deployment logs, which don't go throughinternal/output.Testing
go build ./...,go vet ./...andgolangci-lintv2.11.4 (the CI pin) are clean, apart fromgcicomplaints caused by CRLF line endings on my Windows checkout.go test ./...passes exceptTestFirewallServiceUnit_GoldenFixture_TwoNamespaces, which also fails onmainhere (CRLF in the golden fixture).mainonly setHOME, but the CLI reads%APPDATA%on Windows. I used a local shim for that and didn't commit it.internal/redact100%,internal/output85.6%,internal/api84.3%.go test -racelocally (no cgo), so CI will be its first run.redact_test.go,redact_untyped_test.go,keys_test.go,sensitive_keys_test.go,client_test.go,cmd/context/get_test.goandcmd/server/registry_test.go. Each fails without its fix.