From 4350ed3c3556388eaa7f8623ed05b5adc86e9c16 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Fri, 8 Mar 2019 15:24:24 -0500 Subject: [PATCH] Better handle ports in security groups After taking a closer look at bug 1818385, I found a couple of follow-on things to fix in the security group code. First, there are very few protocols that accept ports, especially via iptables. For this reason I think it's acceptable that the API rejects them as invalid. Second, UDPlite has some interesting support in iptables. It does not support using --dport directly, but does using '-m multiport --dports 123', and also supports port ranges using '-m multiport --dports 123:124'. Added code for this special case. Change-Id: Ifb2e6bb6c7a2e2987ba95040ef5a98ed50aa36d4 Closes-Bug: #1818385 --- neutron/agent/linux/iptables_firewall.py | 19 ++++++-------- neutron/common/constants.py | 22 ++++++++++++++++ neutron/db/securitygroups_db.py | 20 +++++++------- neutron/extensions/securitygroup.py | 7 +++-- .../agent/linux/test_iptables_firewall.py | 26 +++++++++++++++++++ .../tests/unit/db/test_securitygroups_db.py | 14 +++++++++- .../unit/extensions/test_securitygroup.py | 17 ++++++------ ...up-port-check-in-api-d1fd84d9663e04ab.yaml | 11 ++++++++ 8 files changed, 102 insertions(+), 34 deletions(-) create mode 100644 releasenotes/notes/stricter-security-group-port-check-in-api-d1fd84d9663e04ab.yaml diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py index bc2a11ab983..dc188591d70 100644 --- a/neutron/agent/linux/iptables_firewall.py +++ b/neutron/agent/linux/iptables_firewall.py @@ -46,15 +46,6 @@ IPSET_DIRECTION = {constants.INGRESS_DIRECTION: 'src', comment_rule = iptables_manager.comment_rule libc = ctypes.CDLL(util.find_library('libc.so.6')) -# iptables protocols that support --dport and --sport -IPTABLES_PORT_PROTOCOLS = [ - constants.PROTO_NAME_DCCP, - constants.PROTO_NAME_SCTP, - constants.PROTO_NAME_TCP, - constants.PROTO_NAME_UDP, - constants.PROTO_NAME_UDPLITE -] - def get_hybrid_port_name(port_name): return (constants.TAP_DEVICE_PREFIX + port_name)[:n_const.LINUX_DEV_LEN] @@ -742,9 +733,15 @@ class IptablesFirewallDriver(firewall.FirewallDriver): # icmp code can be 0 so we cannot use "if port_range_max" here if port_range_max is not None: args[-1] += '/%s' % port_range_max - elif protocol in IPTABLES_PORT_PROTOCOLS: + elif protocol in n_const.SG_PORT_PROTO_NAMES: + # iptables protocols that support --dport, --sport and -m multiport if port_range_min == port_range_max: - args += ['--%s' % direction, '%s' % (port_range_min,)] + if protocol in n_const.IPTABLES_MULTIPORT_ONLY_PROTOCOLS: + # use -m multiport, but without a port range + args += ['-m', 'multiport', '--%ss' % direction, + '%s' % port_range_min] + else: + args += ['--%s' % direction, '%s' % port_range_min] else: args += ['-m', 'multiport', '--%ss' % direction, '%s:%s' % (port_range_min, port_range_max)] diff --git a/neutron/common/constants.py b/neutron/common/constants.py index 3139ac5ee2e..caab2f9d2fc 100644 --- a/neutron/common/constants.py +++ b/neutron/common/constants.py @@ -134,6 +134,28 @@ IPTABLES_PROTOCOL_NAME_MAP = {lib_constants.PROTO_NAME_IPV6_ENCAP: 'ipv6', '141': 'wesp', '142': 'rohc'} +# Security group protocols that support ports +SG_PORT_PROTO_NUMS = [ + lib_constants.PROTO_NUM_DCCP, + lib_constants.PROTO_NUM_SCTP, + lib_constants.PROTO_NUM_TCP, + lib_constants.PROTO_NUM_UDP, + lib_constants.PROTO_NUM_UDPLITE +] + +SG_PORT_PROTO_NAMES = [ + lib_constants.PROTO_NAME_DCCP, + lib_constants.PROTO_NAME_SCTP, + lib_constants.PROTO_NAME_TCP, + lib_constants.PROTO_NAME_UDP, + lib_constants.PROTO_NAME_UDPLITE +] + +# iptables protocols that only support --dport and --sport using -m multiport +IPTABLES_MULTIPORT_ONLY_PROTOCOLS = [ + lib_constants.PROTO_NAME_UDPLITE +] + # A length of a iptables chain name must be less than or equal to 11 # characters. # - ( + '-') = 28-(16+1) = 11 diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index ecec8cf5d6c..885e59e299d 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -473,14 +473,14 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, ip_proto = self._get_ip_proto_number(rule['protocol']) # Not all firewall_driver support all these protocols, # but being strict here doesn't hurt. - if ip_proto in [constants.PROTO_NUM_DCCP, constants.PROTO_NUM_SCTP, - constants.PROTO_NUM_TCP, constants.PROTO_NUM_UDP, - constants.PROTO_NUM_UDPLITE]: + if (ip_proto in n_const.SG_PORT_PROTO_NUMS or + ip_proto in n_const.SG_PORT_PROTO_NAMES): if rule['port_range_min'] == 0 or rule['port_range_max'] == 0: raise ext_sg.SecurityGroupInvalidPortValue(port=0) elif (rule['port_range_min'] is not None and rule['port_range_max'] is not None and rule['port_range_min'] <= rule['port_range_max']): + # When min/max are the same it is just a single port pass else: raise ext_sg.SecurityGroupInvalidPortRange() @@ -496,13 +496,13 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, raise ext_sg.SecurityGroupMissingIcmpType( value=rule['port_range_max']) else: - # Only the protocols above support port ranges, raise otherwise. - # When min/max are the same it is just a single port. - if (rule['port_range_min'] is not None and - rule['port_range_max'] is not None and - rule['port_range_min'] != rule['port_range_max']): - raise ext_sg.SecurityGroupInvalidProtocolForPortRange( - protocol=ip_proto) + # Only the protocols above support ports, raise otherwise. + if (rule['port_range_min'] is not None or + rule['port_range_max'] is not None): + port_protocols = ( + ', '.join(s.upper() for s in n_const.SG_PORT_PROTO_NAMES)) + raise ext_sg.SecurityGroupInvalidProtocolForPort( + protocol=ip_proto, valid_port_protocols=port_protocols) def _validate_ethertype_and_protocol(self, rule): """Check if given ethertype and protocol are valid or not""" diff --git a/neutron/extensions/securitygroup.py b/neutron/extensions/securitygroup.py index a25dc4b5a98..a1e25e09363 100644 --- a/neutron/extensions/securitygroup.py +++ b/neutron/extensions/securitygroup.py @@ -40,10 +40,9 @@ class SecurityGroupInvalidPortRange(exceptions.InvalidInput): "<= port_range_max") -class SecurityGroupInvalidProtocolForPortRange(exceptions.InvalidInput): - message = _("Port range cannot be specified for protocol %(protocol)s. " - "Port range is only supported for " - "TCP, UDP, UDPLITE, SCTP and DCCP.") +class SecurityGroupInvalidProtocolForPort(exceptions.InvalidInput): + message = _("Ports cannot be specified for protocol %(protocol)s. " + "Ports are only supported for %(valid_port_protocols)s.") class SecurityGroupInvalidPortValue(exceptions.InvalidInput): diff --git a/neutron/tests/unit/agent/linux/test_iptables_firewall.py b/neutron/tests/unit/agent/linux/test_iptables_firewall.py index 42d151adf59..2400c181400 100644 --- a/neutron/tests/unit/agent/linux/test_iptables_firewall.py +++ b/neutron/tests/unit/agent/linux/test_iptables_firewall.py @@ -415,6 +415,32 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase): egress = None self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_udplite_port(self): + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': 'udplite', + 'port_range_min': 10, + 'port_range_max': 10} + ingress = mock.call.add_rule( + 'ifake_dev', + '-p udplite -m multiport --dports 10 -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + + def test_filter_ipv4_ingress_udplite_mport(self): + rule = {'ethertype': 'IPv4', + 'direction': 'ingress', + 'protocol': 'udplite', + 'port_range_min': 10, + 'port_range_max': 100} + ingress = mock.call.add_rule( + 'ifake_dev', + '-p udplite -m multiport --dports 10:100 -j RETURN', + top=False, comment=None) + egress = None + self._test_prepare_port_filter(rule, ingress, egress) + def test_filter_ipv4_ingress_protocol_blank(self): rule = {'ethertype': 'IPv4', 'direction': 'ingress', diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 3258ffdc261..bc3a4353e15 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -460,8 +460,20 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): 'port_range_max': 1, 'protocol': constants.PROTO_NAME_UDPLITE}) self.assertRaises( - securitygroup.SecurityGroupInvalidProtocolForPortRange, + securitygroup.SecurityGroupInvalidProtocolForPort, self.mixin._validate_port_range, {'port_range_min': 100, 'port_range_max': 200, 'protocol': '111'}) + self.assertRaises( + securitygroup.SecurityGroupInvalidProtocolForPort, + self.mixin._validate_port_range, + {'port_range_min': 100, + 'port_range_max': None, + 'protocol': constants.PROTO_NAME_VRRP}) + self.assertRaises( + securitygroup.SecurityGroupInvalidProtocolForPort, + self.mixin._validate_port_range, + {'port_range_min': None, + 'port_range_max': 200, + 'protocol': constants.PROTO_NAME_VRRP}) diff --git a/neutron/tests/unit/extensions/test_securitygroup.py b/neutron/tests/unit/extensions/test_securitygroup.py index 643b1f80f83..b0d56b7bc73 100644 --- a/neutron/tests/unit/extensions/test_securitygroup.py +++ b/neutron/tests/unit/extensions/test_securitygroup.py @@ -608,17 +608,18 @@ class TestSecurityGroups(SecurityGroupDBTestCase): self.deserialize(self.fmt, res) self.assertEqual(webob.exc.HTTPCreated.code, res.status_int) - def test_create_security_group_rule_protocol_as_number_with_port(self): + def test_create_security_group_rule_protocol_as_number_with_port_bad(self): + # When specifying ports, neither can be None name = 'webservers' description = 'my webservers' with self.security_group(name, description) as sg: security_group_id = sg['security_group']['id'] - protocol = 111 + protocol = 6 rule = self._build_security_group_rule( - security_group_id, 'ingress', protocol, '70') + security_group_id, 'ingress', protocol, '70', None) res = self._create_security_group_rule(self.fmt, rule) self.deserialize(self.fmt, res) - self.assertEqual(webob.exc.HTTPCreated.code, res.status_int) + self.assertEqual(webob.exc.HTTPBadRequest.code, res.status_int) def test_create_security_group_rule_protocol_as_number_range(self): # This is a SG rule with a port range, but treated as a single @@ -627,22 +628,22 @@ class TestSecurityGroups(SecurityGroupDBTestCase): description = 'my webservers' with self.security_group(name, description) as sg: security_group_id = sg['security_group']['id'] - protocol = 111 + protocol = 6 rule = self._build_security_group_rule( security_group_id, 'ingress', protocol, '70', '70') res = self._create_security_group_rule(self.fmt, rule) self.deserialize(self.fmt, res) self.assertEqual(webob.exc.HTTPCreated.code, res.status_int) - def test_create_security_group_rule_protocol_as_number_range_bad(self): - # Only certain protocols support a SG rule with a port range + def test_create_security_group_rule_protocol_as_number_port_bad(self): + # Only certain protocols support a SG rule with a port name = 'webservers' description = 'my webservers' with self.security_group(name, description) as sg: security_group_id = sg['security_group']['id'] protocol = 111 rule = self._build_security_group_rule( - security_group_id, 'ingress', protocol, '70', '71') + security_group_id, 'ingress', protocol, '70', '70') res = self._create_security_group_rule(self.fmt, rule) self.deserialize(self.fmt, res) self.assertEqual(webob.exc.HTTPBadRequest.code, res.status_int) diff --git a/releasenotes/notes/stricter-security-group-port-check-in-api-d1fd84d9663e04ab.yaml b/releasenotes/notes/stricter-security-group-port-check-in-api-d1fd84d9663e04ab.yaml new file mode 100644 index 00000000000..4d6a6114811 --- /dev/null +++ b/releasenotes/notes/stricter-security-group-port-check-in-api-d1fd84d9663e04ab.yaml @@ -0,0 +1,11 @@ +--- +upgrade: + - | + The Neutron API now enforces that ports are a valid option for + security group rules based on the protocol given, instead of + relying on the backend firewall driver to do this enforcement, + typically silently ignoring the port option in the rule. The + valid set of whitelisted protocols that support ports are TCP, + UDP, UDPLITE, SCTP and DCCP. Ports used with other protocols + will now generate an HTTP 400 error. For more information, see + bug `1818385 `_.