fix(rules): floor admin-ips warnings at warn level - #111
Open
AntiD2ta wants to merge 1 commit into
Open
Conversation
parseAdminIPs logs malformed server.rules.admin-ips entries with the rules module logger, so configuring that module above warn hid the warnings entirely. A dropped entry narrows the set of addresses trusted for voluntary exits, which fails closed but silently: the operator sees a clean startup while their allow-list has shrunk. Floor the level of the logger used for those messages at warn, leaving a more verbose setting untouched. Note that this covers the module log level only; zerolog also gates on the global level, which the server pins to trace but the slashing protection export/import commands disable. Follow-up to a review comment on #108.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to a review finding on #108:
The problem
initLoggingpins zerolog's global level to trace so that per-module levels mean something. That leaves the rules module's own level as the only gate on these messages. Setlog-levelabove warn and every warning about a malformedserver.rules.admin-ipsentry disappears.A dropped entry can only narrow the set of addresses trusted for voluntary exits, so it fails closed. It also fails silently. The operator reads a clean startup log while the allow-list has shrunk under them.
The tests on master already showed this. Every bad-entry case in
rules_test.gohad to setlogLevel: zerolog.WarnLevelbefore its assertion would hold.The change
parseAdminIPsnow floors the level of the logger it uses.zerolog.Levelis an orderedint8where lower means more verbose, so this leaves a caller configured for debug or info alone and only lifts error, fatal and disabled up to warn. I put it insideparseAdminIPsrather than at the call site so no future caller has to remember the rule.What the guarantee covers
The module log level, and nothing beyond it. zerolog checks the per-logger level and the process global level, so
SetGlobalLevelabove warn still drops these entries. The slashing protection export and import commands do exactly that to keep their JSON output clean. Neither serves signing requests, so nothing is weakened there, anddocs/configuration.mdnow says where the guarantee stops rather than implying it has no limit.The floor also overrides a module level of
disabled. I think that is the right call. A misconfigured security control should not be silenceable with a verbosity knob. The override stays narrow: warn level, startup only, one line per malformed entry, and nothing at all when the configuration is correct.Tests
rules/standard/adminips_internal_test.gois new and unit-testsparseAdminIPsatFatalLevelandDisabled. The invalid-IP case at error level drove the first failing test and now lives inrules_test.go, which already covered that message through the public API.In
rules_test.gothe three bad-entry cases move fromWarnLeveltoErrorLevel, so they prove the guarantee throughNew()instead of avoiding it.