From 42f71b9c06e89d3785582e95e08872be0500aff1 Mon Sep 17 00:00:00 2001 From: Aras Ergus Date: Tue, 1 Apr 2025 10:15:29 +0200 Subject: [PATCH 1/5] Make matchers part of the Policy interface --- hscontrol/mapper/mapper.go | 3 ++- hscontrol/policy/matcher/matcher.go | 8 ++++++++ hscontrol/policy/pm.go | 3 +++ hscontrol/policy/policy.go | 5 +++-- hscontrol/policy/policy_test.go | 4 +++- hscontrol/policy/v1/policy.go | 6 ++++++ hscontrol/policy/v2/policy.go | 11 +++++++++++ hscontrol/types/node.go | 10 +--------- hscontrol/types/node_test.go | 4 +++- 9 files changed, 40 insertions(+), 14 deletions(-) diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 7a297bd3..4da94c31 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -529,7 +529,8 @@ func appendPeerChanges( // If there are filter rules present, see if there are any nodes that cannot // access each-other at all and remove them from the peers. if len(filter) > 0 { - changed = policy.FilterNodesByACL(node, changed, filter) + matchers := polMan.Matchers() + changed = policy.FilterNodesByACL(node, changed, matchers) } profiles := generateUserProfiles(node, changed) diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index 2b86416e..1d4f09d2 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -13,6 +13,14 @@ type Match struct { dests *netipx.IPSet } +func MatchesFromFilterRules(rules []tailcfg.FilterRule) []Match { + matches := make([]Match, 0, len(rules)) + for _, rule := range rules { + matches = append(matches, MatchFromFilterRule(rule)) + } + return matches +} + func MatchFromFilterRule(rule tailcfg.FilterRule) Match { dests := []string{} for _, dest := range rule.DstPorts { diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index 24f68ca1..5df7da76 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -1,6 +1,7 @@ package policy import ( + "github.com/juanfont/headscale/hscontrol/policy/matcher" "net/netip" policyv1 "github.com/juanfont/headscale/hscontrol/policy/v1" @@ -16,6 +17,8 @@ var ( type PolicyManager interface { Filter() []tailcfg.FilterRule + // Matchers returns the matchers for the current filter rules. + Matchers() []matcher.Match SSHPolicy(*types.Node) (*tailcfg.SSHPolicy, error) SetPolicy([]byte) (bool, error) SetUsers(users []types.User) (bool, error) diff --git a/hscontrol/policy/policy.go b/hscontrol/policy/policy.go index ba375beb..d86de29b 100644 --- a/hscontrol/policy/policy.go +++ b/hscontrol/policy/policy.go @@ -1,6 +1,7 @@ package policy import ( + "github.com/juanfont/headscale/hscontrol/policy/matcher" "net/netip" "slices" @@ -15,7 +16,7 @@ import ( func FilterNodesByACL( node *types.Node, nodes types.Nodes, - filter []tailcfg.FilterRule, + matchers []matcher.Match, ) types.Nodes { var result types.Nodes @@ -24,7 +25,7 @@ func FilterNodesByACL( continue } - if node.CanAccess(filter, nodes[index]) || peer.CanAccess(filter, node) { + if node.CanAccess(matchers, nodes[index]) || peer.CanAccess(matchers, node) { result = append(result, peer) } } diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index cfd38765..597172fb 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -2,6 +2,7 @@ package policy import ( "fmt" + "github.com/juanfont/headscale/hscontrol/policy/matcher" "net/netip" "testing" @@ -1425,10 +1426,11 @@ func TestFilterNodesByACL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + matchers := matcher.MatchesFromFilterRules(tt.args.rules) got := FilterNodesByACL( tt.args.node, tt.args.nodes, - tt.args.rules, + matchers, ) if diff := cmp.Diff(tt.want, got, util.Comparers...); diff != "" { t.Errorf("FilterNodesByACL() unexpected result (-want +got):\n%s", diff) diff --git a/hscontrol/policy/v1/policy.go b/hscontrol/policy/v1/policy.go index 0ac49d04..43efba5d 100644 --- a/hscontrol/policy/v1/policy.go +++ b/hscontrol/policy/v1/policy.go @@ -2,6 +2,7 @@ package v1 import ( "fmt" + "github.com/juanfont/headscale/hscontrol/policy/matcher" "io" "net/netip" "os" @@ -92,6 +93,11 @@ func (pm *PolicyManager) Filter() []tailcfg.FilterRule { return pm.filter } +func (pm *PolicyManager) Matchers() []matcher.Match { + filter := pm.Filter() + return matcher.MatchesFromFilterRules(filter) +} + func (pm *PolicyManager) SSHPolicy(node *types.Node) (*tailcfg.SSHPolicy, error) { pm.mu.Lock() defer pm.mu.Unlock() diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 41f51487..8fbedd06 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -3,6 +3,7 @@ package v2 import ( "encoding/json" "fmt" + "github.com/juanfont/headscale/hscontrol/policy/matcher" "net/netip" "strings" "sync" @@ -22,6 +23,7 @@ type PolicyManager struct { filterHash deephash.Sum filter []tailcfg.FilterRule + matchers []matcher.Match tagOwnerMapHash deephash.Sum tagOwnerMap map[Tag]*netipx.IPSet @@ -69,6 +71,9 @@ func (pm *PolicyManager) updateLocked() (bool, error) { filterChanged := filterHash == pm.filterHash pm.filter = filter pm.filterHash = filterHash + if filterChanged { + pm.matchers = matcher.MatchesFromFilterRules(pm.filter) + } // Order matters, tags might be used in autoapprovers, so we need to ensure // that the map for tag owners is resolved before resolving autoapprovers. @@ -149,6 +154,12 @@ func (pm *PolicyManager) Filter() []tailcfg.FilterRule { return pm.filter } +func (pm *PolicyManager) Matchers() []matcher.Match { + pm.mu.Lock() + defer pm.mu.Unlock() + return pm.matchers +} + // SetUsers updates the users in the policy manager and updates the filter rules. func (pm *PolicyManager) SetUsers(users []types.User) (bool, error) { pm.mu.Lock() diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index c333a148..ebbfdb8b 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -258,18 +258,10 @@ func (node *Node) AppendToIPSet(build *netipx.IPSetBuilder) { } } -func (node *Node) CanAccess(filter []tailcfg.FilterRule, node2 *Node) bool { +func (node *Node) CanAccess(matchers []matcher.Match, node2 *Node) bool { src := node.IPs() allowedIPs := node2.IPs() - // TODO(kradalby): Regenerate this every time the filter change, instead of - // every time we use it. - // Part of #2416 - matchers := make([]matcher.Match, len(filter)) - for i, rule := range filter { - matchers[i] = matcher.MatchFromFilterRule(rule) - } - for _, matcher := range matchers { if !matcher.SrcsContainsIPs(src...) { continue diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index d439d483..9b7c23a9 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -2,6 +2,7 @@ package types import ( "fmt" + "github.com/juanfont/headscale/hscontrol/policy/matcher" "net/netip" "strings" "testing" @@ -116,7 +117,8 @@ func Test_NodeCanAccess(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := tt.node1.CanAccess(tt.rules, &tt.node2) + matchers := matcher.MatchesFromFilterRules(tt.rules) + got := tt.node1.CanAccess(matchers, &tt.node2) if got != tt.want { t.Errorf("canAccess() failed: want (%t), got (%t)", tt.want, got) From d1812eeec9ff7f65b053015cacf27c83f2ac0b06 Mon Sep 17 00:00:00 2001 From: Aras Ergus Date: Wed, 2 Apr 2025 08:16:14 +0200 Subject: [PATCH 2/5] Prevent race condition between rules and matchers --- hscontrol/debug.go | 2 +- hscontrol/mapper/mapper.go | 3 +-- hscontrol/policy/pm.go | 5 ++--- hscontrol/policy/policy_test.go | 2 +- hscontrol/policy/v1/policy.go | 9 ++------- hscontrol/policy/v1/policy_test.go | 3 ++- hscontrol/policy/v2/policy.go | 12 +++--------- hscontrol/policy/v2/policy_test.go | 2 +- 8 files changed, 13 insertions(+), 25 deletions(-) diff --git a/hscontrol/debug.go b/hscontrol/debug.go index 2b245b58..ef28a955 100644 --- a/hscontrol/debug.go +++ b/hscontrol/debug.go @@ -40,7 +40,7 @@ func (h *Headscale) debugHTTPServer() *http.Server { w.Write(pol) })) debug.Handle("filter", "Current filter", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - filter := h.polMan.Filter() + filter, _ := h.polMan.Filter() filterJSON, err := json.MarshalIndent(filter, "", " ") if err != nil { diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 4da94c31..8401d5d4 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -519,7 +519,7 @@ func appendPeerChanges( changed types.Nodes, cfg *types.Config, ) error { - filter := polMan.Filter() + filter, matchers := polMan.Filter() sshPolicy, err := polMan.SSHPolicy(node) if err != nil { @@ -529,7 +529,6 @@ func appendPeerChanges( // If there are filter rules present, see if there are any nodes that cannot // access each-other at all and remove them from the peers. if len(filter) > 0 { - matchers := polMan.Matchers() changed = policy.FilterNodesByACL(node, changed, matchers) } diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index 5df7da76..d19aae52 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -16,9 +16,8 @@ var ( ) type PolicyManager interface { - Filter() []tailcfg.FilterRule - // Matchers returns the matchers for the current filter rules. - Matchers() []matcher.Match + // Filter returns the current filter rules for the entire tailnet and the associated matchers. + Filter() ([]tailcfg.FilterRule, []matcher.Match) SSHPolicy(*types.Node) (*tailcfg.SSHPolicy, error) SetPolicy([]byte) (bool, error) SetUsers(users []types.User) (bool, error) diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index 597172fb..cebda65f 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -770,7 +770,7 @@ func TestReduceFilterRules(t *testing.T) { var err error pm, err = pmf(users, append(tt.peers, tt.node)) require.NoError(t, err) - got := pm.Filter() + got, _ := pm.Filter() got = ReduceFilterRules(tt.node, got) if diff := cmp.Diff(tt.want, got); diff != "" { diff --git a/hscontrol/policy/v1/policy.go b/hscontrol/policy/v1/policy.go index 43efba5d..0394a5b3 100644 --- a/hscontrol/policy/v1/policy.go +++ b/hscontrol/policy/v1/policy.go @@ -87,15 +87,10 @@ func (pm *PolicyManager) updateLocked() (bool, error) { return true, nil } -func (pm *PolicyManager) Filter() []tailcfg.FilterRule { +func (pm *PolicyManager) Filter() ([]tailcfg.FilterRule, []matcher.Match) { pm.mu.Lock() defer pm.mu.Unlock() - return pm.filter -} - -func (pm *PolicyManager) Matchers() []matcher.Match { - filter := pm.Filter() - return matcher.MatchesFromFilterRules(filter) + return pm.filter, matcher.MatchesFromFilterRules(pm.filter) } func (pm *PolicyManager) SSHPolicy(node *types.Node) (*tailcfg.SSHPolicy, error) { diff --git a/hscontrol/policy/v1/policy_test.go b/hscontrol/policy/v1/policy_test.go index e250db2a..f1d899ba 100644 --- a/hscontrol/policy/v1/policy_test.go +++ b/hscontrol/policy/v1/policy_test.go @@ -150,7 +150,8 @@ func TestPolicySetChange(t *testing.T) { assert.Equal(t, tt.wantNodesChange, change) } - if diff := cmp.Diff(tt.wantFilter, pm.Filter()); diff != "" { + filter, _ := pm.Filter() + if diff := cmp.Diff(tt.wantFilter, filter); diff != "" { t.Errorf("TestPolicySetChange() unexpected result (-want +got):\n%s", diff) } }) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 8fbedd06..7712fe5f 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -147,17 +147,11 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { return pm.updateLocked() } -// Filter returns the current filter rules for the entire tailnet. -func (pm *PolicyManager) Filter() []tailcfg.FilterRule { +// Filter returns the current filter rules for the entire tailnet and the associated matchers. +func (pm *PolicyManager) Filter() ([]tailcfg.FilterRule, []matcher.Match) { pm.mu.Lock() defer pm.mu.Unlock() - return pm.filter -} - -func (pm *PolicyManager) Matchers() []matcher.Match { - pm.mu.Lock() - defer pm.mu.Unlock() - return pm.matchers + return pm.filter, pm.matchers } // SetUsers updates the users in the policy manager and updates the filter rules. diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index ee26c596..2bc26760 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -47,7 +47,7 @@ func TestPolicyManager(t *testing.T) { pm, err := NewPolicyManager([]byte(tt.pol), users, tt.nodes) require.NoError(t, err) - filter := pm.Filter() + filter, _ := pm.Filter() if diff := cmp.Diff(filter, tt.wantFilter); diff != "" { t.Errorf("Filter() mismatch (-want +got):\n%s", diff) } From 2073a84e3128d87bcc445c628109fd18e720d082 Mon Sep 17 00:00:00 2001 From: Aras Ergus Date: Wed, 2 Apr 2025 08:46:20 +0200 Subject: [PATCH 3/5] Test also matchers in tests for Policy.Filter --- hscontrol/policy/v1/policy_test.go | 25 +++++++++++++++++++++-- hscontrol/policy/v2/policy_test.go | 32 ++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/hscontrol/policy/v1/policy_test.go b/hscontrol/policy/v1/policy_test.go index f1d899ba..c9f98079 100644 --- a/hscontrol/policy/v1/policy_test.go +++ b/hscontrol/policy/v1/policy_test.go @@ -1,6 +1,7 @@ package v1 import ( + "github.com/juanfont/headscale/hscontrol/policy/matcher" "testing" "github.com/google/go-cmp/cmp" @@ -27,6 +28,7 @@ func TestPolicySetChange(t *testing.T) { wantNodesChange bool wantPolicyChange bool wantFilter []tailcfg.FilterRule + wantMatchers []matcher.Match }{ { name: "set-nodes", @@ -42,6 +44,9 @@ func TestPolicySetChange(t *testing.T) { DstPorts: []tailcfg.NetPortRange{{IP: "100.64.0.1/32", Ports: tailcfg.PortRangeAny}}, }, }, + wantMatchers: []matcher.Match{ + matcher.MatchFromStrings([]string{}, []string{"100.64.0.1/32"}), + }, }, { name: "set-users", @@ -52,6 +57,9 @@ func TestPolicySetChange(t *testing.T) { DstPorts: []tailcfg.NetPortRange{{IP: "100.64.0.1/32", Ports: tailcfg.PortRangeAny}}, }, }, + wantMatchers: []matcher.Match{ + matcher.MatchFromStrings([]string{}, []string{"100.64.0.1/32"}), + }, }, { name: "set-users-and-node", @@ -70,6 +78,9 @@ func TestPolicySetChange(t *testing.T) { DstPorts: []tailcfg.NetPortRange{{IP: "100.64.0.1/32", Ports: tailcfg.PortRangeAny}}, }, }, + wantMatchers: []matcher.Match{ + matcher.MatchFromStrings([]string{"100.64.0.2/32"}, []string{"100.64.0.1/32"}), + }, }, { name: "set-policy", @@ -95,6 +106,9 @@ func TestPolicySetChange(t *testing.T) { DstPorts: []tailcfg.NetPortRange{{IP: "100.64.0.62/32", Ports: tailcfg.PortRangeAny}}, }, }, + wantMatchers: []matcher.Match{ + matcher.MatchFromStrings([]string{"100.64.0.61/32"}, []string{"100.64.0.62/32"}), + }, }, } @@ -150,9 +164,16 @@ func TestPolicySetChange(t *testing.T) { assert.Equal(t, tt.wantNodesChange, change) } - filter, _ := pm.Filter() + filter, matchers := pm.Filter() if diff := cmp.Diff(tt.wantFilter, filter); diff != "" { - t.Errorf("TestPolicySetChange() unexpected result (-want +got):\n%s", diff) + t.Errorf("TestPolicySetChange() unexpected filter (-want +got):\n%s", diff) + } + if diff := cmp.Diff( + tt.wantMatchers, + matchers, + cmp.AllowUnexported(matcher.Match{}), + ); diff != "" { + t.Errorf("TestPolicySetChange() unexpected matchers (-want +got):\n%s", diff) } }) } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 2bc26760..47057bf9 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -1,6 +1,7 @@ package v2 import ( + "github.com/juanfont/headscale/hscontrol/policy/matcher" "testing" "github.com/google/go-cmp/cmp" @@ -29,16 +30,18 @@ func TestPolicyManager(t *testing.T) { } tests := []struct { - name string - pol string - nodes types.Nodes - wantFilter []tailcfg.FilterRule + name string + pol string + nodes types.Nodes + wantFilter []tailcfg.FilterRule + wantMatchers []matcher.Match }{ { - name: "empty-policy", - pol: "{}", - nodes: types.Nodes{}, - wantFilter: nil, + name: "empty-policy", + pol: "{}", + nodes: types.Nodes{}, + wantFilter: nil, + wantMatchers: nil, }, } @@ -47,9 +50,16 @@ func TestPolicyManager(t *testing.T) { pm, err := NewPolicyManager([]byte(tt.pol), users, tt.nodes) require.NoError(t, err) - filter, _ := pm.Filter() - if diff := cmp.Diff(filter, tt.wantFilter); diff != "" { - t.Errorf("Filter() mismatch (-want +got):\n%s", diff) + filter, matchers := pm.Filter() + if diff := cmp.Diff(tt.wantFilter, filter); diff != "" { + t.Errorf("Filter() filter mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff( + tt.wantMatchers, + matchers, + cmp.AllowUnexported(matcher.Match{}), + ); diff != "" { + t.Errorf("Filter() matchers mismatch (-want +got):\n%s", diff) } // TODO(kradalby): Test SSH Policy From c98692a26b448fb50286c7035477fe8375656eda Mon Sep 17 00:00:00 2001 From: Aras Ergus Date: Mon, 7 Apr 2025 16:32:13 +0200 Subject: [PATCH 4/5] Compute `filterChanged` in v2 policy correctly --- hscontrol/policy/v2/policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 7712fe5f..3bd71568 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -68,7 +68,7 @@ func (pm *PolicyManager) updateLocked() (bool, error) { } filterHash := deephash.Hash(&filter) - filterChanged := filterHash == pm.filterHash + filterChanged := filterHash != pm.filterHash pm.filter = filter pm.filterHash = filterHash if filterChanged { From 7de444f5e572164d5a91ac7a7c1950daf4d1e7b4 Mon Sep 17 00:00:00 2001 From: Aras Ergus Date: Mon, 7 Apr 2025 16:50:45 +0200 Subject: [PATCH 5/5] Fix nil vs. empty list issue in v2 policy test --- hscontrol/policy/v2/policy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 47057bf9..b61c5758 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -41,7 +41,7 @@ func TestPolicyManager(t *testing.T) { pol: "{}", nodes: types.Nodes{}, wantFilter: nil, - wantMatchers: nil, + wantMatchers: []matcher.Match{}, }, }