diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index 427aaa1..658ae0f 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -160,7 +160,7 @@ func fullMapResponse( CollectServices: "false", // TODO: Only send if updated - PacketFilter: rules, + PacketFilter: policy.ReduceFilterRules(machine, rules), UserProfiles: profiles, diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index 009d014..0ca9633 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -433,7 +433,6 @@ func Test_fullMapResponse(t *testing.T) { SrcIPs: []string{"100.64.0.2/32"}, DstPorts: []tailcfg.NetPortRange{ {IP: "100.64.0.1/32", Ports: tailcfg.PortRangeAny}, - {IP: "100.64.0.2/32", Ports: tailcfg.PortRangeAny}, }, }, }, diff --git a/hscontrol/policy/acls.go b/hscontrol/policy/acls.go index 87bf40a..dcf1ae3 100644 --- a/hscontrol/policy/acls.go +++ b/hscontrol/policy/acls.go @@ -132,14 +132,19 @@ func GenerateFilterRules( return []tailcfg.FilterRule{}, &tailcfg.SSHPolicy{}, err } - log.Trace().Interface("ACL", rules).Msg("ACL rules generated") + log.Trace().Interface("ACL", rules).Str("machine", machine.GivenName).Msg("ACL rules") var sshPolicy *tailcfg.SSHPolicy sshRules, err := policy.generateSSHRules(machine, peers) if err != nil { return []tailcfg.FilterRule{}, &tailcfg.SSHPolicy{}, err } - log.Trace().Interface("SSH", sshRules).Msg("SSH rules generated") + + log.Trace(). + Interface("SSH", sshRules). + Str("machine", machine.GivenName). + Msg("SSH rules") + if sshPolicy == nil { sshPolicy = &tailcfg.SSHPolicy{} } @@ -185,9 +190,6 @@ func (pol *ACLPolicy) generateFilterRules( return nil, err } - // record if the rule is actually relevant for the given machine. - isRelevant := false - destPorts := []tailcfg.NetPortRange{} for _, dest := range acl.Destinations { alias, port, err := parseDestination(dest) @@ -203,10 +205,6 @@ func (pol *ACLPolicy) generateFilterRules( return nil, err } - if machine.IPAddresses.InIPSet(expanded) { - isRelevant = true - } - ports, err := expandPorts(port, isWildcard) if err != nil { return nil, err @@ -225,12 +223,6 @@ func (pol *ACLPolicy) generateFilterRules( destPorts = append(destPorts, dests...) } - // if the rule does not apply to the machine we are evaluating, - // do not add it to the list and continue. - if !isRelevant { - continue - } - rules = append(rules, tailcfg.FilterRule{ SrcIPs: srcIPs, DstPorts: destPorts, @@ -241,6 +233,40 @@ func (pol *ACLPolicy) generateFilterRules( return rules, nil } +// ReduceFilterRules takes a machine and a set of rules and removes all rules and destinations +// that are not relevant to that particular node. +func ReduceFilterRules(machine *types.Machine, rules []tailcfg.FilterRule) []tailcfg.FilterRule { + ret := []tailcfg.FilterRule{} + + for _, rule := range rules { + // record if the rule is actually relevant for the given machine. + dests := []tailcfg.NetPortRange{} + + for _, dest := range rule.DstPorts { + expanded, err := util.ParseIPSet(dest.IP, nil) + // Fail closed, if we cant parse it, then we should not allow + // access. + if err != nil { + continue + } + + if machine.IPAddresses.InIPSet(expanded) { + dests = append(dests, dest) + } + } + + if len(dests) > 0 { + ret = append(ret, tailcfg.FilterRule{ + SrcIPs: rule.SrcIPs, + DstPorts: dests, + IPProto: rule.IPProto, + }) + } + } + + return ret +} + func (pol *ACLPolicy) generateSSHRules( machine *types.Machine, peers types.Machines, diff --git a/hscontrol/policy/acls_test.go b/hscontrol/policy/acls_test.go index b0cea58..0808345 100644 --- a/hscontrol/policy/acls_test.go +++ b/hscontrol/policy/acls_test.go @@ -1775,7 +1775,7 @@ func TestACLPolicy_generateFilterRules(t *testing.T) { wantErr: false, }, { - name: "host1-can-reach-host2-no-rules", + name: "host1-can-reach-host2-full", field: field{ pol: ACLPolicy{ ACLs: []ACL{ @@ -1831,40 +1831,6 @@ func TestACLPolicy_generateFilterRules(t *testing.T) { }, wantErr: false, }, - { - name: "host1-can-reach-host2-no-rules", - field: field{ - pol: ACLPolicy{ - ACLs: []ACL{ - { - Action: "accept", - Sources: []string{"100.64.0.1"}, - Destinations: []string{"100.64.0.2:*"}, - }, - }, - }, - }, - args: args{ - machine: types.Machine{ - IPAddresses: types.MachineAddresses{ - netip.MustParseAddr("100.64.0.1"), - netip.MustParseAddr("fd7a:115c:a1e0:ab12:4843:2222:6273:2221"), - }, - User: types.User{Name: "mickael"}, - }, - peers: types.Machines{ - { - IPAddresses: types.MachineAddresses{ - netip.MustParseAddr("100.64.0.2"), - netip.MustParseAddr("fd7a:115c:a1e0:ab12:4843:2222:6273:2222"), - }, - User: types.User{Name: "mickael"}, - }, - }, - }, - want: []tailcfg.FilterRule{}, - wantErr: false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1886,6 +1852,62 @@ func TestACLPolicy_generateFilterRules(t *testing.T) { } } +func TestReduceFilterRules(t *testing.T) { + tests := []struct { + name string + machine types.Machine + peers types.Machines + pol ACLPolicy + want []tailcfg.FilterRule + }{ + { + name: "host1-can-reach-host2-no-rules", + pol: ACLPolicy{ + ACLs: []ACL{ + { + Action: "accept", + Sources: []string{"100.64.0.1"}, + Destinations: []string{"100.64.0.2:*"}, + }, + }, + }, + machine: types.Machine{ + IPAddresses: types.MachineAddresses{ + netip.MustParseAddr("100.64.0.1"), + netip.MustParseAddr("fd7a:115c:a1e0:ab12:4843:2222:6273:2221"), + }, + User: types.User{Name: "mickael"}, + }, + peers: types.Machines{ + { + IPAddresses: types.MachineAddresses{ + netip.MustParseAddr("100.64.0.2"), + netip.MustParseAddr("fd7a:115c:a1e0:ab12:4843:2222:6273:2222"), + }, + User: types.User{Name: "mickael"}, + }, + }, + want: []tailcfg.FilterRule{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rules, _ := tt.pol.generateFilterRules( + &tt.machine, + tt.peers, + ) + + got := ReduceFilterRules(&tt.machine, rules) + + if diff := cmp.Diff(tt.want, got); diff != "" { + log.Trace().Interface("got", got).Msg("result") + t.Errorf("TestReduceFilterRules() unexpected result (-want +got):\n%s", diff) + } + }) + } +} + func Test_getTags(t *testing.T) { type args struct { aclPolicy *ACLPolicy @@ -2030,6 +2052,10 @@ func Test_getTags(t *testing.T) { } func Test_getFilteredByACLPeers(t *testing.T) { + ipComparer := cmp.Comparer(func(x, y netip.Addr) bool { + return x.Compare(y) == 0 + }) + type args struct { machines types.Machines rules []tailcfg.FilterRule @@ -2523,6 +2549,168 @@ func Test_getFilteredByACLPeers(t *testing.T) { }, }, }, + { + name: "p4-host-in-netmap-user2-dest-bug", + args: args{ + machines: []types.Machine{ + { + ID: 1, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.2")}, + Hostname: "user1-2", + User: types.User{Name: "user1"}, + }, + { + ID: 0, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.1")}, + Hostname: "user1-1", + User: types.User{Name: "user1"}, + }, + { + ID: 3, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.4")}, + Hostname: "user2-2", + User: types.User{Name: "user2"}, + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{ + "100.64.0.3/32", + "100.64.0.4/32", + "fd7a:115c:a1e0::3/128", + "fd7a:115c:a1e0::4/128", + }, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.3/32", Ports: tailcfg.PortRangeAny}, + {IP: "100.64.0.4/32", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::3/128", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::4/128", Ports: tailcfg.PortRangeAny}, + }, + }, + { + SrcIPs: []string{ + "100.64.0.1/32", + "100.64.0.2/32", + "fd7a:115c:a1e0::1/128", + "fd7a:115c:a1e0::2/128", + }, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.3/32", Ports: tailcfg.PortRangeAny}, + {IP: "100.64.0.4/32", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::3/128", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::4/128", Ports: tailcfg.PortRangeAny}, + }, + }, + }, + machine: &types.Machine{ + ID: 2, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.3")}, + Hostname: "user-2-1", + User: types.User{Name: "user2"}, + }, + }, + want: []types.Machine{ + { + ID: 1, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.2")}, + Hostname: "user1-2", + User: types.User{Name: "user1"}, + }, + { + ID: 0, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.1")}, + Hostname: "user1-1", + User: types.User{Name: "user1"}, + }, + { + ID: 3, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.4")}, + Hostname: "user2-2", + User: types.User{Name: "user2"}, + }, + }, + }, + { + name: "p4-host-in-netmap-user1-dest-bug", + args: args{ + machines: []types.Machine{ + { + ID: 1, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.2")}, + Hostname: "user1-2", + User: types.User{Name: "user1"}, + }, + { + ID: 2, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.3")}, + Hostname: "user-2-1", + User: types.User{Name: "user2"}, + }, + { + ID: 3, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.4")}, + Hostname: "user2-2", + User: types.User{Name: "user2"}, + }, + }, + rules: []tailcfg.FilterRule{ + { + SrcIPs: []string{ + "100.64.0.1/32", + "100.64.0.2/32", + "fd7a:115c:a1e0::1/128", + "fd7a:115c:a1e0::2/128", + }, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.1/32", Ports: tailcfg.PortRangeAny}, + {IP: "100.64.0.2/32", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::1/128", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::2/128", Ports: tailcfg.PortRangeAny}, + }, + }, + { + SrcIPs: []string{ + "100.64.0.1/32", + "100.64.0.2/32", + "fd7a:115c:a1e0::1/128", + "fd7a:115c:a1e0::2/128", + }, + DstPorts: []tailcfg.NetPortRange{ + {IP: "100.64.0.3/32", Ports: tailcfg.PortRangeAny}, + {IP: "100.64.0.4/32", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::3/128", Ports: tailcfg.PortRangeAny}, + {IP: "fd7a:115c:a1e0::4/128", Ports: tailcfg.PortRangeAny}, + }, + }, + }, + machine: &types.Machine{ + ID: 0, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.1")}, + Hostname: "user1-1", + User: types.User{Name: "user1"}, + }, + }, + want: []types.Machine{ + { + ID: 1, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.2")}, + Hostname: "user1-2", + User: types.User{Name: "user1"}, + }, + { + ID: 2, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.3")}, + Hostname: "user-2-1", + User: types.User{Name: "user2"}, + }, + { + ID: 3, + IPAddresses: []netip.Addr{netip.MustParseAddr("100.64.0.4")}, + Hostname: "user2-2", + User: types.User{Name: "user2"}, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -2531,8 +2719,8 @@ func Test_getFilteredByACLPeers(t *testing.T) { tt.args.machines, tt.args.rules, ) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("filterMachinesByACL() = %v, want %v", got, tt.want) + if diff := cmp.Diff(tt.want, got, ipComparer); diff != "" { + t.Errorf("FilterMachinesByACL() unexpected result (-want +got):\n%s", diff) } }) }