Blueprint xenapi-security-groups

Provides two drivers for implementing security groups in xenapi:
1) domU driver that enforces security groups on the Openstack virtual appliance (use advised with FlatDHCP in HA mode)
2) dom0 driver that enforces security groups where VIFs are attached

Both drivers translate security groups into iptables rules.
Existing libvirt code has been refactored to reduce the amount of duplicated code to a minimum

Now Addressing reviewers's comments on style. Fixing issue spotted with snapshots
Change-Id: Ifa16a8f2508a709be03241bac0f942fe1a51d1e8

10 files changed, 486 insertions, 270 deletions

diff --git a/nova/virt/firewall.py b/nova/virt/firewall.py
new file mode 100644
index 000000000..c60148231
--- /dev/null
+++ b/nova/virt/firewall.py
@@ -0,0 +1,336 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 United States Government as represented by the
+# Administrator of the National Aeronautics and Space Administration.
+# All Rights Reserved.
+# Copyright (c) 2011 Citrix Systems, Inc.
+#
+#    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
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from nova import context
+from nova import db
+from nova import flags
+from nova import log as logging
+from nova import utils
+from nova.virt import netutils
+
+LOG = logging.getLogger("nova.virt.firewall")
+FLAGS = flags.FLAGS
+flags.DEFINE_bool('allow_same_net_traffic',
+                  True,
+                  'Whether to allow network traffic from same network')
+
+
+class FirewallDriver(object):
+    """ Firewall Driver base class.
+
+        Defines methos that any driver providing security groups
+        and provider fireall functionality should implement.
+    """
+    def prepare_instance_filter(self, instance, network_info):
+        """Prepare filters for the instance.
+        At this point, the instance isn't running yet."""
+        raise NotImplementedError()
+
+    def unfilter_instance(self, instance, network_info):
+        """Stop filtering instance"""
+        raise NotImplementedError()
+
+    def apply_instance_filter(self, instance, network_info):
+        """Apply instance filter.
+
+        Once this method returns, the instance should be firewalled
+        appropriately. This method should as far as possible be a
+        no-op. It's vastly preferred to get everything set up in
+        prepare_instance_filter.
+        """
+        raise NotImplementedError()
+
+    def refresh_security_group_rules(self, security_group_id):
+        """Refresh security group rules from data store
+
+        Gets called when a rule has been added to or removed from
+        the security group."""
+        raise NotImplementedError()
+
+    def refresh_security_group_members(self, security_group_id):
+        """Refresh security group members from data store
+
+        Gets called when an instance gets added to or removed from
+        the security group."""
+        raise NotImplementedError()
+
+    def refresh_provider_fw_rules(self):
+        """Refresh common rules for all hosts/instances from data store.
+
+        Gets called when a rule has been added to or removed from
+        the list of rules (via admin api).
+
+        """
+        raise NotImplementedError()
+
+    def setup_basic_filtering(self, instance, network_info):
+        """Create rules to block spoofing and allow dhcp.
+
+        This gets called when spawning an instance, before
+        :method:`prepare_instance_filter`.
+
+        """
+        raise NotImplementedError()
+
+    def instance_filter_exists(self, instance, network_info):
+        """Check nova-instance-instance-xxx exists"""
+        raise NotImplementedError()
+
+
+class IptablesFirewallDriver(FirewallDriver):
+    """ Driver which enforces security groups through iptables rules. """
+
+    def __init__(self, **kwargs):
+        from nova.network import linux_net
+        self.iptables = linux_net.iptables_manager
+        self.instances = {}
+        self.network_infos = {}
+        self.basicly_filtered = False
+
+        self.iptables.ipv4['filter'].add_chain('sg-fallback')
+        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
+        self.iptables.ipv6['filter'].add_chain('sg-fallback')
+        self.iptables.ipv6['filter'].add_rule('sg-fallback', '-j DROP')
+
+    def setup_basic_filtering(self, instance, network_info):
+        pass
+
+    def apply_instance_filter(self, instance, network_info):
+        """No-op. Everything is done in prepare_instance_filter"""
+        pass
+
+    def unfilter_instance(self, instance, network_info):
+        if self.instances.pop(instance['id'], None):
+            # NOTE(vish): use the passed info instead of the stored info
+            self.network_infos.pop(instance['id'])
+            self.remove_filters_for_instance(instance)
+            self.iptables.apply()
+        else:
+            LOG.info(_('Attempted to unfilter instance %s which is not '
+                     'filtered'), instance['id'])
+
+    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)
+        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):
+        ips_v4 = [ip['ip'] for (_n, mapping) in network_info
+                 for ip in mapping['ips']]
+        ipv4_rules = self._create_filter(ips_v4, chain_name)
+
+        ipv6_rules = []
+        if FLAGS.use_ipv6:
+            ips_v6 = [ip['ip'] for (_n, mapping) in network_info
+                     for ip in mapping['ip6s']]
+            ipv6_rules = self._create_filter(ips_v6, chain_name)
+
+        return ipv4_rules, ipv6_rules
+
+    def _add_filters(self, chain_name, ipv4_rules, ipv6_rules):
+        for rule in ipv4_rules:
+            self.iptables.ipv4['filter'].add_rule(chain_name, rule)
+
+        if FLAGS.use_ipv6:
+            for rule in ipv6_rules:
+                self.iptables.ipv6['filter'].add_rule(chain_name, rule)
+
+    def add_filters_for_instance(self, instance):
+        network_info = self.network_infos[instance['id']]
+        chain_name = self._instance_chain_name(instance)
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].add_chain(chain_name)
+        self.iptables.ipv4['filter'].add_chain(chain_name)
+        ipv4_rules, ipv6_rules = self._filters_for_instance(chain_name,
+                                                            network_info)
+        self._add_filters('local', ipv4_rules, ipv6_rules)
+        ipv4_rules, ipv6_rules = self.instance_rules(instance, network_info)
+        self._add_filters(chain_name, ipv4_rules, ipv6_rules)
+
+    def remove_filters_for_instance(self, instance):
+        chain_name = self._instance_chain_name(instance)
+
+        self.iptables.ipv4['filter'].remove_chain(chain_name)
+        if FLAGS.use_ipv6:
+            self.iptables.ipv6['filter'].remove_chain(chain_name)
+
+    @staticmethod
+    def _security_group_chain_name(security_group_id):
+        return 'nova-sg-%s' % (security_group_id,)
+
+    def _instance_chain_name(self, instance):
+        return 'inst-%s' % (instance['id'],)
+
+    def _do_basic_rules(self, ipv4_rules, ipv6_rules, network_info):
+        # Always drop invalid packets
+        ipv4_rules += ['-m state --state ' 'INVALID -j DROP']
+        ipv6_rules += ['-m state --state ' 'INVALID -j DROP']
+
+        # Allow established connections
+        ipv4_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
+        ipv6_rules += ['-m state --state ESTABLISHED,RELATED -j ACCEPT']
+
+    def _do_dhcp_rules(self, ipv4_rules, network_info):
+        dhcp_servers = [info['dhcp_server'] for (_n, info) in network_info]
+
+        for dhcp_server in dhcp_servers:
+            ipv4_rules.append('-s %s -p udp --sport 67 --dport 68 '
+                              '-j ACCEPT' % (dhcp_server,))
+
+    def _do_project_network_rules(self, ipv4_rules, ipv6_rules, network_info):
+        cidrs = [network['cidr'] for (network, _i) in network_info]
+        for cidr in cidrs:
+            ipv4_rules.append('-s %s -j ACCEPT' % (cidr,))
+        if FLAGS.use_ipv6:
+            cidrv6s = [network['cidr_v6'] for (network, _i) in
+                       network_info]
+
+            for cidrv6 in cidrv6s:
+                ipv6_rules.append('-s %s -j ACCEPT' % (cidrv6,))
+
+    def _do_ra_rules(self, ipv6_rules, network_info):
+        gateways_v6 = [mapping['gateway_v6'] for (_n, mapping) in
+                       network_info]
+        for gateway_v6 in gateways_v6:
+            ipv6_rules.append(
+                    '-s %s/128 -p icmpv6 -j ACCEPT' % (gateway_v6,))
+
+    def _build_icmp_rule(self, rule, version):
+        icmp_type = rule.from_port
+        icmp_code = rule.to_port
+
+        if icmp_type == -1:
+            icmp_type_arg = None
+        else:
+            icmp_type_arg = '%s' % icmp_type
+            if not icmp_code == -1:
+                icmp_type_arg += '/%s' % icmp_code
+
+        if icmp_type_arg:
+            if version == 4:
+                return ['-m', 'icmp', '--icmp-type', icmp_type_arg]
+            elif version == 6:
+                return ['-m', 'icmp6', '--icmpv6-type', icmp_type_arg]
+        # return empty list if icmp_type == -1
+        return []
+
+    def _build_tcp_udp_rule(self, rule, version):
+        if rule.from_port == rule.to_port:
+            return ['--dport', '%s' % (rule.from_port,)]
+        else:
+            return ['-m', 'multiport',
+                    '--dports', '%s:%s' % (rule.from_port,
+                                           rule.to_port)]
+
+    def instance_rules(self, instance, network_info):
+        ctxt = context.get_admin_context()
+
+        ipv4_rules = []
+        ipv6_rules = []
+
+        # Initialize with basic rules
+        self._do_basic_rules(ipv4_rules, ipv6_rules, network_info)
+        # Set up rules to allow traffic to/from DHCP server
+        self._do_dhcp_rules(ipv4_rules, network_info)
+
+        #Allow project network traffic
+        if FLAGS.allow_same_net_traffic:
+            self._do_project_network_rules(ipv4_rules, ipv6_rules,
+                                           network_info)
+        # We wrap these in FLAGS.use_ipv6 because they might cause
+        # a DB lookup. The other ones are just list operations, so
+        # they're not worth the clutter.
+        if FLAGS.use_ipv6:
+            # Allow RA responses
+            self._do_ra_rules(ipv6_rules, network_info)
+
+        security_groups = db.security_group_get_by_instance(ctxt,
+                                                            instance['id'])
+
+        # then, security group chains and rules
+        for security_group in security_groups:
+            rules = db.security_group_rule_get_by_security_group(ctxt,
+                                                          security_group['id'])
+
+            for rule in rules:
+                LOG.debug(_('Adding security group rule: %r'), rule)
+
+                if not rule.cidr:
+                    version = 4
+                else:
+                    version = netutils.get_ip_version(rule.cidr)
+
+                if version == 4:
+                    fw_rules = ipv4_rules
+                else:
+                    fw_rules = ipv6_rules
+
+                protocol = rule.protocol
+                if version == 6 and rule.protocol == 'icmp':
+                    protocol = 'icmpv6'
+
+                args = ['-j ACCEPT']
+                if protocol:
+                    args += ['-p', protocol]
+
+                if protocol in ['udp', 'tcp']:
+                    args += self._build_tcp_udp_rule(rule, version)
+                elif protocol == 'icmp':
+                    args += self._build_icmp_rule(rule, version)
+                if rule.cidr:
+                    LOG.info('Using cidr %r', rule.cidr)
+                    args += ['-s', rule.cidr]
+                    fw_rules += [' '.join(args)]
+                else:
+                    if rule['grantee_group']:
+                        for instance in rule['grantee_group']['instances']:
+                            LOG.info('instance: %r', instance)
+                            ips = db.instance_get_fixed_addresses(ctxt,
+                                                                instance['id'])
+                            LOG.info('ips: %r', ips)
+                            for ip in ips:
+                                subrule = args + ['-s %s' % ip]
+                                fw_rules += [' '.join(subrule)]
+
+                LOG.info('Using fw_rules: %r', fw_rules)
+        ipv4_rules += ['-j $sg-fallback']
+        ipv6_rules += ['-j $sg-fallback']
+
+        return ipv4_rules, ipv6_rules
+
+    def instance_filter_exists(self, instance, network_info):
+        pass
+
+    def refresh_security_group_members(self, security_group):
+        self.do_refresh_security_group_rules(security_group)
+        self.iptables.apply()
+
+    def refresh_security_group_rules(self, security_group):
+        self.do_refresh_security_group_rules(security_group)
+        self.iptables.apply()
+
+    @utils.synchronized('iptables', external=True)
+    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)
diff --git a/nova/virt/libvirt/connection.py b/nova/virt/libvirt/connection.py
index 096bf2922..9a6eb57e0 100644
--- a/nova/virt/libvirt/connection.py
+++ b/nova/virt/libvirt/connection.py
@@ -93,18 +93,12 @@ flags.DEFINE_string('libvirt_uri',
                     '',
                     'Override the default libvirt URI (which is dependent'
                     ' on libvirt_type)')
