Fix dead securityLevelGRPC settings.conf key - #1674
Conversation
settings.conf shipped securityLevelGRPC, but the loader reads security_level_grpc (settings/settings.go) and the struct tag is key:"security_level_grpc" (settings/interface.go). The line was harmless at its default of 0, since the real default is also 0, but an operator who edited it to 1 to enable TLS on inter-service gRPC got silent plaintext: util/grpc_helper.go GetGRPCClient() only ever saw the unread default and picked insecure.NewCredentials(). The likely cause is that the sibling setting genuinely is camelCase - SecurityLevelHTTP carries key:"securityLevelHTTP" - so securityLevelGRPC looked correct by analogy. That broader camelCase/snake_case inconsistency across the settings package is out of scope here and remains a follow-up. Also corrects the same wrong spelling in operator-facing recommendation text (cmd/diagnose/config_checks.go, util/admin_api_key.go) and docs (docs/howto/diagnose.md, docs/references/services/rpc_reference.md), all of which told operators to set the dead key. Adds TestSettingsConfNoShadowedKeys, a regression test that fails any settings.conf key whose case/underscore-normalized form collides with a real key tag while the literal key does not - the exact failure mode that let this typo through silently. Six normalized collisions already present in settings.conf are exempted with reasoning; two of them (kafka_unitTest and coinbase_Store) look like the same bug class but are left for the settings owner to triage separately, to keep this change scoped to securityLevelGRPC.
|
🤖 Claude Code Review Status: Complete Current Review: No issues found. This is a clean, correct fix. Verified the core claim against the code: the loader reads Confirmed no stray occurrences of the old spelling remain outside the intentional doc comments in the new test. The operator-facing text corrections ( The regression test ( |
|
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-08-31 13:10 UTC |
ctnguyen
left a comment
ctnguyen
left a comment
There was a problem hiding this comment.
Review at commit bd6376f
The core change is plainly right and unusually well evidenced. settings/interface.go:59 carries key:"security_level_grpc", settings/settings.go reads that string, and gocore's lookup is exact — os.LookupEnv(key) then a case-sensitive map probe in findValue — so the old settings.conf line really was inert, and the "typed by analogy to the genuinely-camelCase securityLevelHTTP" diagnosis is convincing. The blast radius is correctly identified too: git grep over tracked files shows the old spelling now survives only in the new test's own explanatory comments, and every operator-facing string that told people to set the dead key is caught. The regression test targets the right signature — normalized collision without exact match — rather than reaching for a blanket "every key resolves" sweep it would have had to exempt half of settings.conf to pass; the six exemptions are individually reasoned and the two that admit to being the same bug class are honestly labelled as deferred rather than quietly dropped. I traced the two COINBASE_GRPC_ADDRESS claims and both halves hold: nothing in Go reads that literal key, and since gocore looks up the requested key name, the docker-compose-3blasters.yml env var never reaches coinbase_grpcAddress. Adding a base assignment equal to the struct default also can't shadow anything — env still wins, and findValue walks context suffixes before falling back to base, so the only observable change is the reported source in the settings dump.
Nothing here should hold up the merge. Both notes below are about the neighbourhood the fix lands in: now that the key resolves and the diagnose output names it correctly, operators are much more likely to actually turn gRPC TLS on, and the two places that meet them when they do are a little out of step with the rest of the codebase.
Non-blocking issues
ChiR1 — Diagnose cert-file check gates only on HTTP, so a gRPC TLS misconfiguration passes
Problem: cmd/diagnose/config_checks.go:237 guards the cert-file check with s.SecurityLevelHTTP > 0 && (s.ServerCertFile == "" || s.ServerKeyFile == "") — SecurityLevelGRPC is not consulted. But util/grpc.go:48-57 returns a ConfigurationError (server_certFile is required for security level %d) whenever SecurityLevelGRPC > 0 and the cert or key path is empty, so every gRPC service refuses to start. The docs table this commit touched describes the check generically — docs/howto/diagnose.md:180 reads TLS cert files | TLS enabled but server_certFile or server_keyFile empty | ERROR — which overstates what it covers. This path was always possible to reach, since gocore layers settings_local.conf and environment overrides ahead of the struct-tag default; what this commit changes is that the shipped base config now exposes the key and the diagnose recommendation names it correctly, so an operator is far more likely to walk into it.
Why it matters: An operator who acts on the gRPC TLS recommendation without cert paths configured gets a green gRPC TLS row from diagnose and then a node that will not come up. Catching exactly that before a restart is what the config checks are for.
Fix: Include the gRPC level in the condition:
if (s.SecurityLevelHTTP > 0 || s.SecurityLevelGRPC > 0) && (s.ServerCertFile == "" || s.ServerKeyFile == "") {Both server paths read the same ServerCertFile / ServerKeyFile, so one combined check is sufficient.
ChiR2 — Diagnose reports level 1 as OK while the same commit warns about it elsewhere
Problem: cmd/diagnose/config_checks.go:213-219 reports any non-zero SecurityLevelGRPC as SeverityOK with the value level 1 and no caveat. On that same configuration, util/admin_api_key.go:76 — changed in this commit — warns that security_level_grpc=1 "does not provide verified transport security, so the admin key can be harvested in transit". So two parts of the node disagree about whether the operator is in good shape. The recommendation strings show the same split: config_checks.go:211 now says Set security_level_grpc >= 1 for production, while the two other strings this commit touched say >= 2 (util/admin_api_key.go:76 and docs/references/services/rpc_reference.md:435). Level 1 client-side is credentials.NewTLS(&tls.Config{InsecureSkipVerify: true}), which util/grpc_helper.go:389 and :426 describe as MITM-vulnerable by design.
Why it matters: Level 1 is a legitimate supported mode, so recommending it is not wrong in itself — but a green OK on it is, because it is the one signal that tells an operator to stop looking. Having the diagnostic bless a configuration the runtime warns about undercuts both messages, and since this commit is what makes all three strings take effect, it is the natural place to line them up.
Fix: Report level 1 at the same severity the runtime uses, and match the other two call sites on the recommended floor:
Recommended: "Set security_level_grpc >= 2 for production (level 1 does not verify certificates)",with the non-zero branch treating level 1 as a warning rather than SeverityOK. The sample output at docs/howto/diagnose.md:242 needs the same edit.
Recap
| ID | Description | Required | Criticality |
|---|---|---|---|
| ChiR1 | Cert check ignores gRPC level | 40% | |
| ChiR2 | Level 1 reported as OK | 30% |
Problem
settings.confshippedsecurityLevelGRPC = 0. Nothing reads that key. The loaderreads
security_level_grpc(settings/settings.go), and the struct tag iskey:"security_level_grpc"(settings/interface.go). The committed line is dead.It is harmless at its current value because the real default is also
0- but anoperator who edits it to
1expecting to enable TLS on inter-service gRPC getssilent plaintext instead:
util/grpc_helper.goGetGRPCClient()never sees thechange and keeps picking
insecure.NewCredentials().The likely cause: the sibling setting genuinely is camelCase -
SecurityLevelHTTPcarries
key:"securityLevelHTTP"(settings/interface.go) - sosecurityLevelGRPClooked correct by analogy.
securityLevelHTTPitself is untouched by this PR; thebroader camelCase/snake_case inconsistency across the settings package is a
separate, larger question and remains a follow-up.
Fix
settings.conf:securityLevelGRPC->security_level_grpc, value and commentsunchanged.
told operators to set the dead key - corrected in
cmd/diagnose/config_checks.go,util/admin_api_key.go,docs/howto/diagnose.md, anddocs/references/services/rpc_reference.md.deploy/,kubernetes/,compose/, and every.conf/.yml/.yaml/.envfile in the repo for other occurrences of the wrong spelling (including
context-suffixed variants like
securityLevelGRPC.docker) - found none beyond theones listed above.
Regression test
Added
TestSettingsConfNoShadowedKeys(settings/settings_conf_dead_key_test.go),which fails on any
settings.confkey whose case/underscore-normalized formcollides with a real
key:tag while the literal key does not match it exactly -the precise failure mode that let
securityLevelGRPCthrough silently, sincegocore's config lookup is exact-case and exact-string.
This is deliberately narrower than a full "every settings.conf key resolves to
something real" sweep.
TestSettingsConfHasNoDeadKeys(added onmainseparately)already covers a related but different case - asserting that two specific,
already-removed keys stay removed - and does not attempt the full sweep either,
since most unresolved base keys are legitimate (interpolation
${VAR}targets,startXxxbootstrap flags, keys read outside the reflection-tagged struct). Thistest targets only the mechanically-detectable signature of a copy-paste-by-analogy
typo.
Six existing normalized collisions are exempted in the test with individual
reasoning. Four are genuinely distinct ALL_CAPS interpolation constants
(
P2P_PORT,P2P_PORT_COINBASE- confirmed referenced via${...}elsewhere insettings.conf;
LOG_LEVEL,COINBASE_GRPC_ADDRESS- same naming block, noconfirmed Go reader). The other two look like the same bug class as
securityLevelGRPCbut are left for the settings owner to triage rather than fixedhere, to keep this change scoped:
kafka_unitTest(settings.conf, builds a full Kafka URL) is never read by any Gocode; the real setting is the ALL_CAPS
KAFKA_UNITTEST, a bare topic-name string.coinbase_Store.dockeris a lone context-only override with no base assignment,so it cannot resolve to anything regardless; the real key is
coinbase_store.Verification
go build ./...go vet ./settings/... ./util/...go test ./settings/... -racego test ./util/ -race(admin API key + gRPC TLS/security-level tests)golangci-lint run/staticcheckon the touched packages - no new issues (threepre-existing
preallocwarnings confirmed present onmainbefore this change)settings.conffixtures so gocore's config singleton can't carry state between runs:
security_level_grpc = 1->NewSettings().SecurityLevelGRPC == 1securityLevelGRPC = 1(old spelling) ->NewSettings().SecurityLevelGRPC == 0TestSettingsConfNoShadowedKeysfails andnames the dead key, then reverted.