Blueprint xenapi-provider-firewall and Bug #915403.

  1. Provides dom0 IPtables driver to implement the Provider firewall rules.
  2. Existing libvirt code has been refactored to reduce the amount of duplicated code to a minimum
  3. The three provider apis in ec2/admin.py file are now fixed the following way:
    a.    remove_external_address_block returned 'OK' on removing blocks which didn't exist. This is now fixed.
    b.    block_external_addresses raised exception earlier on duplicate network blocks. Now the exception is logged and failed status message is returned.
    c.  all the three provider apis now logs for invalid and improper inputs and return uniform (a dictionary ) and proper status messages for all cases.
  4. appropriate unit tests added to cover the same

Change-Id: I27d83186f850423a6268947aed0c9a349d8f8d65

Authors

@@ -41,6 +41,7 @@ Dave Walker <DaveWalker@ubuntu.com>
 David Pravec <David.Pravec@danix.org>
 David Subiros <david.perez5@hp.com>
 Dean Troyer <dtroyer@gmail.com>
+Deepak Garg <deepak.garg@citrix.com>
 Derek Higgins <higginsd@gmail.com>
 Devendra Modium <dmodium@isi.edu>
 Devin Carlen <devin.carlen@gmail.com>

nova/api/ec2/admin.py

@@ -349,8 +349,15 @@ def block_external_addresses(self, context, cidr):
         LOG.audit(_('Blocking traffic to all projects incoming from %s'),
                   cidr, context=context)
         cidr = urllib.unquote(cidr).decode()
-        # raise if invalid
-        netaddr.IPNetwork(cidr)
+        failed = {'status': 'Failed', 'message': ' 0 rules added'}
+        if not utils.is_valid_cidr(cidr):
+            msg = 'Improper input. Please provide a valid cidr: ' \
+                                                        'e.g. 121.12.10.11/24.'
+            failed['message'] = msg + failed['message']
+            return failed
+        #Normalizing cidr. e.g. '20.20.20.11/24' -> '20.20.20.0/24', so that
+        #db values stay in sync with filters' values (e.g. in iptables)
+        cidr = str(netaddr.IPNetwork(cidr).cidr)
         rule = {'cidr': cidr}
         tcp_rule = rule.copy()
         tcp_rule.update({'protocol': 'tcp', 'from_port': 1, 'to_port': 65535})
@@ -370,7 +377,9 @@ def block_external_addresses(self, context, cidr):
             db.provider_fw_rule_create(context, icmp_rule)
             rules_added += 1
         if not rules_added:
-            raise exception.ApiError(_('Duplicate rule'))
+                msg = 'Duplicate Rule.'
+                failed['message'] = msg + failed['message']
+                return failed
         self.compute_api.trigger_provider_fw_rules_refresh(context)
         return {'status': 'OK', 'message': 'Added %s rules' % rules_added}
 
@@ -385,11 +394,25 @@ def describe_external_address_blocks(self, context):
     def remove_external_address_block(self, context, cidr):
         LOG.audit(_('Removing ip block from %s'), cidr, context=context)
         cidr = urllib.unquote(cidr).decode()
-        # raise if invalid
-        netaddr.IPNetwork(cidr)
+        # Catch the exception and LOG for improper or malicious inputs.
+        # Also return a proper status and message in that case
+        failed = {'status': 'Failed', 'message': ' 0 rules deleted'}
+        if not utils.is_valid_cidr(cidr):
+            msg = 'Improper input. Please provide a valid cidr: ' \
+                                                    'e.g. 121.12.10.11/24.'
+            failed['message'] = msg + failed['message']
+            return failed
+        #Normalizing cidr. e.g. '20.20.20.11/24' -> '20.20.20.0/24', so that
+        #db values stay in sync with filters' values (e.g. in iptables)
+        cidr = str(netaddr.IPNetwork(cidr).cidr)
         rules = db.provider_fw_rule_get_all_by_cidr(context, cidr)