-flags.DEFINE_bool('allow_same_net_traffic',
-                  True,
-                  'Whether to allow network traffic from same network')
 flags.DEFINE_bool('use_cow_images',
                   True,
                   'Whether to use cow images')
 flags.DEFINE_string('ajaxterm_portrange',
                     '10000-12000',
                     'Range of ports that ajaxterm should randomly try to bind')
-flags.DEFINE_string('firewall_driver',
-                    'nova.virt.libvirt.firewall.IptablesFirewallDriver',
-                    'Firewall driver (defaults to iptables)')
 flags.DEFINE_string('cpuinfo_xml_template',
                     utils.abspath('virt/cpuinfo.xml.template'),
                     'CpuInfo XML Template (Used only live migration now)')
diff --git a/nova/virt/libvirt/firewall.py b/nova/virt/libvirt/firewall.py
index debc186d6..9b9e3540e 100644
--- a/nova/virt/libvirt/firewall.py
+++ b/nova/virt/libvirt/firewall.py
@@ -25,7 +25,9 @@ from nova import db
 from nova import flags
 from nova import log as logging
 from nova import utils
-from nova.virt.libvirt import netutils
+
+import nova.virt.firewall as base_firewall
+from nova.virt import netutils
 
 
 LOG = logging.getLogger("nova.virt.libvirt.firewall")
