From b6035d6137f357d3208af10fd36b98f6eb868e1f Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Fri, 8 Nov 2019 18:41:39 +0100
Subject: [PATCH] Prevent localhost from being used as ironic-inspector
 callback URL

At least bifrost used to do it, so it's better to prevent users
from shooting their legs.

Change-Id: Idf25d9f434483f023ad7a40b6c242635ab89a804
Story: #1528920
---
 ironic/drivers/modules/inspector.py           | 27 ++++++++++++++++---
 .../unit/drivers/modules/test_inspector.py    |  6 +++++
 .../notes/no-localhost-596adedfe388dfae.yaml  |  7 +++++
 3 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/no-localhost-596adedfe388dfae.yaml

diff --git a/ironic/drivers/modules/inspector.py b/ironic/drivers/modules/inspector.py
index 6339c59cba..e71f1902e5 100644
--- a/ironic/drivers/modules/inspector.py
+++ b/ironic/drivers/modules/inspector.py
@@ -15,12 +15,14 @@ Modules required to work with ironic_inspector:
     https://pypi.org/project/ironic-inspector
 """
 
+import ipaddress
 import shlex
 
 import eventlet
 from futurist import periodics
 import openstack
 from oslo_log import log as logging
+from six.moves.urllib import parse as urlparse
 
 from ironic.common import exception
 from ironic.common.i18n import _
@@ -72,12 +74,29 @@ def _get_callback_endpoint(client):
     root = CONF.inspector.callback_endpoint_override or client.get_endpoint()
     if root == 'mdns':
         return root
-    root = root.rstrip('/')
+
+    parts = urlparse.urlsplit(root)
+    is_loopback = False
+    try:
+        # ip_address requires a unicode string on Python 2
+        is_loopback = ipaddress.ip_address(parts.hostname).is_loopback
+    except ValueError:  # host name
+        is_loopback = (parts.hostname == 'localhost')
+
+    if is_loopback:
+        raise exception.InvalidParameterValue(
+            _('Loopback address %s cannot be used as an introspection '
+              'callback URL') % parts.hostname)
+
     # NOTE(dtantsur): the IPA side is quite picky about the exact format.
-    if root.endswith('/v1'):
-        return '%s/continue' % root
+    if parts.path.endswith('/v1'):
+        add = '/continue'
     else:
-        return '%s/v1/continue' % root
+        add = '/v1/continue'
+
+    return urlparse.urlunsplit((parts.scheme, parts.netloc,
+                                parts.path.rstrip('/') + add,
+                                parts.query, parts.fragment))
 
 
 def _tear_down_managed_boot(task):
diff --git a/ironic/tests/unit/drivers/modules/test_inspector.py b/ironic/tests/unit/drivers/modules/test_inspector.py
index f894fcc91e..c81ea0a8ff 100644
--- a/ironic/tests/unit/drivers/modules/test_inspector.py
+++ b/ironic/tests/unit/drivers/modules/test_inspector.py
@@ -107,6 +107,12 @@ class CommonFunctionsTestCase(BaseTestCase):
         self.assertEqual('mdns', inspector._get_callback_endpoint(client))
         self.assertFalse(client.get_endpoint.called)
 
+    def test_get_callback_endpoint_no_loopback(self):
+        client = mock.Mock()
+        client.get_endpoint.return_value = 'http://127.0.0.1:5050'
+        self.assertRaisesRegex(exception.InvalidParameterValue, 'Loopback',
+                               inspector._get_callback_endpoint, client)
+
 
 @mock.patch.object(eventlet, 'spawn_n', lambda f, *a, **kw: f(*a, **kw))
 @mock.patch('ironic.drivers.modules.inspector._get_client', autospec=True)
diff --git a/releasenotes/notes/no-localhost-596adedfe388dfae.yaml b/releasenotes/notes/no-localhost-596adedfe388dfae.yaml
new file mode 100644
index 0000000000..5e56962811
--- /dev/null
+++ b/releasenotes/notes/no-localhost-596adedfe388dfae.yaml
@@ -0,0 +1,7 @@
+---
+upgrade:
+  - |
+    Using localhost as the Bare Metal Introspection endpoint will not work
+    when managed boot is used for in-band inspection. Either update the
+    endpoint to a real IP address or use the new configuration option
+    ``[inspector]callback_endpoint_override``.