-        for rule in rules:
-            db.provider_fw_rule_destroy(context, rule['id'])
-        if rules:
+
+        if not rules:
+            msg = 'No such CIDR currently blocked.'
+            failed['message'] = msg + failed['message']
+            return failed
+        else:
+            for rule in rules:
+                db.provider_fw_rule_destroy(context, rule['id'])
             self.compute_api.trigger_provider_fw_rules_refresh(context)
-        return {'status': 'OK', 'message': 'Deleted %s rules' % len(rules)}
+            return {'status': 'OK', 'message': 'Deleted %s rules' % len(rules)}

nova/compute/api.py

@@ -710,7 +710,7 @@ def trigger_security_group_members_refresh(self, context, group_ids):
                       "args": {"security_group_id": group_id}})
 
     def trigger_provider_fw_rules_refresh(self, context):
-        """Called when a rule is added to or removed from a security_group"""
+        """Called when a rule is added/removed from a provider firewall"""
 
         hosts = [x['host'] for (x, idx)
                            in self.db.service_get_all_compute_sorted(context)]

nova/compute/manager.py

@@ -261,9 +261,9 @@ def refresh_security_group_members(self, context,
         return self.driver.refresh_security_group_members(security_group_id)
 
     @exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
-    def refresh_provider_fw_rules(self, context, **_kwargs):
+    def refresh_provider_fw_rules(self, context, **kwargs):
         """This call passes straight through to the virtualization driver."""
-        return self.driver.refresh_provider_fw_rules()
+        return self.driver.refresh_provider_fw_rules(**kwargs)
 
     def _get_instance_nw_info(self, context, instance):
         """Get a list of dictionaries of network data of an instance.

nova/network/linux_net.py

@@ -303,16 +303,17 @@ def apply(self):
 
         for cmd, tables in s:
             for table in tables:
-                current_table, _ = self.execute('%s-save' % (cmd,),
-                                                '-t', '%s' % (table,),
-                                                run_as_root=True,
-                                                attempts=5)
+                current_table, _err = self.execute('%s-save' % (cmd,),
+                                                   '-t', '%s' % (table,),
+                                                   run_as_root=True,
+                                                   attempts=5)
                 current_lines = current_table.split('\n')
                 new_filter = self._modify_rules(current_lines,
                                                 tables[table])
                 self.execute('%s-restore' % (cmd,), run_as_root=True,
                              process_input='\n'.join(new_filter),
                              attempts=5)
+        LOG.debug(_("IPTablesManager.apply completed with success"))
 
     def _modify_rules(self, current_lines, table, binary=None):
         unwrapped_chains = table.unwrapped_chains

nova/tests/api/ec2/test_admin.py

@@ -365,24 +365,43 @@ def test_describe_hosts_volume(self):
         hosts = self._ac.describe_hosts(self._c)['hosts']
         self.assertEqual('volume1', hosts[0]['hostname'])
 
-    def test_block_external_addresses(self):
+    def test_block_external_addresses_validate_output_for_valid_input(self):
         result = self._ac.block_external_addresses(self._c, '192.168.100.1/24')
         self.assertEqual('OK', result['status'])
         self.assertEqual('Added 3 rules', result['message'])
 
-    def test_block_external_addresses_already_existent_rule(self):
-        self._ac.block_external_addresses(self._c, '192.168.100.1/24')
-        self.assertRaises(exception.ApiError,
-                          self._ac.block_external_addresses,
-                          self._c, '192.168.100.1/24')
+    def test_block_external_addresses_validate_output_for_invalid_input(self):
+        result = self._ac.block_external_addresses(self._c, '12.10.10.256/24')
+        self.assertEqual('Failed', result['status'])
+        value = '0 rules added' in result['message']
+        self.assertEqual(value, True)
 
-    def test_describe_external_address_blocks(self):
-        self._ac.block_external_addresses(self._c, '192.168.100.1/24')
+    def test_block_external_addresses_already_existent_rule(self):
+        self._ac.block_external_addresses(self._c, '192.168.100.0/24')
+        result = self._ac.block_external_addresses(self._c, '192.168.100.0/24')
+        self.assertEqual('Failed', result['status'])
+        value = '0 rules added' in result['message']
+        self.assertEqual(value, True)
+
+    def test_describe_external_address_blocks_normalized_output(self):
+        self._ac.block_external_addresses(self._c, '192.168.100.11/24')
         self.assertEqual(
-                {'externalIpBlockInfo': [{'cidr': u'192.168.100.1/24'}]},
+                {'externalIpBlockInfo': [{'cidr': u'192.168.100.0/24'}]},
                 self._ac.describe_external_address_blocks(self._c))
 
-    def test_remove_external_address_block(self):
+    def test_describe_external_address_blocks_many_inputs(self):
+        self._ac.block_external_addresses(self._c, '192.168.100.11/24')
+        self._ac.block_external_addresses(self._c, '12.12.12.10/24')
+        self._ac.block_external_addresses(self._c, '18.18.18.0/24')
+        output1 = {'cidr': u'192.168.100.0/24'}
+        output2 = {'cidr': u'12.12.12.0/24'}
+        output3 = {'cidr': u'18.18.18.0/24'}
+        result = self._ac.describe_external_address_blocks(self._c)
+        result = sorted(result['externalIpBlockInfo'])
+        output = sorted([output1, output2, output3])
+        self.assertEqual(result, output)
+
+    def test_remove_external_address_block_existent_rule(self):
         self._ac.block_external_addresses(self._c, '192.168.100.1/24')
 
         result = self._ac.remove_external_address_block(self._c,
@@ -393,6 +412,26 @@ def test_remove_external_address_block(self):
         result = self._ac.describe_external_address_blocks(self._c)
         self.assertEqual([], result['externalIpBlockInfo'])
 
+    def test_remove_external_address_block_non_existent_rule(self):
+        result = self._ac.remove_external_address_block(self._c,
+                                                        '192.168.100.1/24')
+        self.assertEqual('Failed', result['status'])
+        value = '0 rules deleted' in result['message']
+        self.assertEqual(value, True)
+
+        result = self._ac.describe_external_address_blocks(self._c)
+        self.assertEqual([], result['externalIpBlockInfo'])
+
+    def test_remove_external_address_block_invalid_input(self):
+        result = self._ac.remove_external_address_block(self._c,
+                                                        '192.168.100/24')
+        self.assertEqual('Failed', result['status'])
+        value = '0 rules deleted' in result['message']
+        self.assertEqual(value, True)
+
+        result = self._ac.describe_external_address_blocks(self._c)
+        self.assertEqual([], result['externalIpBlockInfo'])
+
     def test_start_vpn(self):
 
         def fake_launch_vpn_instance(self, *args):

nova/tests/test_xenapi.py

@@ -1334,6 +1334,9 @@ def test_get_all_bw_usage_in_failure_case(self):
         self.assertEqual(result, [])
 
 
+# TODO(salvatore-orlando): this class and
+# nova.tests.test_libvirt.IPTablesFirewallDriverTestCase share a lot of code.
+# Consider abstracting common code in a base class for firewall driver testing.
 class XenAPIDom0IptablesFirewallTestCase(test.TestCase):
 
     _in_nat_rules = [
@@ -1581,3 +1584,60 @@ def test_do_refresh_security_group_rules(self):
         self.assertTrue(len(filter(regex.match, self._out_rules)) > 0,
                         "Rules were not updated properly."
                         "The rule for UDP acceptance is missing")
+
+    def test_provider_firewall_rules(self):
+        # setup basic instance data
+        instance_ref = self._create_instance_ref()
+        # FRAGILE: as in libvirt tests
+        # peeks at how the firewall names chains
+        chain_name = 'inst-%s' % instance_ref['id']
+
+        network_info = fake_network.fake_get_instance_nw_info(self.stubs, 1, 1)
+        self.fw.prepare_instance_filter(instance_ref, network_info)
+        self.assertTrue('provider' in self.fw.iptables.ipv4['filter'].chains)
+        rules = [rule for rule in self.fw.iptables.ipv4['filter'].rules
+                      if rule.chain == 'provider']
+        self.assertEqual(0, len(rules))
+
+        admin_ctxt = context.get_admin_context()
+        # add a rule and send the update message, check for 1 rule
+        provider_fw0 = db.provider_fw_rule_create(admin_ctxt,
+                                                  {'protocol': 'tcp',
+                                                   'cidr': '10.99.99.99/32',
+                                                   'from_port': 1,
+                                                   'to_port': 65535})
+        self.fw.refresh_provider_fw_rules()
+        rules = [rule for rule in self.fw.iptables.ipv4['filter'].rules
+                      if rule.chain == 'provider']
+        self.assertEqual(1, len(rules))
+
+        # Add another, refresh, and make sure number of rules goes to two
+        provider_fw1 = db.provider_fw_rule_create(admin_ctxt,
+                                                  {'protocol': 'udp',
+                                                   'cidr': '10.99.99.99/32',
+                                                   'from_port': 1,
+                                                   'to_port': 65535})
+        self.fw.refresh_provider_fw_rules()
+        rules = [rule for rule in self.fw.iptables.ipv4['filter'].rules
+                      if rule.chain == 'provider']
+        self.assertEqual(2, len(rules))
+
+        # create the instance filter and make sure it has a jump rule
+        self.fw.prepare_instance_filter(instance_ref, network_info)
+        self.fw.apply_instance_filter(instance_ref, network_info)
+        inst_rules = [rule for rule in self.fw.iptables.ipv4['filter'].rules
+                           if rule.chain == chain_name]
+        jump_rules = [rule for rule in inst_rules if '-j' in rule.rule]
+        provjump_rules = []
+        # IptablesTable doesn't make rules unique internally
+        for rule in jump_rules:
+            if 'provider' in rule.rule and rule not in provjump_rules:
+                provjump_rules.append(rule)
+        self.assertEqual(1, len(provjump_rules))
+
+        # remove a rule from the db, cast to compute to refresh rule
+        db.provider_fw_rule_destroy(admin_ctxt, provider_fw1['id'])
+        self.fw.refresh_provider_fw_rules()
+        rules = [rule for rule in self.fw.iptables.ipv4['filter'].rules
+                      if rule.chain == 'provider']
+        self.assertEqual(1, len(rules))

nova/tests/xenapi/stubs.py

@@ -32,7 +32,6 @@ def fake_none(self, *args):
         return
 
     vmops = conn._vmops
-    stubs.Set(vmops.firewall_driver, 'setup_basic_filtering', fake_none)
     stubs.Set(vmops.firewall_driver, 'prepare_instance_filter', fake_none)
     stubs.Set(vmops.firewall_driver, 'instance_filter_exists', fake_none)
 

nova/utils.py

@@ -1075,12 +1075,12 @@ def monkey_patch():
             if isinstance(module_data[key], pyclbr.Class):
                 clz = import_class("%s.%s" % (module, key))
                 for method, func in inspect.getmembers(clz, inspect.ismethod):
-                    setattr(clz, method,\
+                    setattr(clz, method,
                         decorator("%s.%s.%s" % (module, key, method), func))
             # set the decorator for the function
             if isinstance(module_data[key], pyclbr.Function):
                 func = import_class("%s.%s" % (module, key))
-                setattr(sys.modules[module], key,\
+                setattr(sys.modules[module], key,
                     decorator("%s.%s" % (module, key), func))
 
 

nova/virt/driver.py

@@ -380,7 +380,7 @@ def refresh_security_group_members(self, security_group_id):
             * another host 'H1' runs an instance 'i-1'
             * instance 'i-1' is a member of security group 'b'
 
-            When 'i-1' launches or terminates we will recieve the message
+            When 'i-1' launches or terminates we will receive the message
             to update members of group 'b', at which time we will make
             any changes needed to the rules for instance 'i-0' to allow
             or deny traffic coming from 'i-1', depending on if it is being
@@ -399,7 +399,7 @@ def refresh_security_group_members(self, security_group_id):
         # TODO(Vek): Need to pass context in for access to auth_token
         raise NotImplementedError()
 
-    def refresh_provider_fw_rules(self, security_group_id):
+    def refresh_provider_fw_rules(self):
         """This triggers a firewall update based on database changes.
 
         When this is called, rules have either been added or removed from the

nova/virt/firewall.py

@@ -34,7 +34,7 @@
 class FirewallDriver(object):
     """ Firewall Driver base class.
 
-        Defines methos that any driver providing security groups
+        Defines methods that any driver providing security groups
         and provider fireall functionality should implement.
     """
     def prepare_instance_filter(self, instance, network_info):
@@ -129,12 +129,18 @@ def prepare_instance_filter(self, instance, network_info):
         self.instances[instance['id']] = instance
         self.network_infos[instance['id']] = network_info
         self.add_filters_for_instance(instance)
+        LOG.debug(_('Filters added to the instance: %r'), instance)
+        self.refresh_provider_fw_rules()
+        LOG.debug(_('Provider Firewall Rules refreshed'))
         self.iptables.apply()
 
     def _create_filter(self, ips, chain_name):
         return ['-d %s -j $%s' % (ip, chain_name) for ip in ips]
 
     def _filters_for_instance(self, chain_name, network_info):
+        """Creates a rule corresponding to each ip that defines a
+             jump to the corresponding instance - chain for all the traffic
+             destined to that ip"""
         ips_v4 = [ip['ip'] for (_n, mapping) in network_info
                  for ip in mapping['ips']]
         ipv4_rules = self._create_filter(ips_v4, chain_name)
@@ -190,6 +196,10 @@ def _do_basic_rules(self, ipv4_rules, ipv6_rules, network_info):
         ipv4_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
         ipv6_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
 
+        # Pass through provider-wide drops
+        ipv4_rules += ['-j $provider']
+        ipv6_rules += ['-j $provider']
+
     def _do_dhcp_rules(self, ipv4_rules, network_info):
         dhcp_servers = [info['dhcp_server'] for (_n, info) in network_info]
 
@@ -345,3 +355,38 @@ def do_refresh_security_group_rules(self, security_group):
         for instance in self.instances.values():
             self.remove_filters_for_instance(instance)
             self.add_filters_for_instance(instance)
+
+    def refresh_provider_fw_rules(self):
+        """See class:FirewallDriver: docs."""
+        self._do_refresh_provider_fw_rules()
+        self.iptables.apply()
+
+    @utils.synchronized('iptables', external=True)
+    def _do_refresh_provider_fw_rules(self):
+        """Internal, synchronized version of refresh_provider_fw_rules."""
+        self._purge_provider_fw_rules()
+        self._build_provider_fw_rules()
+
+    def _purge_provider_fw_rules(self):
+        """Remove all rules from the provider chains."""
+        self.iptables.ipv4['filter'].empty_chain('provider')
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].empty_chain('provider')
+
+    def _build_provider_fw_rules(self):
+        """Create all rules for the provider IP DROPs."""
+        self.iptables.ipv4['filter'].add_chain('provider')
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].add_chain('provider')
+        ipv4_rules, ipv6_rules = self._provider_rules()
+        for rule in ipv4_rules:
+            self.iptables.ipv4['filter'].add_rule('provider', rule)
+
+        if FLAGS.use_ipv6:
+            for rule in ipv6_rules:
+                self.iptables.ipv6['filter'].add_rule('provider', rule)
+
+    @staticmethod
+    def _provider_rules():
+        """Generate a list of rules from provider for IP4 & IP6."""
+        raise NotImplementedError()