diff --git a/hscontrol/apiv2_compat_test.go b/hscontrol/apiv2_compat_test.go index 37f6d5c8c..79b818995 100644 --- a/hscontrol/apiv2_compat_test.go +++ b/hscontrol/apiv2_compat_test.go @@ -80,10 +80,8 @@ var apiv2CompatSkipReasons = map[string]string{ "oauth/K2/0/outcome": unknownKeyType, "oauth/K5/0/status": unknownKeyType, - "oauth/J3/1/hostnames": sshTestHostnames, - "oauth/J4/0/hostnames": sshTestHostnames, - "oauth/J3/1/detail": refusalDetail, - "oauth/J4/0/detail": refusalDetail, + "oauth/J3/1/detail": refusalDetail, + "oauth/J4/0/detail": refusalDetail, "oauth/E2/0/status": taggedAllClientAnyTag, "oauth/E2/0/outcome": taggedAllClientAnyTag, @@ -217,7 +215,6 @@ const ( bodyBeforeScope = "BODY_BEFORE_SCOPE: headscale validates a createKey body before the token's scope and answers 422; SaaS answers 404 for the missing scope" taggedAllClientAnyTag = "TAGGED_ALL_CLIENT_ANY_TAG: SaaS lets a tagged all-scope client's token ask only for the client's tags; headscale lets it ask for any tag" retagRemovesUnownedTag = "RETAG_REMOVES_UNOWNED_TAG: SaaS refuses a token's retag of a device holding a tag outside the token's grant with 400 cannot remove current tags; headscale replaces the tags" - sshTestHostnames = "SSH_TEST_HOSTNAMES: headscale names the nodes behind a failing sshTests destination; SaaS names the destination as written" ) // Differences no planned change removes yet. diff --git a/hscontrol/policy/v2/policytester_compat_test.go b/hscontrol/policy/v2/policytester_compat_test.go index e253ac562..0c2bcee0b 100644 --- a/hscontrol/policy/v2/policytester_compat_test.go +++ b/hscontrol/policy/v2/policytester_compat_test.go @@ -53,8 +53,6 @@ var knownPolicyTesterDivergences = map[string]string{ //nolint:gosec // strings "policytest-src-wildcard-deny/verdict": testSrcShape, "policytest-dst-multinode-partial-accept/verdict": "TEST_DST_EVERY_NODE: SaaS accepts only when every node of the dst alias is reached; headscale when one is", - - "policytest-report-both-blocks/label": sshTestNodeLabel, } const ( @@ -179,8 +177,7 @@ var ( // with the ones headscale's evaluators report, as sorted lines of // "src -> dst: want" (tests), "src/user: want @ dst" (sshTests) and // "src: error" for an entry that could not be evaluated. Repeats are kept. -// An sshTests dst is the entry's alias in headscale's form: SaaS names the -// alias, headscale the node, which the label check compares. +// Both name an sshTests src and dst as written. func requireFailureBreakdown( t *testing.T, skips map[string]string, @@ -203,14 +200,7 @@ func requireFailureBreakdown( } if m := saasSSHTestFailRE.FindStringSubmatch(line); m != nil { - dst := m[1] - - alias, err := parseAlias(dst) - if err == nil { - dst = alias.String() - } - - want = append(want, fmt.Sprintf("%s/%s: %s @ %s", d.User, m[2], m[3], dst)) + want = append(want, fmt.Sprintf("%s/%s: %s @ %s", d.User, m[2], m[3], m[1])) continue } @@ -306,14 +296,11 @@ func testerBreakdown(pm *PolicyManager) []string { cache := make(map[types.NodeID]*tailcfg.SSHPolicy) - // One result per entry, in policy order. - for i, res := range runSSHPolicyTests(pm.pol, pm.users, pm.nodes, cache).Results { + for _, res := range runSSHPolicyTests(pm.pol, pm.users, pm.nodes, cache).Results { for range res.Errors { out = append(out, res.Src+": error") } - aliases := sshTestDstAliases(pm, pm.pol.SSHTests[i]) - for want, fails := range map[string]map[string][]string{ "accept": res.AcceptFail, "deny": res.DenyFail, @@ -321,7 +308,7 @@ func testerBreakdown(pm *PolicyManager) []string { } { for user, dsts := range fails { for _, dst := range dsts { - out = append(out, fmt.Sprintf("%s/%s: %s @ %s", res.Src, user, want, aliases[dst])) + out = append(out, fmt.Sprintf("%s/%s: %s @ %s", res.Src, user, want, dst)) } } } @@ -332,30 +319,6 @@ func testerBreakdown(pm *PolicyManager) []string { return out } -// sshTestDstAliases maps each node hostname an sshTests entry reaches to the -// first of its dst aliases that selects it, the one the evaluator checks the -// node under. -func sshTestDstAliases(pm *PolicyManager, test SSHPolicyTest) map[string]string { - _, srcUserID, _ := resolveSSHTestSource(test.Src, pm.pol, pm.users, pm.nodes) - - out := make(map[string]string) - - for _, alias := range test.Dst { - nodes, _, err := resolveSSHTestDestNodes(SSHTestDestinations{alias}, pm.pol, pm.users, pm.nodes, srcUserID) - if err != nil { - continue - } - - for _, n := range nodes { - if _, ok := out[n.Hostname()]; !ok { - out[n.Hostname()] = alias.String() - } - } - } - - return out -} - // nodeLabels lists the capture's hostnames that text names while the policy // does not, so they are there only because the report labels nodes. func nodeLabels(c *testcapture.Capture, text string) []string { diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index cd932fd70..4dc0ceff8 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -35,6 +35,11 @@ type SSHPolicyTestResult struct { DenyFail map[string][]string `json:"deny_fail,omitempty"` CheckOK map[string][]string `json:"check_ok,omitempty"` CheckFail map[string][]string `json:"check_fail,omitempty"` + + // CheckDenied holds the check failures where a node was not reached at + // all. A dst alias can cover nodes that fail in different ways, and one + // node not reached makes the alias DENIED. + CheckDenied map[string][]string `json:"check_denied,omitempty"` } // SSHPolicyTestResults aggregates one evaluation run. @@ -43,53 +48,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 CheckDenied; 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() } // failures is the breakdown apart per failing entry, so an API answer can @@ -123,11 +138,11 @@ func displayUser(u string) string { // checkFailReason annotates a check-fail with whether the user reached // the dst via an accept rule or did not reach at all. func checkFailReason(res SSHPolicyTestResult, user, dst string) string { - if slices.Contains(res.AcceptOK[user], dst) { - return "ALLOWED via accept" + if slices.Contains(res.CheckDenied[user], dst) { + return "DENIED" } - return "DENIED" + return "ALLOWED via accept" } // RunSSHTests evaluates the live policy's sshTests block and wraps any @@ -199,7 +214,7 @@ func runSSHPolicyTest( ) SSHPolicyTestResult { srcLabel := "" if test.Src != nil { - srcLabel = test.Src.String() + srcLabel = sshTestLabel(test.Src) } res := SSHPolicyTestResult{ @@ -233,7 +248,7 @@ func runSSHPolicyTest( return res } - dstNodes, emptyDsts, err := resolveSSHTestDestNodes(test.Dst, pol, users, nodes, srcUserID) + dsts, err := resolveSSHTestDests(test.Dst, pol, users, nodes, srcUserID) if err != nil { res.Passed = false res.Errors = append(res.Errors, @@ -242,17 +257,6 @@ func runSSHPolicyTest( return res } - // A dst resolving to zero nodes would silently pass. - for _, dst := range emptyDsts { - res.Passed = false - res.Errors = append(res.Errors, - fmt.Sprintf("dst alias %q resolved to no nodes", dst)) - } - - if len(dstNodes) == 0 { - return res - } - for _, g := range []struct { users []SSHUser kind sshAssertion @@ -264,7 +268,7 @@ func runSSHPolicyTest( for _, user := range g.users { evaluateAssertion( pol, users, nodes, cache, - srcAddrs, dstNodes, user.String(), + srcAddrs, dsts, user.String(), g.kind, &res, ) } @@ -281,10 +285,77 @@ const ( assertCheck ) -// evaluateAssertion walks every (srcAddr, dstNode) pair for one user -// and records the outcome. Empty username fails — SSH login users -// cannot be empty even when parse accepted it. +// evaluateAssertion records one user's outcome for each dst alias of an +// entry, under the alias as written: the report goes back to the policy +// author, who may not be allowed to list nodes. An alias passes when every +// node it selects does, for every src address; one selecting no node is not +// reached. Each distinct alias is evaluated once and reported at every +// occurrence, as Tailscale SaaS reports them. func evaluateAssertion( + pol *Policy, + users []types.User, + nodes views.Slice[types.NodeView], + cache map[types.NodeID]*tailcfg.SSHPolicy, + srcAddrs []netip.Addr, + dsts []sshTestDst, + user string, + kind sshAssertion, + res *SSHPolicyTestResult, +) { + type outcome struct{ passed, notReached, failed bool } + + seen := make(map[string]outcome, len(dsts)) + + for _, dst := range dsts { + o, ok := seen[dst.label] + if !ok { + passed, notReached, err := sshTestAliasOutcome( + pol, users, nodes, cache, srcAddrs, dst.nodes, user, kind, + ) + if err != nil { + res.Passed = false + res.Errors = append(res.Errors, + fmt.Sprintf("compiling SSH policy for %s: %v", dst.label, err)) + } + + o = outcome{passed, notReached, err != nil} + seen[dst.label] = o + } + + if o.failed { + continue + } + + if !o.passed { + res.Passed = false + } + + switch { + case kind == assertAccept && o.passed: + res.AcceptOK = appendUserDst(res.AcceptOK, user, dst.label) + case kind == assertAccept: + res.AcceptFail = appendUserDst(res.AcceptFail, user, dst.label) + case kind == assertDeny && o.passed: + res.DenyOK = appendUserDst(res.DenyOK, user, dst.label) + case kind == assertDeny: + res.DenyFail = appendUserDst(res.DenyFail, user, dst.label) + case o.passed: + res.CheckOK = appendUserDst(res.CheckOK, user, dst.label) + default: + res.CheckFail = appendUserDst(res.CheckFail, user, dst.label) + + if o.notReached { + res.CheckDenied = appendUserDst(res.CheckDenied, user, dst.label) + } + } + } +} + +// sshTestAliasOutcome reports whether user's assertion holds on every node +// of one dst alias from every src address, and whether a failing node was +// not reached at all. All src addresses must agree: the first +// counter-example fails the node. +func sshTestAliasOutcome( pol *Policy, users []types.User, nodes views.Slice[types.NodeView], @@ -293,82 +364,43 @@ func evaluateAssertion( dstNodes []types.NodeView, user string, kind sshAssertion, - res *SSHPolicyTestResult, -) { -dstLoop: +) (bool, bool, error) { + if len(dstNodes) == 0 { + return kind == assertDeny, true, nil + } + + passed, notReached := true, false + for _, dst := range dstNodes { dstPol, err := compiledSSHPolicy(pol, users, nodes, cache, dst) if err != nil { - res.Passed = false - res.Errors = append(res.Errors, - fmt.Sprintf("compiling SSH policy for %s: %v", - dst.Hostname(), err)) - - continue + return false, false, err } - dstLabel := dst.Hostname() - - acceptHit := false - checkHit := false - for _, srcAddr := range srcAddrs { a, c := reachability(dstPol, srcAddr, user) - if a { - acceptHit = true - } - if c { - checkHit = true - } + var holds bool - // All src IPs must agree; one counter-example fails - // the whole (user, dst) pair. switch kind { case assertAccept: - if !a { - res.Passed = false - res.AcceptFail = appendUserDst(res.AcceptFail, user, dstLabel) - - continue dstLoop - } + holds = a case assertDeny: - if a { - res.Passed = false - res.DenyFail = appendUserDst(res.DenyFail, user, dstLabel) - - continue dstLoop - } + holds = !a case assertCheck: - if !c { - res.Passed = false - res.CheckFail = appendUserDst(res.CheckFail, user, dstLabel) - - // Record whether the accept side passed so - // the rendered error can say "ALLOWED via - // accept" instead of "DENIED". - if a { - res.AcceptOK = appendUserDst(res.AcceptOK, user, dstLabel) - } - - continue dstLoop - } + holds = c } - } - switch kind { - case assertAccept: - if acceptHit { - res.AcceptOK = appendUserDst(res.AcceptOK, user, dstLabel) - } - case assertDeny: - res.DenyOK = appendUserDst(res.DenyOK, user, dstLabel) - case assertCheck: - if checkHit { - res.CheckOK = appendUserDst(res.CheckOK, user, dstLabel) + if !holds { + passed = false + notReached = notReached || !a + + break } } } + + return passed, notReached, nil } // appendUserDst appends dst to m[user], allocating m on first use. @@ -462,105 +494,107 @@ func resolveSSHTestSource( return out, userID, nil } -// resolveSSHTestDestNodes maps each dst alias to its destination -// [types.NodeView]s. autogroup:self needs special handling: it cannot -// resolve without per-node context, so it walks the node set keyed on -// src's owning user. Other aliases resolve to an [netipx.IPSet] and match -// via [types.NodeView.InIPSet]. -func resolveSSHTestDestNodes( +// sshTestLabel names an sshTests alias as its author writes it: a single +// address bare, not as a /32 or /128 prefix. +func sshTestLabel(alias Alias) string { + if p, ok := alias.(*Prefix); ok { + return filterIP(netip.Prefix(*p)) + } + + return alias.String() +} + +// sshTestDst is one dst alias of an sshTests entry and the nodes it +// selects. +type sshTestDst struct { + label string + nodes []types.NodeView +} + +// resolveSSHTestDests resolves each dst alias of an entry, in order and +// once per occurrence, so the report can name every alias as written. +// Each distinct alias resolves once. +func resolveSSHTestDests( dsts SSHTestDestinations, pol *Policy, users []types.User, nodes views.Slice[types.NodeView], srcUserID uint, -) ([]types.NodeView, []string, error) { - seen := make(map[types.NodeID]struct{}) - - var ( - out []types.NodeView - emptyDsts []string - ) +) ([]sshTestDst, error) { + selected := make(map[string][]types.NodeView, len(dsts)) + out := make([]sshTestDst, 0, len(dsts)) for _, alias := range dsts { - dstLabel := alias.String() - matched := false + label := sshTestLabel(alias) - if ag, ok := alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - // autogroup:self resolves to non-tagged nodes owned by - // the same user as src; tagged/IP sources have no user. - if srcUserID == 0 { - emptyDsts = append(emptyDsts, dstLabel) + dstNodes, ok := selected[label] + if !ok { + var err error - continue + dstNodes, err = sshTestDstNodes(alias, pol, users, nodes, srcUserID) + if err != nil { + return nil, err } - for _, n := range nodes.All() { - if n.IsTagged() { - continue - } - - if !n.User().Valid() { - continue - } - - if n.User().ID() != srcUserID { - continue - } - - matched = true - - if _, dup := seen[n.ID()]; dup { - continue - } - - seen[n.ID()] = struct{}{} - out = append(out, n) - } - - if !matched { - emptyDsts = append(emptyDsts, dstLabel) - } - - continue + selected[label] = dstNodes } - ips, err := alias.Resolve(pol, users, nodes) - if err != nil { - return nil, nil, fmt.Errorf("resolving destination %q: %w", dstLabel, err) - } + out = append(out, sshTestDst{label: label, nodes: dstNodes}) + } - if ips == nil || ips.Empty() { - emptyDsts = append(emptyDsts, dstLabel) + return out, nil +} - continue - } +// sshTestDstNodes returns the nodes one dst alias selects. autogroup:self +// cannot resolve without per-node context, so it walks the node set keyed +// on src's owning user. Other aliases resolve to an [netipx.IPSet] and +// match via [types.NodeView.InIPSet]. +func sshTestDstNodes( + alias Alias, + pol *Policy, + users []types.User, + nodes views.Slice[types.NodeView], + srcUserID uint, +) ([]types.NodeView, error) { + var out []types.NodeView - set, err := prefixesToIPSet(ips.Prefixes()) - if err != nil { - return nil, nil, fmt.Errorf("building IPSet for %q: %w", dstLabel, err) + if ag, ok := alias.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { + // autogroup:self is the src user's untagged nodes; tagged and + // address sources have no user. + if srcUserID == 0 { + return nil, nil } for _, n := range nodes.All() { - if !n.InIPSet(set) { - continue + if !n.IsTagged() && n.User().Valid() && n.User().ID() == srcUserID { + out = append(out, n) } - - matched = true - - if _, dup := seen[n.ID()]; dup { - continue - } - - seen[n.ID()] = struct{}{} - out = append(out, n) } - if !matched { - emptyDsts = append(emptyDsts, dstLabel) + return out, nil + } + + ips, err := alias.Resolve(pol, users, nodes) + if err != nil { + return nil, fmt.Errorf("resolving destination %q: %w", sshTestLabel(alias), err) + } + + if ips == nil || ips.Empty() { + return nil, nil + } + + set, err := prefixesToIPSet(ips.Prefixes()) + if err != nil { + return nil, fmt.Errorf("building IPSet for %q: %w", sshTestLabel(alias), err) + } + + for _, n := range nodes.All() { + if n.InIPSet(set) { + out = append(out, n) } } - return out, emptyDsts, nil + return out, nil } // prefixesToIPSet builds the [netipx.IPSet] that [types.NodeView.InIPSet] diff --git a/hscontrol/policy/v2/sshtest_test.go b/hscontrol/policy/v2/sshtest_test.go index 99e4a6fde..92b1010c6 100644 --- a/hscontrol/policy/v2/sshtest_test.go +++ b/hscontrol/policy/v2/sshtest_test.go @@ -715,11 +715,9 @@ func TestRunSSHTests(t *testing.T) { wantErrSub: []string{"alice@headscale.net", "expected ALLOWED"}, }, { - // tag:empty has an owner but no tagged nodes, so the dst - // alias resolves to no nodes. Without the empty-dst guard - // the per-assertion loop runs zero iterations and the - // test silently passes — exactly the regression the - // guard exists to catch. + // tag:empty has an owner but no tagged nodes: an alias + // selecting no node is not reached, so the accept fails + // instead of passing over zero nodes. name: "dst-tag-with-no-tagged-nodes-fails", policy: `{ "tagOwners": { "tag:empty": ["alice@headscale.net"] }, @@ -730,12 +728,11 @@ func TestRunSSHTests(t *testing.T) { }] }`, wantPass: false, - wantErrSub: []string{"tag:empty", "resolved to no nodes"}, + wantErrSub: []string{"-> tag:empty: expected ALLOWED, got DENIED"}, }, { // autogroup:self from a tag src has no user identity to - // scope to, so the dst alias resolves to no nodes. Same - // empty-dst guard, distinct trigger path. + // scope to, so the dst alias selects no node either. name: "dst-autogroup-self-from-tag-src-fails", policy: `{ "tagOwners": { "tag:prod": ["alice@headscale.net"] }, @@ -746,7 +743,7 @@ func TestRunSSHTests(t *testing.T) { }] }`, wantPass: false, - wantErrSub: []string{"autogroup:self", "resolved to no nodes"}, + wantErrSub: []string{"-> autogroup:self: expected ALLOWED, got DENIED"}, }, { name: "src-address-space-deny-pass", @@ -1066,22 +1063,22 @@ func TestSSHPolicyTestResultsErrorsRendering(t *testing.T) { { Src: "alice@headscale.net", AcceptFail: map[string][]string{ - "root": {"server"}, + "root": {"tag:server"}, }, }, { Src: "bob@headscale.net", DenyFail: map[string][]string{ - "root": {"alice-laptop"}, + "root": {"group:people"}, }, }, { Src: "alice@headscale.net", CheckFail: map[string][]string{ - "ubuntu": {"server"}, + "ubuntu": {"tag:server", "tag:prod"}, }, - AcceptOK: map[string][]string{ - "ubuntu": {"server"}, + CheckDenied: map[string][]string{ + "ubuntu": {"tag:prod"}, }, }, }, @@ -1089,13 +1086,66 @@ func TestSSHPolicyTestResultsErrorsRendering(t *testing.T) { rendered := results.Errors() for _, sub := range []string{ - "alice@headscale.net/root -> server: expected ALLOWED, got DENIED", - "bob@headscale.net/root -> alice-laptop: expected DENIED, got ALLOWED", - "alice@headscale.net/ubuntu -> server: expected ALLOWED via check, got ALLOWED via accept", + "alice@headscale.net/root -> tag:server: expected ALLOWED, got DENIED", + "bob@headscale.net/root -> group:people: expected DENIED, got ALLOWED", + "alice@headscale.net/ubuntu -> tag:server: expected ALLOWED via check, got ALLOWED via accept", + "alice@headscale.net/ubuntu -> tag:prod: expected ALLOWED via check, got DENIED", } { assert.Contains(t, rendered, sub) } - assert.Equal(t, 3, strings.Count(rendered, "\n")+1, + assert.Equal(t, 4, strings.Count(rendered, "\n")+1, "expected one line per failing assertion") } + +// TestSSHTestsReportNamesAliasNotNode: the report goes back to the policy +// author, who may not be allowed to list nodes, so src and dst are named as +// written, one line per dst occurrence, as Tailscale SaaS reports them. +func TestSSHTestsReportNamesAliasNotNode(t *testing.T) { + users := sshTestUsers() + nodes := sshTestNodes(users) + + pm, err := NewPolicyManager([]byte(`{}`), users, nodes.ViewSlice()) + require.NoError(t, err) + + // group:people covers alice's nodes, which her own rule reaches by + // accept, and bob's and thor's, not reached at all: it is DENIED. + const policy = `{ + "groups": {"group:people": ["alice@headscale.net", "bob@headscale.net", "thor@example.org"]}, + "tagOwners": {"tag:server": ["alice@headscale.net"], "tag:empty": ["alice@headscale.net"]}, + "ssh": [{ + "action": "accept", + "src": ["autogroup:member"], + "dst": ["autogroup:self"], + "users": ["root"] + }], + "sshTests": [ + { + "src": "10.0.0.1", + "dst": ["100.64.0.3", "tag:server", "tag:server", "tag:empty"], + "accept": ["root"] + }, + {"src": "alice@headscale.net", "dst": ["autogroup:self"], "deny": ["root"]}, + {"src": "alice@headscale.net", "dst": ["group:people"], "check": ["root"]} + ] + }` + + _, err = pm.SetPolicy([]byte(policy)) + require.ErrorIs(t, err, errSSHPolicyTestsFailed) + + for line, want := range map[string]int{ + "10.0.0.1/root -> 100.64.0.3: expected ALLOWED, got DENIED": 1, + "10.0.0.1/root -> tag:server: expected ALLOWED, got DENIED": 2, + "10.0.0.1/root -> tag:empty: expected ALLOWED, got DENIED": 1, + "alice@headscale.net/root -> autogroup:self: expected DENIED, got ALLOWED": 1, + "alice@headscale.net/root -> group:people: expected ALLOWED via check, got DENIED": 1, + } { + assert.Equal(t, want, strings.Count(err.Error(), line), line) + } + + for _, node := range nodes { + if !strings.Contains(policy, node.Hostname) { + assert.NotContains(t, err.Error(), node.Hostname) + } + } +} diff --git a/hscontrol/policy/v2/sshtester_compat_test.go b/hscontrol/policy/v2/sshtester_compat_test.go index 49befe3e8..9f5f92f0f 100644 --- a/hscontrol/policy/v2/sshtester_compat_test.go +++ b/hscontrol/policy/v2/sshtester_compat_test.go @@ -14,33 +14,7 @@ import ( // knownSSHTesterDivergences names the engine gap for each check where // headscale and upstream disagree, keyed /. -var knownSSHTesterDivergences = map[string]string{ - "sshtest-accept-fail-no-ssh-rule/label": sshTestNodeLabel, - "sshtest-accept-fail-wrong-login-user/label": sshTestNodeLabel, - "sshtest-both-tests-pass-sshTests-fail/label": sshTestNodeLabel, - "sshtest-deny-fail-policy-allows/label": sshTestNodeLabel, - "sshtest-malformed-empty-user/label": sshTestNodeLabel, - "sshtest-mixed-acls-tcp22-allow-no-ssh-rule/label": sshTestNodeLabel, - "sshtest-user-localpart-domain-mismatch/label": sshTestNodeLabel, - "sshtest-user-nonroot-blocks-root/label": sshTestNodeLabel, - "sshtest-fail-label-host-alias/label": sshTestNodeLabel, - "sshtest-fail-label-ip-dst/label": sshTestNodeLabel, - "sshtest-fail-label-same-node-twice/label": sshTestNodeLabel, - "sshtest-fail-label-user-dst/label": sshTestNodeLabel, - "sshtest-repeated-names/label": sshTestNodeLabel, - "sshtest-report-multi/label": sshTestNodeLabel, - "sshtest-src-unassigned-ip-accept/label": sshTestNodeLabel, - - "sshtest-fail-label-same-node-twice/breakdown": "SSHTEST_PER_ALIAS: SaaS reports each dst alias, also two naming one node; headscale checks each node once", - "sshtest-fail-label-ip-no-device/breakdown": sshTestNoDevice, - "sshtest-report-multi/breakdown": "SSHTEST_PER_ALIAS+SSHTEST_NO_DEVICE: SaaS reports one line per dst alias and a deviceless alias as not reached; headscale one per node and an error", - "sshtest-src-unassigned-ip-accept/breakdown": "SSHTEST_SRC_LABEL: SaaS labels a single-address source as written, headscale as a /32 prefix", -} - -const ( - sshTestNodeLabel = "SSHTEST_NODE_LABEL: headscale names each failing dst node by hostname; SaaS names the dst as written" - sshTestNoDevice = "SSHTEST_NO_DEVICE: SaaS reports a dst that selects no device as not reached; headscale as an error" -) +var knownSSHTesterDivergences = map[string]string{} func TestSSHTesterCompat(t *testing.T) { t.Parallel() diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 51816f6a2..3ad1e0be3 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], "") + "..." +} + // TestFailure is one failing tests or sshTests entry: its src as written and // its lines of the failure breakdown. type TestFailure struct { diff --git a/hscontrol/policy/v2/test_test.go b/hscontrol/policy/v2/test_test.go index b692617bc..0a41f11d0 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) + `], "accept": ["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") + }) + } +}