Skip to content

Commit 77adfb8

Browse files
gcoxmozgcoxmoz
committed
Make sure v6 ACLs don't get into v4 nftable sets
1 parent 6423dee commit 77adfb8

3 files changed

Lines changed: 75 additions & 34 deletions

File tree

‎netfilter_openvpn.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,10 @@ def _build_firewall_rule_iptables(self, name, usersrcip, protocol, acl):
406406
"""
407407
if self.nf_framework != 'iptables': # pragma: no cover
408408
raise RuntimeError('invalid call into _build_firewall_rule_iptables')
409+
if self.ip_family(name) != self.ip_family(acl.address):
410+
# a v4 set can't take in v6 members, and vice versa
411+
# We should maybe raise here
412+
return
409413
comment = ''
410414
if acl.description:
411415
_commentstring = f'{self.username_is}:{acl.rule} ACL {acl.description}'
@@ -440,6 +444,10 @@ def _build_firewall_rule_nftables(self, name, usersrcip, protocol, acl):
440444
"""
441445
if self.nf_framework != 'nftables': # pragma: no cover
442446
raise RuntimeError('invalid call into _build_firewall_rule_nftables')
447+
if self.ip_family(name) != self.ip_family(acl.address):
448+
# a v4 set can't take in v6 members, and vice versa
449+
# We should maybe raise here
450+
return
443451
if protocol and acl.portstring:
444452
dports = [int(x) for x in acl.portstring.split(',')]
445453
comment = None

‎test/test_netfilter_openvpn_iptables.py‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -305,30 +305,38 @@ def test_30_build_fw_rule(self):
305305
iptables_acl1 = iamvpnlibrary.iamvpnbase.ParsedACL(
306306
rule='', address='5.6.7.8', portstring='80', description='')
307307
with mock.patch.object(self.library, 'iptables') as mock_ipt:
308-
self.library._build_firewall_rule_iptables('chain1', '1.2.3.4', 'tcp', iptables_acl1)
309-
mock_ipt.assert_called_once_with(('-A chain1 -s 1.2.3.4 -d 5.6.7.8 -p tcp '
308+
self.library._build_firewall_rule_iptables('10.20.30.1', '1.2.3.4', 'tcp', iptables_acl1)
309+
mock_ipt.assert_called_once_with(('-A 10.20.30.1 -s 1.2.3.4 -d 5.6.7.8 -p tcp '
310310
'-m multiport --dports 80 -j ACCEPT'))
311311

312312
iptables_acl2 = iamvpnlibrary.iamvpnbase.ParsedACL(
313313
rule='rule2', address='5.6.7.9', portstring='80', description='I HAZ COMMENT')
314314
with mock.patch.object(self.library, 'iptables') as mock_ipt:
315-
self.library._build_firewall_rule_iptables('chain2', '1.2.3.4', 'tcp', iptables_acl2)
316-
mock_ipt.assert_called_once_with(('-A chain2 -s 1.2.3.4 -d 5.6.7.9 -p tcp -m multiport '
315+
self.library._build_firewall_rule_iptables('10.20.30.2', '1.2.3.4', 'tcp', iptables_acl2)
316+
mock_ipt.assert_called_once_with(('-A 10.20.30.2 -s 1.2.3.4 -d 5.6.7.9 -p tcp -m multiport '
317317
'--dports 80 -m comment '
318318
'--comment "bob:rule2 ACL I HAZ COMMENT" '
319319
'-j ACCEPT'))
320320

321321
ipset_acl1 = iamvpnlibrary.iamvpnbase.ParsedACL(
322322
rule='', address='5.6.7.10', portstring='', description='')
323323
with mock.patch.object(self.library, 'ipset') as mock_ips:
324-
self.library._build_firewall_rule_iptables('chain3', '1.2.3.4', '', ipset_acl1)
325-
mock_ips.assert_called_once_with('--add chain3 5.6.7.10')
324+
self.library._build_firewall_rule_iptables('10.20.30.3', '1.2.3.4', '', ipset_acl1)
325+
mock_ips.assert_called_once_with('--add 10.20.30.3 5.6.7.10')
326326

327327
ipset_acl2 = iamvpnlibrary.iamvpnbase.ParsedACL(
328328
rule='rule4', address='5.6.7.11', portstring='', description='IPSET SET SET')
329329
with mock.patch.object(self.library, 'ipset') as mock_ips:
330-
self.library._build_firewall_rule_iptables('chain4', '1.2.3.4', '', ipset_acl2)
331-
mock_ips.assert_called_once_with('--add chain4 5.6.7.11 comment "bob:rule4 ACL IPSET SET SET"')
330+
self.library._build_firewall_rule_iptables('10.20.30.4', '1.2.3.4', '', ipset_acl2)
331+
mock_ips.assert_called_once_with('--add 10.20.30.4 5.6.7.11 comment "bob:rule4 ACL IPSET SET SET"')
332+
333+
# Don't let v6 ACLs into v4 chains
334+
ip_set_acl3 = iamvpnlibrary.iamvpnbase.ParsedACL(
335+
rule='rule8', address='2001:db8:1:2:3:4:5:8', portstring='', description='SOME SET')
336+
with mock.patch.object(self.library, 'ipset') as mock_ips:
337+
self.library._build_firewall_rule_iptables('10.20.30.4', '1.2.3.4', '', ip_set_acl3)
338+
mock_ips.assert_not_called()
339+
332340

333341
def test_31_create_rules(self):
334342
''' Test create_user_rules function '''

‎test/test_netfilter_openvpn_nftables.py‎

Lines changed: 51 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,10 @@ def test_17_update_chain(self):
155155
mock_a.assert_called_once_with()
156156

157157

158-
def test_18_add_chain(self):
159-
''' Test add_chain function '''
158+
def test_18_add_chain_failure(self):
159+
''' Test add_chain handles catastrophic failure '''
160160
self.library.client_ip = '3.4.5.6'
161161

162-
# First assume horrific failure:
163162
with mock.patch.object(self.library, 'chain_exists', side_effect=[True, True]), \
164163
mock.patch.object(self.library, 'del_chain') as mock_delchain, \
165164
mock.patch.object(self.library, 'send_event') as mock_logger:
@@ -168,7 +167,11 @@ def test_18_add_chain(self):
168167
mock_delchain.assert_called_once_with()
169168
self.assertEqual(mock_logger.call_count, 2, 'Unremovable chain fails twice')
170169

171-
# Now, assume simple success:
170+
171+
def test_18_add_chain_simple_add_4(self):
172+
''' Test add_chain function, assume simple success in v4 '''
173+
self.library.client_ip = '3.4.5.6'
174+
172175
with mock.patch.object(self.library, 'chain_exists', return_value=False), \
173176
mock.patch.object(self.library.nft, 'json_cmd') as mock_nft, \
174177
mock.patch.object(self.library, 'get_acls_for_user',
@@ -198,7 +201,11 @@ def test_18_add_chain(self):
198201
{'jump': {'target': '3.4.5.6'}}]}}},
199202
]})
200203

