Remove disable option for default FWG and allow only on VM ports

Currently, auto associate default FWG works only one time and the logic
is broken if the new port is a DHCP port or router port. This patch
fixes the problem by validating if a port is a VM port or not,
ignores port binding failed or unbound and also adds trusted port
handling. In addition, for security perspective,
'auto_associate_default_firewall_group' CfgOpt is no longer used.
Automatic association with default firewall group with VM port
works by default.

Closes-Bug: #1746404
Co-Authored-By: Yushiro FURUKAWA<y.furukawa_2@jp.fujitsu.com>
Co-Authored-By: Chandan Dutta Chowdhury<chandanc@juniper.net>
Change-Id: Ib567c0e0333335a99b851162d87f17f1a8ceb2dd

neutron_fwaas/db/firewall/v2/firewall_db_v2.py

@@ -1108,16 +1108,30 @@ class Firewall_db_mixin_v2(fw_ext.Firewallv2PluginBase, base_db.CommonDbMixin):
         query = self._model_query(context, DefaultFirewallGroup)
         def_fwg_id = query.filter_by(
             project_id=project_id).one().firewall_group_id
-        return self._get_firewall_group(context, def_fwg_id)
+        return self.get_firewall_group(context, def_fwg_id)
 
-    def set_port_for_default_firewall_group(self, context, port, project_id):
+    def set_port_for_default_firewall_group(self, context, port_id,
+                                            project_id):
+        """Set a port into default firewall group
+
+        :param context: Context object
+        :param port_id: Port ID
+        :param project_id: ProjectID
+        """
         with context.session.begin(subtransactions=True):
             def_fwg_db = self._get_default_fwg(context, project_id)
-            new_ports = [p.port_id for p in def_fwg_db['ports']] + [port]
-            fwg_ports = {'ports': new_ports}
-            self._set_ports_for_firewall_group(context, def_fwg_db, fwg_ports)
-            return self._make_firewall_group_dict_with_rules(
-                context, def_fwg_db['id'])
+            try:
+                self._set_ports_for_firewall_group(
+                    context, def_fwg_db, {'ports': [port_id]})
+            except f_exc.FirewallGroupPortInUse:
+                LOG.warning("Port %s has been already associated with default "
+                            "firewall group %s and skip association", port_id,
+                            def_fwg_db['id'])
+            else:
+                # Update default fwg status to PENDING_UPDATE to wait updating
+                # from agent
+                self.update_firewall_group_status(
+                    context, def_fwg_db['id'], nl_constants.PENDING_UPDATE)
 
 
 def _is_default(fwg_db):

neutron_fwaas/services/firewall/agents/l2/fwaas_v2.py

@@ -1,4 +1,4 @@
-# Copyright 2017 FUJITSU LIMITED
+# Copyright 2017-2018 FUJITSU LIMITED
 #
 # Licensed under the Apache License, Version 2.0 (the "License"); you may
 # not use this file except in compliance with the License. You may obtain
@@ -24,6 +24,7 @@ from neutron.plugins.ml2.drivers.openvswitch.agent import vlanmanager
 from neutron_lib.agent import l2_extension
 from neutron_lib import constants as nl_const
 from neutron_lib.exceptions import firewall_v2 as f_exc
+from neutron_lib.utils import net as nl_net
 
 from neutron_fwaas._i18n import _
 from neutron_fwaas.common import fwaas_constants as consts
@@ -288,6 +289,9 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
     @lockutils.synchronized('fwg')
     def create_firewall_group(self, context, firewall_group, host):
         """Handles create firewall group event"""
+
+        # TODO(chandanc): Fix agent RPC endpoint to remove host arg
+        host = cfg.CONF.host
         with self.driver.defer_apply():
             try:
                 self._create_firewall_group(context, firewall_group, host)
@@ -300,6 +304,9 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
     @lockutils.synchronized('fwg')
     def delete_firewall_group(self, context, firewall_group, host):
         """Handles delete firewall group event"""
