From d17b1a6abbd0d4e68ce305c54f17e90e547f1ba8 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Thu, 22 Feb 2024 18:33:14 -0500 Subject: [PATCH 1/2] Fix iptables mapping of 'ipip' protocol Map 'ipip' to use the string 'ipencap' so the IptablesFirewallDriver class in neutron works correctly. Once neutron-lib is bumped this can be removed. Add tests for IP protocol 'ipip', '4' and '94' to make sure the IptablesFirewallDriver class in neutron treats them correctly. Long description below. This is one of those confusing edge cases and I think Linux is conspiring against us. Let me explain. 1) neutron-lib does correctly define the protocol name 'ipip' as 4. 2) The linux kernel uses the same in in.h: IPPROTO_IPIP = 4 IPPROTO_BEETPH = 94 (?) 3) iptables maps 'ipip' to 94 and 'ipencap' to 4. # for num in {0..255}; do iptables -A INPUT -p $num; done # iptables-save | grep -E 'ipip|ipencap' -A INPUT -p ipencap -A INPUT -p ipip 4) /etc/protocols does the same as iptables: grep -E 'ipencap|ipip' /etc/protocols ipencap 4 IP-ENCAP # IP encapsulated in IP (officially ``IP'') ipip 94 IPIP # IP-within-IP Encapsulation Protocol 5) getprotoby{name|number} does what /etc/protocols does: $ getprotobyname ipip struct protoent: (0x7fbbbcca9c60) p_name ipip p_aliases IPIP p_proto 94 $ getprotobynumber 4 struct protoent: (0x7fc51ad86be0) p_name ipencap p_aliases IP-ENCAP p_proto 4 Neutron actually builds a mapping based on the getprotoby* calls, so in the iptables case it winds-up doing the wrong thing. Partial-bug: #2054324 Change-Id: Icc84b54be07d39059723d6c233c03aa130102423 (cherry picked from commit 793dfb04d087c96470dc4913eb4f17aeff777534) --- neutron/agent/linux/iptables_firewall.py | 4 +++ .../agent/linux/test_iptables_firewall.py | 36 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index 631fcc6bc12..c6e2da8e1be 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -769,6 +769,10 @@ def _protocol_name_map(self): if not self._iptables_protocol_name_map: tmp_map = constants.IPTABLES_PROTOCOL_NAME_MAP.copy() tmp_map.update(self._local_protocol_name_map()) + # TODO(haleyb): remove once neutron-lib with fix is available + # - 'ipip' uses 'ipencap' to match IPPROTO_IPIP from in.h, + # which is IP-ENCAP/'4' in /etc/protocols (see bug #2054324) + tmp_map[constants.PROTO_NAME_IPIP] = 'ipencap' self._iptables_protocol_name_map = tmp_map return self._iptables_protocol_name_map diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index b177c7ee06c..a2bf18c3652 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -489,6 +489,42 @@ def test_filter_ipv4_ingress_protocol_encap_by_num(self): egress = None self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_protocol_ipip(self): + # 'ipip' via the API uses 'ipencap' to match what iptables-save + # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': 'ipip'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipencap -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_ipip_by_num(self): + # '4' via the API uses 'ipencap' to match what iptables-save + # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '4'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipencap -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_protocol_ipencap_by_num(self): + # '94' via the API uses 'ipip' to match what iptables-save + # uses, which is IPIP/'94' from /etc/protocols (see bug #2054324) + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': '94'} + ingress = mock.call.add_rule('ifake_dev', + '-p ipip -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_protocol_999_local(self): # There is no protocol 999, so let's return a mapping # that says there is and make sure the rule is created From 10af328885ac0f85f161c454dd53463cee57bb23 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Tue, 5 Mar 2024 11:59:05 -0500 Subject: [PATCH 2/2] Use the system-dependent string for IP protocol 4 iptables-save uses a system-dependent value, usually that found in /etc/protocols, when 'ipip' is given as the security group protocol. The intent is to always use the string value for IP protocol '4', as iptables-save has no '-n' flag to print values numerically. This updates a previous change (793dfb04d) that hard-coded that string to 'ipencap', which broke CentOS/Fedora, which uses 'ipv4'. For this reason we cannot hard-code anything in neutron-lib, this needs to be added dynamically, so this one-line change needs to stay here, and effectively closes the bug. Closes-bug: #2054324 Change-Id: Ic40b539c9ef5cfa4cbbd6575e19e653342e8342b (cherry picked from commit cd1d191e335dca707ac9324fd81e642cb453a6cf) --- neutron/agent/linux/iptables_firewall.py | 12 +++++--- .../agent/linux/test_iptables_firewall.py | 28 +++++++++++-------- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index c6e2da8e1be..05d78fc1526 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -769,10 +769,14 @@ def _protocol_name_map(self): if not self._iptables_protocol_name_map: tmp_map = constants.IPTABLES_PROTOCOL_NAME_MAP.copy() tmp_map.update(self._local_protocol_name_map()) - # TODO(haleyb): remove once neutron-lib with fix is available - # - 'ipip' uses 'ipencap' to match IPPROTO_IPIP from in.h, - # which is IP-ENCAP/'4' in /etc/protocols (see bug #2054324) - tmp_map[constants.PROTO_NAME_IPIP] = 'ipencap' + # iptables-save uses different strings for 'ipip' (protocol 4) + # depending on the distro, which corresponds to the entry for + # '4' in /etc/protocols. For example: + # - 'ipencap' in Ubuntu + # - 'ipv4' in CentOS/Fedora + # For this reason, we need to map the string for 'ipip' to the + # system-dependent string for '4', see bug #2054324. + tmp_map[constants.PROTO_NAME_IPIP] = tmp_map['4'] self._iptables_protocol_name_map = tmp_map return self._iptables_protocol_name_map diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index a2bf18c3652..fec4e9c1eb9 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -490,37 +490,43 @@ def test_filter_ipv4_ingress_protocol_encap_by_num(self): self._test_prepare_port_filter(rule, ingress, egress) def test_filter_ipv4_ingress_protocol_ipip(self): - # 'ipip' via the API uses 'ipencap' to match what iptables-save - # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + # We want to use what the system-dependent string here is for 'ipip', + # as it could be 'ipencap' or 'ipv4' depending on the distro. + # See bug #2054324. rule = {'ethertype': 'IPv4', 'direction': 'ingress', 'protocol': 'ipip'} + expected_proto_name = self.firewall._iptables_protocol_name('ipip') ingress = mock.call.add_rule('ifake_dev', - '-p ipencap -j RETURN', + '-p %s -j RETURN' % expected_proto_name, top=False, comment=None) egress = None self._test_prepare_port_filter(rule, ingress, egress) - def test_filter_ipv4_ingress_protocol_ipip_by_num(self): - # '4' via the API uses 'ipencap' to match what iptables-save - # uses, which is IP-ENCAP/'4' from /etc/protocols (see bug #2054324) + def test_filter_ipv4_ingress_protocol_4(self): + # We want to use what the system-dependent string here is for '4', + # as it could be 'ipencap' or 'ipv4' depending on the distro. + # See bug #2054324. rule = {'ethertype': 'IPv4', 'direction': 'ingress', 'protocol': '4'} + expected_proto_name = self.firewall._iptables_protocol_name('4') ingress = mock.call.add_rule('ifake_dev', - '-p ipencap -j RETURN', + '-p %s -j RETURN' % expected_proto_name, top=False, comment=None) egress = None self._test_prepare_port_filter(rule, ingress, egress) - def test_filter_ipv4_ingress_protocol_ipencap_by_num(self): - # '94' via the API uses 'ipip' to match what iptables-save - # uses, which is IPIP/'94' from /etc/protocols (see bug #2054324) + def test_filter_ipv4_ingress_protocol_94(self): + # We want to use what the system-dependent string here is for '94', + # as it could be 'ipip' or something else depending on the distro. + # See bug #2054324. rule = {'ethertype': 'IPv4', 'direction': 'ingress', 'protocol': '94'} + expected_proto_name = self.firewall._iptables_protocol_name('94') ingress = mock.call.add_rule('ifake_dev', - '-p ipip -j RETURN', + '-p %s -j RETURN' % expected_proto_name, top=False, comment=None) egress = None self._test_prepare_port_filter(rule, ingress, egress)