security: validate molds url against swift in keystone catalog

Adds a security check to ensure the URL "netloc", i.e. hostname
and port, matches the user supplied URL if the configuration
molds feature for Dell hardware goes down the path of attempting
to get or set the URL. This works by sending the url to the
authorization code and handling checking the URL there as a
safety check prior to proceeding.

Closes-Bug: 2148317
Change-Id: I22a476dd3734ae4c1940c595f537be1c1a94acf5
Signed-off-by: Julia Kreger <juliaashleykreger@gmail.com>
(cherry picked from commit c1c9930fbe)

ironic/common/molds.py

@@ -13,6 +13,7 @@
 # under the License.
 
 import json
+from urllib.parse import urlparse
 
 from oslo_config import cfg
 from oslo_log import log as logging
@@ -22,6 +23,7 @@ import tenacity
 
 from ironic.common import exception
 from ironic.common.i18n import _
+from ironic.common import keystone
 from ironic.common import swift
 
 LOG = logging.getLogger(__name__)
@@ -55,7 +57,7 @@ def save_configuration(task, url, data):
             url, data=json.dumps(data, indent=2), headers=auth_header,
             timeout=CONF.webserver_connection_timeout)
 
-    auth_header = _get_auth_header(task)
+    auth_header = _get_auth_header(task, url)
     response = _request(url, data, auth_header)
     response.raise_for_status()
 
@@ -86,7 +88,7 @@ def get_configuration(task, url):
         return requests.get(url, headers=auth_header,
                             timeout=CONF.webserver_connection_timeout)
 
-    auth_header = _get_auth_header(task)
+    auth_header = _get_auth_header(task, url)
     response = _request(url, auth_header)
     if response.status_code == requests.codes.ok:
         if not response.content:
@@ -107,7 +109,7 @@ def get_configuration(task, url):
 # NOTE(TheJulia): Deprecated after the 2024.1 PTG in favor of
 # a future step templating interface.
 
-def _get_auth_header(task):
+def _get_auth_header(task, url):
     """Based on setup of configuration mold storage gets authentication header
 
     :param task: A TaskManager instance.
@@ -116,8 +118,21 @@ def _get_auth_header(task):
     """
     auth_header = None
     if CONF.molds.storage == 'swift':
+        swift_session = swift.get_swift_session()
+        # NOTE(TheJulia): First validate the URL.
+        if url:
+            endpoint = keystone.get_endpoint('swift',
+                                             session=swift_session)
+            if (not endpoint
+                    or urlparse(endpoint).netloc != urlparse(url).netloc):
+                # Raise an InvalidParameterValue should the value not
+                # match swift configuration.
+                raise exception.InvalidParameterValue(
+                    _('Supplied URL for a mold target does not match the '
+                      'swift configuration.'))
+
         # TODO(ajya) Need to update to use Swift client and context session
-        auth_token = swift.get_swift_session().get_token()
+        auth_token = swift_session.get_token()
         if auth_token:
             auth_header = {'X-Auth-Token': auth_token}
         else:

ironic/tests/unit/common/test_molds.py

@@ -19,6 +19,7 @@ from oslo_config import cfg
 import requests
 
 from ironic.common import exception
+from ironic.common import keystone
 from ironic.common import molds
 from ironic.common import swift
 from ironic.conductor import task_manager
@@ -32,15 +33,18 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
         super(ConfigurationMoldTestCase, self).setUp()
         self.node = obj_utils.create_test_node(self.context)
 
+    @mock.patch.object(keystone, 'get_endpoint', autospec=True)
     @mock.patch.object(swift, 'get_swift_session', autospec=True)
     @mock.patch.object(requests, 'put', autospec=True)
-    def test_save_configuration_swift(self, mock_put, mock_swift):
+    def test_save_configuration_swift(self, mock_put, mock_swift,
+                                      mock_endpoint):
         mock_session = mock.Mock()
         mock_session.get_token.return_value = 'token'
         mock_swift.return_value = mock_session
         cfg.CONF.set_override('storage', 'swift', 'molds')
         url = 'https://example.com/file1'
         data = {'key': 'value'}
+        mock_endpoint.return_value = 'https://example.com/v1'
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             molds.save_configuration(task, url, data)
@@ -49,15 +53,18 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
                                          headers={'X-Auth-Token': 'token'},
                                          timeout=60)
 
