From ffe8080a91e27233299a38eb845a028682bde167 Mon Sep 17 00:00:00 2001
From: Bernard Cafarelli <bcafarel@redhat.com>
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.

Conflicts:
	neutron/agent/linux/dhcp.py
	neutron/tests/unit/agent/linux/test_ip_lib.py

Change-Id: I388391cf697dade1a163d15ab568b33134f7b2d9
Co-Authored-By: Andrey Arapov <andrey.arapov@nixaid.com>
Closes-Bug: #1745618
(cherry picked from commit 6124f6029729c3c800287f3f02329901d93ea021)
(cherry picked from commit 13d23ba36340023bceaed46c5c13942b64f9e42f)
---
 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 1fabc12fbc..0abfa8171e 100644
--- a/neutron/agent/dhcp/agent.py
+++ b/neutron/agent/dhcp/agent.py
@@ -553,7 +553,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 b3083807ff..6e48b303ff 100644
--- a/neutron/agent/l3/dvr_fip_ns.py
+++ b/neutron/agent/l3/dvr_fip_ns.py
@@ -201,16 +201,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 80d363253c..30c0db9582 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 96369ef00b..6921eedf03 100644
--- a/neutron/agent/l3/ha_router.py
+++ b/neutron/agent/l3/ha_router.py
@@ -54,7 +54,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 6ea4ba3b26..ec2888c71c 100644
--- a/neutron/agent/linux/dhcp.py
+++ b/neutron/agent/linux/dhcp.py
@@ -1490,6 +1490,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)
         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 fb405c8128..df89a5d738 100644
--- a/neutron/agent/linux/ip_lib.py
+++ b/neutron/agent/linux/ip_lib.py
@@ -1217,18 +1217,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 6d96a8ac4c..4ebce7fb93 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,
@@ -201,8 +203,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 = (
@@ -210,6 +212,7 @@ class MetadataDriver(object):
             haproxy = HaproxyConfigurator(network_id,
                                           router_id,
                                           metadata_proxy_socket,
+                                          bind_address,
                                           port,
                                           user,
                                           group,
@@ -224,10 +227,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)
@@ -282,7 +287,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 24e08ba16e..64bdbc54f3 100644
--- a/neutron/tests/unit/agent/dhcp/test_agent.py
+++ b/neutron/tests/unit/agent/dhcp/test_agent.py
@@ -215,6 +215,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)
@@ -464,6 +467,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(
@@ -847,10 +851,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 20820fef33..3ab3759e1c 100644
--- a/neutron/tests/unit/agent/linux/test_ip_lib.py
+++ b/neutron/tests/unit/agent/linux/test_ip_lib.py
@@ -1768,4 +1768,4 @@ 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)
diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py
index 2833b3697a..d1fd47d584 100644
--- a/neutron/tests/unit/agent/metadata/test_driver.py
+++ b/neutron/tests/unit/agent/metadata/test_driver.py
@@ -57,6 +57,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'
@@ -139,6 +140,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 = [
@@ -150,6 +152,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',
@@ -171,7 +174,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,
@@ -183,7 +187,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,
-- 
2.25.1

