From 4b4b713539fa1acd7d7c767aacad8258cf8943e5 Mon Sep 17 00:00:00 2001 From: Michael Reber Date: Mon, 27 Jul 2026 09:04:40 +0200 Subject: [PATCH 1/2] Fixed parser problem where the new summary could not parse correctly when the fail2ban service prints config warnings to stderr --- internal/fail2ban/connector_global.go | 30 +++++++++++++++++++++++++-- internal/fail2ban/connector_local.go | 19 ++++++++++++++--- internal/fail2ban/connector_ssh.go | 2 +- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/internal/fail2ban/connector_global.go b/internal/fail2ban/connector_global.go index 1b74665..2a8df60 100644 --- a/internal/fail2ban/connector_global.go +++ b/internal/fail2ban/connector_global.go @@ -55,11 +55,37 @@ const managedJailLocalMarker = "ui-custom-action" // Shared fail2ban-client Output Parsers // ========================================================================= +// Locates the banned payload line. fail2ban prints the whole repr as one line on stdout, but a fail2ban.conf with 'logtarget = STDOUT' makes the +// client write its own log lines to stdout first, so skip anything ahead of the first line that starts the list. +func extractBannedPayload(out string) (string, bool) { + for _, line := range strings.Split(out, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "[") { + return trimmed, true + } + } + return "", false +} + +// Appends the minimum-version hint only when fail2ban actually rejected the command +func bannedVersionHint(out string) string { + lower := strings.ToLower(out) + if strings.Contains(lower, "not supported") || strings.Contains(lower, "invalid command") || strings.Contains(lower, "usage:") { + return " (the `banned` command needs fail2ban 0.11 or newer)" + } + return "" +} + func parseBannedJails(out string) ([]JailInfo, error) { - s := &reprScanner{in: strings.TrimSpace(out)} - if s.rest() == "" { + trimmed := strings.TrimSpace(out) + if trimmed == "" { return nil, fmt.Errorf("empty output from `fail2ban-client banned`") } + payload, found := extractBannedPayload(trimmed) + if !found { + return nil, fmt.Errorf("unexpected output from `fail2ban-client banned`: %s%s", truncateForError(trimmed), bannedVersionHint(trimmed)) + } + s := &reprScanner{in: payload} if !s.accept('[') { return nil, fmt.Errorf("unexpected output from `fail2ban-client banned`: %s", truncateForError(s.in)) } diff --git a/internal/fail2ban/connector_local.go b/internal/fail2ban/connector_local.go index ce3cd36..6f631e8 100644 --- a/internal/fail2ban/connector_local.go +++ b/internal/fail2ban/connector_local.go @@ -17,6 +17,7 @@ package fail2ban import ( + "bytes" "context" "errors" "fmt" @@ -70,7 +71,7 @@ func (lc *LocalConnector) GetJailSummary(ctx context.Context) (*JailSummary, err } infos, err := parseBannedJails(out) if err != nil { - return nil, fmt.Errorf("%w (fail2ban 0.11 or newer is required)", err) + return nil, err } exists, managed, err := lc.CheckJailLocalIntegrity(ctx) if err != nil { @@ -124,6 +125,9 @@ func (lc *LocalConnector) BanIP(ctx context.Context, jail, ip string) error { func (lc *LocalConnector) Reload(ctx context.Context) error { out, err := lc.runFail2banClient(ctx, "reload") if err != nil { + if strings.Contains(err.Error(), "Found no accessible config files") { + return fmt.Errorf("fail2ban reload error: %w - fail2ban-ui cannot see the complete fail2ban configuration: when sharing a socket with a fail2ban container, /etc/fail2ban inside the fail2ban-ui container must contain the full configuration tree (including fail2ban.conf and jail.conf), not only the custom jail/filter files", err) + } return fmt.Errorf("fail2ban reload error: %w (output: %s)", err, strings.TrimSpace(out)) } return checkReloadOutput(out) @@ -171,8 +175,17 @@ func (lc *LocalConnector) SetFilterConfig(ctx context.Context, jail, content str func (lc *LocalConnector) runFail2banClient(ctx context.Context, args ...string) (string, error) { cmd := exec.CommandContext(ctx, "fail2ban-client", fail2banArgs(lc.server.SocketPath, args...)...) - out, err := cmd.CombinedOutput() - return string(out), err + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + runErr := cmd.Run() + output, err := selectCommandOutput(stdout.String(), stderr.String(), runErr) + if err == nil { + if s := strings.TrimSpace(stderr.String()); s != "" { + debugf("fail2ban-client stderr ignored [%s]: %s", lc.server.Name, truncateForLog(s, maxLoggedOutputBytes)) + } + } + return output, err } func (lc *LocalConnector) checkFail2banHealthy(ctx context.Context) error { diff --git a/internal/fail2ban/connector_ssh.go b/internal/fail2ban/connector_ssh.go index 967547c..7ebf639 100644 --- a/internal/fail2ban/connector_ssh.go +++ b/internal/fail2ban/connector_ssh.go @@ -136,7 +136,7 @@ func (sc *SSHConnector) GetJailSummary(ctx context.Context) (*JailSummary, error } infos, err := parseBannedJails(bannedOut) if err != nil { - return nil, fmt.Errorf("%s: %w (fail2ban 0.11 or newer is required)", sc.server.Name, err) + return nil, fmt.Errorf("%s: %w", sc.server.Name, err) } return &JailSummary{ Jails: infos, From 2727c3935b4bf512d97c2cd72a95e7c60c11a436 Mon Sep 17 00:00:00 2001 From: Michael Reber Date: Mon, 27 Jul 2026 09:09:47 +0200 Subject: [PATCH 2/2] Write test-file for the local connector and update parsing tests if a fail2ban server mixes 'banned'-output with stderr --- internal/fail2ban/banned_parser_test.go | 35 +++++ internal/fail2ban/connector_local_test.go | 128 +++++++++++++++++++ internal/fail2ban/connector_ssh_exec_test.go | 12 +- 3 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 internal/fail2ban/connector_local_test.go diff --git a/internal/fail2ban/banned_parser_test.go b/internal/fail2ban/banned_parser_test.go index 4dd9072..b87a0d5 100644 --- a/internal/fail2ban/banned_parser_test.go +++ b/internal/fail2ban/banned_parser_test.go @@ -151,6 +151,41 @@ func TestParseBannedJails(t *testing.T) { } }) + // A fail2ban.conf with 'logtarget = STDOUT' makes the client print its log lines to stdout ahead of the payload -> the parser must skip them. + t.Run("log noise ahead of the payload is skipped", func(t *testing.T) { + in := "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR Found no accessible config files for 'fail2ban' under /etc/fail2ban\n" + + "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR No section: 'Definition'\n" + + "[{'sshd': ['1.2.3.4']}]\n" + infos, err := parseBannedJails(in) + if err != nil { + t.Fatalf("leading log lines must not break parsing: %v", err) + } + if len(infos) != 1 || infos[0].JailName != "sshd" || infos[0].BannedIPs[0] != "1.2.3.4" { + t.Fatalf("unexpected result: %+v", infos) + } + }) + + t.Run("noise without any payload is an error", func(t *testing.T) { + in := "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR Found no accessible config files\n" + if _, err := parseBannedJails(in); err == nil { + t.Fatal("expected an error when no payload line exists") + } + }) + + t.Run("version hint only for actual command rejection", func(t *testing.T) { + _, err := parseBannedJails("Sorry but the command 'banned' is not supported") + if err == nil || !strings.Contains(err.Error(), "0.11") { + t.Fatalf("unsupported-command output must carry the version hint, got: %v", err) + } + _, err = parseBannedJails("2026-07-26 18:20:56 fail2ban.configreader [18]: ERROR Found no accessible config files") + if err == nil { + t.Fatal("expected an error") + } + if strings.Contains(err.Error(), "0.11") { + t.Fatalf("config noise is not a version problem; hint must be absent, got: %v", err) + } + }) + t.Run("parse errors stay readable for huge payloads", func(t *testing.T) { _, err := parseBannedJails(strings.Repeat("x", 5000)) if err == nil { diff --git a/internal/fail2ban/connector_local_test.go b/internal/fail2ban/connector_local_test.go new file mode 100644 index 0000000..df48472 --- /dev/null +++ b/internal/fail2ban/connector_local_test.go @@ -0,0 +1,128 @@ +// Fail2ban UI - A Swiss made, management interface for Fail2ban. +// +// Copyright (C) 2026 Swissmakers GmbH (https://swissmakers.ch) +// +// Licensed under the GNU Affero General Public License, Version 3 (AGPL-3.0) +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.gnu.org/licenses/agpl-3.0.en.html +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fail2ban + +import ( + "context" + "strings" + "testing" + + "github.com/swissmakers/fail2ban-ui/internal/shared" +) + +const testNoiceString = `echo "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR Found no accessible config files for 'fail2ban' under /etc/fail2ban" >&2 +echo "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR No section: 'Definition'" >&2 +` + +func testLocalConnector(t *testing.T) *LocalConnector { + t.Helper() + return NewLocalConnector(shared.Fail2banServer{ + Name: "local-test", + Type: "local", + ConfigPath: t.TempDir(), + }) +} + +func TestLocalGetJailSummarySurvivesStderrNoise(t *testing.T) { + withFakeBinary(t, "fail2ban-client", testNoiceString+`echo "[{'sshd': ['1.2.3.4', '5.6.7.8']}, {'nginx': []}]" +exit 0 +`) + lc := testLocalConnector(t) + + summary, err := lc.GetJailSummary(context.Background()) + if err != nil { + t.Fatalf("stderr noise must not break the summary (issue #186): %v", err) + } + if len(summary.Jails) != 2 { + t.Fatalf("expected 2 jails, got %+v", summary.Jails) + } + byName := map[string]JailInfo{} + for _, j := range summary.Jails { + byName[j.JailName] = j + } + if got := byName["sshd"]; got.TotalBanned != 2 || got.BannedIPs[1] != "5.6.7.8" { + t.Fatalf("sshd wrong: %+v", got) + } + if got := byName["nginx"]; got.TotalBanned != 0 { + t.Fatalf("nginx wrong: %+v", got) + } +} + +func TestLocalGetBannedIPsSurvivesStderrNoise(t *testing.T) { + withFakeBinary(t, "fail2ban-client", testNoiceString+`echo "1.2.3.4 5.6.7.8" +exit 0 +`) + lc := testLocalConnector(t) + + ips, err := lc.GetBannedIPs(context.Background(), "sshd") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(ips) != 2 || ips[0] != "1.2.3.4" || ips[1] != "5.6.7.8" { + t.Fatalf("stderr noise leaked into the IP list: %v", ips) + } +} + +func TestLocalCommandFailureKeepsBothStreams(t *testing.T) { + withFakeBinary(t, "fail2ban-client", `echo "partial stdout" +echo "stderr detail" >&2 +exit 255 +`) + lc := testLocalConnector(t) + + _, err := lc.GetJailSummary(context.Background()) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "partial stdout") || !strings.Contains(err.Error(), "stderr detail") { + t.Fatalf("failure diagnostics must carry both streams, got: %v", err) + } +} + +func TestLocalReloadIncompleteMountHint(t *testing.T) { + withFakeBinary(t, "fail2ban-client", `echo "2026-07-26 18:20:56,221 fail2ban.configreader [18]: ERROR Found no accessible config files for 'fail2ban' under /etc/fail2ban" >&2 +exit 255 +`) + lc := testLocalConnector(t) + + err := lc.Reload(context.Background()) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "full configuration tree") { + t.Fatalf("expected the incomplete-mount hint, got: %v", err) + } + // The auto-disable ladder in handlers greps for "output:" in the error. + if !strings.Contains(err.Error(), "output:") { + t.Fatalf("reload errors must keep the output: marker for the recovery ladder, got: %v", err) + } +} + +func TestLocalReloadPlainFailureHasNoMountHint(t *testing.T) { + withFakeBinary(t, "fail2ban-client", `echo "ERROR NOK: something else" >&2 +exit 255 +`) + lc := testLocalConnector(t) + + err := lc.Reload(context.Background()) + if err == nil { + t.Fatal("expected an error") + } + if strings.Contains(err.Error(), "full configuration tree") { + t.Fatalf("mount hint must only fire for the config-visibility case, got: %v", err) + } +} diff --git a/internal/fail2ban/connector_ssh_exec_test.go b/internal/fail2ban/connector_ssh_exec_test.go index b0b49e5..0cb875c 100644 --- a/internal/fail2ban/connector_ssh_exec_test.go +++ b/internal/fail2ban/connector_ssh_exec_test.go @@ -27,18 +27,22 @@ import ( "github.com/swissmakers/fail2ban-ui/internal/shared" ) -// The following tests run against a fake ssh rather than a real host -func withFakeSSH(t *testing.T, body string) { +func withFakeBinary(t *testing.T, name, body string) { t.Helper() dir := t.TempDir() script := "#!/bin/sh\n" + body - path := filepath.Join(dir, "ssh") + path := filepath.Join(dir, name) if err := os.WriteFile(path, []byte(script), 0o700); err != nil { - t.Fatalf("failed to write fake ssh: %v", err) + t.Fatalf("failed to write fake %s: %v", name, err) } t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) } +func withFakeSSH(t *testing.T, body string) { + t.Helper() + withFakeBinary(t, "ssh", body) +} + func testSSHConnector() *SSHConnector { return &SSHConnector{ server: shared.Fail2banServer{Name: "test", Host: "10.0.0.1", SSHUser: "f2b"},