From 6124f6029729c3c800287f3f02329901d93ea021 Mon Sep 17 00:00:00 2001 From: Bernard Cafarelli Date: Thu, 6 Sep 2018 10:48:13 +0200 Subject: [PATCH] Switch isolated metadata proxy to bind to 169.254.169.254 Currently the metadata proxy binds to default 0.0.0.0, which does not add any advantage (metadata requests are not sent to random IP addresses), and may allow access to cloud information from third parties. This changes the generated configuration to bind to METADATA_DEFAULT_IP address instead. This is not enabled in other metadata proxy configuration (in the L3 agent), as this would require net.ipv4.ip_nonlocal_bind everywhere (currently only enabled for DVR) or transparent mode in haproxy (which requires net.ipv4.ip_nonlocal_bind anyway) Changed set_ip_nonlocal_bind_for_namespace() to support setting the value in both the given and root namespace correctly, since it was only used from inside the neutron codebase according to codesearch. Change-Id: I388391cf697dade1a163d15ab568b33134f7b2d9 Co-Authored-By: Andrey Arapov Closes-Bug: #1745618 --- neutron/agent/dhcp/agent.py | 2 +- neutron/agent/l3/dvr_fip_ns.py | 12 ++--------- neutron/agent/l3/dvr_snat_ns.py | 2 +- neutron/agent/l3/ha_router.py | 2 +- neutron/agent/linux/dhcp.py | 2 ++ neutron/agent/linux/ip_lib.py | 15 +++++++++---- neutron/agent/metadata/driver.py | 21 ++++++++++++------- neutron/tests/unit/agent/dhcp/test_agent.py | 6 ++++++ neutron/tests/unit/agent/linux/test_ip_lib.py | 2 +- .../tests/unit/agent/metadata/test_driver.py | 9 ++++++-- 10 files changed, 45 insertions(+), 28 deletions(-) diff --git a/neutron/agent/dhcp/agent.py b/neutron/agent/dhcp/agent.py index 5dae493d3b2..19b950877f0 100644 --- a/neutron/agent/dhcp/agent.py +++ b/neutron/agent/dhcp/agent.py @@ -657,7 +657,7 @@ class DhcpAgent(manager.Manager): metadata_driver.MetadataDriver.spawn_monitored_metadata_proxy( self._process_monitor, network.namespace, dhcp.METADATA_PORT, - self.conf, **kwargs) + self.conf, bind_address=dhcp.METADATA_DEFAULT_IP, **kwargs) def disable_isolated_metadata_proxy(self, network): if (self.conf.enable_metadata_network and diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index b87aa18ee29..ccbc285acca 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -208,16 +208,8 @@ class FipNamespace(namespaces.Namespace): LOG.debug("DVR: add fip namespace: %s", self.name) # parent class will ensure the namespace exists and turn-on forwarding super(FipNamespace, self).create() - # Somewhere in the 3.19 kernel timeframe ip_nonlocal_bind was - # changed to be a per-namespace attribute. To be backwards - # compatible we need to try both if at first we fail. - failed = ip_lib.set_ip_nonlocal_bind( - value=1, namespace=self.name, log_fail_as_error=False) - if failed: - LOG.debug('DVR: fip namespace (%s) does not support setting ' - 'net.ipv4.ip_nonlocal_bind, trying in root namespace', - self.name) - ip_lib.set_ip_nonlocal_bind(value=1) + ip_lib.set_ip_nonlocal_bind_for_namespace(self.name, 1, + root_namespace=True) # no connection tracking needed in fip namespace self._iptables_manager.ipv4['raw'].add_rule('PREROUTING', diff --git a/neutron/agent/l3/dvr_snat_ns.py b/neutron/agent/l3/dvr_snat_ns.py index 80d363253cc..30c0db95821 100644 --- a/neutron/agent/l3/dvr_snat_ns.py +++ b/neutron/agent/l3/dvr_snat_ns.py @@ -32,7 +32,7 @@ class SnatNamespace(namespaces.Namespace): super(SnatNamespace, self).create() # This might be an HA router namespaces and it should not have # ip_nonlocal_bind enabled - ip_lib.set_ip_nonlocal_bind_for_namespace(self.name) + ip_lib.set_ip_nonlocal_bind_for_namespace(self.name, 0) # Set nf_conntrack_tcp_loose to 0 to ensure mid-stream # TCP conversations aren't taken over by SNAT cmd = ['net.netfilter.nf_conntrack_tcp_loose=0'] diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 9b80280d6e1..8606e9e6c23 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -57,7 +57,7 @@ class HaRouterNamespace(namespaces.RouterNamespace): def create(self): super(HaRouterNamespace, self).create(ipv6_forwarding=False) # HA router namespaces should not have ip_nonlocal_bind enabled - ip_lib.set_ip_nonlocal_bind_for_namespace(self.name) + ip_lib.set_ip_nonlocal_bind_for_namespace(self.name, 0) class HaRouter(router.RouterInfo): diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 0c078e22837..9670adee681 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -1486,6 +1486,8 @@ class DeviceManager(object): # and added back statically in the call to init_l3() below. if network.namespace: ip_lib.IPWrapper().ensure_namespace(network.namespace) + ip_lib.set_ip_nonlocal_bind_for_namespace(network.namespace, 1, + root_namespace=True) if ipv6_utils.is_enabled_and_bind_by_default(): self.driver.configure_ipv6_ra(network.namespace, 'default', n_const.ACCEPT_RA_DISABLED) diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 2698d1df060..20450fe84eb 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -1175,18 +1175,25 @@ def set_ip_nonlocal_bind(value, namespace=None, log_fail_as_error=True): log_fail_as_error=log_fail_as_error) -def set_ip_nonlocal_bind_for_namespace(namespace): +def set_ip_nonlocal_bind_for_namespace(namespace, value, root_namespace=False): """Set ip_nonlocal_bind but don't raise exception on failure.""" - failed = set_ip_nonlocal_bind(value=0, namespace=namespace, + failed = set_ip_nonlocal_bind(value, namespace=namespace, log_fail_as_error=False) + if failed and root_namespace: + # Somewhere in the 3.19 kernel timeframe ip_nonlocal_bind was + # changed to be a per-namespace attribute. To be backwards + # compatible we need to try both if at first we fail. + LOG.debug('Namespace (%s) does not support setting %s, ' + 'trying in root namespace', namespace, IP_NONLOCAL_BIND) + return set_ip_nonlocal_bind(value) if failed: LOG.warning( - "%s will not be set to 0 in the root namespace in order to " + "%s will not be set to %d in the root namespace in order to " "not break DVR, which requires this value be set to 1. This " "may introduce a race between moving a floating IP to a " "different network node, and the peer side getting a " "populated ARP cache for a given floating IP address.", - IP_NONLOCAL_BIND) + IP_NONLOCAL_BIND, value) def get_ipv6_forwarding(device, namespace=None): diff --git a/neutron/agent/metadata/driver.py b/neutron/agent/metadata/driver.py index d21ba0180ed..b5d4f9e8ded 100644 --- a/neutron/agent/metadata/driver.py +++ b/neutron/agent/metadata/driver.py @@ -61,7 +61,7 @@ defaults timeout http-keep-alive 30s listen listener - bind 0.0.0.0:%(port)s + bind %(host)s:%(port)s server metadata %(unix_socket_path)s http-request add-header X-Neutron-%(res_type)s-ID %(res_id)s """ @@ -72,13 +72,14 @@ class InvalidUserOrGroupException(Exception): class HaproxyConfigurator(object): - def __init__(self, network_id, router_id, unix_socket_path, port, user, - group, state_path, pid_file): + def __init__(self, network_id, router_id, unix_socket_path, host, port, + user, group, state_path, pid_file): self.network_id = network_id self.router_id = router_id if network_id is None and router_id is None: raise exceptions.NetworkIdOrRouterIdRequiredError() + self.host = host self.port = port self.user = user self.group = group @@ -116,6 +117,7 @@ class HaproxyConfigurator(object): _("Invalid group/gid: '%s'") % self.group) cfg_info = { + 'host': self.host, 'port': self.port, 'unix_socket_path': self.unix_socket_path, 'user': username, @@ -209,8 +211,8 @@ class MetadataDriver(object): return user, group @classmethod - def _get_metadata_proxy_callback(cls, port, conf, network_id=None, - router_id=None): + def _get_metadata_proxy_callback(cls, bind_address, port, conf, + network_id=None, router_id=None): def callback(pid_file): metadata_proxy_socket = conf.metadata_proxy_socket user, group = ( @@ -218,6 +220,7 @@ class MetadataDriver(object): haproxy = HaproxyConfigurator(network_id, router_id, metadata_proxy_socket, + bind_address, port, user, group, @@ -232,10 +235,12 @@ class MetadataDriver(object): @classmethod def spawn_monitored_metadata_proxy(cls, monitor, ns_name, port, conf, - network_id=None, router_id=None): + bind_address="0.0.0.0", network_id=None, + router_id=None): uuid = network_id or router_id callback = cls._get_metadata_proxy_callback( - port, conf, network_id=network_id, router_id=router_id) + bind_address, port, conf, network_id=network_id, + router_id=router_id) pm = cls._get_metadata_proxy_process_manager(uuid, conf, ns_name=ns_name, callback=callback) @@ -269,7 +274,7 @@ def after_router_added(resource, event, l3_agent, **kwargs): router = kwargs['router'] proxy = l3_agent.metadata_driver for c, r in proxy.metadata_filter_rules(proxy.metadata_port, - proxy.metadata_access_mark): + proxy.metadata_access_mark): router.iptables_manager.ipv4['filter'].add_rule(c, r) for c, r in proxy.metadata_nat_rules(proxy.metadata_port): router.iptables_manager.ipv4['nat'].add_rule(c, r) diff --git a/neutron/tests/unit/agent/dhcp/test_agent.py b/neutron/tests/unit/agent/dhcp/test_agent.py index 40b3c9930c9..05f1ba74eaa 100644 --- a/neutron/tests/unit/agent/dhcp/test_agent.py +++ b/neutron/tests/unit/agent/dhcp/test_agent.py @@ -220,6 +220,9 @@ fake_down_network = dhcp.NetModel( class TestDhcpAgent(base.BaseTestCase): + + METADATA_DEFAULT_IP = dhcp.METADATA_DEFAULT_IP + def setUp(self): super(TestDhcpAgent, self).setUp() entry.register_options(cfg.CONF) @@ -494,6 +497,7 @@ class TestDhcpAgent(base.BaseTestCase): dhcp.configure_dhcp_for_network(fake_network) md_cls.spawn_monitored_metadata_proxy.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, mock.ANY, + bind_address=self.METADATA_DEFAULT_IP, network_id=fake_network.id) dhcp.disable_dhcp_helper(fake_network.id) md_cls.destroy_monitored_metadata_proxy.assert_called_once_with( @@ -889,10 +893,12 @@ class TestDhcpAgentEventHandler(base.BaseTestCase): '.spawn_monitored_metadata_proxy') with mock.patch(method_path) as spawn: self.dhcp.enable_isolated_metadata_proxy(network) + metadata_ip = dhcp.METADATA_DEFAULT_IP spawn.assert_called_once_with(self.dhcp._process_monitor, network.namespace, dhcp.METADATA_PORT, cfg.CONF, + bind_address=metadata_ip, router_id='forzanapoli') def test_enable_isolated_metadata_proxy_with_metadata_network(self): diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 620ce35609d..de098929a1e 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -1713,7 +1713,7 @@ class TestSetIpNonlocalBindForHaNamespace(base.BaseTestCase): def test_setting_failure(self): """Make sure message is formatted correctly.""" with mock.patch.object(ip_lib, 'set_ip_nonlocal_bind', return_value=1): - ip_lib.set_ip_nonlocal_bind_for_namespace('foo') + ip_lib.set_ip_nonlocal_bind_for_namespace('foo', value=1) class TestSysctl(base.BaseTestCase): diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py index fba33e663f3..be8178037b3 100644 --- a/neutron/tests/unit/agent/metadata/test_driver.py +++ b/neutron/tests/unit/agent/metadata/test_driver.py @@ -64,6 +64,7 @@ class TestMetadataDriverProcess(base.BaseTestCase): EUNAME = 'neutron' EGNAME = 'neutron' + METADATA_DEFAULT_IP = '169.254.169.254' METADATA_PORT = 8080 METADATA_SOCKET = '/socket/path' PIDFILE = 'pidfile' @@ -146,6 +147,7 @@ class TestMetadataDriverProcess(base.BaseTestCase): router_ns, self.METADATA_PORT, agent.conf, + bind_address=self.METADATA_DEFAULT_IP, router_id=router_id) netns_execute_args = [ @@ -157,6 +159,7 @@ class TestMetadataDriverProcess(base.BaseTestCase): cfg_contents = metadata_driver._HAPROXY_CONFIG_TEMPLATE % { 'user': self.EUNAME, 'group': self.EGNAME, + 'host': self.METADATA_DEFAULT_IP, 'port': self.METADATA_PORT, 'unix_socket_path': self.METADATA_SOCKET, 'res_type': 'Router', @@ -178,7 +181,8 @@ class TestMetadataDriverProcess(base.BaseTestCase): def test_create_config_file_wrong_user(self): with mock.patch('pwd.getpwnam', side_effect=KeyError): - config = metadata_driver.HaproxyConfigurator(_uuid(), mock.ANY, + config = metadata_driver.HaproxyConfigurator(_uuid(), + mock.ANY, mock.ANY, mock.ANY, mock.ANY, self.EUNAME, self.EGNAME, @@ -190,7 +194,8 @@ class TestMetadataDriverProcess(base.BaseTestCase): with mock.patch('grp.getgrnam', side_effect=KeyError),\ mock.patch('pwd.getpwnam', return_value=test_utils.FakeUser(self.EUNAME)): - config = metadata_driver.HaproxyConfigurator(_uuid(), mock.ANY, + config = metadata_driver.HaproxyConfigurator(_uuid(), + mock.ANY, mock.ANY, mock.ANY, mock.ANY, self.EUNAME, self.EGNAME,