From f85060946a9c7e153196bfeb7b84323e46a22e07 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 14:09:42 +0000 Subject: [PATCH] policy/v2: bound the tests and sshTests failure report The report printed every failure with the full labels, so its size was labels times failures. Print 100 lines of 256-byte labels, then a count. --- hscontrol/policy/v2/sshtest.go | 39 ++++++++++++------- hscontrol/policy/v2/test.go | 67 ++++++++++++++++++++++++++++++-- hscontrol/policy/v2/test_test.go | 64 ++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 19 deletions(-) diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index 9cb252686..70c4b3050 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -5,7 +5,6 @@ import ( "maps" "net/netip" "slices" - "strings" "github.com/juanfont/headscale/hscontrol/types" "go4.org/netipx" @@ -43,53 +42,63 @@ type SSHPolicyTestResults struct { Results []SSHPolicyTestResult `json:"results"` } -// Errors renders the per-test failure breakdown joined by newlines. +// Errors renders the per-test failure breakdown joined by newlines, +// bounded by [testFailures]. func (r SSHPolicyTestResults) Errors() string { if r.AllPassed { return "" } - var lines []string + var failures testFailures for _, res := range r.Results { if res.Passed { continue } + src := clipTestLabel(res.Src) + for _, e := range res.Errors { - lines = append(lines, fmt.Sprintf("%s: %s", res.Src, e)) + failures.addf("%s: %s", src, clipTestLabel(e)) } for _, user := range sortedUsers(res.AcceptFail) { for _, dst := range res.AcceptFail[user] { - lines = append(lines, fmt.Sprintf( + failures.addf( "%s/%s -> %s: expected ALLOWED, got DENIED", - res.Src, displayUser(user), dst, - )) + src, clipTestLabel(displayUser(user)), clipTestLabel(dst), + ) } } for _, user := range sortedUsers(res.DenyFail) { for _, dst := range res.DenyFail[user] { - lines = append(lines, fmt.Sprintf( + failures.addf( "%s/%s -> %s: expected DENIED, got ALLOWED", - res.Src, displayUser(user), dst, - )) + src, clipTestLabel(displayUser(user)), clipTestLabel(dst), + ) } } for _, user := range sortedUsers(res.CheckFail) { for _, dst := range res.CheckFail[user] { - lines = append(lines, fmt.Sprintf( + // The reason scans AcceptOK; skip it for lines that + // are only counted. + var reason string + if !failures.full() { + reason = checkFailReason(res, user, dst) + } + + failures.addf( "%s/%s -> %s: expected ALLOWED via check, got %s", - res.Src, displayUser(user), dst, - checkFailReason(res, user, dst), - )) + src, clipTestLabel(displayUser(user)), clipTestLabel(dst), + reason, + ) } } } - return strings.Join(lines, "\n") + return failures.String() } func sortedUsers(m map[string][]string) []string { diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index e06e0b995..020f8a9e2 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -163,39 +163,98 @@ type PolicyTestResults struct { // keep the per-test detail because it is significantly more useful in // CLI / config-reload paths where the user does not have a separate // audit endpoint to consult. +// +// The breakdown is bounded by [testFailures]. func (r PolicyTestResults) Errors() string { if r.AllPassed { return "" } - var lines []string + var failures testFailures for _, res := range r.Results { if res.Passed { continue } + src := clipTestLabel(res.Src) + protoSuffix := "" if res.Proto != "" { protoSuffix = fmt.Sprintf(" (%s)", res.Proto) } for _, e := range res.Errors { - lines = append(lines, fmt.Sprintf("%s%s: %s", res.Src, protoSuffix, e)) + failures.addf("%s%s: %s", src, protoSuffix, clipTestLabel(e)) } for _, dst := range res.AcceptFail { - lines = append(lines, fmt.Sprintf("%s -> %s%s: expected ALLOWED, got DENIED", res.Src, dst, protoSuffix)) + failures.addf( + "%s -> %s%s: expected ALLOWED, got DENIED", + src, clipTestLabel(dst), protoSuffix, + ) } for _, dst := range res.DenyFail { - lines = append(lines, fmt.Sprintf("%s -> %s%s: expected DENIED, got ALLOWED", res.Src, dst, protoSuffix)) + failures.addf( + "%s -> %s%s: expected DENIED, got ALLOWED", + src, clipTestLabel(dst), protoSuffix, + ) } } + return failures.String() +} + +// The policy author sizes both the labels on a failure line and the +// number of failures, so a breakdown rendered in full grows with their +// product. It is rendered on the write path and, for a stored policy +// whose tests stopped passing, at every boot, so both factors are capped. +const ( + maxTestFailureLines = 100 + maxTestLabelBytes = 256 +) + +// testFailures collects the lines of a `tests` or `sshTests` failure +// breakdown. Failures past [maxTestFailureLines] are counted, not +// rendered. +type testFailures struct { + lines []string + omitted int +} + +func (f *testFailures) full() bool { + return len(f.lines) >= maxTestFailureLines +} + +func (f *testFailures) addf(format string, args ...any) { + if f.full() { + f.omitted++ + + return + } + + f.lines = append(f.lines, fmt.Sprintf(format, args...)) +} + +func (f *testFailures) String() string { + lines := f.lines + if f.omitted > 0 { + lines = append(lines, fmt.Sprintf("... and %d more", f.omitted)) + } + return strings.Join(lines, "\n") } +// clipTestLabel shortens s to [maxTestLabelBytes] for a failure line. +func clipTestLabel(s string) string { + if len(s) <= maxTestLabelBytes { + return s + } + + return strings.ToValidUTF8(s[:maxTestLabelBytes], "") + "..." +} + // wrapTestResult returns nil when every test passed, otherwise wraps the // rendered failure breakdown in sentinel. errs is passed uncalled so it is // only evaluated on the failure path. diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index b692617bc..aaa57c446 100644 --- a/hscontrol/policy/v2/test_test.go +++ b/hscontrol/policy/v2/test_test.go @@ -1,6 +1,7 @@ package v2 import ( + "fmt" "strings" "testing" @@ -439,3 +440,66 @@ func TestPolicyTestResultsErrorsRendering(t *testing.T) { // Lines should be newline-separated, not space-joined. assert.Equal(t, 2, strings.Count(rendered, "\n")+1, "expected one line per failing assertion") } + +// TestFailureBreakdownIsBounded pins that the rendered breakdown of +// failing tests and sshTests does not grow with label length times +// failures: the policy author sizes both, and the breakdown is also +// rendered at boot. +func TestFailureBreakdownIsBounded(t *testing.T) { + users := sshTestUsers() + nodes := sshTestNodes(users) + + const failures = 2000 + + name := strings.Repeat("a", 64<<10) + hosts := `"hosts": {"` + name + `": "100.64.0.1", "gone": "100.64.0.99"},` + + quoted := make([]string, failures) + for i := range failures { + quoted[i] = fmt.Sprintf(`"x%d"`, i) + } + + names := strings.Join(quoted, ",") + + for _, tt := range []struct { + name, block string + sentinel error + }{ + { + name: "sshTests dst without node", + block: `"sshTests": [{"src": "` + name + `", "dst": [` + + strings.Repeat(`"gone",`, failures) + `], "deny": ["root"]}]`, + sentinel: errSSHPolicyTestsFailed, + }, + { + name: "sshTests accept", + block: `"sshTests": [{"src": "` + name + + `", "dst": ["100.64.0.2"], "accept": [` + names + `]}]`, + sentinel: errSSHPolicyTestsFailed, + }, + { + name: "sshTests check", + block: `"sshTests": [{"src": "` + name + + `", "dst": ["100.64.0.2"], "check": [` + names + `]}]`, + sentinel: errSSHPolicyTestsFailed, + }, + { + name: "tests accept", + block: `"tests": [{"src": "` + name + `", "accept": [` + + strings.Repeat(`"gone:22",`, failures) + `]}]`, + sentinel: errPolicyTestsFailed, + }, + } { + t.Run(tt.name, func(t *testing.T) { + pm, err := NewPolicyManager([]byte(`{}`), users, nodes.ViewSlice()) + require.NoError(t, err) + + _, err = pm.SetPolicy([]byte(`{"acls": [], ` + hosts + tt.block + `}`)) + require.ErrorIs(t, err, tt.sentinel) + require.Less(t, len(err.Error()), 100<<10) + require.True(t, strings.HasSuffix(err.Error(), fmt.Sprintf( + "... and %d more", failures-maxTestFailureLines, + )), "error ends with the count") + }) + } +}