diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index d0abdc920..4d848630f 100644 --- a/client/internal/acl/manager.go +++ b/client/internal/acl/manager.go @@ -22,7 +22,7 @@ type iFaceMapper interface { // Manager is a ACL rules manager type Manager interface { - ApplyFiltering(rules []*mgmProto.FirewallRule, allowByDefault bool) + ApplyFiltering(networkMap *mgmProto.NetworkMap) Stop() } @@ -36,7 +36,7 @@ type DefaultManager struct { // ApplyFiltering firewall rules to the local firewall manager processed by ACL policy. // // If allowByDefault is ture it appends allow ALL traffic rules to input and output chains. -func (d *DefaultManager) ApplyFiltering(rules []*mgmProto.FirewallRule, allowByDefault bool) { +func (d *DefaultManager) ApplyFiltering(networkMap *mgmProto.NetworkMap) { d.mutex.Lock() defer d.mutex.Unlock() @@ -45,11 +45,12 @@ func (d *DefaultManager) ApplyFiltering(rules []*mgmProto.FirewallRule, allowByD return } - var ( - applyFailed bool - newRulePairs = make(map[string][]firewall.Rule) - ) - if allowByDefault { + rules := d.squashAcceptRules(networkMap) + + // if we got empty rules list but management not set networkMap.FirewallRulesIsEmpty flag + // we have old version of management without rules handling, we should allow all traffic + if len(networkMap.FirewallRules) == 0 && !networkMap.FirewallRulesIsEmpty { + log.Warn("this peer is connected to a NetBird Management service with an older version. Allowing all traffic from connected peers") rules = append(rules, &mgmProto.FirewallRule{ PeerIP: "0.0.0.0", @@ -65,14 +66,17 @@ func (d *DefaultManager) ApplyFiltering(rules []*mgmProto.FirewallRule, allowByD }, ) } + + applyFailed := false + newRulePairs := make(map[string][]firewall.Rule) for _, r := range rules { - rules, err := d.protoRuleToFirewallRule(r) + rulePair, err := d.protoRuleToFirewallRule(r) if err != nil { log.Errorf("failed to apply firewall rule: %+v, %v", r, err) applyFailed = true break } - newRulePairs[rules[0].GetRuleID()] = rules + newRulePairs[rulePair[0].GetRuleID()] = rulePair } if applyFailed { log.Error("failed to apply firewall rules, rollback ACL to previous state") @@ -165,7 +169,7 @@ func (d *DefaultManager) addInRules(ip net.IP, protocol firewall.Protocol, port } rules = append(rules, rule) - if shouldSkipInvertedRule(protocol) { + if shouldSkipInvertedRule(protocol, port) { return rules, nil } @@ -185,7 +189,7 @@ func (d *DefaultManager) addOutRules(ip net.IP, protocol firewall.Protocol, port } rules = append(rules, rule) - if shouldSkipInvertedRule(protocol) { + if shouldSkipInvertedRule(protocol, port) { return rules, nil } @@ -196,6 +200,125 @@ func (d *DefaultManager) addOutRules(ip net.IP, protocol firewall.Protocol, port return append(rules, rule), nil } + +// squashAcceptRules does complex logic to convert many rules which allows connection by traffic type +// to all peers in the network man to one rule which just accepts that type of the traffic. +// +// NOTE: It will not squash two rules for same protocol if one covers all peers in the network, +// but other has port definitions or has drop policy. +func (d *DefaultManager) squashAcceptRules(networkMap *mgmProto.NetworkMap) []*mgmProto.FirewallRule { + totalIPs := 0 + for _, p := range networkMap.RemotePeers { + for range p.AllowedIps { + totalIPs++ + } + } + + type protoMatch map[mgmProto.FirewallRuleProtocol]map[string]int + + in := protoMatch{} + out := protoMatch{} + + // this function we use to do calculation, can we squash the rules by protocol or not. + // We summ amount of Peers IP for given protocol we found in original rules list. + // But we zeroed the IP's for protocol if: + // 1. Any of the rule has DROP action type. + // 2. Any of rule contains Port. + // + // We zeroed this to notify squash function that this protocol can't be squashed. + addRuleToCalculationMap := func(i int, r *mgmProto.FirewallRule, protocols protoMatch) { + drop := r.Action == mgmProto.FirewallRule_DROP || r.Port != "" + if drop { + protocols[r.Protocol] = map[string]int{} + return + } + if _, ok := protocols[r.Protocol]; !ok { + protocols[r.Protocol] = map[string]int{} + } + match := protocols[r.Protocol] + + if _, ok := match[r.PeerIP]; ok { + return + } + match[r.PeerIP] = i + } + + for i, r := range networkMap.FirewallRules { + // calculate squash for different directions + if r.Direction == mgmProto.FirewallRule_IN { + addRuleToCalculationMap(i, r, in) + } else { + addRuleToCalculationMap(i, r, out) + } + } + + // order of squashing by protocol is important + // only for ther first element ALL, it must be done first + protocolOrders := []mgmProto.FirewallRuleProtocol{ + mgmProto.FirewallRule_ALL, + mgmProto.FirewallRule_ICMP, + mgmProto.FirewallRule_TCP, + mgmProto.FirewallRule_UDP, + } + + // trace which type of protocols was squashed + squashedRules := []*mgmProto.FirewallRule{} + squashedProtocols := map[mgmProto.FirewallRuleProtocol]struct{}{} + squash := func(matches protoMatch, direction mgmProto.FirewallRuleDirection) { + for _, protocol := range protocolOrders { + if ipset, ok := matches[protocol]; !ok || len(ipset) != totalIPs || len(ipset) < 2 { + // don't squash if : + // 1. Rules not cover all peers in the network + // 2. Rules cover only one peer in the network. + continue + } + + // add special rule 0.0.0.0 which allows all IP's in our firewall implementations + squashedRules = append(squashedRules, &mgmProto.FirewallRule{ + PeerIP: "0.0.0.0", + Direction: direction, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: protocol, + }) + squashedProtocols[protocol] = struct{}{} + + if protocol == mgmProto.FirewallRule_ALL { + // if we have ALL traffic type squashed rule + // it allows all other type of traffic, so we can stop processing + break + } + } + } + + squash(in, mgmProto.FirewallRule_IN) + squash(out, mgmProto.FirewallRule_OUT) + + if len(squashedRules) == 0 { + return networkMap.FirewallRules + } + + // if all protocol was squashed everything is allow and we can ignore all other rules + if _, ok := squashedProtocols[mgmProto.FirewallRule_ALL]; ok { + return squashedRules + } + + var rules []*mgmProto.FirewallRule + // filter out rules which was squashed from final list + // if we also have other not squashed rules. + for i, r := range networkMap.FirewallRules { + if _, ok := squashedProtocols[r.Protocol]; ok { + if m, ok := in[r.Protocol]; ok && m[r.PeerIP] == i { + continue + } else if m, ok := out[r.Protocol]; ok && m[r.PeerIP] == i { + continue + } + } + rules = append(rules, r) + } + + return append(rules, squashedRules...) +} + func convertToFirewallProtocol(protocol mgmProto.FirewallRuleProtocol) firewall.Protocol { switch protocol { case mgmProto.FirewallRule_TCP: @@ -211,8 +334,8 @@ func convertToFirewallProtocol(protocol mgmProto.FirewallRuleProtocol) firewall. } } -func shouldSkipInvertedRule(protocol firewall.Protocol) bool { - return protocol == firewall.ProtocolALL || protocol == firewall.ProtocolICMP +func shouldSkipInvertedRule(protocol firewall.Protocol, port *firewall.Port) bool { + return protocol == firewall.ProtocolALL || protocol == firewall.ProtocolICMP || port == nil } func convertFirewallAction(action mgmProto.FirewallRuleAction) firewall.Action { diff --git a/client/internal/acl/manager_test.go b/client/internal/acl/manager_test.go index fa8d922ac..a92d5a07e 100644 --- a/client/internal/acl/manager_test.go +++ b/client/internal/acl/manager_test.go @@ -10,20 +10,22 @@ import ( ) func TestDefaultManager(t *testing.T) { - fwRules := []*mgmProto.FirewallRule{ - { - PeerIP: "10.93.0.1", - Direction: mgmProto.FirewallRule_OUT, - Action: mgmProto.FirewallRule_ACCEPT, - Protocol: mgmProto.FirewallRule_TCP, - Port: "80", - }, - { - PeerIP: "10.93.0.2", - Direction: mgmProto.FirewallRule_OUT, - Action: mgmProto.FirewallRule_DROP, - Protocol: mgmProto.FirewallRule_UDP, - Port: "53", + networkMap := &mgmProto.NetworkMap{ + FirewallRules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_TCP, + Port: "80", + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_DROP, + Protocol: mgmProto.FirewallRule_UDP, + Port: "53", + }, }, } @@ -44,7 +46,7 @@ func TestDefaultManager(t *testing.T) { defer acl.Stop() t.Run("apply firewall rules", func(t *testing.T) { - acl.ApplyFiltering(fwRules, false) + acl.ApplyFiltering(networkMap) if len(acl.rulesPairs) != 2 { t.Errorf("firewall rules not applied: %v", acl.rulesPairs) @@ -54,20 +56,23 @@ func TestDefaultManager(t *testing.T) { t.Run("add extra rules", func(t *testing.T) { // remove first rule - fwRules = fwRules[1:] - fwRules = append(fwRules, &mgmProto.FirewallRule{ - PeerIP: "10.93.0.3", - Direction: mgmProto.FirewallRule_IN, - Action: mgmProto.FirewallRule_DROP, - Protocol: mgmProto.FirewallRule_ICMP, - }) + networkMap.FirewallRules = networkMap.FirewallRules[1:] + networkMap.FirewallRules = append( + networkMap.FirewallRules, + &mgmProto.FirewallRule{ + PeerIP: "10.93.0.3", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_DROP, + Protocol: mgmProto.FirewallRule_ICMP, + }, + ) existedRulesID := map[string]struct{}{} for id := range acl.rulesPairs { existedRulesID[id] = struct{}{} } - acl.ApplyFiltering(fwRules, false) + acl.ApplyFiltering(networkMap) // we should have one old and one new rule in the existed rules if len(acl.rulesPairs) != 2 { @@ -85,16 +90,183 @@ func TestDefaultManager(t *testing.T) { }) t.Run("handle default rules", func(t *testing.T) { - acl.ApplyFiltering(nil, false) - if len(acl.rulesPairs) != 0 { - t.Errorf("rules should be empty if default not allowed, got: %v rules", len(acl.rulesPairs)) + networkMap.FirewallRules = networkMap.FirewallRules[:0] + + networkMap.FirewallRulesIsEmpty = true + if acl.ApplyFiltering(networkMap); len(acl.rulesPairs) != 0 { + t.Errorf("rules should be empty if FirewallRulesIsEmpty is set, got: %v", len(acl.rulesPairs)) return } - acl.ApplyFiltering(nil, true) + networkMap.FirewallRulesIsEmpty = false + acl.ApplyFiltering(networkMap) if len(acl.rulesPairs) != 2 { - t.Errorf("two default rules should be set if default is allowed, got: %v rules", len(acl.rulesPairs)) + t.Errorf("rules should contain 2 rules if FirewallRulesIsEmpty is not set, got: %v", len(acl.rulesPairs)) return } }) } + +func TestDefaultManagerSquashRules(t *testing.T) { + networkMap := &mgmProto.NetworkMap{ + RemotePeers: []*mgmProto.RemotePeerConfig{ + {AllowedIps: []string{"10.93.0.1"}}, + {AllowedIps: []string{"10.93.0.2"}}, + {AllowedIps: []string{"10.93.0.3"}}, + {AllowedIps: []string{"10.93.0.4"}}, + }, + FirewallRules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.1", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + }, + } + + manager := &DefaultManager{} + rules := manager.squashAcceptRules(networkMap) + if len(rules) != 2 { + t.Errorf("rules should contain 2, got: %v", rules) + return + } + + r := rules[0] + if r.PeerIP != "0.0.0.0" { + t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP) + return + } else if r.Direction != mgmProto.FirewallRule_IN { + t.Errorf("direction should be IN, got: %v", r.Direction) + return + } else if r.Protocol != mgmProto.FirewallRule_ALL { + t.Errorf("protocol should be ALL, got: %v", r.Protocol) + return + } else if r.Action != mgmProto.FirewallRule_ACCEPT { + t.Errorf("action should be ACCEPT, got: %v", r.Action) + return + } + + r = rules[1] + if r.PeerIP != "0.0.0.0" { + t.Errorf("IP should be 0.0.0.0, got: %v", r.PeerIP) + return + } else if r.Direction != mgmProto.FirewallRule_OUT { + t.Errorf("direction should be OUT, got: %v", r.Direction) + return + } else if r.Protocol != mgmProto.FirewallRule_ALL { + t.Errorf("protocol should be ALL, got: %v", r.Protocol) + return + } else if r.Action != mgmProto.FirewallRule_ACCEPT { + t.Errorf("action should be ACCEPT, got: %v", r.Action) + return + } +} + +func TestDefaultManagerSquashRulesNoAffect(t *testing.T) { + networkMap := &mgmProto.NetworkMap{ + RemotePeers: []*mgmProto.RemotePeerConfig{ + {AllowedIps: []string{"10.93.0.1"}}, + {AllowedIps: []string{"10.93.0.2"}}, + {AllowedIps: []string{"10.93.0.3"}}, + {AllowedIps: []string{"10.93.0.4"}}, + }, + FirewallRules: []*mgmProto.FirewallRule{ + { + PeerIP: "10.93.0.1", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.FirewallRule_IN, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_TCP, + }, + { + PeerIP: "10.93.0.1", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.2", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.3", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_ALL, + }, + { + PeerIP: "10.93.0.4", + Direction: mgmProto.FirewallRule_OUT, + Action: mgmProto.FirewallRule_ACCEPT, + Protocol: mgmProto.FirewallRule_UDP, + }, + }, + } + + manager := &DefaultManager{} + if rules := manager.squashAcceptRules(networkMap); len(rules) != len(networkMap.FirewallRules) { + t.Errorf("we should got same amount of rules as intput, got %v", len(rules)) + } +} diff --git a/client/internal/engine.go b/client/internal/engine.go index 79f22de70..228d97d3a 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -637,13 +637,7 @@ func (e *Engine) updateNetworkMap(networkMap *mgmProto.NetworkMap) error { } if e.acl != nil { - // if we got empty rules list but management not set networkMap.FirewallRulesIsEmpty flag - // we have old version of management without rules handling, we should allow all traffic - allowByDefault := len(networkMap.FirewallRules) == 0 && !networkMap.FirewallRulesIsEmpty - if allowByDefault { - log.Warn("this peer is connected to a NetBird Management service with an older version. Allowing all traffic from connected peers") - } - e.acl.ApplyFiltering(networkMap.FirewallRules, allowByDefault) + e.acl.ApplyFiltering(networkMap) } e.networkSerial = serial return nil