201-
# Now, copy all that again but assume that we had to do housekeeping:
204+
205+
def test_18_add_chain_with_housekeeping_4(self):
206+
''' Test add_chain function, assume that we had to do housekeeping in v4 '''
207+
self.library.client_ip = '3.4.5.6'
208+
202209
with mock.patch.object(self.library, 'chain_exists', side_effect=[True, False]), \
203210
mock.patch.object(self.library.nft, 'json_cmd') as mock_nft, \
204211
mock.patch.object(self.library, 'del_chain') as mock_delchain, \
@@ -219,6 +226,11 @@ def test_18_add_chain(self):
219226
mock_nft.assert_called_once()
220227
mock_block.assert_called_once_with()
221228

229+
230+
def test_18_add_chain_with_failed_add_4(self):
231+
''' Test add_chain function with a failed add in v4'''
232+
self.library.client_ip = '3.4.5.6'
233+
222234
# One last time, but fail the add:
223235
with mock.patch.object(self.library, 'chain_exists', return_value=False), \
224236
mock.patch.object(self.library.nft, 'json_cmd') as mock_nft, \
@@ -241,7 +253,7 @@ def test_18_add_chain(self):
241253
# a weird situation and if you ever stumble over this, look at it closely.
242254

243255

244-
def test_19_del_chain(self):
256+
def test_19_del_chain_4(self):
245257
''' Test del_chain function '''
246258
self.library.client_ip = '2.3.4.5'
247259

@@ -332,19 +344,19 @@ def test_19_del_chain(self):
332344
# or built tests for, because we've not seen them/thought of them.
333345

334346

