From ef590014597bdf661d04fe3369c67ec16256910f Mon Sep 17 00:00:00 2001 From: Givi Khojanashvili Date: Wed, 7 Jun 2023 17:24:27 +0400 Subject: [PATCH] Fix routes allow acl rule (#940) Modify rules in iptables and nftables to accept all traffic not from netbird network but routed through it. --- client/firewall/iptables/manager_linux.go | 41 +++++---- .../firewall/iptables/manager_linux_test.go | 55 +++++++++++- client/firewall/nftables/manager_linux.go | 87 +++++++++++++++++-- .../firewall/nftables/manager_linux_test.go | 67 ++++++++++++-- client/internal/acl/manager.go | 5 +- client/internal/acl/manager_create.go | 2 +- client/internal/acl/manager_create_linux.go | 6 +- client/internal/acl/mocks/README.md | 7 ++ client/internal/acl/mocks/iface_mapper.go | 14 +++ 9 files changed, 245 insertions(+), 39 deletions(-) create mode 100644 client/internal/acl/mocks/README.md diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index 6eec75c64..84e20bd0b 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -11,6 +11,7 @@ import ( log "github.com/sirupsen/logrus" fw "github.com/netbirdio/netbird/client/firewall" + "github.com/netbirdio/netbird/iface" ) const ( @@ -21,12 +22,6 @@ const ( ChainOutputFilterName = "NETBIRD-ACL-OUTPUT" ) -// jumpNetbirdInputDefaultRule always added by manager to the input chain for all trafic from the Netbird interface -var jumpNetbirdInputDefaultRule = []string{"-j", ChainInputFilterName} - -// jumpNetbirdOutputDefaultRule always added by manager to the output chain for all trafic from the Netbird interface -var jumpNetbirdOutputDefaultRule = []string{"-j", ChainOutputFilterName} - // dropAllDefaultRule in the Netbird chain var dropAllDefaultRule = []string{"-j", "DROP"} @@ -37,13 +32,25 @@ type Manager struct { ipv4Client *iptables.IPTables ipv6Client *iptables.IPTables - wgIfaceName string + inputDefaultRuleSpecs []string + outputDefaultRuleSpecs []string + wgIface iFaceMapper +} + +// iFaceMapper defines subset methods of interface required for manager +type iFaceMapper interface { + Name() string + Address() iface.WGAddress } // Create iptables firewall manager -func Create(wgIfaceName string) (*Manager, error) { +func Create(wgIface iFaceMapper) (*Manager, error) { m := &Manager{ - wgIfaceName: wgIfaceName, + wgIface: wgIface, + inputDefaultRuleSpecs: []string{ + "-i", wgIface.Name(), "-j", ChainInputFilterName, "-s", wgIface.Address().String()}, + outputDefaultRuleSpecs: []string{ + "-o", wgIface.Name(), "-j", ChainOutputFilterName, "-d", wgIface.Address().String()}, } // init clients for booth ipv4 and ipv6 @@ -193,11 +200,10 @@ func (m *Manager) reset(client *iptables.IPTables, table string) error { return fmt.Errorf("failed to check if input chain exists: %w", err) } if ok { - specs := append([]string{"-i", m.wgIfaceName}, jumpNetbirdInputDefaultRule...) - if ok, err := client.Exists("filter", "INPUT", specs...); err != nil { + if ok, err := client.Exists("filter", "INPUT", m.inputDefaultRuleSpecs...); err != nil { return err } else if ok { - if err := client.Delete("filter", "INPUT", specs...); err != nil { + if err := client.Delete("filter", "INPUT", m.inputDefaultRuleSpecs...); err != nil { log.WithError(err).Errorf("failed to delete default input rule: %v", err) } } @@ -208,11 +214,10 @@ func (m *Manager) reset(client *iptables.IPTables, table string) error { return fmt.Errorf("failed to check if output chain exists: %w", err) } if ok { - specs := append([]string{"-o", m.wgIfaceName}, jumpNetbirdOutputDefaultRule...) - if ok, err := client.Exists("filter", "OUTPUT", specs...); err != nil { + if ok, err := client.Exists("filter", "OUTPUT", m.outputDefaultRuleSpecs...); err != nil { return err } else if ok { - if err := client.Delete("filter", "OUTPUT", specs...); err != nil { + if err := client.Delete("filter", "OUTPUT", m.outputDefaultRuleSpecs...); err != nil { log.WithError(err).Errorf("failed to delete default output rule: %v", err) } } @@ -296,8 +301,7 @@ func (m *Manager) client(ip net.IP) (*iptables.IPTables, error) { return nil, fmt.Errorf("failed to create default drop all in netbird input chain: %w", err) } - specs := append([]string{"-i", m.wgIfaceName}, jumpNetbirdInputDefaultRule...) - if err := client.AppendUnique("filter", "INPUT", specs...); err != nil { + if err := client.AppendUnique("filter", "INPUT", m.inputDefaultRuleSpecs...); err != nil { return nil, fmt.Errorf("failed to create input chain jump rule: %w", err) } @@ -317,8 +321,7 @@ func (m *Manager) client(ip net.IP) (*iptables.IPTables, error) { return nil, fmt.Errorf("failed to create default drop all in netbird output chain: %w", err) } - specs := append([]string{"-o", m.wgIfaceName}, jumpNetbirdOutputDefaultRule...) - if err := client.AppendUnique("filter", "OUTPUT", specs...); err != nil { + if err := client.AppendUnique("filter", "OUTPUT", m.outputDefaultRuleSpecs...); err != nil { return nil, fmt.Errorf("failed to create output chain jump rule: %w", err) } } diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index bc6e14f92..7c78a4ee2 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -10,14 +10,50 @@ import ( "github.com/stretchr/testify/require" fw "github.com/netbirdio/netbird/client/firewall" + "github.com/netbirdio/netbird/iface" ) +// iFaceMapper defines subset methods of interface required for manager +type iFaceMock struct { + NameFunc func() string + AddressFunc func() iface.WGAddress +} + +func (i *iFaceMock) Name() string { + if i.NameFunc != nil { + return i.NameFunc() + } + panic("NameFunc is not set") +} + +func (i *iFaceMock) Address() iface.WGAddress { + if i.AddressFunc != nil { + return i.AddressFunc() + } + panic("AddressFunc is not set") +} + func TestIptablesManager(t *testing.T) { ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) require.NoError(t, err) + mock := &iFaceMock{ + NameFunc: func() string { + return "lo" + }, + AddressFunc: func() iface.WGAddress { + return iface.WGAddress{ + IP: net.ParseIP("10.20.0.1"), + Network: &net.IPNet{ + IP: net.ParseIP("10.20.0.0"), + Mask: net.IPv4Mask(255, 255, 255, 0), + }, + } + }, + } + // just check on the local interface - manager, err := Create("lo") + manager, err := Create(mock) require.NoError(t, err) time.Sleep(time.Second) @@ -94,10 +130,25 @@ func checkRuleSpecs(t *testing.T, ipv4Client *iptables.IPTables, chainName strin } func TestIptablesCreatePerformance(t *testing.T) { + mock := &iFaceMock{ + NameFunc: func() string { + return "lo" + }, + AddressFunc: func() iface.WGAddress { + return iface.WGAddress{ + IP: net.ParseIP("10.20.0.1"), + Network: &net.IPNet{ + IP: net.ParseIP("10.20.0.0"), + Mask: net.IPv4Mask(255, 255, 255, 0), + }, + } + }, + } + for _, testMax := range []int{10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1000} { t.Run(fmt.Sprintf("Testing %d rules", testMax), func(t *testing.T) { // just check on the local interface - manager, err := Create("lo") + manager, err := Create(mock) require.NoError(t, err) time.Sleep(time.Second) diff --git a/client/firewall/nftables/manager_linux.go b/client/firewall/nftables/manager_linux.go index 7b30cfcd1..e94f93f9e 100644 --- a/client/firewall/nftables/manager_linux.go +++ b/client/firewall/nftables/manager_linux.go @@ -15,6 +15,7 @@ import ( "golang.org/x/sys/unix" fw "github.com/netbirdio/netbird/client/firewall" + "github.com/netbirdio/netbird/iface" ) const ( @@ -42,14 +43,20 @@ type Manager struct { filterInputChainIPv6 *nftables.Chain filterOutputChainIPv6 *nftables.Chain - wgIfaceName string + wgIface iFaceMapper +} + +// iFaceMapper defines subset methods of interface required for manager +type iFaceMapper interface { + Name() string + Address() iface.WGAddress } // Create nftables firewall manager -func Create(wgIfaceName string) (*Manager, error) { +func Create(wgIface iFaceMapper) (*Manager, error) { m := &Manager{ - conn: &nftables.Conn{}, - wgIfaceName: wgIfaceName, + conn: &nftables.Conn{}, + wgIface: wgIface, } if err := m.Reset(); err != nil { @@ -109,7 +116,7 @@ func (m *Manager) AddFiltering( &expr.Cmp{ Op: expr.CmpOpEq, Register: 1, - Data: ifname(m.wgIfaceName), + Data: ifname(m.wgIface.Name()), }, } @@ -358,15 +365,82 @@ func (m *Manager) createChainIfNotExists( chain = m.conn.AddChain(chain) ifaceKey := expr.MetaKeyIIFNAME + shiftDSTAddr := 0 if name == FilterOutputChainName { ifaceKey = expr.MetaKeyOIFNAME + shiftDSTAddr = 1 } + expressions := []expr.Any{ &expr.Meta{Key: ifaceKey, Register: 1}, &expr.Cmp{ Op: expr.CmpOpEq, Register: 1, - Data: ifname(m.wgIfaceName), + Data: ifname(m.wgIface.Name()), + }, + } + + mask, _ := netip.AddrFromSlice(m.wgIface.Address().Network.Mask) + if m.wgIface.Address().IP.To4() == nil { + ip, _ := netip.AddrFromSlice(m.wgIface.Address().Network.IP.To16()) + expressions = append(expressions, + &expr.Payload{ + DestRegister: 2, + Base: expr.PayloadBaseNetworkHeader, + Offset: uint32(8 + (16 * shiftDSTAddr)), + Len: 16, + }, + &expr.Bitwise{ + SourceRegister: 2, + DestRegister: 2, + Len: 16, + Xor: []byte{0x0, 0x0, 0x0, 0x0}, + Mask: mask.Unmap().AsSlice(), + }, + &expr.Cmp{ + Op: expr.CmpOpNeq, + Register: 2, + Data: ip.Unmap().AsSlice(), + }, + &expr.Verdict{Kind: expr.VerdictAccept}, + ) + } else { + ip, _ := netip.AddrFromSlice(m.wgIface.Address().Network.IP.To4()) + expressions = append(expressions, + &expr.Payload{ + DestRegister: 2, + Base: expr.PayloadBaseNetworkHeader, + Offset: uint32(12 + (4 * shiftDSTAddr)), + Len: 4, + }, + &expr.Bitwise{ + SourceRegister: 2, + DestRegister: 2, + Len: 4, + Xor: []byte{0x0, 0x0, 0x0, 0x0}, + Mask: m.wgIface.Address().Network.Mask, + }, + &expr.Cmp{ + Op: expr.CmpOpNeq, + Register: 2, + Data: ip.Unmap().AsSlice(), + }, + &expr.Verdict{Kind: expr.VerdictAccept}, + ) + } + + _ = m.conn.AddRule(&nftables.Rule{ + Table: table, + Chain: chain, + Exprs: expressions, + }) + + expressions = []expr.Any{ + &expr.Meta{Key: ifaceKey, Register: 1}, + &expr.Cmp{ + Op: expr.CmpOpEq, + Register: 1, + Data: ifname(m.wgIface.Name()), }, &expr.Verdict{Kind: expr.VerdictDrop}, } @@ -375,7 +449,6 @@ func (m *Manager) createChainIfNotExists( Chain: chain, Exprs: expressions, }) - if err := m.conn.Flush(); err != nil { return nil, err } diff --git a/client/firewall/nftables/manager_linux_test.go b/client/firewall/nftables/manager_linux_test.go index 558ff005f..9c0d247c5 100644 --- a/client/firewall/nftables/manager_linux_test.go +++ b/client/firewall/nftables/manager_linux_test.go @@ -13,11 +13,47 @@ import ( "golang.org/x/sys/unix" fw "github.com/netbirdio/netbird/client/firewall" + "github.com/netbirdio/netbird/iface" ) +// iFaceMapper defines subset methods of interface required for manager +type iFaceMock struct { + NameFunc func() string + AddressFunc func() iface.WGAddress +} + +func (i *iFaceMock) Name() string { + if i.NameFunc != nil { + return i.NameFunc() + } + panic("NameFunc is not set") +} + +func (i *iFaceMock) Address() iface.WGAddress { + if i.AddressFunc != nil { + return i.AddressFunc() + } + panic("AddressFunc is not set") +} + func TestNftablesManager(t *testing.T) { + mock := &iFaceMock{ + NameFunc: func() string { + return "lo" + }, + AddressFunc: func() iface.WGAddress { + return iface.WGAddress{ + IP: net.ParseIP("100.96.0.1"), + Network: &net.IPNet{ + IP: net.ParseIP("100.96.0.0"), + Mask: net.IPv4Mask(255, 255, 255, 0), + }, + } + }, + } + // just check on the local interface - manager, err := Create("lo") + manager, err := Create(mock) require.NoError(t, err) time.Sleep(time.Second) @@ -44,8 +80,11 @@ func TestNftablesManager(t *testing.T) { rules, err := testClient.GetRules(manager.tableIPv4, manager.filterInputChainIPv4) require.NoError(t, err, "failed to get rules") - // 1 regular rule and other "drop all rule" for the interface - require.Len(t, rules, 2, "expected 1 rule") + // test expectations: + // 1) regular rule + // 2) "accept extra routed traffic rule" for the interface + // 3) "drop all rule" for the interface + require.Len(t, rules, 3, "expected 3 rules") ipToAdd, _ := netip.AddrFromSlice(ip) add := ipToAdd.Unmap() @@ -98,17 +137,35 @@ func TestNftablesManager(t *testing.T) { rules, err = testClient.GetRules(manager.tableIPv4, manager.filterInputChainIPv4) require.NoError(t, err, "failed to get rules") - require.Len(t, rules, 1, "expected 1 rules after deleteion") + // test expectations: + // 1) "accept extra routed traffic rule" for the interface + // 2) "drop all rule" for the interface + require.Len(t, rules, 2, "expected 2 rules after deleteion") err = manager.Reset() require.NoError(t, err, "failed to reset") } func TestNFtablesCreatePerformance(t *testing.T) { + mock := &iFaceMock{ + NameFunc: func() string { + return "lo" + }, + AddressFunc: func() iface.WGAddress { + return iface.WGAddress{ + IP: net.ParseIP("100.96.0.1"), + Network: &net.IPNet{ + IP: net.ParseIP("100.96.0.0"), + Mask: net.IPv4Mask(255, 255, 255, 0), + }, + } + }, + } + for _, testMax := range []int{10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1000} { t.Run(fmt.Sprintf("Testing %d rules", testMax), func(t *testing.T) { // just check on the local interface - manager, err := Create("lo") + manager, err := Create(mock) require.NoError(t, err) time.Sleep(time.Second) diff --git a/client/internal/acl/manager.go b/client/internal/acl/manager.go index 3fe77d0c9..dcc682d3d 100644 --- a/client/internal/acl/manager.go +++ b/client/internal/acl/manager.go @@ -14,9 +14,10 @@ import ( mgmProto "github.com/netbirdio/netbird/management/proto" ) -// iFaceMapper defines subset methods of interface required for manager -type iFaceMapper interface { +// IFaceMapper defines subset methods of interface required for manager +type IFaceMapper interface { Name() string + Address() iface.WGAddress IsUserspaceBind() bool SetFiltering(iface.PacketFilter) error } diff --git a/client/internal/acl/manager_create.go b/client/internal/acl/manager_create.go index 335bfcac0..4987ec587 100644 --- a/client/internal/acl/manager_create.go +++ b/client/internal/acl/manager_create.go @@ -11,7 +11,7 @@ import ( ) // Create creates a firewall manager instance -func Create(iface iFaceMapper) (manager *DefaultManager, err error) { +func Create(iface IFaceMapper) (manager *DefaultManager, err error) { if iface.IsUserspaceBind() { // use userspace packet filtering firewall fm, err := uspfilter.Create(iface) diff --git a/client/internal/acl/manager_create_linux.go b/client/internal/acl/manager_create_linux.go index 536f1ce4d..168114fc2 100644 --- a/client/internal/acl/manager_create_linux.go +++ b/client/internal/acl/manager_create_linux.go @@ -10,7 +10,7 @@ import ( ) // Create creates a firewall manager instance for the Linux -func Create(iface iFaceMapper) (manager *DefaultManager, err error) { +func Create(iface IFaceMapper) (manager *DefaultManager, err error) { var fm firewall.Manager if iface.IsUserspaceBind() { // use userspace packet filtering firewall @@ -19,10 +19,10 @@ func Create(iface iFaceMapper) (manager *DefaultManager, err error) { return nil, err } } else { - if fm, err = nftables.Create(iface.Name()); err != nil { + if fm, err = nftables.Create(iface); err != nil { log.Debugf("failed to create nftables manager: %s", err) // fallback to iptables - if fm, err = iptables.Create(iface.Name()); err != nil { + if fm, err = iptables.Create(iface); err != nil { log.Errorf("failed to create iptables manager: %s", err) return nil, err } diff --git a/client/internal/acl/mocks/README.md b/client/internal/acl/mocks/README.md new file mode 100644 index 000000000..f02c7e67c --- /dev/null +++ b/client/internal/acl/mocks/README.md @@ -0,0 +1,7 @@ +## Mocks + +To generate (or refresh) mocks from acl package please install [mockgen](https://github.com/golang/mock). +Run this command from the `./client/internal/acl` folder to update iface mapper interface mock: +```bash +mockgen -destination mocks/iface_mapper.go -package mocks . IFaceMapper +``` diff --git a/client/internal/acl/mocks/iface_mapper.go b/client/internal/acl/mocks/iface_mapper.go index cd4fb75ee..cdfa7c46a 100644 --- a/client/internal/acl/mocks/iface_mapper.go +++ b/client/internal/acl/mocks/iface_mapper.go @@ -34,6 +34,20 @@ func (m *MockIFaceMapper) EXPECT() *MockIFaceMapperMockRecorder { return m.recorder } +// Address mocks base method. +func (m *MockIFaceMapper) Address() iface.WGAddress { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Address") + ret0, _ := ret[0].(iface.WGAddress) + return ret0 +} + +// Address indicates an expected call of Address. +func (mr *MockIFaceMapperMockRecorder) Address() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Address", reflect.TypeOf((*MockIFaceMapper)(nil).Address)) +} + // IsUserspaceBind mocks base method. func (m *MockIFaceMapper) IsUserspaceBind() bool { m.ctrl.T.Helper()