Skip to content

fix(rules): floor admin-ips warnings at warn level - #111

Open
AntiD2ta wants to merge 1 commit into
masterfrom
fix-admin-ips-warning-visibility
Open

fix(rules): floor admin-ips warnings at warn level#111
AntiD2ta wants to merge 1 commit into
masterfrom
fix-admin-ips-warning-visibility

Conversation

@AntiD2ta

@AntiD2ta AntiD2ta commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Follow-up to a review finding on #108:

Log visibility depends on level. parseAdminIPs uses the module logger, so if logLevel is above Warn, misconfiguration goes completely silent. Given the fail-closed impact above, this reinforces the case for using at least warn-level (or higher) for these particular messages.

The problem

initLogging pins 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. Set log-level above warn and every warning about a malformed server.rules.admin-ips entry 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.go had to set logLevel: zerolog.WarnLevel before its assertion would hold.

The change

parseAdminIPs now floors the level of the logger it uses.

log = log.Level(min(log.GetLevel(), zerolog.WarnLevel))

zerolog.Level is an ordered int8 where 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 inside parseAdminIPs rather 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 SetGlobalLevel above 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, and docs/configuration.md now 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.go is new and unit-tests parseAdminIPs at FatalLevel and Disabled. The invalid-IP case at error level drove the first failing test and now lives in rules_test.go, which already covered that message through the public API.

In rules_test.go the three bad-entry cases move from WarnLevel to ErrorLevel, so they prove the guarantee through New() instead of avoiding it.

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.
@AntiD2ta AntiD2ta self-assigned this Aug 20, 2026
@AntiD2ta
AntiD2ta requested a review from Bez625 August 20, 2026 16:10
@AntiD2ta
AntiD2ta marked this pull request as ready for review August 20, 2026 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant