Skip to content

fix(database): keep IP limits when the fail2ban probe is inconclusive - #6176

Open
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-preserve-configured-iplimit
Open

fix(database): keep IP limits when the fail2ban probe is inconclusive#6176
n0ctal wants to merge 1 commit into
MHSanaei:mainfrom
n0ctal:upstream-preserve-configured-iplimit

Conversation

@n0ctal

@n0ctal n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

ResetIpLimitNoFail2ban wiped every configured limitIp whenever fail2ban-client -h failed for any reason, including a transient one, and then recorded itself so the decision was never revisited. Distinguish "fail2ban is absent" from "the probe failed".

Why

The seeder's intent is sound: with no fail2ban there is nothing to enforce limitIp, so clearing it avoids a setting that silently does nothing.

The problem is the evidence it acts on. exec.Command("fail2ban-client", "-h").Run() == nil is false both when the binary does not exist and when it exists but that one run failed — a panel started before fail2ban is up, an image where fail2ban is installed a moment later, a momentarily unavailable exec.

The consequence is not proportional to the uncertainty: it rewrites every inbound's settings JSON and every clients.limit_ip to 0, records ResetIpLimitNoFail2ban in the seeder history, and returns. There is no log line on the destructive path, and because the history row exists the seeder never looks again. Operator configuration is gone with nothing to restore it from.

Scope

  • internal/database/db.gofail2banEnforcementState() returns fail2banEnforcing / fail2banAbsent / fail2banUnknown; a missing binary (exec.LookPath) is still absent, a present-but-unrunnable one is unknown. fail2banCanEnforce() keeps its signature for existing callers.
  • The seeder skips the cleanup on fail2banUnknown, logs the probe error, and does not record itself, so the next start re-evaluates. Behaviour on absent and on enforcing is unchanged.
  • internal/database/fail2ban_state_test.go — covers all three states with a stub fail2ban-client on PATH.

Validation

  • go test ./internal/database/ -count=1 green; go build ./... and go vet clean.
  • The new subtest "present but failing" is the case that previously mapped to false and triggered the wipe.

Risk

Low, and strictly less destructive than before. A host that genuinely lacks fail2ban still gets the cleanup on the same schedule. The only new outcome is that an inconclusive probe defers instead of deleting.

@n0ctal
n0ctal force-pushed the upstream-preserve-configured-iplimit branch from be29a2c to 65e882a Compare August 5, 2026 01:20
@n0ctal
n0ctal marked this pull request as draft August 5, 2026 01:20
@n0ctal

n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Why the checks are red.

One of them was mine and is now fixed. The first push left fail2banCanEnforce in place "for existing callers" when the only call site had already been replaced, so golangci correctly reported func fail2banCanEnforce is unused. It is removed; the current head lints clean in internal/database.

What remains is not from this PR:

  • frontendnpm audit --omit=dev --audit-level=high reports a high-severity advisory in the transitive brace-expansion. The same job fails on main itself: run 30898275209 on 38838827, the commit this PR branches from.
  • golangci — the six remaining findings are all SA5011 in internal/web/service/client_paging_test.go and internal/web/service/setting_mtls_test.go, neither of which this PR touches. golangci passes on main at the same commit with the same linter (v2.12.2), so this looks like an analysis-cache difference between the push and pull_request runs rather than a real regression — worth a look on your side, but not something this branch introduced.

Local gate on the branch: go build ./..., go vet ./..., gofmt -l, golangci-lint run ./internal/database/... (0 issues) and go test ./internal/database/ -count=1 all clean.

@n0ctal
n0ctal marked this pull request as ready for review August 5, 2026 09:19
@n0ctal

n0ctal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the two remaining red checks, with what I could verify from here.

golangci — the tree is clean; this looks like a stale analysis cache, not a real finding.

I checked out upstream/main unmodified, installed the same linter CI resolves (golangci-lint v2.12.2, version: latest picks it in both runs) and ran it:

  • golangci-lint run over the whole module → 0 issues
  • golangci-lint run ./internal/web/service/..., the package CI reports SA5011 in → 0 issues

The same job is green on main at the very commit these PRs branch from, and the reported files are ones no PR here touches. The findings themselves are the t.Fatal-then-dereference shape, which staticcheck does not flag on a cold run. golangci-lint-action restores an analysis cache (Cache hit for: golangci-lint.cache-Linux-2952-…), so busting that cache is the thing I would try first — I cannot do it from a fork.

frontend — that one is real and fixable: npm audit --omit=dev --audit-level=high trips on GHSA-rgw5-rvv9-x895 via swagger-ui-react → … → brace-expansion@5.0.8. It fails on main too. Opened #6180 with a scoped override to the patched 5.0.9; merging it turns this job green for every open PR, including this one.

Nothing in either check comes from this branch.

@n0ctal
n0ctal force-pushed the upstream-preserve-configured-iplimit branch from 65e882a to 89dcc16 Compare August 6, 2026 20:56
ResetIpLimitNoFail2ban clears limitIp on every client — inbound settings JSON
and the clients table — whenever fail2banCanEnforce() returns false, then
records itself in the seeder history so it never re-evaluates. The probe was a
single `fail2ban-client -h` run, so it answered false both when fail2ban is
genuinely absent and when the command merely failed that once: a panel that
starts before fail2ban is up, or in a container where it is installed a moment
later, permanently loses every configured limit with no log line and no way
back.

Separate the two. A missing binary still means "absent" and the cleanup runs as
before; a binary that exists but will not run is reported as unknown, leaves the
configured values untouched, logs why, and does not record the seeder, so the
next start decides again.
@n0ctal
n0ctal force-pushed the upstream-preserve-configured-iplimit branch from 89dcc16 to dfd4ed3 Compare August 8, 2026 02:11
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