openstack/nova
Code Issues Proposed changes

Blueprint xenapi-provider-firewall and Bug #915403.

Browse Source 
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
This commit is contained in:
Deepak Garg
2012-01-13 16:03:45 +05:30
committed by Vishvananda Ishaya
parent 2594e480b2
commit fe1c97ff4c
15 changed files with 263 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
Authors
View File

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

41
nova/api/ec2/admin.py
View File

@@ -349,8 +349,15 @@ class AdminController(object):
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 @@ class AdminController(object):
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 @@ class AdminController(object):
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)}

2
nova/compute/api.py
View File

@@ -710,7 +710,7 @@ class API(base.Base):
"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)]

4
nova/compute/manager.py
View File

@@ -261,9 +261,9 @@ class ComputeManager(manager.SchedulerDependentManager):
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.

9
nova/network/linux_net.py
View File

@@ -303,16 +303,17 @@ class IptablesManager(object):
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

59
nova/tests/api/ec2/test_admin.py
View File

@@ -365,24 +365,43 @@ class AdminControllerTestCase(test.TestCase):
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 @@ class AdminControllerTestCase(test.TestCase):
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):

60
nova/tests/test_xenapi.py
View File

@@ -1334,6 +1334,9 @@ class XenAPIBWUsageTestCase(test.TestCase):
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 @@ class XenAPIDom0IptablesFirewallTestCase(test.TestCase):
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))

1
nova/tests/xenapi/stubs.py
View File

@@ -32,7 +32,6 @@ def stubout_firewall_driver(stubs, conn):
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)

4
nova/utils.py
View File

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

4
nova/virt/driver.py
View File

@@ -380,7 +380,7 @@ class ComputeDriver(object):
* 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 @@ class ComputeDriver(object):
# 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

47
nova/virt/firewall.py
View File

@@ -34,7 +34,7 @@ flags.DEFINE_bool('allow_same_net_traffic',
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 @@ class IptablesFirewallDriver(FirewallDriver):
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 @@ class IptablesFirewallDriver(FirewallDriver):
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 @@ class IptablesFirewallDriver(FirewallDriver):
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()

45
nova/virt/libvirt/firewall.py
View File

@@ -259,7 +259,7 @@ class NWFilterFirewall(base_firewall.FirewallDriver):
LOG.debug(_('The nwfilter(%(instance_filter_name)s) '
'for %(instance_name)s is not found.') % locals())
instance_secgroup_filter_name =\
instance_secgroup_filter_name = \
'%s-secgroup' % (self._instance_filter_name(instance))
try:
@@ -486,53 +486,10 @@ class IptablesFirewallDriver(base_firewall.IptablesFirewallDriver):
LOG.info(_('Attempted to unfilter instance %s which is not '
'filtered'), 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']
# Pass through provider-wide drops
ipv4_rules += ['-j $provider']
ipv6_rules += ['-j $provider']
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_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."""

56
nova/virt/xenapi/firewall.py
View File

@@ -19,8 +19,12 @@
import json
from nova import context
from nova import flags
from nova import log as logging
from nova.db import api as db
from nova.virt import netutils
from nova.virt.firewall import IptablesFirewallDriver
@@ -65,3 +69,55 @@ class Dom0IptablesFirewallDriver(IptablesFirewallDriver):
# No multiport needed for XS!
return ['--dport', '%s:%s' % (rule.from_port,
rule.to_port)]
@staticmethod
def _provider_rules():
"""Generate a list of rules from provider for IP4 & IP6.
Note: We could not use the common code from virt.firewall because
XS doesn't accept the '-m multiport' option"""
ctxt = context.get_admin_context()
ipv4_rules = []
ipv6_rules = []
rules = db.provider_fw_rule_get_all(ctxt)
for rule in rules:
LOG.debug(_('Adding provider rule: %s'), rule['cidr'])
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 protocol == 'icmp':
protocol = 'icmpv6'
args = ['-p', protocol, '-s', rule['cidr']]
if protocol in ['udp', 'tcp']:
if rule['from_port'] == rule['to_port']:
args += ['--dport', '%s' % (rule['from_port'],)]
else:
args += ['--dport', '%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]
args += ['-j DROP']
fw_rules += [' '.join(args)]
return ipv4_rules, ipv6_rules

3
nova/virt/xenapi/vmops.py
View File

@@ -1744,6 +1744,9 @@ class VMOps(object):
""" recreates security group rules for every instance """
self.firewall_driver.refresh_security_group_members(security_group_id)
def refresh_provider_fw_rules(self):
self.firewall_driver.refresh_provider_fw_rules()
def unfilter_instance(self, instance_ref, network_info):
"""Removes filters for each VIF of the specified instance."""
self.firewall_driver.unfilter_instance(instance_ref,

3
nova/virt/xenapi_conn.py
View File

@@ -443,6 +443,9 @@ class XenAPIConnection(driver.ComputeDriver):
"""
return self._vmops.refresh_security_group_members(security_group_id)
def refresh_provider_fw_rules(self):
return self._vmops.refresh_provider_fw_rules()
def update_host_status(self):
"""Update the status info of the host, and return those values
to the calling program."""