FW rule applied incorrectly if port specified is a range
When creating a firewall rule with port specified as a range of values, e.g. [1], conntrack command for deleting current conntrack entries is applied to the first number in the range, e.g. port #8778 in [1], instead of the range of ports 8778:9000. This incorrect behavior occurs because conntrack-tools does not understand the port as a range of values. This patch set fixes that issue by following the same method as done in the netlink implementation in [2]. [1] "neutron firewall-rule-create --protocol tcp --action allow --ip-version 4 --destination-port 8778:9000 --enabled True" [2] https://review.openstack.org/#/c/438445/ Closes-Bug: #1702242 Co-Authored-By: Vu Cong Tuan <tuanvc@vn.fujitsu.com> Change-Id: Ib17db09069a07f35109357d20b67b1acfa85c1a4
This commit is contained in:
Cuong Nguyen
Akihiro Motoki
parent
a6b0f95241
commit
f589293aec
|
|
@ -14,6 +14,7 @@
|
|||
# under the License.
|
||||
|
||||
from neutron.agent.linux import utils as linux_utils
|
||||
from neutron_lib import constants
|
||||
from oslo_log import log as logging
|
||||
|
||||
from neutron_fwaas._i18n import _
|
||||
|
|
@ -22,6 +23,15 @@ from neutron_fwaas.services.firewall.drivers import conntrack_base
|
|||
|
||||
LOG = logging.getLogger(__name__)
|
||||
|
||||
IP_VERSIONS = [constants.IP_VERSION_4, constants.IP_VERSION_6]
|
||||
|
||||
ATTR_POSITIONS = {
|
||||
'icmp': (('type', 5), ('code', 6), ('src', 3), ('dst', 4), ('id', 7)),
|
||||
'icmpv6': (('type', 5), ('code', 6), ('src', 3), ('dst', 4), ('id', 7)),
|
||||
'tcp': (('sport', 6), ('dport', 7), ('src', 4), ('dst', 5)),
|
||||
'udp': (('sport', 5), ('dport', 6), ('src', 3), ('dst', 4))
|
||||
}
|
||||
|
||||
|
||||
class ConntrackLegacy(conntrack_base.ConntrackDriverBase):
|
||||
def initialize(self, execute=None):
|
||||
|
|
@ -34,47 +44,173 @@ class ConntrackLegacy(conntrack_base.ConntrackDriverBase):
|
|||
self._execute_command(cmd)
|
||||
|
||||
def delete_entries(self, rules, namespace):
|
||||
for rule in rules:
|
||||
cmd = self._get_conntrack_cmd_from_rule(rule, namespace)
|
||||
rule_filters = sorted(self._get_filter_from_rule(r) for r in rules)
|
||||
delete_entries = self._get_entries_to_delete(
|
||||
rule_filters, self.list_entries(namespace))
|
||||
for delete_entry in delete_entries:
|
||||
cmd = self._get_conntrack_cmd_from_entry(delete_entry, namespace)
|
||||
self._execute_command(cmd)
|
||||
|
||||
def _execute_command(self, cmd):
|
||||
try:
|
||||
self.execute(cmd,
|
||||
run_as_root=True,
|
||||
check_exit_code=True,
|
||||
extra_ok_codes=[1])
|
||||
output = self.execute(cmd,
|
||||
run_as_root=True,
|
||||
check_exit_code=True,
|
||||
extra_ok_codes=[1])
|
||||
except RuntimeError:
|
||||
msg = _("Failed execute conntrack command %s") % cmd
|
||||
raise RuntimeError(msg)
|
||||
return output
|
||||
|
||||
def _get_conntrack_cmd_from_rule(self, rule, namespace):
|
||||
prefixcmd = (['ip', 'netns', 'exec', namespace]
|
||||
if namespace else [])
|
||||
def list_entries(self, namespace):
|
||||
"""List and parse all conntrack entries
|
||||
|
||||
:param namespace: namespace to get conntrack entries
|
||||
:returns: sorted list of conntrack entries in Python tuple
|
||||
for example: [(4, 'icmp', 8, 0, '1.1.1.1', '2.2.2.2', 1234),
|
||||
(4, 'tcp', 1, 2, '1.1.1.1', '2.2.2.2')]
|
||||
"""
|
||||
parsed_entries = []
|
||||
prefixcmd = ['ip', 'netns', 'exec', namespace] if namespace else []
|
||||
for ip_version in IP_VERSIONS:
|
||||
cmd = prefixcmd + ['conntrack', '-L',
|
||||
'-f', 'ipv' + str(ip_version)]
|
||||
raw_entries = self._execute_command(cmd).splitlines()
|
||||
for raw_entry in raw_entries:
|
||||
parsed_entry = self._parse_entry(raw_entry.split(), ip_version)
|
||||
parsed_entries.append(parsed_entry)
|
||||
return sorted(parsed_entries)
|
||||
|
||||
def _get_conntrack_cmd_from_entry(self, entry, namespace):
|
||||
prefixcmd = ['ip', 'netns', 'exec', namespace] if namespace else []
|
||||
cmd = ['conntrack', '-D']
|
||||
if rule:
|
||||
conntrack_filter = self._get_conntrack_filter_from_rule(rule)
|
||||
exec_cmd = prefixcmd + cmd + conntrack_filter
|
||||
contrack_filter = ['-f', 'ipv' + str(entry[0]), '-p', entry[1]]
|
||||
if entry[1] in ['icmp', 'icmpv6']:
|
||||
contrack_filter.extend(['--icmp-type', entry[2],
|
||||
'--icmp-code', entry[3],
|
||||
'-s', entry[4],
|
||||
'-d', entry[5],
|
||||
'--icmp-id', entry[6]])
|
||||
else:
|
||||
exec_cmd = prefixcmd + cmd
|
||||
contrack_filter.extend(['--sport', entry[2],
|
||||
'--dport', entry[3],
|
||||
'-s', entry[4],
|
||||
'-d', entry[5]])
|
||||
exec_cmd = prefixcmd + cmd + contrack_filter
|
||||
return exec_cmd
|
||||
|
||||
def _get_conntrack_filter_from_rule(self, rule):
|
||||
"""Get conntrack filter from rule
|
||||
def _parse_entry(self, entry, ip_version):
|
||||
"""Parse entry from text to Python tuple
|
||||
|
||||
The key for get conntrack filter is protocol, destination_port
|
||||
and source_port. If we want to take more keys, add to the list.
|
||||
:param entry: conntrack entry in text
|
||||
:param ip_version: ip version 4 or 6
|
||||
:returns: conntrack entry in Python tuple
|
||||
for example: (4, 'tcp', 1, 2, '1.1.1.1', '2.2.2.2')
|
||||
The attributes are ordered to be easy to compare with other entries
|
||||
and compare with firewall rule
|
||||
"""
|
||||
conntrack_filter = []
|
||||
keys = [['-p', 'protocol'], ['-f', 'ip_version'],
|
||||
['--dport', 'destination_port'], ['--sport', 'source_port']]
|
||||
for arg_key, rule_key in keys:
|
||||
val = rule.get(rule_key)
|
||||
if val:
|
||||
if rule_key == 'ip_version':
|
||||
conntrack_filter.append(arg_key)
|
||||
conntrack_filter.append('ipv' + str(val))
|
||||
protocol = entry[0]
|
||||
parsed_entry = [ip_version, protocol]
|
||||
for attr, position in ATTR_POSITIONS[protocol]:
|
||||
val = entry[position].partition('=')[2]
|
||||
parsed_entry.append(int(val) if attr in ['sport', 'dport', 'type',
|
||||
'code', 'id'] else val)
|
||||
return tuple(parsed_entry)
|
||||
|
||||
def _get_entries_to_delete(self, rule_filters, entries):
|
||||
"""Specify conntrack entries to delete
|
||||
|
||||
:param rule_filters: List of filters parsed from firewall rules
|
||||
:param entries: all entries within namespace
|
||||
:returns: conntrack entries to delete
|
||||
"""
|
||||
# List all entries from namespace, they are already parsed
|
||||
# to a list of tuples:
|
||||
# [(4, 'icmp', 8, 0, '1.1.1.1', '2.2.2.2', 1234),
|
||||
# (4, 'tcp', 1, 2, '1.1.1.1', '2.2.2.2')]
|
||||
delete_entries = []
|
||||
entry_index = 0
|
||||
entry_number = len(entries)
|
||||
for rule_filter in rule_filters:
|
||||
while entry_index < entry_number:
|
||||
# Compare entry with rule
|
||||
comp = self._compare_entry_and_rule_filter(
|
||||
rule_filter, entries[entry_index])
|
||||
# Increase entry_index when entry is under rule
|
||||
if comp < 0:
|
||||
entry_index += 1
|
||||
# Append entry to delete_entry if it matches with rule
|
||||
elif comp == 0:
|
||||
delete_entries.append(entries[entry_index])
|
||||
entry_index += 1
|
||||
# Switch to new higher rule
|
||||
else:
|
||||
conntrack_filter.append(arg_key)
|
||||
conntrack_filter.append(val)
|
||||
return conntrack_filter
|
||||
break
|
||||
return delete_entries
|
||||
|
||||
@staticmethod
|
||||
def _get_filter_from_rule(rule):
|
||||
"""Parse the firewall rule to a tuple
|
||||
|
||||
:param rule: firewall rule
|
||||
:returns: a tuple of parsed information
|
||||
"""
|
||||
rule_filter = []
|
||||
keys = ('ip_version', 'protocol',
|
||||
'source_port', 'destination_port',
|
||||
'source_ip_address', 'destination_ip_address')
|
||||
for key in keys:
|
||||
if key in ('source_port', 'destination_port'):
|
||||
port_range = rule.get(key, [])
|
||||
if port_range:
|
||||
port_lower, sep, port_upper = port_range.partition(':')
|
||||
port_upper = port_upper if sep else port_lower
|
||||
port_range = [port_lower, port_upper]
|
||||
rule_filter.append(port_range or [])
|
||||
else:
|
||||
rule_filter.append(rule.get(key, []))
|
||||
return tuple(rule_filter)
|
||||
|
||||
@staticmethod
|
||||
def _compare_entry_and_rule_filter(rule_filter, entry):
|
||||
"""Define that the entry should be deleted or not
|
||||
|
||||
:param rule_filter: filter that is parsed from a firewall rule
|
||||
for example: (4, 'tcp', ['22', '33'], ['44', '55'])
|
||||
:param entry: parsed conntrack entry,
|
||||
for example: (4, 'tcp', 1, 2, '1.1.1.1', '2.2.2.2')
|
||||
:returns: -1 if entry is lower than rule
|
||||
0 if entry matches rule,
|
||||
1 if entry is higher than rule
|
||||
"""
|
||||
ENTRY_IS_LOWER = -1
|
||||
ENTRY_MATCHES = 0
|
||||
ENTRY_IS_HIGHER = 1
|
||||
rule_ip_version = rule_filter[0]
|
||||
if entry[0] < rule_ip_version:
|
||||
return ENTRY_IS_LOWER
|
||||
elif entry[0] > rule_ip_version:
|
||||
return ENTRY_IS_HIGHER
|
||||
rule_protocol = rule_filter[1]
|
||||
if rule_protocol == constants.PROTO_NAME_IPV6_ICMP:
|
||||
rule_protocol = constants.PROTO_NAME_IPV6_ICMP_LEGACY
|
||||
if rule_protocol:
|
||||
if entry[1] < rule_protocol:
|
||||
return ENTRY_IS_LOWER
|
||||
elif entry[1] > rule_protocol:
|
||||
return ENTRY_IS_HIGHER
|
||||
sport_range = rule_filter[2]
|
||||
if sport_range:
|
||||
sport_range = [int(port) for port in sport_range]
|
||||
if entry[2] < min(sport_range[0], sport_range[-1]):
|
||||
return ENTRY_IS_LOWER
|
||||
elif entry[2] > max(sport_range[0], sport_range[-1]):
|
||||
return ENTRY_IS_HIGHER
|
||||
dport_range = rule_filter[3]
|
||||
if dport_range:
|
||||
dport_range = [int(port) for port in dport_range]
|
||||
if entry[3] < min(dport_range[0], dport_range[-1]):
|
||||
return ENTRY_IS_LOWER
|
||||
elif entry[3] > max(dport_range[0], dport_range[-1]):
|
||||
return ENTRY_IS_HIGHER
|
||||
return ENTRY_MATCHES
|
||||
|
|
|
|||
|
|
@ -21,30 +21,66 @@ from neutron_fwaas.services.firewall.drivers.linux import legacy_conntrack
|
|||
|
||||
|
||||
FW_RULES = [
|
||||
{'position': '2',
|
||||
{'position': '1',
|
||||
'protocol': 'icmp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule1'},
|
||||
{'source_port': '1',
|
||||
'destination_port': '2',
|
||||
{'source_port': '0:10',
|
||||
'destination_port': '0:10',
|
||||
'position': '2',
|
||||
'protocol': 'tcp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule2'},
|
||||
{'source_port': '1',
|
||||
'destination_port': '2',
|
||||
{'source_port': '0:10',
|
||||
'destination_port': '0:20',
|
||||
'position': '3',
|
||||
'protocol': 'udp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule3'},
|
||||
{'source_port': '1',
|
||||
'destination_port': '0:10',
|
||||
'position': '4',
|
||||
'protocol': 'tcp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule5'},
|
||||
{'source_port': '0:10',
|
||||
'destination_port': None,
|
||||
'position': '5',
|
||||
'protocol': 'udp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule5'},
|
||||
{'source_port': '1',
|
||||
'destination_port': '3',
|
||||
'position': '6',
|
||||
'protocol': 'tcp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule6'},
|
||||
{'source_port': '1',
|
||||
'destination_port': '2',
|
||||
'position': '7',
|
||||
'protocol': 'udp',
|
||||
'ip_version': 4,
|
||||
'enabled': True,
|
||||
'action': 'reject',
|
||||
'id': 'fake-fw-rule7'},
|
||||
]
|
||||
|
||||
ICMP_ENTRY = (4, 'icmp', 8, 0, '1.1.1.1', '2.2.2.2', '1234')
|
||||
TCP_ENTRY = (4, 'tcp', 1, 2, '1.1.1.1', '2.2.2.2')
|
||||
UDP_ENTRY = (4, 'udp', 1, 2, '1.1.1.1', '2.2.2.2')
|
||||
|
||||
ROUTER_NAMESPACE = 'qrouter-fake-namespace'
|
||||
|
||||
|
||||
|
|
@ -55,6 +91,11 @@ class ConntrackLegacyTestCase(base.BaseTestCase):
|
|||
self.conntrack_driver = legacy_conntrack.ConntrackLegacy()
|
||||
self.conntrack_driver.initialize(execute=self.utils_exec)
|
||||
|
||||
list_entries_mock = mock.patch(
|
||||
'neutron_fwaas.services.firewall.drivers.linux'
|
||||
'.legacy_conntrack.ConntrackLegacy.list_entries')
|
||||
self.list_entries = list_entries_mock.start()
|
||||
|
||||
def test_excecute_command_failed(self):
|
||||
with testtools.ExpectedException(RuntimeError):
|
||||
self.conntrack_driver._execute_command(['fake', 'command'])
|
||||
|
|
@ -70,22 +111,28 @@ class ConntrackLegacyTestCase(base.BaseTestCase):
|
|||
run_as_root=True)
|
||||
|
||||
def test_delete_entries(self):
|
||||
self.conntrack_driver.list_entries.return_value = [
|
||||
ICMP_ENTRY, TCP_ENTRY, UDP_ENTRY]
|
||||
self.conntrack_driver.delete_entries(FW_RULES, ROUTER_NAMESPACE)
|
||||
calls = [
|
||||
mock.call(['ip', 'netns', 'exec', ROUTER_NAMESPACE,
|
||||
'conntrack', '-D', '-p', 'icmp', '-f', 'ipv4'],
|
||||
'conntrack', '-D', '-f', 'ipv4', '-p', 'icmp',
|
||||
'--icmp-type', 8, '--icmp-code', 0,
|
||||
'-s', '1.1.1.1', '-d', '2.2.2.2', '--icmp-id', '1234'],
|
||||
check_exit_code=True,
|
||||
extra_ok_codes=[1],
|
||||
run_as_root=True),
|
||||
mock.call(['ip', 'netns', 'exec', ROUTER_NAMESPACE,
|
||||
'conntrack', '-D', '-p', 'tcp', '-f', 'ipv4',
|
||||
'--dport', '2', '--sport', '1'],
|
||||
'conntrack', '-D', '-f', 'ipv4', '-p', 'tcp',
|
||||
'--sport', 1, '--dport', 2,
|
||||
'-s', '1.1.1.1', '-d', '2.2.2.2'],
|
||||
check_exit_code=True,
|
||||
extra_ok_codes=[1],
|
||||
run_as_root=True),
|
||||
mock.call(['ip', 'netns', 'exec', ROUTER_NAMESPACE,
|
||||
'conntrack', '-D', '-p', 'udp', '-f', 'ipv4',
|
||||
'--dport', '2', '--sport', '1'],
|
||||
'conntrack', '-D', '-f', 'ipv4', '-p', 'udp',
|
||||
'--sport', 1, '--dport', 2,
|
||||
'-s', '1.1.1.1', '-d', '2.2.2.2'],
|
||||
check_exit_code=True,
|
||||
extra_ok_codes=[1],
|
||||
run_as_root=True),
|
||||
|
|
|
|||
|
|
@ -0,0 +1,11 @@
|
|||
---
|
||||
fixes:
|
||||
- |
|
||||
[`bug 1702242 <https://bugs.launchpad.net/neutron/+bug/1702242>`__]
|
||||
Port range specification of a firewall rule now works expectedly
|
||||
with the reference L3 agent based implementation.
|
||||
Previously, when creating a firewall rule with port range like
|
||||
``8778:9000``, the rule was not deleted correctly and only entries
|
||||
associated with the first port number were clean up.
|
||||
Note that this bug is only applied to the reference L3 agent
|
||||
based implementation.
|
||||
Loading…
Reference in New Issue