335-
def test_30_build_fw_rule(self):
347+
def test_30_build_fw_rule_4(self):
336348
''' Test _build_firewall_rule_nftables function '''
337349
self.library.username_is = 'bob'
338350

339351
in_acl1 = iamvpnlibrary.iamvpnbase.ParsedACL(
340352
rule='', address=IPNetwork('5.6.7.8'), portstring='80', description='')
341353
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft:
342354
mock_nft.return_value = (0, '', '')
343-
self.library._build_firewall_rule_nftables('chain1', '1.2.3.4', 'tcp', in_acl1)
355+
self.library._build_firewall_rule_nftables('10.20.30.1', '1.2.3.4', 'tcp', in_acl1)
344356
mock_nft.assert_called_once_with({'nftables': [
345357
{'add': {'rule': {'family': 'inet',
346358
'table': 'openvpn_netfilter',
347-
'chain': 'chain1',
359+
'chain': '10.20.30.1',
348360
'comment': None,
349361
'expr': [{'match': {
350362
'op': '==',
@@ -363,17 +375,17 @@ def test_30_build_fw_rule(self):
363375
mock_nft.return_value = (-1, '', 'someerror')
364376
with self.assertRaises(NftablesFailure,
365377
msg='_build_firewall_rule_nftables raises when a rule add fails'):
366-
self.library._build_firewall_rule_nftables('chain1', '1.2.3.4', 'tcp', in_acl1)
378+
self.library._build_firewall_rule_nftables('10.20.30.1', '1.2.3.4', 'tcp', in_acl1)
367379

368380
in_acl2 = iamvpnlibrary.iamvpnbase.ParsedACL(
369381
rule='rule2', address=IPNetwork('5.6.7.9'), portstring='80', description='I HAZ COMMENT')
370382
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft:
371383
mock_nft.return_value = (0, '', '')
372-
self.library._build_firewall_rule_nftables('chain2', '1.2.3.4', 'tcp', in_acl2)
384+
self.library._build_firewall_rule_nftables('10.20.30.2', '1.2.3.4', 'tcp', in_acl2)
373385
mock_nft.assert_called_once_with({'nftables': [
374386
{'add': {'rule': {'family': 'inet',
375387
'table': 'openvpn_netfilter',
376-
'chain': 'chain2',
388+
'chain': '10.20.30.2',
377389
'comment': 'bob:rule2 ACL I HAZ COMMENT',
378390
'expr': [{'match': {
379391
'op': '==',
@@ -393,11 +405,11 @@ def test_30_build_fw_rule(self):
393405
rule='', address=IPNetwork('5.6.7.0/24'), portstring='', description='')
394406
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft:
395407
mock_nft.return_value = (0, '', '')
396-
self.library._build_firewall_rule_nftables('chain3', '1.2.3.4', '', ip_set_acl1)
408+
self.library._build_firewall_rule_nftables('10.20.30.3', '1.2.3.4', '', ip_set_acl1)
397409
mock_nft.assert_called_once_with({'nftables': [
398410
{'add': {'element': {'family': 'inet',
399411
'table': 'openvpn_netfilter',
400-
'name': 'chain3',
412+
'name': '10.20.30.3',
401413
'elem': [ { 'prefix': {
402414
'addr': '5.6.7.0',
403415
'len': 24 } }
@@ -407,30 +419,43 @@ def test_30_build_fw_rule(self):
407419
mock_nft.return_value = (-1, '', 'someerror')
408420
with self.assertRaises(NftablesFailure,
409421
msg='_build_firewall_rule_nftables raises when a set-element add fails'):
410-
self.library._build_firewall_rule_nftables('chain3', '1.2.3.4', '', ip_set_acl1)
422+
self.library._build_firewall_rule_nftables('10.20.30.3', '1.2.3.4', '', ip_set_acl1)
411423

412424
# comments don't work in nftables sets, so "the output here is the same"
413425
ip_set_acl2 = iamvpnlibrary.iamvpnbase.ParsedACL(
414426
rule='rule4', address=IPNetwork('5.6.7.11'), portstring='', description='IPSET SET SET')
415427
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft:
416428
mock_nft.return_value = (0, '', '')
417-
self.library._build_firewall_rule_nftables('chain4', '1.2.3.4', '', ip_set_acl2)
429+
self.library._build_firewall_rule_nftables('10.20.30.4', '1.2.3.4', '', ip_set_acl2)
418430
mock_nft.assert_called_once_with({'nftables': [
419431
{'add': {'element': {'family': 'inet',
420432
'table': 'openvpn_netfilter',
421-
'name': 'chain4',
433+
'name': '10.20.30.4',
422434
'elem': ['5.6.7.11'] }}}]})
423435

436+
# Don't let v6 ACLs into v4 chains
437+
ip_set_acl3 = iamvpnlibrary.iamvpnbase.ParsedACL(
438+
rule='rule8', address=IPNetwork('2001:db8:1:2:3:4:5:8'), portstring='', description='SOME SET')
439+
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft:
440+
mock_nft.return_value = (0, '', '')
441+
self.library._build_firewall_rule_nftables('10.20.30.4', '1.2.3.4', '', ip_set_acl3)
442+
mock_nft.assert_not_called()
443+
424444

425-
def test_31_create_rules(self):
426-
''' Test create_user_rules function '''
445+
def test_31_create_rules_4(self):
446+
''' Test create_user_rules function in v4 '''
427447
self.library.username_is = 'larry'
428448
self.library.client_ip = '2.3.4.5'
429449

430450
acl1 = iamvpnlibrary.iamvpnbase.ParsedACL(
431451
rule='rule2', address=IPNetwork('5.6.7.9'), portstring='80', description='I HAZ COMMENT')
432452
acl2 = iamvpnlibrary.iamvpnbase.ParsedACL(
433453
rule='rule4', address=IPNetwork('5.6.7.11'), portstring='', description='IPSET SET SET')
454+
# Send in v6 ACLs, but since we're v4 they won't appear at the exit
455+
acl3 = iamvpnlibrary.iamvpnbase.ParsedACL(
456+
rule='rule6', address=IPNetwork('2001:db8:1:2:3:4:5:6'), portstring='80', description='MOAR COMMENTS')
457+
acl4 = iamvpnlibrary.iamvpnbase.ParsedACL(
458+
rule='rule8', address=IPNetwork('2001:db8:1:2:3:4:5:8'), portstring='', description='SOME SET')
434459

435460
# Before we get too far into this, let's do the bad cases.
436461
# What if we can't make a chain/set for this person?
@@ -439,7 +464,7 @@ def test_31_create_rules(self):
439464
mock_nft.return_value = (-1, '', '')
440465
with self.assertRaises(NftablesFailure,
441466
msg='create_user_rules raises when creation fails early'):
442-
self.library.create_user_rules([acl1, acl2])
467+
self.library.create_user_rules([acl1, acl2, acl3, acl4])
443468

444469
# What if we can make a chain/set for this person but then populating it fails?
445470
# skip going to _build_firewall_rule_nftables.. this failure check is to make sure
@@ -451,13 +476,13 @@ def test_31_create_rules(self):
451476
(-1, '', '')]
452477
with self.assertRaises(NftablesFailure,
453478
msg='create_user_rules raises when creation fails in the second phase'):
454-
self.library.create_user_rules([acl1, acl2])
479+
self.library.create_user_rules([acl1, acl2, acl3, acl4])
455480

456481
# and now, the very complicated success route:
457482
with mock.patch.object(self.library.nft, 'json_cmd') as mock_nft, \
458483
mock.patch.object(self.library, '_ensure_nftables_framework') as mock_framework:
459484
mock_nft.return_value = (0, '', '')
460-
self.library.create_user_rules([acl1, acl2])
485+
self.library.create_user_rules([acl1, acl2, acl3, acl4])
461486
mock_framework.assert_called_once()
462487
# A lot just happened. Time to check that nftables did the right things.
463488
# This is not necessarily written not in the order they were invoked.
@@ -657,8 +682,8 @@ def test_32_ensure_nftables_framework(self):
657682
# IMPROVEME: needs bad tests
658683

659684

660-
def test_add_safety(self):
661-
''' Test add_safety_block function '''
685+
def test_add_safety_4(self):
686+
''' Test add_safety_block function in v4 '''
662687
self.library.client_ip = '10.20.30.40'
663688

664689
# Assume an add works:
@@ -685,8 +710,8 @@ def test_add_safety(self):
685710
self.library.add_safety_block()
686711

687712

688-
def test_del_safety(self):
689-
''' Test add_safety_block function '''
713+
def test_del_safety_4(self):
714+
''' Test add_safety_block function in v4 '''
690715
self.library.client_ip = '20.30.40.50'
691716

692717
good_rule_to_delete = {

0 commit comments

Comments
 (0)