fix(security): redact credentials in Settings and aerospike debug logs#927
Conversation
|
🤖 Claude Code Review Status: Complete SummaryThis PR implements credential redaction for Settings logging and Aerospike connection URLs to address CodeQL alerts #7 and #39. The implementation is well-structured with good test coverage. FindingsNo critical issues found. The implementation follows security best practices:
Design ObservationsReflection Trade-offs (Documented) The PR description correctly notes that the JSON round-trip clone does not preserve non-JSON-marshalable fields (*chaincfg.Params internals, function pointers, channels). The comment at settings/redact.go:16-22 documents this clearly. This is acceptable since:
Test Coverage Strong regression protection:
Implementation Quality
Code References
The PR is ready for merge. |
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-05-26 13:47 UTC |
ordishs
left a comment
There was a problem hiding this comment.
Approve. Solid security fix — tag-driven design, good regression tests, clean commit history. A few suggestions:
Please fix before merge
1. Godoc misattribution in util/aerospike.go
The two new helpers are inserted between GetAerospikeClient's doc comment and its func line:
// GetAerospikeClient creates or retrieves a cached Aerospike client for the given URL.
// It configures connection policies, authentication, and connection pooling based on settings.
// Returns a thread-safe client instance that can be shared across goroutines.
// redactURL returns the URL string with the userinfo password component
// masked. It does not mutate the input. A nil input returns ""<nil>"".
func redactURL(u *url.URL) string { ... }
...
func GetAerospikeClient(...) { ... } // ← now undocumentedGo treats contiguous // lines as one comment block, so the doc rebinds to redactURL and the exported GetAerospikeClient loses its godoc (lint will flag). Move redactURL / aerospikePolicySummary below the existing functions, or above the GetAerospikeClient doc with a blank line separator.
Please track as follow-up
2. *url.URL fields with user:pass@ still leak via PrintSettings
Redact() only handles tagged scalars. Several settings hold *url.URL whose documented values include credentials:
Coinbase.Store→postgres://user:pass@host:port/dbBlockChain.StoreURL→postgres://user:pass@localhost:5432/blockchainAlert.StoreURL,BlockPersister.Store, etc.
After this PR, JSON-marshaling these still emits credentials in cleartext from PrintSettings. The Aerospike URL is fixed; PostgreSQL URLs in Settings are not. Tagging them redact:""true"" would not help — zeroSecret would clobber the whole URL, losing host/scheme/path. A typed branch in redactValue for *url.URL that calls redactURL-equivalent (keep host/scheme, mask password) would close this. Acknowledged as out-of-scope in the PR body — please file an issue so it doesn't get lost between triage sessions.
Nits (optional)
3. Redact() JSON-roundtrip silently drops chaincfg.Params function fields (Subsidy, deployment activators). Worth a sentence in the godoc warning that ""func/chan fields are not preserved"" so future readers aren't surprised by missing fields in PrintSettings output.
4. walkSensitiveTags doesn't recurse into slices/maps/interfaces. Correct today (no such patterns with secrets) but a one-line comment documenting the ""structs and pointer-to-struct only"" contract would prevent future surprise.
5. aerospikePolicySummary only prints 3 of many ClientPolicy fields vs the original %#v. Lost debug visibility for MinConnectionsPerNode, LoginTimeout, TlsConfig, etc. Consider whitelisting more non-secret fields — Password is the only one that needs hiding.
6. TestRedactPreservesNonSecretFields test name overpromises — the body only asserts the placeholder appears. Either rename to TestRedactProducesPlaceholder or add an assertion that a known non-secret value survives the roundtrip.
7. Placeholder inconsistency (""REDACTED"" for URL passwords vs ""********"" elsewhere) is fine given URL-encoding constraints, but a sentence-comment near redactURL explaining why it differs would help.
Highlights
TestSensitiveFieldsHaveRedactTagwith name-pattern enforcement + allowlist-with-rationale is the right design — exactly the kind of guardrail that prevents the next leak.TestSensitiveKeysDerivedMatchesExpectedpins the derived set to the old hardcoded list, catching accidental tag removal.TestRedactURLDoesNotMutateInputis thoughtful given the shallowclone := *ucould easily have aliased*Userinfo.- 5 logical commits, one per concern — easy to review.
LGTM modulo #1.
Adds a deep-clone redactor that walks the Settings struct via reflection
and replaces any field tagged redact:"true" with a placeholder. Tags 10
known secret fields: GRPCAdminAPIKey, Coinbase.{UserPwd, P2PPrivateKey,
WalletPrivateKey, SlackToken}, P2P.PrivateKey, Alert.P2PPrivateKey,
RPC.{RPCPass, RPCLimitPass}, BlockAssembly.MinerWalletPrivateKeys.
Prepares for CodeQL alert bsv-blockchain#7 fix.
Closes CodeQL alert bsv-blockchain#7. PrintSettings was marshaling the full Settings struct to Info logs, exposing GRPCAdminAPIKey and 9 other tagged secret fields. PrintSettings now calls settings.Redact before marshal so the log dump shows only non-secret configuration.
Closes CodeQL alert bsv-blockchain#39. The Aerospike connection URL (which may contain user:password in the userinfo) and the ClientPolicy struct (which has a Password field) were being logged at Debug level. Adds redactURL and aerospikePolicySummary helpers and rewires the log line to use them. URL password placeholder uses 'REDACTED' (no special chars) to avoid percent-encoding by url.UserPassword.
Follow-up to the previous commit. Five additional Infof log sites in getAerospikeClient log the URL (which may contain user:password userinfo) without redaction. Info logs are on by default, so the exposure here was worse than the Debug log already fixed. Sites covered: readPolicy, writePolicy, batchPolicy, queryPolicy, and base/connection policy. Each is wrapped with redactURL.
…rom tags
Two parallel sources of truth (redact tags vs the hardcoded sensitiveKeys
map in export.go) drift over time. Consolidate:
- Drop redactedPlaceholder constant in redact.go; redact and export both
now use export.go's redactedValue ("********"). One placeholder string.
- Replace the hardcoded sensitiveKeys map literal with a single derived
value: extractSensitiveKeys() walks Settings via reflection, collects
every field tagged redact:"true" alongside its key:"X" tag.
Also adds TestSensitiveFieldsHaveRedactTag — a regression guard that fails
if a new Settings field whose name matches /password|pwd|token|apikey|
secret|privatekey/ lacks the redact:"true" tag. Documented false-matches
(SecretMiningThreshold, GenesisKeys, chaincfg.Params version bytes) are
in sensitiveNameAllowlist.
And TestSensitiveKeysDerivedMatchesExpected pins the derived map to the
former hardcoded list to catch accidental tag removal.
8e6576c to
b39c42c
Compare
- Fix godoc misattribution in util/aerospike.go: redactURL and aerospikePolicySummary were inserted between GetAerospikeClient's doc comment and its func line, rebinding the doc to redactURL. Move the helpers below getAerospikeClient (and add their own godoc) so GetAerospikeClient regains its documentation. - Expand aerospikePolicySummary to whitelist 15 non-secret ClientPolicy fields (AuthMode, ClusterName, IdleTimeout, LoginTimeout, MinConnectionsPerNode, MaxErrorRate, ErrorRateWindow, LimitConnectionsToQueueSize, FailIfNotConnected, TendInterval, UseServicesAlternate, RackAware, TLS presence). Recovers debug visibility that the original Password-only redaction had stripped. - Add Redact godoc warning about JSON-roundtrip dropping non-marshalable fields (func/chan/unexported state/big.Int internals). - Add walkSensitiveTags contract comment documenting "struct + pointer- to-struct only" descent rule. - Add comment near redactURL explaining the REDACTED vs *** placeholder choice (URL-safety vs encoding by url.UserPassword). - Strengthen TestRedactPreservesNonSecretFields: now asserts non-secret sentinel values survive the redact pipeline (LogLevel, ProfilerAddr, Coinbase.ArbitraryText) and the secret sentinel does not. Note on *url.URL credentials leak (PR bsv-blockchain#927 review item 2): the JSON round-trip in Redact() is already safe because url.Userinfo has only unexported fields and marshals as {} — credentials never reach the SETTINGS JSON dump. Direct %s/%v formatting on *url.URL elsewhere is a separate audit; not addressed in this PR.
|
Pushed #1 godoc misattribution — fixed. Moved #2 #3 chaincfg.Params drop — added a godoc warning on #4 walker contract — added comment on #5 policy summary — expanded to 15 non-secret fields: AuthMode, ClusterName, Idle/Login Timeouts, MinConnectionsPerNode, MaxErrorRate, ErrorRateWindow, LimitConnectionsToQueueSize, FailIfNotConnected, TendInterval, UseServicesAlternate, RackAware, plus TLS presence flag. Recovers debug visibility the original #6 test name — #7 placeholder rationale — added comment on 1453 race tests pass post-rebase + new commit. |
Fixes CI gci linter failure on redact_test.go:170: the const block in TestRedactPreservesNonSecretFields had inconsistent = alignment that gofmt/gci flagged. Cosmetic only.
|
Summary
Closes 2 CodeQL alerts identified by the 2026-05-21 vulnerability triage as
fix_class=codeql-redact-log, plus 5 additional same-class log sites found during review:cmd/settings/Settings.go:176—PrintSettingsmarshaled the fullSettingsstruct to Info logs, exposingGRPCAdminAPIKeyand 9 other tagged secret fields.util/aerospike.go:367— the Aerospike connection URL (withuser:passworduserinfo) and theClientPolicystruct (withPasswordfield) were logged at Debug level.Approach
settings.Redact(s *Settings) (*Settings, error)that deep-clones via JSON round-trip and uses reflection to replace any field taggedredact:"true"with a placeholder.settings/:GRPCAdminAPIKey,Coinbase.{UserPwd, P2PPrivateKey, WalletPrivateKey, SlackToken},P2P.PrivateKey,Alert.P2PPrivateKey,RPC.{RPCPass, RPCLimitPass},BlockAssembly.MinerWalletPrivateKeys.PrintSettingsto callRedactbefore marshal.util.redactURLandutil.aerospikePolicySummaryhelpers; rewired the Debug log call to use them. URL password placeholder usesREDACTED(URL-safe;***would be percent-encoded byurl.UserPassword).Infoflog sites that leaked the same URL with userinfo (review catch — worse than the original Debug log, since Info is on by default):readPolicy,writePolicy,batchPolicy,queryPolicy,base/connection policyURLs all now wrapped withredactURL.redactedPlaceholderremoved,redactedValue(existing insettings/export.go) used by both consumers.sensitiveKeysmap inexport.gois now derived from struct tags viaextractSensitiveKeys()instead of being hardcoded — tags are the single source of truth.TestSensitiveFieldsHaveRedactTag— name-pattern enforcement: any Settings field whose name matches/password|pwd|token|apikey|secret|privatekey/must haveredact:"true"(or be insensitiveNameAllowlistwith a documented rationale).TestSensitiveKeysDerivedMatchesExpected— pins the derived map to the original hardcoded list; catches accidental tag removal.5 commits, one per logical change.
Test plan
go build ./...go test ./...— 8150+ tests pass; new tests insettings/redact_test.go(5 tests) andutil/aerospike_redact_test.go(9 tests)go test -race ./...go vet ./...(4 pre-existing issues intest/utils/, unchanged)golangci-lint run(1 pre-existing prealloc nit incrosscheck_test.go, none in new files)staticcheck ./...govulncheck ./...(18 pre-existing vulns; this PR does not touch deps)gosec ./...(informational, 22 pre-existing in affected pkgs)Frontend verification (npm) skipped — this PR touches zero UI files.
Out of scope
PrintSettingsoutput forSettings.ChainCfgParamsafter merge — the JSON round-trip through reflection may not preserve*chaincfg.Paramsfields containing*big.Int,*chainhash.Hash, etc. Not a security concern (those are public protocol parameters), but downstream tooling parsing the SETTINGS JSON should be checked.CodeQL alerts #7 and #39 will close automatically on the next default-branch CodeQL scan after merge.
Source:
docs/superpowers/specs/2026-05-21-vuln-triage/report.mdanddocs/superpowers/specs/2026-05-21-codeql-redact-log-design.md.