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

@@ -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)]

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.
 # <max length of iptables chain name> - (<binary_name> + '-') = 28-(16+1) = 11

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,
+        if (ip_proto in n_const.SG_PORT_PROTO_NUMS or
-                        constants.PROTO_NUM_TCP, constants.PROTO_NUM_UDP,
+                ip_proto in n_const.SG_PORT_PROTO_NAMES):
-                        constants.PROTO_NUM_UDPLITE]:
             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.
+            # Only the protocols above support ports, raise otherwise.
-            # When min/max are the same it is just a single port.
+            if (rule['port_range_min'] is not None or
-            if (rule['port_range_min'] is not None and
+                    rule['port_range_max'] is not None):
-                    rule['port_range_max'] is not None and
+                port_protocols = (
-                    rule['port_range_min'] != rule['port_range_max']):
+                    ', '.join(s.upper() for s in n_const.SG_PORT_PROTO_NAMES))
-                raise ext_sg.SecurityGroupInvalidProtocolForPortRange(
+                raise ext_sg.SecurityGroupInvalidProtocolForPort(
-                    protocol=ip_proto)
+                    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"""

neutron/extensions/securitygroup.py

@@ -40,10 +40,9 @@ class SecurityGroupInvalidPortRange(exceptions.InvalidInput):
                 "<= port_range_max")
 
 
-class SecurityGroupInvalidProtocolForPortRange(exceptions.InvalidInput):
+class SecurityGroupInvalidProtocolForPort(exceptions.InvalidInput):
-    message = _("Port range cannot be specified for protocol %(protocol)s. "
+    message = _("Ports cannot be specified for protocol %(protocol)s. "
-                "Port range is only supported for "
+                "Ports are only supported for %(valid_port_protocols)s.")
-                "TCP, UDP, UDPLITE, SCTP and DCCP.")
 
 
 class SecurityGroupInvalidPortValue(exceptions.InvalidInput):

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',

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})

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):
+    def test_create_security_group_rule_protocol_as_number_port_bad(self):
-        # Only certain protocols support a SG rule with a port range
+        # 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)

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 <https://bugs.launchpad.net/neutron/+bug/1818385>`_.