diff --git a/hscontrol/policy/v2/filter.go b/hscontrol/policy/v2/filter.go index 7029b107c..54d93b4fb 100644 --- a/hscontrol/policy/v2/filter.go +++ b/hscontrol/policy/v2/filter.go @@ -254,29 +254,105 @@ func sshCheck(baseURL string) tailcfg.SSHAction { } } -//nolint:gocyclo // SSH compilation walks per-rule branches with intertwined autogroup:self handling -func (pol *Policy) compileSSHPolicy( - baseURL string, - users types.Users, - node types.NodeView, - nodes views.Slice[types.NodeView], -) (*tailcfg.SSHPolicy, error) { - // Never nil: clients read a nil SSHPolicy as "keep the previous - // rules", so revoked access would stay. SaaS sends "rules":[]. - if pol == nil || len(pol.SSHs) == 0 { - return &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{}}, nil +// compiledSSHRule is an [SSH] rule with everything that does not depend on +// the node it is delivered to already resolved, as [compiledGrant] is for +// grants. It is read-only once built: the SSH policies of all nodes point +// into it, so what a rule expands to is kept once and not once per node. +type compiledSSHRule struct { + srcs *sshSources + + sshUsers map[string]string + localpartByUser map[uint]string + action *tailcfg.SSHAction + acceptEnv []string + + hasLocalpart bool + hasSelfDest bool + + // userRules holds the rules scoped to one source user's own nodes, + // for autogroup:self destinations and localpart self-access. + userRules map[uint][]*tailcfg.SSHRule + + // dests holds the destinations other than autogroup:self, nil if + // there are none. Every node in it gets destRules. + dests *netipx.IPSet + destRules []*tailcfg.SSHRule +} + +// sshSources is what the sources of an [SSH] rule resolve to. It is +// read-only once built: rules with the same source addresses share it. +type sshSources struct { + ips ResolvedAddresses + userIDs []uint + principalsByUser map[uint][]*tailcfg.SSHPrincipal + taggedPrincipals []*tailcfg.SSHPrincipal + + // allPrincipals merges user and tagged principals into a single + // list. Tagged principals preserve per-tag duplication (a node with + // N tags appears N times, matching SaaS behavior). + allPrincipals []*tailcfg.SSHPrincipal +} + +// rules emits a common rule and, if the user has a localpart match, a +// per-user localpart rule. +func (r *compiledSSHRule) rules( + principals []*tailcfg.SSHPrincipal, + uid uint, + hasUID bool, +) []*tailcfg.SSHRule { + rules := []*tailcfg.SSHRule{{ + Principals: principals, + SSHUsers: r.sshUsers, + Action: r.action, + AcceptEnv: r.acceptEnv, + }} + + if hasUID { + if lp, ok := r.localpartByUser[uid]; ok { + rules = append(rules, &tailcfg.SSHRule{ + Principals: principals, + SSHUsers: map[string]string{lp: lp}, + Action: r.action, + AcceptEnv: r.acceptEnv, + }) + } } - log.Trace().Caller().Msgf("compiling SSH policy for node %q", node.Hostname()) + return rules +} - rules := []*tailcfg.SSHRule{} +// compileSSHRules resolves all SSH rules of the policy into +// [compiledSSHRule] structs. Rules whose sources resolve to nothing are +// left out. +// +//nolint:gocyclo // SSH compilation walks per-rule branches with intertwined autogroup:self handling +func (pol *Policy) compileSSHRules( + baseURL string, + users types.Users, + nodes views.Slice[types.NodeView], +) ([]compiledSSHRule, error) { + if pol == nil { + return nil, nil + } + + compiled := make([]compiledSSHRule, 0, len(pol.SSHs)) + + // There is one principal per source address and the policy author + // decides how many rules there are, so rules whose sources resolve to + // the same addresses share their principals. + sources := make(map[string]*sshSources) for index, rule := range pol.SSHs { - var autogroupSelfDests, otherDests []Alias + var otherDests []Alias + + cr := compiledSSHRule{ + hasLocalpart: rule.Users.ContainsLocalpart(), + acceptEnv: rule.AcceptEnv, + } for _, dst := range rule.Destinations { if ag, ok := dst.(*AutoGroup); ok && ag.Is(AutoGroupSelf) { - autogroupSelfDests = append(autogroupSelfDests, dst) + cr.hasSelfDest = true } else { otherDests = append(otherDests, dst) } @@ -307,73 +383,64 @@ func (pol *Policy) compileSSHPolicy( ) } - acceptEnv := rule.AcceptEnv + cr.action = &action // Build the common userMap (always has at least a root entry). const rootUser = "root" - baseUserMap := make(map[string]string, len(rule.Users)) + cr.sshUsers = make(map[string]string, len(rule.Users)) if rule.Users.ContainsNonRoot() { - baseUserMap["*"] = "=" + cr.sshUsers["*"] = "=" } if rule.Users.ContainsRoot() { - baseUserMap[rootUser] = rootUser + cr.sshUsers[rootUser] = rootUser } else { - baseUserMap[rootUser] = "" + cr.sshUsers[rootUser] = "" } for _, u := range rule.Users.NormalUsers() { - baseUserMap[u.String()] = u.String() + cr.sshUsers[u.String()] = u.String() } - hasLocalpart := rule.Users.ContainsLocalpart() - - var localpartByUser map[uint]string - if hasLocalpart { - localpartByUser = resolveLocalparts( + if cr.hasLocalpart { + cr.localpartByUser = resolveLocalparts( rule.Users.LocalpartEntries(), users, ) } - userIDs, principalsByUser, taggedPrincipals := groupSourcesByUser( - nodes, srcIPs, - ) + srcKey := strings.Join(srcIPs.Strings(), ",") - // appendRules emits a common rule and, if the user has a - // localpart match, a per-user localpart rule. - appendRules := func(principals []*tailcfg.SSHPrincipal, uid uint, hasUID bool) { - rules = append(rules, &tailcfg.SSHRule{ - Principals: principals, - SSHUsers: baseUserMap, - Action: &action, - AcceptEnv: acceptEnv, - }) + srcs, ok := sources[srcKey] + if !ok { + userIDs, byUser, tagged := groupSourcesByUser(nodes, srcIPs) + srcs = &sshSources{ + ips: srcIPs, + userIDs: userIDs, + principalsByUser: byUser, + taggedPrincipals: tagged, + } - if hasUID { - if lp, ok := localpartByUser[uid]; ok { - rules = append(rules, &tailcfg.SSHRule{ - Principals: principals, - SSHUsers: map[string]string{lp: lp}, - Action: &action, - AcceptEnv: acceptEnv, - }) - } + for _, uid := range srcs.userIDs { + srcs.allPrincipals = append( + srcs.allPrincipals, srcs.principalsByUser[uid]..., + ) + } + + srcs.allPrincipals = append(srcs.allPrincipals, srcs.taggedPrincipals...) + + sources[srcKey] = srcs + } + + cr.srcs = srcs + + if cr.hasSelfDest || cr.hasLocalpart { + cr.userRules = make(map[uint][]*tailcfg.SSHRule, len(srcs.userIDs)) + for _, uid := range srcs.userIDs { + cr.userRules[uid] = cr.rules(srcs.principalsByUser[uid], uid, true) } } - // Handle autogroup:self destinations. - // Tagged nodes can't match autogroup:self. - if len(autogroupSelfDests) > 0 && - !node.IsTagged() && node.User().Valid() { - uid := node.User().ID() - - if principals := principalsByUser[uid]; len(principals) > 0 { - appendRules(principals, uid, true) - } - } - - // Handle other destinations. if len(otherDests) > 0 { var dest netipx.IPSetBuilder @@ -391,60 +458,78 @@ func (pol *Policy) compileSSHPolicy( } } - destSet, err := dest.IPSet() + cr.dests, err = dest.IPSet() if err != nil { return nil, err } - if node.InIPSet(destSet) { - // Node is a destination — emit rules. - // When localpart entries exist, interleave common - // and localpart rules per source user to match - // Tailscale SaaS first-match-wins ordering. - if hasLocalpart { - for _, uid := range userIDs { - appendRules(principalsByUser[uid], uid, true) - } + // When localpart entries exist, interleave common + // and localpart rules per source user to match + // Tailscale SaaS first-match-wins ordering. + if cr.hasLocalpart { + for _, uid := range srcs.userIDs { + cr.destRules = append(cr.destRules, cr.userRules[uid]...) + } - if len(taggedPrincipals) > 0 { - appendRules(taggedPrincipals, 0, false) - } - } else { - // Merge user and tagged principals into a - // single list. Tagged principals preserve - // per-tag duplication (a node with N tags - // appears N times, matching SaaS behavior). - var allPrincipals []*tailcfg.SSHPrincipal - for _, uid := range userIDs { - allPrincipals = append(allPrincipals, principalsByUser[uid]...) - } + if len(srcs.taggedPrincipals) > 0 { + cr.destRules = append( + cr.destRules, cr.rules(srcs.taggedPrincipals, 0, false)..., + ) + } + } else if len(srcs.allPrincipals) > 0 { + cr.destRules = cr.rules(srcs.allPrincipals, 0, false) + } + } - allPrincipals = append(allPrincipals, taggedPrincipals...) + compiled = append(compiled, cr) + } - if len(allPrincipals) > 0 { - appendRules(allPrincipals, 0, false) - } - } - } else if hasLocalpart && slices.ContainsFunc(node.IPs(), srcIPs.Contains) { - // Self-access: source node not in destination set - // receives rules scoped to its own user. - if node.IsTagged() { - var builder netipx.IPSetBuilder - - node.AppendToIPSet(&builder) - - ipSet, err := builder.IPSet() - if err == nil && ipSet != nil { - if principals := ipSetToPrincipals(ipSet); len(principals) > 0 { - appendRules(principals, 0, false) - } - } - } else if node.User().Valid() { - uid := node.User().ID() - if principals := principalsByUser[uid]; len(principals) > 0 { - appendRules(principals, uid, true) + return compiled, nil +} + +// sshPolicyForNode picks the rules that apply to node out of compiled. +func sshPolicyForNode( + compiled []compiledSSHRule, + node types.NodeView, +) *tailcfg.SSHPolicy { + // Never nil: clients read a nil SSHPolicy as "keep the previous + // rules", so revoked access would stay. SaaS sends "rules":[]. + rules := []*tailcfg.SSHRule{} + + for i := range compiled { + cr := &compiled[i] + + // Handle autogroup:self destinations. + // Tagged nodes can't match autogroup:self. + if cr.hasSelfDest && !node.IsTagged() && node.User().Valid() { + rules = append(rules, cr.userRules[node.User().ID()]...) + } + + // Handle other destinations. + if cr.dests == nil { + continue + } + + if node.InIPSet(cr.dests) { + // Node is a destination — emit rules. + rules = append(rules, cr.destRules...) + } else if cr.hasLocalpart && + slices.ContainsFunc(node.IPs(), cr.srcs.ips.Contains) { + // Self-access: source node not in destination set + // receives rules scoped to its own user. + if node.IsTagged() { + var builder netipx.IPSetBuilder + + node.AppendToIPSet(&builder) + + ipSet, err := builder.IPSet() + if err == nil && ipSet != nil { + if principals := ipSetToPrincipals(ipSet); len(principals) > 0 { + rules = append(rules, cr.rules(principals, 0, false)...) } } + } else if node.User().Valid() { + rules = append(rules, cr.userRules[node.User().ID()]...) } } } @@ -468,7 +553,26 @@ func (pol *Policy) compileSSHPolicy( return &tailcfg.SSHPolicy{ Rules: rules, - }, nil + } +} + +// compileSSHPolicy compiles the SSH policy of a single node. +// [PolicyManager.SSHPolicy], which is asked for every node, keeps the +// result of [Policy.compileSSHRules] instead. +func (pol *Policy) compileSSHPolicy( + baseURL string, + users types.Users, + node types.NodeView, + nodes views.Slice[types.NodeView], +) (*tailcfg.SSHPolicy, error) { + log.Trace().Caller().Msgf("compiling SSH policy for node %q", node.Hostname()) + + compiled, err := pol.compileSSHRules(baseURL, users, nodes) + if err != nil { + return nil, err + } + + return sshPolicyForNode(compiled, node), nil } // ipSetToPrincipals converts an [netipx.IPSet] into SSH principals, one per address. @@ -617,7 +721,15 @@ func mergeFilterRules(rules []tailcfg.FilterRule) []tailcfg.FilterRule { return rules } + // sharedSrcIPs identifies a SrcIPs slice by its backing array. + type sharedSrcIPs struct { + first *string + n int + ipProto string + } + keyToIdx := make(map[string]int) + sharedToIdx := make(map[sharedSrcIPs]int) result := make([]tailcfg.FilterRule, 0, len(rules)) for _, rule := range rules { @@ -629,14 +741,35 @@ func mergeFilterRules(rules []tailcfg.FilterRule) []tailcfg.FilterRule { continue } - key := filterRuleKey(rule) + // An ACL compiles to one rule per destination, all with the same + // SrcIPs slice. Build the key of a slice once, or merging them is + // len(src) × len(dst) work. + shared := sharedSrcIPs{ + n: len(rule.SrcIPs), + ipProto: fmt.Sprint(rule.IPProto), + } + if len(rule.SrcIPs) > 0 { + shared.first = &rule.SrcIPs[0] + } - if idx, exists := keyToIdx[key]; exists { + idx, exists := sharedToIdx[shared] + if !exists { + key := filterRuleKey(rule) + + idx, exists = keyToIdx[key] + if !exists { + idx = len(result) + keyToIdx[key] = idx + } + + sharedToIdx[shared] = idx + } + + if exists { // Merge: append DstPorts to existing rule result[idx].DstPorts = append(result[idx].DstPorts, rule.DstPorts...) } else { // New unique combination - keyToIdx[key] = len(result) result = append(result, tailcfg.FilterRule{ SrcIPs: rule.SrcIPs, DstPorts: slices.Clone(rule.DstPorts), diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 4daa314a0..4ae4f1639 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/hscontrol/policy/v2/filter_test.go @@ -2,7 +2,9 @@ package v2 import ( "encoding/json" + "fmt" "net/netip" + "runtime" "slices" "strings" "testing" @@ -4283,3 +4285,94 @@ func TestACLSourcesResolvedOnce(t *testing.T) { require.Len(t, rules, 1) require.Len(t, rules[0].DstPorts, 3) } + +// TestMergeFilterRulesSharedSrcIPs pins that recognising a shared SrcIPs +// slice merges exactly the rules that comparing its content would. +func TestMergeFilterRulesSharedSrcIPs(t *testing.T) { + t.Parallel() + + shared := []string{"100.64.0.1", "100.64.0.2"} + rule := func(srcIPs []string, proto int, port uint16) tailcfg.FilterRule { + return tailcfg.FilterRule{ + SrcIPs: srcIPs, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.3", Ports: tailcfg.PortRange{First: port, Last: port}}, + }, + IPProto: []int{proto}, + } + } + + got := mergeFilterRules([]tailcfg.FilterRule{ + rule(shared, ProtocolTCP, 1), + rule(shared, ProtocolUDP, 2), + rule(shared[:1], ProtocolTCP, 3), + rule(shared, ProtocolTCP, 4), + rule(slices.Clone(shared), ProtocolTCP, 5), + rule(shared, ProtocolUDP, 6), + }) + + want := []tailcfg.FilterRule{ + rule(shared, ProtocolTCP, 1), + rule(shared, ProtocolUDP, 2), + rule(shared[:1], ProtocolTCP, 3), + } + want[0].DstPorts = append( + want[0].DstPorts, rule(nil, 0, 4).DstPorts[0], rule(nil, 0, 5).DstPorts[0], + ) + want[1].DstPorts = append(want[1].DstPorts, rule(nil, 0, 6).DstPorts[0]) + + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("mergeFilterRules() mismatch (-want +got):\n%s", diff) + } +} + +// TestMergeFilterRulesKeysSharedSrcIPsOnce bounds what merging the rules of +// one wide ACL allocates: their shared SrcIPs are keyed once, not once per +// rule. It reads the heap counters, so it does not run in parallel. +func TestMergeFilterRulesKeysSharedSrcIPsOnce(t *testing.T) { + const numRules = 100 + + rulesSharing := func(numSrcs int) []tailcfg.FilterRule { + srcIPs := make([]string, numSrcs) + for i := range srcIPs { + srcIPs[i] = fmt.Sprintf("100.64.%d.%d", i/256, i%256) + } + + rules := make([]tailcfg.FilterRule, numRules) + for i := range rules { + rules[i] = tailcfg.FilterRule{ + SrcIPs: srcIPs, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.100.0.1", Ports: tailcfg.PortRangeAny}, + }, + IPProto: []int{ProtocolTCP}, + } + } + + return rules + } + + bytesPerMerge := func(rules []tailcfg.FilterRule) int64 { + const runs = 10 + + var before, after runtime.MemStats + + runtime.ReadMemStats(&before) + + for range runs { + mergeFilterRules(rules) + } + + runtime.ReadMemStats(&after) + + return int64(after.TotalAlloc-before.TotalAlloc) / runs //nolint:gosec + } + + wide := rulesSharing(1024) + keyBytes := int64(len(filterRuleKey(wide[0]))) + + // Only the key, built once, may grow with the sources. + extra := bytesPerMerge(wide) - bytesPerMerge(rulesSharing(1)) + assert.Less(t, extra, 8*keyBytes, + "merging %d rules that share %d sources", numRules, len(wide[0].SrcIPs)) +} diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index ede844fbe..d9070dde9 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -40,6 +40,12 @@ type PolicyManager struct { // nodesByID indexes nodes; see [PolicyManager.cacheableLocked]. nodesByID map[types.NodeID]types.NodeView + // resolvedPol is pol with its tags indexed and its groups resolved + // against users and nodes, see [Policy.withNodesByTag] and + // [Policy.withResolvedGroups]. Readers that resolve aliases on demand + // go through it; pol itself is what is hashed. + resolvedPol *Policy + filterHash deephash.Sum filter []tailcfg.FilterRule matchers []matcher.Match @@ -64,6 +70,15 @@ type PolicyManager struct { // Lazy map of SSH policies sshPolicyMap *xsync.Map[types.NodeID, *tailcfg.SSHPolicy] + // sshRules are the SSH rules with sources and destinations + // pre-resolved, shared by every node's policy in sshPolicyMap so + // the principal lists exist once rather than once per rule and + // node. They are reset to nil wherever sshPolicyMap is cleared + // and filled by the first SSHPolicy call after that; sshRulesMu + // serialises that fill between readers of mu. + sshRulesMu sync.Mutex + sshRules []compiledSSHRule + // compiledGrants are the grants with sources pre-resolved. // The single source of truth for filter compilation. Both // global and per-node filter rules are derived from these. @@ -246,8 +261,10 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node // pm is written, so a caller that restores its own input on error leaves pm // as it was, and a retry with the same input recompiles. func (pm *PolicyManager) updateLocked() (bool, error) { - // One tag index serves every resolution in this compile. - pol := pm.pol.withNodesByTag(pm.nodes) + // One tag index and one resolution of every group serve this whole + // compile; each step below would otherwise resolve them again for + // every rule naming them. + pol := pm.pol.withNodesByTag(pm.nodes).withResolvedGroups(pm.users, pm.nodes) // Compile all grants once. Both global and per-node filter // rules are derived from these compiled grants. @@ -288,6 +305,7 @@ func (pm *PolicyManager) updateLocked() (bool, error) { } } + pm.resolvedPol = pol pm.compiledGrants = grants pm.userNodeIdx = buildUserNodeIndex(pm.nodes) pm.needsPerNodeFilter = hasPerNodeGrants(pm.compiledGrants) @@ -394,6 +412,7 @@ func (pm *PolicyManager) updateLocked() (bool, error) { // policies for nodes that have changed. Particularly if the only difference is // that nodes has been added or removed. pm.sshPolicyMap.Clear() + pm.sshRules = nil pm.filterRulesMap.Clear() pm.matchersForNodeMap.Clear() } @@ -468,11 +487,13 @@ func (pm *PolicyManager) SSHPolicy(baseURL string, node types.NodeView) (*tailcf return sshPol, nil } - sshPol, err := pm.pol.compileSSHPolicy(baseURL, pm.users, node, pm.nodes) + rules, err := pm.sshRulesLocked(baseURL) if err != nil { return nil, fmt.Errorf("compiling SSH policy: %w", err) } + sshPol := sshPolicyForNode(rules, node) + if pm.cacheableLocked(node) { pm.sshPolicyMap.Store(node.ID(), sshPol) } @@ -480,6 +501,26 @@ func (pm *PolicyManager) SSHPolicy(baseURL string, node types.NodeView) (*tailcf return sshPol, nil } +// sshRulesLocked returns the compiled SSH rules, compiling them on first +// access. It must be called with pm.mu held, for reading or writing; +// writers reset pm.sshRules directly. The rules keep the baseURL of the +// call that compiled them; [state.State] always passes the server URL. +func (pm *PolicyManager) sshRulesLocked(baseURL string) ([]compiledSSHRule, error) { + pm.sshRulesMu.Lock() + defer pm.sshRulesMu.Unlock() + + if pm.sshRules == nil { + rules, err := pm.resolvedPol.compileSSHRules(baseURL, pm.users, pm.nodes) + if err != nil { + return nil, err + } + + pm.sshRules = rules + } + + return pm.sshRules, nil +} + // SSHCheckParams resolves the SSH check period for src logging in to dst // as localUser by looking up the current policy. This avoids trusting URL // parameters that a client could tamper with. First-match wins across @@ -529,7 +570,7 @@ func (pm *PolicyManager) SSHCheckParams( } // Resolve sources and check if src node matches. - srcIPs, err := rule.Sources.Resolve(pm.pol, pm.users, pm.nodes) + srcIPs, err := rule.Sources.Resolve(pm.resolvedPol, pm.users, pm.nodes) if err != nil || srcIPs == nil { continue } @@ -556,7 +597,7 @@ func (pm *PolicyManager) SSHCheckParams( hasOtherDests = true - dstIPs, err := dst.Resolve(pm.pol, pm.users, pm.nodes) + dstIPs, err := dst.Resolve(pm.resolvedPol, pm.users, pm.nodes) if err != nil || dstIPs == nil { continue } @@ -989,6 +1030,7 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) { // the self sources are outside the filter hash, so updateLocked can // report no change while per-node results moved. pm.sshPolicyMap.Clear() + pm.sshRules = nil pm.filterRulesMap.Clear() pm.matchersForNodeMap.Clear() @@ -1071,6 +1113,7 @@ func (pm *PolicyManager) SetNodes(nodes views.Slice[types.NodeView]) (bool, erro if !needsUpdate { // This ensures fresh filter rules are generated for all nodes pm.sshPolicyMap.Clear() + pm.sshRules = nil pm.filterRulesMap.Clear() pm.matchersForNodeMap.Clear() } @@ -1477,7 +1520,7 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via resolved[i] = true for _, src := range grants[i].Sources { - ips, err := src.Resolve(pm.pol, pm.users, pm.nodes) + ips, err := src.Resolve(pm.resolvedPol, pm.users, pm.nodes) if err != nil { continue } @@ -1491,7 +1534,7 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via if viewerMatchesGrant[i] { resolvedDstPrefixes[i], grantHasAutoGroupInternet[i] = resolveViaDestinations( - pm.pol, pm.users, pm.nodes, grants[i].Destinations, + pm.resolvedPol, pm.users, pm.nodes, grants[i].Destinations, ) } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 3f7485ed7..cc13857a0 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1055,6 +1055,52 @@ func TestSSHPolicyRemovalClearsRules(t *testing.T) { } } +// TestSSHPolicySharedBetweenNodes pins that what an SSH rule expands to is +// kept once, not once per rule and destination node. There is one principal +// per source address, so separate copies grow with rules × nodes². +func TestSSHPolicySharedBetweenNodes(t *testing.T) { + users := types.Users{{Name: "user1", Email: "user1@headscale.net"}} + users[0].ID = 1 + + nodes := types.Nodes{ + node("node1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("node2", "100.64.0.2", "fd7a:115c:a1e0::2", users[0]), + node("node3", "100.64.0.3", "fd7a:115c:a1e0::3", users[0]), + } + for i, n := range nodes { + n.ID = types.NodeID(i + 1) //nolint:gosec // small test value + } + + pm, err := NewPolicyManager([]byte(`{"ssh": [ + {"action": "accept", "src": ["autogroup:member"], "dst": ["autogroup:member"], "users": ["a"]}, + {"action": "accept", "src": ["user1@headscale.net"], "dst": ["autogroup:member"], "users": ["b"]} + ]}`), users, nodes[:2].ViewSlice()) + require.NoError(t, err) + + first, err := pm.SSHPolicy("", nodes[0].View()) + require.NoError(t, err) + second, err := pm.SSHPolicy("", nodes[1].View()) + require.NoError(t, err) + + require.Len(t, first.Rules, 2) + require.Len(t, first.Rules[0].Principals, 4) + require.Equal(t, first, second) + + require.Same(t, first.Rules[0], second.Rules[0], "between nodes") + require.Same(t, first.Rules[0].Principals[0], first.Rules[1].Principals[0], + "between rules whose sources resolve to the same addresses") + + // The shared rules are dropped together with the per-node policies. + changed, err := pm.SetNodes(nodes.ViewSlice()) + require.NoError(t, err) + require.True(t, changed) + + after, err := pm.SSHPolicy("", nodes[0].View()) + require.NoError(t, err) + require.Len(t, after.Rules, 2) + require.Len(t, after.Rules[0].Principals, 6) +} + // TestTagPropagationToPeerMap tests that when a node's tags change, // the peer map is correctly updated. This is a regression test for // https://github.com/juanfont/headscale/issues/2389 @@ -4465,3 +4511,155 @@ func BenchmarkCompileWideACL(b *testing.B) { } }) } + +// TestWithResolvedGroups checks that the copy answers for a group exactly +// as resolving it member by member does, without looking at users and +// nodes again, and that the policy it was made from is left untouched. +func TestWithResolvedGroups(t *testing.T) { + users := types.Users{ + {ID: 1, Name: "alice"}, + {ID: 2, Name: "bob"}, + {ID: 3, Name: "twin"}, + {ID: 4, Name: "twin"}, + } + nodes := types.Nodes{ + node("alice1", "100.64.0.1", "fd7a:115c:a1e0::1", users[0]), + node("bob1", "100.64.0.2", "fd7a:115c:a1e0::2", users[1]), + }.ViewSlice() + + pol := &Policy{ + Groups: Groups{ + "group:dev": Usernames{"alice@", "bob@", "missing@"}, + "group:ambiguous": Usernames{"alice@", "twin@"}, + "group:empty": Usernames{}, + }, + } + + resolved := pol.withResolvedGroups(users, nodes) + require.Nil(t, pol.resolvedGroups, + "the parsed policy is hashed and must stay as is") + require.Len(t, resolved.resolvedGroups, len(pol.Groups)) + + for g := range pol.Groups { + want, wantErr := g.resolve(pol, users, nodes) + got, gotErr := g.resolve(resolved, nil, views.Slice[types.NodeView]{}) + + require.Equal(t, want.Prefixes(), got.Prefixes(), g) + + if wantErr != nil { + require.EqualError(t, gotErr, wantErr.Error(), g) + } else { + require.NoError(t, gotErr, g) + } + } + + require.ErrorIs(t, + resolved.resolvedGroups["group:ambiguous"].err, ErrMultipleUsersFound) + + unknown := Group("group:unknown") + got, err := unknown.resolve(resolved, users, nodes) + require.NoError(t, err) + require.Empty(t, got.Prefixes()) + + require.Nil(t, (*Policy)(nil).withResolvedGroups(users, nodes)) +} + +// TestResolvedGroupsFollowUsersAndNodes checks that readers resolving +// aliases on demand through the groups resolved by updateLocked see the +// users and nodes of the latest SetUsers, SetNodes and SetPolicy. +func TestResolvedGroupsFollowUsersAndNodes(t *testing.T) { + alice := types.User{ID: 1, Name: "alice"} + bob := types.User{ID: 2, Name: "bob"} + + srv := node("srv", "100.64.0.10", "fd7a:115c:a1e0::10", alice) + srv.ID = 10 + srv.Tags = []string{"tag:srv"} + alice1 := node("alice1", "100.64.0.1", "fd7a:115c:a1e0::1", alice) + alice1.ID = 1 + bob1 := node("bob1", "100.64.0.3", "fd7a:115c:a1e0::3", bob) + bob1.ID = 2 + + pol := func(members string) []byte { + return []byte(`{ + "groups": {"group:dev": [` + members + `]}, + "tagOwners": {"tag:srv": ["group:dev"]}, + "acls": [{"action": "accept", "src": ["group:dev"], "dst": ["tag:srv:22"]}], + "ssh": [{"action": "accept", "src": ["group:dev"], "dst": ["tag:srv"], "users": ["root"]}] + }`) + } + + pm, err := NewPolicyManager( + pol(`"alice@", "bob@"`), + types.Users{alice}, + types.Nodes{srv, alice1}.ViewSlice(), + ) + require.NoError(t, err) + + sshSources := func() []string { + t.Helper() + + sshPol, err := pm.SSHPolicy("", srv.View()) + require.NoError(t, err) + + var ips []string + + for _, rule := range sshPol.Rules { + for _, principal := range rule.Principals { + ips = append(ips, principal.NodeIP) + } + } + + slices.Sort(ips) + + return ips + } + + filterSources := func() []string { + t.Helper() + + filter, _ := pm.Filter() + require.Len(t, filter, 1) + + ips := slices.Clone(filter[0].SrcIPs) + slices.Sort(ips) + + return ips + } + + aliceIPs := []string{"100.64.0.1", "fd7a:115c:a1e0::1"} + bothIPs := []string{ + "100.64.0.1", "100.64.0.3", "fd7a:115c:a1e0::1", "fd7a:115c:a1e0::3", + } + + require.Equal(t, aliceIPs, filterSources()) + require.Equal(t, aliceIPs, sshSources()) + + _, _, err = pm.SetUsers(types.Users{alice, bob}) + require.NoError(t, err) + + _, err = pm.SetNodes(types.Nodes{srv, alice1, bob1}.ViewSlice()) + require.NoError(t, err) + require.Equal(t, bothIPs, filterSources()) + require.Equal(t, bothIPs, sshSources()) + + _, err = pm.SetPolicy(pol(`"alice@"`)) + require.NoError(t, err) + require.Equal(t, aliceIPs, filterSources()) + require.Equal(t, aliceIPs, sshSources()) +} + +// TestResolvedPolicyKeepsTagIndex checks that the copy readers resolve +// through carries both the tag index and the resolved groups. +func TestResolvedPolicyKeepsTagIndex(t *testing.T) { + users := types.Users{{ID: 1, Name: "alice"}} + nodes := types.Nodes{node("a", "100.64.0.1", "fd7a:115c:a1e0::1", users[0])} + + pm, err := NewPolicyManager([]byte(`{ + "groups": {"group:a": ["alice@"]}, + "tagOwners": {"tag:x": ["alice@"]}, + "acls": [{"action": "accept", "src": ["group:a"], "dst": ["tag:x:*"]}] + }`), users, nodes.ViewSlice()) + require.NoError(t, err) + require.NotNil(t, pm.resolvedPol.nodesByTag) + require.NotNil(t, pm.resolvedPol.resolvedGroups) +} diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index 23da92aac..5d01d29d6 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -141,7 +141,7 @@ func (pm *PolicyManager) RunSSHTests() error { defer pm.mu.RUnlock() cache := make(map[types.NodeID]*tailcfg.SSHPolicy) - results := runSSHPolicyTests(pm.pol, pm.users, pm.nodes, cache) + results := runSSHPolicyTests(pm.resolvedPol, pm.users, pm.nodes, cache) return wrapTestResult(errSSHPolicyTestsFailed, results.AllPassed, results) } @@ -156,6 +156,7 @@ func evaluateSSHTests( return nil } + pol = pol.withResolvedGroups(users, nodes) cache := make(map[types.NodeID]*tailcfg.SSHPolicy) results := runSSHPolicyTests(pol, users, nodes, cache) diff --git a/hscontrol/policy/v2/test.go b/hscontrol/policy/v2/test.go index 96ee1445a..51816f6a2 100644 --- a/hscontrol/policy/v2/test.go +++ b/hscontrol/policy/v2/test.go @@ -260,7 +260,7 @@ func (pm *PolicyManager) RunTests() error { pm.mu.RLock() defer pm.mu.RUnlock() - results := runPolicyTests(pm.pol, pm.filter, pm.users, pm.nodes) + results := runPolicyTests(pm.resolvedPol, pm.filter, pm.users, pm.nodes) return wrapTestResult(errPolicyTestsFailed, results.AllPassed, results) } @@ -273,6 +273,7 @@ func evaluateTests(pol *Policy, users []types.User, nodes views.Slice[types.Node return nil } + pol = pol.withResolvedGroups(users, nodes) grants := pol.compileGrants(users, nodes) var filter []tailcfg.FilterRule diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index c37225624..10819e8bf 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -519,6 +519,13 @@ func (g *Group) resolve(p *Policy, users types.Users, nodes views.Slice[types.No errs []error ) + // A policy compiled through [Policy.withResolvedGroups] has already + // resolved every group once; answer from that instead of walking + // users and nodes again for each rule that names the group. + if r, ok := p.resolvedGroups[*g]; ok { + return r.ips, r.err + } + for _, user := range p.Groups[*g] { uips, err := user.resolve(nil, users, nodes) if err != nil { @@ -1958,6 +1965,11 @@ type Policy struct { // callers using it should panic if not validated bool `json:"-"` + // resolvedGroups is only set on the copy returned by + // [Policy.withResolvedGroups]; a parsed policy never carries it, + // as the policy is hashed for change detection. + resolvedGroups map[Group]resolvedGroup `json:"-"` + Groups Groups `json:"groups,omitempty"` Hosts Hosts `json:"hosts,omitempty"` TagOwners TagOwners `json:"tagOwners,omitempty"` @@ -2001,6 +2013,41 @@ func (pol *Policy) withNodesByTag(nodes views.Slice[types.NodeView]) *Policy { // MarshalJSON is deliberately not implemented for [Policy]. // We use the default JSON marshalling behavior provided by the Go runtime. +// resolvedGroup is what [Group.resolve] returns for one group. +type resolvedGroup struct { + ips *netipx.IPSet + err error +} + +// withResolvedGroups returns a shallow copy of pol on which every group +// is already resolved against users and nodes, so that [Group.resolve] +// is a map lookup. Resolving a group walks users and nodes once per +// member, and a group is typically named by many rules; resolving it per +// mention makes compilation quadratic in the policy size. The copy is +// only valid for the users and nodes it was built from, and must not be +// hashed in place of pol. +func (pol *Policy) withResolvedGroups( + users types.Users, + nodes views.Slice[types.NodeView], +) *Policy { + if pol == nil || len(pol.Groups) == 0 { + return pol + } + + cp := *pol + cp.resolvedGroups = nil + + resolved := make(map[Group]resolvedGroup, len(pol.Groups)) + for g := range pol.Groups { + ips, err := g.resolve(&cp, users, nodes) + resolved[g] = resolvedGroup{ips: ips, err: err} + } + + cp.resolvedGroups = resolved + + return &cp +} + var ( // TODO(kradalby): Add these checks for tagOwners and autoApprovers. autogroupForSrc = []AutoGroup{AutoGroupMember, AutoGroupTagged, AutoGroupDangerAll}