+
+        # TODO(chandanc): Fix agent RPC endpoint to remove host arg
+        host = cfg.CONF.host
         with self.driver.defer_apply():
             try:
                 self._delete_firewall_group(context, firewall_group, host)
@@ -312,6 +319,9 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
     @lockutils.synchronized('fwg')
     def update_firewall_group(self, context, firewall_group, host):
         """Handles update firewall group event"""
+
+        # TODO(chandanc): Fix agent RPC endpoint to remove host arg
+        host = cfg.CONF.host
         with self.driver.defer_apply():
             try:
                 self._delete_firewall_group(
@@ -328,6 +338,12 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
     def handle_port(self, context, port):
         """Handle port update event"""
 
+        # Check if port is trusted and called at once.
+        if nl_net.is_port_trusted(port) and not self.fwg_map.get_port(port):
+            self._add_rule_for_trusted_port(port)
+            self.fwg_map.set_port(port)
+            return
+
         if not self._is_port_layer2(port):
             return
 
@@ -348,6 +364,13 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
         self._send_fwg_status(
             context, fwg_id=fwg['id'], status=status, host=self.conf.host)
 
+    def _add_rule_for_trusted_port(self, port):
+        self._add_local_vlan_to_ports([port])
+        self.driver.process_trusted_ports([port])
+
+    def _delete_rule_for_trusted_port(self, port):
+        self.driver.remove_trusted_ports([port['port_id']])
+
     def delete_port(self, context, port):
         """This is being called when a port is deleted by the agent. """
 
@@ -358,6 +381,12 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
             return
 
         port = self.fwg_map.get_port(port)
+
+        if port and nl_net.is_port_trusted(port):
+            self._delete_rule_for_trusted_port(port)
+            self.fwg_map.remove_port(port)
+            return
+
         if not self._is_port_layer2(port):
             return
 
@@ -381,11 +410,13 @@ class FWaaSV2AgentExtension(l2_extension.L2AgentExtension):
 
 
 class PortFirewallGroupMap(object):
-    """Store relations between Port and FirewallGroup
+    """Store relations between Port and Firewall Group and trusted port
 
     This map is used in deleting firewall_group because the firewall_group has
     been deleted at that time.  Therefore, it is impossible to refer 'ports'.
     This map enables to refer 'ports' for specified firewall_group.
+    Furthermore, it is necessary to check 'device_owner' for trusted port, this
+    Map also stores trusted port data.
     """
     def __init__(self):
         self.known_fwgs = {}
@@ -412,6 +443,11 @@ class PortFirewallGroupMap(object):
         if fwg_id:
             return self.get_fwg(fwg_id)
 
+    def set_port(self, port):
+        """Add a new port into port_detail"""
+        port_id = self.port_id(port)
+        self.port_detail[port_id] = port
+
     def set_port_fwg(self, port, fwg):
         """Add a new port into fwg['ports']"""
         port_id = self.port_id(port)
@@ -436,6 +472,11 @@ class PortFirewallGroupMap(object):
         if port_id in self.port_fwg:
             fwg_id = self.port_fwg.get(port_id)
             if not fwg_id:
+                # This case is trusted port. Try to delete port_detail dict
+                try:
+                    del self.port_detail[port_id]
+                except KeyError:
+                    pass
                 return
             new_fwg = self.known_fwgs[fwg_id]
             new_fwg['ports'] = [p for p in new_fwg['ports'] if p != port_id]

neutron_fwaas/services/firewall/drivers/linux/l2/noop/noop_driver.py

@@ -30,3 +30,11 @@ class NoopFirewallL2Driver(driver_base.FirewallL2DriverBase):
     @log_helpers.log_method_call
     def delete_firewall_group(self, ports, firewall_group):
         pass
+
+    @log_helpers.log_method_call
+    def process_trusted_ports(self, ports):
+        pass
+
+    @log_helpers.log_method_call
+    def remove_trusted_ports(self, port_ids):
+        pass

neutron_fwaas/services/firewall/drivers/linux/l2/openvswitch_firewall/firewall.py

@@ -445,6 +445,21 @@ class OVSFirewallDriver(driver_base.FirewallL2DriverBase):
 
             self.fwg_port_map.delete_fwg(fwg_id)
 
+    def process_trusted_ports(self, ports):
+        """Pass packets from these ports directly to ingress pipeline."""
+        if self.sg_with_ovs:
+            return
+
+        for port in ports:
+            self._initialize_egress_no_port_security(port)
+
+    def remove_trusted_ports(self, port_ids):
+        if self.sg_with_ovs:
+            return
+
+        for port_id in port_ids:
+            self._remove_egress_no_port_security(port_id)
+
     # NOTE(ivasilevskaya) That's a copy-paste from neutron ovsfw driver
     def filter_defer_apply_on(self):
         self._deferred = True
@@ -1001,8 +1016,8 @@ class OVSFirewallDriver(driver_base.FirewallL2DriverBase):
         self._delete_flows(reg_port=port.ofport)
 
     def create_firewall_group(self, ports_for_fwg, firewall_group):
-        ingress_rules = firewall_group['egress_rule_list']
-        egress_rules = firewall_group['ingress_rule_list']
+        egress_rules = firewall_group['egress_rule_list']
+        ingress_rules = firewall_group['ingress_rule_list']
         fwg_id = firewall_group['id']
 
         self.update_firewall_group_rules(fwg_id, ingress_rules, egress_rules)

neutron_fwaas/services/firewall/fwaas_plugin_v2.py

@@ -34,14 +34,6 @@ from neutron_fwaas.db.firewall.v2 import firewall_db_v2
 
 LOG = logging.getLogger(__name__)
 
-FW_V2_OPTS = [
-    cfg.BoolOpt(
-        'auto_associate_default_firewall_group',
-        default=False,
-        help=("Apply default fwg to all new VM ports within a project")),
-]
-cfg.CONF.register_opts(FW_V2_OPTS, 'fwaas')
-
 
 def add_provider_configuration(type_manager, service_type):
     type_manager.add_provider_configuration(
@@ -185,10 +177,6 @@ class FirewallPluginV2(
             cfg.CONF.host
         )
 
-        self.apply_default_fwg = (
-            cfg.CONF.fwaas.auto_associate_default_firewall_group
-        )
-
     @property
     def _core_plugin(self):
         return directory.get_plugin()
@@ -300,22 +288,30 @@ class FirewallPluginV2(
         """Returns an ID of project for specified port_id. """
         return self._core_plugin.get_port(context, port_id)['project_id']
 
-    @registry.receives(resources.PORT, [events.AFTER_CREATE])
-    def handle_create_port_event(self, resource, event, trigger, **kwargs):
+    @registry.receives(resources.PORT, [events.AFTER_UPDATE])
+    def handle_update_port(self, resource, event, trigger, **kwargs):
 
-        if not self.apply_default_fwg:
+        updated_port = kwargs['port']
+        if not updated_port['device_owner'].startswith(
+                nl_constants.DEVICE_OWNER_COMPUTE_PREFIX):
+            return
+
+        if (kwargs.get('original_port')[pb_def.VIF_TYPE] !=
+                pb_def.VIF_TYPE_UNBOUND):
+            # Checking newly vm port binding allows us to avoid call to DB
+            # when a port update_event like restart, setting name, etc...
+            # Moreover, that will help us in case of tenant admin wants to
+            # only attach security group to vm port.
+            return
+
+        if updated_port[pb_def.VIF_TYPE] in [pb_def.VIF_TYPE_UNBOUND,
+                                             pb_def.VIF_TYPE_BINDING_FAILED]:
             return
         context = kwargs['context']
-        port_id = kwargs['port']['id']
-        project_id = kwargs['port']['project_id']
-        fwg_with_rules = self.set_port_for_default_firewall_group(
-            context, port_id, project_id)
-        fwg_with_rules['add-port-ids'] = port_id
-        fwg_with_rules['del-ports-ids'] = []
-        fwg_with_rules['port_details'] = self._get_fwg_port_details(
-            context, port_id)
-
-        self.agent_rpc.update_firewall_group(context, fwg_with_rules)
+        port_id = updated_port['id']
+        project_id = updated_port['project_id']
+        LOG.debug("Try to associate port %s at %s", port_id, project_id)
+        self.set_port_for_default_firewall_group(context, port_id, project_id)
 
     def create_firewall_group(self, context, firewall_group):
         LOG.debug("create_firewall_group() called")
@@ -351,7 +347,7 @@ class FirewallPluginV2(
             self._make_firewall_group_dict_with_rules(context, fwg['id']))
 
         fwg_with_rules['add-port-ids'] = fwg_ports
-        fwg_with_rules['del-ports-ids'] = []
+        fwg_with_rules['del-port-ids'] = []
         fwg_with_rules['port_details'] = self._get_fwg_port_details(
             context, fwg_ports)
 

neutron_fwaas/tests/unit/db/firewall/v2/test_firewall_db_v2.py

@@ -1976,3 +1976,49 @@ class TestFirewallDBPluginV2(FirewallPluginV2DbTestCase):
         with self.firewall_group(name='fireWall1') as fw:
             res = self._show('firewall_groups', fw['firewall_group']['id'])
             self.assertEqual('fireWall1', res['firewall_group']['name'])
+
+    def test_set_port_in_use_for_firewall_group(self):
+        fwg_db = {'id': 'fake_id'}
+        new_ports = {'ports': ['fake_port1', 'fake_port2']}
+        m_context = context.get_admin_context()
+        with mock.patch.object(m_context.session, 'add',
+                               side_effect=[None, f_exc.FirewallGroupPortInUse(
+                                    port_ids=['fake_port2'])]):
+            self.assertRaises(f_exc.FirewallGroupPortInUse,
+                              self.plugin._set_ports_for_firewall_group,
+                              m_context,
+                              fwg_db,
+                              new_ports)
+
+    def test_set_port_for_default_firewall_group(self):
+        ctx = self._get_nonadmin_context()
+        self._build_default_fwg(ctx=ctx)
+        with self.port(project_id=ctx.tenant_id) as port1, \
+            self.port(project_id=ctx.tenant_id) as port2:
+            port1_id = port1['port']['id']
+            port2_id = port2['port']['id']
+            port_ids = [port1_id, port2_id]
+            project_id = ctx.tenant_id
+
+            self.plugin.set_port_for_default_firewall_group(
+                ctx, port1_id, project_id)
+            self.plugin.set_port_for_default_firewall_group(
+                ctx, port2_id, project_id)
+            def_fwg_db = self.plugin._get_default_fwg(ctx, project_id)
+            self.assertEqual('PENDING_UPDATE', def_fwg_db['status'])
+            self.assertEqual(sorted(port_ids), sorted(def_fwg_db['ports']))
+
+    def test_set_port_for_default_firewall_group_raised_port_in_use(self):
+        ctx = self._get_nonadmin_context()
+        self._build_default_fwg(ctx=ctx)
+        self.plugin.update_firewall_group_status = mock.Mock()
+        with self.port(project_id=ctx.tenant_id) as port1:
+            port1_id = port1['port']['id']
+            port_ids = [port1_id]
+            self.plugin._set_ports_for_firewall_group = mock.Mock(
+                side_effect=f_exc.FirewallGroupPortInUse(port_ids=port_ids))
+            project_id = ctx.tenant_id
+
+            self.plugin.set_port_for_default_firewall_group(
+                ctx, port1_id, project_id)
+            self.plugin.update_firewall_group_status.assert_not_called()

neutron_fwaas/tests/unit/services/firewall/agents/l2/test_fwaas_v2.py

@@ -80,6 +80,7 @@ class TestHandlePort(TestFWaasV2AgentExtensionBase):
         self.l2._apply_fwg_rules = mock.Mock(return_value=True)
         self.l2._send_fwg_status = mock.Mock()
         self.ctx = context.get_admin_context()
+        self.l2._add_rule_for_trusted_port = mock.Mock()
 
     def test_normal(self):
         self.l2.fwg_map.get_port_fwg.return_value = None
@@ -127,6 +128,24 @@ class TestHandlePort(TestFWaasV2AgentExtensionBase):
         self.l2.fwg_map.set_port_fwg.assert_not_called()
         self.l2._send_fwg_status.assert_not_called()
 
+    def test_trusted_port(self):
+        self.l2.fwg_map.get_port.return_value = None
+        self.port['device_owner'] = 'network:foo'
+        self.l2.handle_port(self.ctx, self.port)
+
+        self.l2._add_rule_for_trusted_port.assert_called_once_with(self.port)
+        self.l2.fwg_map.set_port.assert_called_once_with(self.port)
+        self.rpc.get_firewall_group_for_port.assert_not_called()
+
+    def test_trusted_port_registered_map(self):
+        self.port['device_owner'] = 'network:dhcp'
+        self.l2.fwg_map.get_port.return_value = self.port
+        self.l2.handle_port(self.ctx, self.port)
+
+        self.l2._add_rule_for_trusted_port.assert_not_called()
+        self.l2.fwg_map.set_port.assert_not_called()
+        self.rpc.get_firewall_group_for_port.assert_not_called()
+
 
 class TestDeletePort(TestFWaasV2AgentExtensionBase):
 
@@ -135,6 +154,7 @@ class TestDeletePort(TestFWaasV2AgentExtensionBase):
         self.l2._compute_status = mock.Mock(return_value=nl_consts.ACTIVE)
         self.l2._apply_fwg_rules = mock.Mock(return_value=True)
         self.l2._send_fwg_status = mock.Mock()
+        self.l2._delete_rule_for_trusted_port = mock.Mock()
 
         self.l2.fwg_map.get_port_fwg = mock.Mock(return_value=self.fwg)
         self.l2.fwg_map.set_fwg = mock.Mock()
@@ -185,6 +205,15 @@ class TestDeletePort(TestFWaasV2AgentExtensionBase):
         self.l2.fwg_map.get_port_fwg.assert_called_once_with(self.port)
         self.l2._apply_fwg_rules.assert_not_called()
 
+    def test_trusted_port_with_map(self):
+        self.port['device_owner'] = 'network:dhcp'
+        self.l2.fwg_map.get_port.return_value = self.port
+        self.l2.delete_port(self.ctx, self.port_minimal)
+
+        self.l2._delete_rule_for_trusted_port.assert_called_once_with(
+            self.port)
+        self.l2.fwg_map.remove_port.assert_called_once_with(self.port)
+
 
 class TestCreateFirewallGroup(TestFWaasV2AgentExtensionBase):
 
@@ -713,13 +742,21 @@ class TestPortFirewallGroupMap(base.BaseTestCase):
         self.assertIsNone(self.map.get_port(port2))
         self.assertEqual([], self.map.get_fwg(self.fwg_id)['ports'])
 
+    def test_remove_non_exist_port(self):
+        port1 = self.port
+        port2 = self.fake.create('port')
+        self.map.set_port_fwg(port1, self.fwg)
+
+        self.map.remove_port(port2)
+        self.assertIsNone(self.map.get_port(port2))
+
     def test_illegal_remove_port_no_relation_with_fwg(self):
         port1 = self.port
         port1_id = port1['port_id']
         self.map.set_port_fwg(port1, self.fwg)
         self.map.port_fwg[port1_id] = None
         self.map.remove_port(port1)
-        self.assertEqual(port1, self.map.get_port(port1))
+        self.assertIsNone(self.map.get_port(port1))
 
     def test_remove_fwg(self):
         self.map.set_fwg(self.fwg)

neutron_fwaas/tests/unit/services/firewall/test_fwaas_plugin_v2.py

@@ -775,33 +775,66 @@ class TestFirewallPluginBasev2(TestFirewallRouterPortBase,
                         None)
 
 
-class TestEnabledAutomaticAssociation(TestFirewallPluginBasev2):
+class TestAutomaticAssociation(TestFirewallPluginBasev2):
 
     def setUp(self):
-        # set auto association fwg
-        cfg.CONF.set_override(
-            'auto_associate_default_firewall_group', True, 'fwaas')
-        super(TestEnabledAutomaticAssociation, self).setUp()
+        # TODO(yushiro): Replace constant value for this test class
+        # Set auto association fwg
+        super(TestAutomaticAssociation, self).setUp()
         self.agent_rpc = self.plugin.agent_rpc
-
-    def test_enabled_auto_association_fwg(self):
-        fwg_with_rule = {'id': 'fake_id',
-                         'name': 'default'}
-        self.plugin.set_port_for_default_firewall_group = \
-            mock.Mock(return_value=fwg_with_rule)
+        self.plugin.set_port_for_default_firewall_group = mock.Mock()
+        self.plugin._get_fwg_port_details = mock.Mock()
+
+    def test_vm_port(self):
         self.plugin._get_fwg_port_details = mock.Mock()
-        self.agent_rpc.update_firewall_group = mock.Mock()
-        m_context = mock.ANY
         kwargs = {
-            "context": m_context,
+            "context": mock.ANY,
             "port": {"id": "fake_port",
-                     "project_id": "fake_project"}
+                     "device_owner": "compute:nova",
+                     "binding:vif_type": "ovs",
+                     "project_id": "fake_project"},
+            "original_port": {"binding:vif_type": "unbound"}
         }
-        self.plugin.handle_create_port_event(
-            "PORT", "after_create", "test_plugin", **kwargs)
+        self.plugin.handle_update_port(
+            "PORT", "after_update", "test_plugin", **kwargs)
         self.plugin.set_port_for_default_firewall_group.\
-            assert_called_once_with(m_context,
+            assert_called_once_with(mock.ANY,
                                     kwargs['port']['id'],
                                     kwargs['port']['project_id'])
-        self.agent_rpc.update_firewall_group.assert_called_once_with(
-            m_context, fwg_with_rule)
+
+    def test_vm_port_not_newly_created(self):
+        # Just updated for VM port(name or description...etc.)
+        kwargs = {
+            "context": mock.ANY,
+            "port": {
+                "id": "fake_port",
+                "device_owner": "compute:nova",
+                "binding:vif_type": "ovs",
+                "project_id": "fake_project"
+            },
+            "original_port": {
+                "device_owner": "compute:nova",
+                "binding:vif_type": "ovs",
+                "project_id": "fake_project"
+            }
+        }
+        self.plugin.handle_update_port(
+            "PORT", "after_update", "test_plugin", **kwargs)
+        self.plugin.set_port_for_default_firewall_group.assert_not_called()
+
+    def test_not_vm_port(self):
+        for device_owner in ["network:router_interface",
+                             "network:router_gateway",
+                             "network:dhcp"]:
+            kwargs = {
+                "context": mock.ANY,
+                "port": {"id": "fake_port",
+                         "device_owner": device_owner,
+                         "project_id": "fake_project"},
+                "original_port": {"device_owner": device_owner,
+                                  "binding:vif_type": "unbound",
+                                  "project_id": "fake_project"}
+            }
+            self.plugin.handle_update_port(
+                "PORT", "after_update", "test_plugin", **kwargs)
+            self.plugin.set_port_for_default_firewall_group.assert_not_called()

releasenotes/notes/bug-1746404-493a66faac333403.yaml

@@ -0,0 +1,10 @@
+---
+prelude: >
+    Taking security for VM instance into consideration, we've removed an option
+    to disable automatic association with default firewall group feature.
+    Therefore, `auto_associate_default_firewall_group` has been removed.
+fixes:
+  - |
+    There is no validation to check if an updated port is for VM or not so far.
+    After this fix, default firewall group association is called only for
+    VM ports which are newly created.