+    @mock.patch.object(keystone, 'get_endpoint', autospec=True)
     @mock.patch.object(swift, 'get_swift_session', autospec=True)
     @mock.patch.object(requests, 'put', autospec=True)
-    def test_save_configuration_swift_noauth(self, mock_put, mock_swift):
+    def test_save_configuration_swift_noauth(self, mock_put, mock_swift,
+                                             mock_endpoint):
         mock_session = mock.Mock()
         mock_session.get_token.return_value = None
         mock_swift.return_value = mock_session
         cfg.CONF.set_override('storage', 'swift', 'molds')
         url = 'https://example.com/file1'
         data = {'key': 'value'}
+        mock_endpoint.return_value = 'https://example.com/v1'
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertRaises(
@@ -164,9 +171,11 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
                 timeout=60)
             self.assertEqual(mock_put.call_count, 2)
 
+    @mock.patch.object(keystone, 'get_endpoint', autospec=True)
     @mock.patch.object(swift, 'get_swift_session', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
-    def test_get_configuration_swift(self, mock_get, mock_swift):
+    def test_get_configuration_swift(self, mock_get, mock_swift,
+                                     mock_endpoint):
         mock_session = mock.Mock()
         mock_session.get_token.return_value = 'token'
         mock_swift.return_value = mock_session
@@ -177,6 +186,7 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
         response.json.return_value = {'key': 'value'}
         mock_get.return_value = response
         url = 'https://example.com/file1'
+        mock_endpoint.return_value = 'https://example.com/v1'
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             result = molds.get_configuration(task, url)
@@ -185,15 +195,45 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
                 url, headers={'X-Auth-Token': 'token'},
                 timeout=60)
             self.assertJsonEqual({'key': 'value'}, result)
+        mock_endpoint.assert_called_once_with('swift', session=mock_session)
 
+    @mock.patch.object(keystone, 'get_endpoint', autospec=True)
     @mock.patch.object(swift, 'get_swift_session', autospec=True)
     @mock.patch.object(requests, 'get', autospec=True)
-    def test_get_configuration_swift_noauth(self, mock_get, mock_swift):
+    def test_get_configuration_swift_url_mismatch(
+            self, mock_get, mock_swift, mock_endpoint):
+        mock_session = mock.Mock()
+        mock_session.get_token.return_value = 'token'
+        mock_swift.return_value = mock_session
+        cfg.CONF.set_override('storage', 'swift', 'molds')
+        response = mock.MagicMock()
+        response.status_code = 200
+        response.content = "{'key': 'value'}"
+        response.json.return_value = {'key': 'value'}
+        mock_get.return_value = response
+        url = 'https://example.com/file1'
+        mock_endpoint.return_value = 'https://cloud.foo/v1'
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.assertRaises(exception.InvalidParameterValue,
+                              molds.get_configuration,
+                              task, url)
+
+        mock_endpoint.assert_called_once_with('swift', session=mock_session)
+        mock_get.assert_not_called()
+
+
+    @mock.patch.object(keystone, 'get_endpoint', autospec=True)
+    @mock.patch.object(swift, 'get_swift_session', autospec=True)
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_configuration_swift_noauth(self, mock_get, mock_swift,
+                                            mock_endpoint):
         mock_session = mock.Mock()
         mock_session.get_token.return_value = None
         mock_swift.return_value = mock_session
         cfg.CONF.set_override('storage', 'swift', 'molds')
         url = 'https://example.com/file1'
+        mock_endpoint.return_value = 'https://example.com/v1'
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             self.assertRaises(

releasenotes/notes/fix-bug-2148317-471adcfac69791dc.yaml

@@ -0,0 +1,19 @@
+---
+fixes:
+  - |
+    Fixes a security issue where the deprecated configuration molds feature
+    would allow an user invoking molds to request authorization
+    to be sent to a remote endpoint. This user supplied URL could be a
+    ``swift`` or ``http`` url. While when used with ``http``, the feature
+    was explicitly designed around a concept of just publishing to a file
+    in a limited context with authentication details provided by the
+    conductor, where as with ``swift`` the impact is greater because the
+    time limited session token for Ironic's access of swift resources could
+    be leaked, captured, and used.
+
+    The configuration molds feature now explicitly checks the swift endpoint
+    URL and raises an exception when the URL does not match the user supplied
+    the configured Swift endpoint.
+
+    More information can be found in
+    `bug 2148317 <https://bugs.launchpad.net/ironic/+bug/2148317>`_.