@@ -39,65 +41,7 @@ except ImportError:
                "not work correctly."))
 
 
-class FirewallDriver(object):
-    def prepare_instance_filter(self, instance, network_info):
-        """Prepare filters for the instance.
-
-        At this point, the instance isn't running yet."""
-        raise NotImplementedError()
-
-    def unfilter_instance(self, instance, network_info):
-        """Stop filtering instance"""
-        raise NotImplementedError()
-
-    def apply_instance_filter(self, instance, network_info):
-        """Apply instance filter.
-
-        Once this method returns, the instance should be firewalled
-        appropriately. This method should as far as possible be a
-        no-op. It's vastly preferred to get everything set up in
-        prepare_instance_filter.
-        """
-        raise NotImplementedError()
-
-    def refresh_security_group_rules(self, security_group_id):
-        """Refresh security group rules from data store
-
-        Gets called when a rule has been added to or removed from
-        the security group."""
-        raise NotImplementedError()
-
-    def refresh_security_group_members(self, security_group_id):
-        """Refresh security group members from data store
-
-        Gets called when an instance gets added to or removed from
-        the security group."""
-        raise NotImplementedError()
-
-    def refresh_provider_fw_rules(self):
-        """Refresh common rules for all hosts/instances from data store.
-
-        Gets called when a rule has been added to or removed from
-        the list of rules (via admin api).
-
-        """
-        raise NotImplementedError()
-
-    def setup_basic_filtering(self, instance, network_info):
-        """Create rules to block spoofing and allow dhcp.
-
-        This gets called when spawning an instance, before
-        :method:`prepare_instance_filter`.
-
-        """
-        raise NotImplementedError()
-
-    def instance_filter_exists(self, instance, network_info):
-        """Check nova-instance-instance-xxx exists"""
-        raise NotImplementedError()
-
-
-class NWFilterFirewall(FirewallDriver):
+class NWFilterFirewall(base_firewall.FirewallDriver):
     """
     This class implements a network filtering mechanism versatile
     enough for EC2 style Security Group filtering by leveraging
@@ -512,19 +456,10 @@ class NWFilterFirewall(FirewallDriver):
         return True
 
 
-class IptablesFirewallDriver(FirewallDriver):
+class IptablesFirewallDriver(base_firewall.IptablesFirewallDriver):
     def __init__(self, execute=None, **kwargs):
-        from nova.network import linux_net
-        self.iptables = linux_net.iptables_manager
-        self.instances = {}
-        self.network_infos = {}
+        super(IptablesFirewallDriver, self).__init__(**kwargs)
         self.nwfilter = NWFilterFirewall(kwargs['get_connection'])
-        self.basicly_filtered = False
-
-        self.iptables.ipv4['filter'].add_chain('sg-fallback')
-        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
-        self.iptables.ipv6['filter'].add_chain('sg-fallback')
-        self.iptables.ipv6['filter'].add_rule('sg-fallback', '-j DROP')
 
     def setup_basic_filtering(self, instance, network_info):
         """Set up provider rules and basic NWFilter."""
@@ -539,6 +474,8 @@ class IptablesFirewallDriver(FirewallDriver):
         pass
 
     def unfilter_instance(self, instance, network_info):
+        # NOTE(salvatore-orlando):
+        # Overriding base class method for applying nwfilter operation
         if self.instances.pop(instance['id'], None):
             # NOTE(vish): use the passed info instead of the stored info
             self.network_infos.pop(instance['id'])
@@ -549,62 +486,7 @@ class IptablesFirewallDriver(FirewallDriver):
             LOG.info(_('Attempted to unfilter instance %s which is not '
                      'filtered'), instance['id'])
 
-    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)
-        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):
-        ips_v4 = [ip['ip'] for (_n, mapping) in network_info
-                 for ip in mapping['ips']]
-        ipv4_rules = self._create_filter(ips_v4, chain_name)
-
-        ipv6_rules = []
-        if FLAGS.use_ipv6:
-            ips_v6 = [ip['ip'] for (_n, mapping) in network_info
-                     for ip in mapping['ip6s']]
-            ipv6_rules = self._create_filter(ips_v6, chain_name)
-
-        return ipv4_rules, ipv6_rules
-
-    def _add_filters(self, chain_name, ipv4_rules, ipv6_rules):
-        for rule in ipv4_rules:
-            self.iptables.ipv4['filter'].add_rule(chain_name, rule)
-
-        if FLAGS.use_ipv6:
-            for rule in ipv6_rules:
-                self.iptables.ipv6['filter'].add_rule(chain_name, rule)
-
-    def add_filters_for_instance(self, instance):
-        network_info = self.network_infos[instance['id']]
-        chain_name = self._instance_chain_name(instance)
-        if FLAGS.use_ipv6:
-            self.iptables.ipv6['filter'].add_chain(chain_name)
-        self.iptables.ipv4['filter'].add_chain(chain_name)
-        ipv4_rules, ipv6_rules = self._filters_for_instance(chain_name,
-                                                            network_info)
-        self._add_filters('local', ipv4_rules, ipv6_rules)
-        ipv4_rules, ipv6_rules = self.instance_rules(instance, network_info)
-        self._add_filters(chain_name, ipv4_rules, ipv6_rules)
-
-    def remove_filters_for_instance(self, instance):
-        chain_name = self._instance_chain_name(instance)
-
-        self.iptables.ipv4['filter'].remove_chain(chain_name)
-        if FLAGS.use_ipv6:
-            self.iptables.ipv6['filter'].remove_chain(chain_name)
-
-    @staticmethod
-    def instance_rules(instance, network_info):
-        ctxt = context.get_admin_context()
-
-        ipv4_rules = []
-        ipv6_rules = []
-
+    def _do_basic_rules(self, ipv4_rules, ipv6_rules, network_info):
         # Always drop invalid packets
         ipv4_rules += ['-m state --state ' 'INVALID -j DROP']
         ipv6_rules += ['-m state --state ' 'INVALID -j DROP']
@@ -617,131 +499,10 @@ class IptablesFirewallDriver(FirewallDriver):
         ipv4_rules += ['-j $provider']
         ipv6_rules += ['-j $provider']
 
-        dhcp_servers = [info['dhcp_server'] for (_n, info) in network_info]
-
-        for dhcp_server in dhcp_servers:
-            ipv4_rules.append('-s %s -p udp --sport 67 --dport 68 '
-                              '-j ACCEPT' % (dhcp_server,))
-
-        #Allow project network traffic
-        if FLAGS.allow_same_net_traffic:
-            cidrs = [network['cidr'] for (network, _m) in network_info]
-            for cidr in cidrs:
-                ipv4_rules.append('-s %s -j ACCEPT' % (cidr,))
-
-        # We wrap these in FLAGS.use_ipv6 because they might cause
-        # a DB lookup. The other ones are just list operations, so
-        # they're not worth the clutter.
-        if FLAGS.use_ipv6:
-            # Allow RA responses
-            gateways_v6 = [mapping['gateway_v6'] for (_n, mapping) in
-                           network_info]
-            for gateway_v6 in gateways_v6:
-                ipv6_rules.append(
-                        '-s %s/128 -p icmpv6 -j ACCEPT' % (gateway_v6,))
-
-            #Allow project network traffic
-            if FLAGS.allow_same_net_traffic:
-                cidrv6s = [network['cidr_v6'] for (network, _m) in
-                           network_info]
-
-                for cidrv6 in cidrv6s:
-                    ipv6_rules.append('-s %s -j ACCEPT' % (cidrv6,))
-
-        security_groups = db.security_group_get_by_instance(ctxt,
-                                                            instance['id'])
-
-        # then, security group chains and rules
-        for security_group in security_groups:
-            rules = db.security_group_rule_get_by_security_group(ctxt,
-                                                          security_group['id'])
-
-            for rule in rules:
-                LOG.debug(_('Adding security group rule: %r'), rule)
-
-                if not rule.cidr:
-                    version = 4
-                else:
-                    version = netutils.get_ip_version(rule.cidr)
-
-                if version == 4:
-                    fw_rules = ipv4_rules
-                else:
-                    fw_rules = ipv6_rules
-
-                protocol = rule.protocol
-                if version == 6 and rule.protocol == 'icmp':
-                    protocol = 'icmpv6'
-
-                args = ['-j ACCEPT']
-                if protocol:
-                    args += ['-p', protocol]
-
-                if protocol in ['udp', 'tcp']:
-                    if rule.from_port == rule.to_port:
-                        args += ['--dport', '%s' % (rule.from_port,)]
-                    else:
-                        args += ['-m', 'multiport',
-                                 '--dports', '%s:%s' % (rule.from_port,
-                                                        rule.to_port)]
-                elif protocol == 'icmp':
-                    icmp_type = rule.from_port
-                    icmp_code = rule.to_port
-
-                    if icmp_type == -1:
-                        icmp_type_arg = None
-                    else:
-                        icmp_type_arg = '%s' % icmp_type
-                        if not icmp_code == -1:
-                            icmp_type_arg += '/%s' % icmp_code
-
-                    if icmp_type_arg:
-                        if version == 4:
-                            args += ['-m', 'icmp', '--icmp-type',
-                                     icmp_type_arg]
-                        elif version == 6:
-                            args += ['-m', 'icmp6', '--icmpv6-type',
-                                     icmp_type_arg]
-
-                if rule.cidr:
-                    LOG.info('Using cidr %r', rule.cidr)
-                    args += ['-s', rule.cidr]
-                    fw_rules += [' '.join(args)]
-                else:
-                    if rule['grantee_group']:
-                        for instance in rule['grantee_group']['instances']:
-                            LOG.info('instance: %r', instance)
-                            ips = db.instance_get_fixed_addresses(ctxt,
-                                                                instance['id'])
-                            LOG.info('ips: %r', ips)
-                            for ip in ips:
-                                subrule = args + ['-s %s' % ip]
-                                fw_rules += [' '.join(subrule)]
-
-                LOG.info('Using fw_rules: %r', fw_rules)
-        ipv4_rules += ['-j $sg-fallback']
-        ipv6_rules += ['-j $sg-fallback']
-
-        return ipv4_rules, ipv6_rules
-
     def instance_filter_exists(self, instance, network_info):
         """Check nova-instance-instance-xxx exists"""
         return self.nwfilter.instance_filter_exists(instance, network_info)
 
-    def refresh_security_group_members(self, security_group):
-        self.do_refresh_security_group_rules(security_group)
-        self.iptables.apply()
-
-    def refresh_security_group_rules(self, security_group):
-        self.do_refresh_security_group_rules(security_group)
-        self.iptables.apply()
-
-    @utils.synchronized('iptables', external=True)
-    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()
@@ -821,11 +582,3 @@ class IptablesFirewallDriver(FirewallDriver):
             args += ['-j DROP']
             fw_rules += [' '.join(args)]
         return ipv4_rules, ipv6_rules
-
-    @staticmethod
-    def _security_group_chain_name(security_group_id):
-        return 'nova-sg-%s' % (security_group_id,)
-
-    @staticmethod
-    def _instance_chain_name(instance):
-        return 'inst-%s' % (instance['id'],)
diff --git a/nova/virt/libvirt/vif.py b/nova/virt/libvirt/vif.py
index 6ce1041cc..d4375f6da 100644
--- a/nova/virt/libvirt/vif.py
+++ b/nova/virt/libvirt/vif.py
@@ -24,9 +24,10 @@ from nova import flags
 from nova import log as logging
 from nova.network import linux_net
 from nova import utils
-from nova.virt.libvirt import netutils
+from nova.virt import netutils
 from nova.virt.vif import VIFDriver
 
+
 LOG = logging.getLogger('nova.virt.libvirt.vif')
 
 FLAGS = flags.FLAGS
diff --git a/nova/virt/libvirt/netutils.py b/nova/virt/netutils.py
index 6f303072d..6f303072d 100644
--- a/nova/virt/libvirt/netutils.py
+++ b/nova/virt/netutils.py
diff --git a/nova/virt/xenapi/fake.py b/nova/virt/xenapi/fake.py
index b7f665fa7..9b0c2adce 100644
--- a/nova/virt/xenapi/fake.py
+++ b/nova/virt/xenapi/fake.py
@@ -378,7 +378,7 @@ class SessionBase(object):
     def PBD_plug(self, _1, pbd_ref):
         rec = get_record('PBD', pbd_ref)
         if rec['currently_attached']:
-            raise Failure(['DEVICE_ALREADY_ATTACHED', ref])
+            raise Failure(['DEVICE_ALREADY_ATTACHED', rec])
         rec['currently_attached'] = True
         sr_ref = rec['SR']
         _db_content['SR'][sr_ref]['PBDs'] = [pbd_ref]
@@ -386,7 +386,7 @@ class SessionBase(object):
     def PBD_unplug(self, _1, pbd_ref):
         rec = get_record('PBD', pbd_ref)
         if not rec['currently_attached']:
-            raise Failure(['DEVICE_ALREADY_DETACHED', ref])
+            raise Failure(['DEVICE_ALREADY_DETACHED', rec])
         rec['currently_attached'] = False
         sr_ref = pbd_ref['SR']
         _db_content['SR'][sr_ref]['PBDs'].remove(pbd_ref)
diff --git a/nova/virt/xenapi/firewall.py b/nova/virt/xenapi/firewall.py
new file mode 100644
index 000000000..282e12d9e
--- /dev/null
+++ b/nova/virt/xenapi/firewall.py
@@ -0,0 +1,70 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2010 United States Government as represented by the
+# Administrator of the National Aeronautics and Space Administration.
+# All Rights Reserved.
+# Copyright (c) 2010 Citrix Systems, Inc.
+#
+#    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
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import json
+
+from nova import context
+from nova import db
+from nova import flags
+from nova import log as logging
+from nova.virt.firewall import IptablesFirewallDriver
+from nova.virt import netutils
+
+
+LOG = logging.getLogger("nova.virt.xenapi.firewall")
+FLAGS = flags.FLAGS
+
+
+class Dom0IptablesFirewallDriver(IptablesFirewallDriver):
+    """ IptablesFirewallDriver class
+
+    This class provides an implementation for nova.virt.Firewall
+    using iptables. This class is meant to be used with the xenapi
+    backend and uses xenapi plugin to enforce iptables rules in dom0
+
+    """
+    def _plugin_execute(self, *cmd, **kwargs):
+        # Prepare arguments for plugin call
+        args = {}
+        args.update(map(lambda x: (x, str(kwargs[x])), kwargs))
+        args['cmd_args'] = json.dumps(cmd)
+        task = self._session.async_call_plugin(
+            'xenhost', 'iptables_config', args)
+        ret = self._session.wait_for_task(task)
+        json_ret = json.loads(ret)
+        return (json_ret['out'], json_ret['err'])
+
+    def __init__(self, xenapi_session=None, **kwargs):
+        from nova.network import linux_net
+        super(Dom0IptablesFirewallDriver, self).__init__(**kwargs)
+        self._session = xenapi_session
+        # Create IpTablesManager with executor through plugin
+        self.iptables = linux_net.IptablesManager(self._plugin_execute)
+        self.iptables.ipv4['filter'].add_chain('sg-fallback')
+        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
+        self.iptables.ipv6['filter'].add_chain('sg-fallback')
+        self.iptables.ipv6['filter'].add_rule('sg-fallback', '-j DROP')
+
+    def _build_tcp_udp_rule(self, rule, version):
+        if rule.from_port == rule.to_port:
+            return ['--dport', '%s' % (rule.from_port,)]
+        else:
+            #  No multiport needed for XS!
+            return ['--dport', '%s:%s' % (rule.from_port,
+                                           rule.to_port)]
diff --git a/nova/virt/xenapi/vm_utils.py b/nova/virt/xenapi/vm_utils.py
index c0ac3ed8d..6cda61204 100644
--- a/nova/virt/xenapi/vm_utils.py
+++ b/nova/virt/xenapi/vm_utils.py
@@ -849,6 +849,14 @@ class VMHelper(HelperBase):
             return (None, None)
 
     @classmethod
+    def is_snapshot(cls, session, vm):
+        vm_rec = session.call_xenapi("VM.get_record", vm)
+        if 'is_a_template' in vm_rec and 'is_a_snapshot' in vm_rec:
+            return vm_rec['is_a_template'] and vm_rec['is_a_snapshot']
+        else:
+            return False
+
+    @classmethod
     def compile_info(cls, record):
         """Fill record with VM status information"""
         LOG.info(_("(VM_UTILS) xenserver vm state -> |%s|"),
diff --git a/nova/virt/xenapi/vmops.py b/nova/virt/xenapi/vmops.py
index bb6239d7e..99f5ca650 100644
--- a/nova/virt/xenapi/vmops.py
+++ b/nova/virt/xenapi/vmops.py
@@ -94,6 +94,8 @@ class VMOps(object):
         self._session = session
         self.poll_rescue_last_ran = None
         VMHelper.XenAPI = self.XenAPI
+        fw_class = utils.import_class(FLAGS.firewall_driver)
+        self.firewall_driver = fw_class(xenapi_session=self._session)
         vif_impl = utils.import_class(FLAGS.xenapi_vif_driver)
         self.vif_driver = vif_impl(xenapi_session=self._session)
         self._product_version = product_version
@@ -207,9 +209,22 @@ class VMOps(object):
             self._update_instance_progress(context, instance,
                                            step=3,
                                            total_steps=BUILD_TOTAL_STEPS)
+            # 4. Prepare security group filters
+            # NOTE(salvatore-orlando): setup_basic_filtering might be empty or
+            # not implemented at all, as basic filter could be implemented
+            # with VIF rules created by xapi plugin
+            try:
+                self.firewall_driver.setup_basic_filtering(
+                        instance, network_info)
+            except NotImplementedError:
+                pass
+            self.firewall_driver.prepare_instance_filter(instance,
+                                                         network_info)
 
-            # 4. Boot the Instance
+            # 5. Boot the Instance
             self._spawn(instance, vm_ref)
+            # The VM has started, let's ensure the security groups are enforced
+            self.firewall_driver.apply_instance_filter(instance, network_info)
             self._update_instance_progress(context, instance,
                                            step=4,
                                            total_steps=BUILD_TOTAL_STEPS)
@@ -828,6 +843,9 @@ class VMOps(object):
 
     def reboot(self, instance, reboot_type):
         """Reboot VM instance."""
+        # Note (salvatore-orlando): security group rules are not re-enforced
+        # upon reboot, since this action on the XenAPI drivers does not
+        # remove existing filters
         vm_ref = self._get_vm_opaque_ref(instance)
 
         if reboot_type == "HARD":
@@ -1117,16 +1135,21 @@ class VMOps(object):
         if vm_ref is None:
             LOG.warning(_("VM is not present, skipping destroy..."))
             return
-
+        is_snapshot = VMHelper.is_snapshot(self._session, vm_ref)
         if shutdown:
             self._shutdown(instance, vm_ref)
 
         self._destroy_vdis(instance, vm_ref)
         if destroy_kernel_ramdisk:
             self._destroy_kernel_ramdisk(instance, vm_ref)
-        self._destroy_vm(instance, vm_ref)
 
+        self._destroy_vm(instance, vm_ref)
         self.unplug_vifs(instance, network_info)
+        # Remove security groups filters for instance
+        # Unless the vm is a snapshot
+        if not is_snapshot:
+            self.firewall_driver.unfilter_instance(instance,
+                                                   network_info=network_info)
 
     def pause(self, instance):
         """Pause VM instance."""
@@ -1683,6 +1706,19 @@ class VMOps(object):
     def clear_param_xenstore(self, instance_or_vm):
         """Removes all data from the xenstore parameter record for this VM."""
         self.write_to_param_xenstore(instance_or_vm, {})
+
+    def refresh_security_group_rules(self, security_group_id):
+        """ recreates security group rules for every instance """
+        self.firewall_driver.refresh_security_group_rules(security_group_id)
+
+    def refresh_security_group_members(self, security_group_id):
+        """ recreates security group rules for every instance """
+        self.firewall_driver.refresh_security_group_members(security_group_id)
+
+    def unfilter_instance(self, instance_ref, network_info):
+        """Removes filters for each VIF of the specified instance."""
+        self.firewall_driver.unfilter_instance(instance_ref,
+                                               network_info=network_info)
     ########################################################################
 
 
diff --git a/nova/virt/xenapi_conn.py b/nova/virt/xenapi_conn.py
index 61b830054..951db00e8 100644
--- a/nova/virt/xenapi_conn.py
+++ b/nova/virt/xenapi_conn.py
@@ -411,6 +411,10 @@ class XenAPIConnection(driver.ComputeDriver):
 
     def ensure_filtering_rules_for_instance(self, instance_ref, network_info):
         """This method is supported only libvirt."""
+        # NOTE(salvatore-orlando): it enforces security groups on
+        # host initialization and live migration.
+        # Live migration is not supported by XenAPI (as of 2011-11-09)
+        # In XenAPI we do not assume instances running upon host initialization
         return
 
     def live_migration(self, context, instance_ref, dest,
@@ -419,8 +423,22 @@ class XenAPIConnection(driver.ComputeDriver):
         return
 
     def unfilter_instance(self, instance_ref, network_info):
-        """This method is supported only by libvirt."""
-        raise NotImplementedError('This method is supported only by libvirt.')
+        """Removes security groups configured for an instance."""
+        return self._vmops.unfilter_instance(instance_ref, network_info)
+
+    def refresh_security_group_rules(self, security_group_id):
+        """ Updates security group rules for all instances
+            associated with a given security group
+            Invoked when security group rules are updated
+        """
+        return self._vmops.refresh_security_group_rules(security_group_id)
+
+    def refresh_security_group_members(self, security_group_id):
+        """ Updates security group rules for all instances
+            associated with a given security group
+            Invoked when instances are added/removed to a security group
+        """
+        return self._vmops.refresh_security_group_members(security_group_id)
 
     def update_host_status(self):
         """Update the status info of the host, and return those values