Mask instance secrets in API responses

This change adds a new policy setting, "show_instance_secrets", whose
behavior mirrors that of the existing "show_passwords" policy setting.

Whereas "show_passwords" has historically blocked all sensitive
information from the node's driver_info field, the new setting blocks
all sensitive information from the node's instance_info field, including
image_url.

The name of the old setting, "show_passwords", is not being changed at
this time because such a change is not backwards-compatible. Instead,
the documentation string for this setting has been changed to clarify
what it does. Note that the behavior has not actually changed.

Note that this change moves the policy.check("show_password") call from
the Pecan hook into the API's Nodes() class, where the
policy.check("show_instance_secrets") is also added. This makes the code
a little cleaner and more maintainable, especially if we want to add any
more checks like this in the future.

As a result of this cleanup, the ironic-specific
RequestContext.show_password property is removed.

Partial-bug: #1530972
Partial-bug: #1526752
Related-bug: #1613903

Change-Id: I48493c53971cdab3b9122897e51322e19ce2f600

etc/ironic/policy.json.sample

@@ -2,8 +2,10 @@
 "admin_api": "role:admin or role:administrator"
 # Internal flag for public API routes
 "public_api": "is_public_api:True"
-# Show or mask passwords in API responses
+# Show or mask secrets within driver_info in API responses
 "show_password": "!"
+# Show or mask secrets within instance_info in API responses
+"show_instance_secrets": "!"
 # May be used to restrict access to specific tenants
 "is_member": "tenant:demo or tenant:baremetal"
 # Read-only API access
@@ -27,7 +29,7 @@
 # Set maintenance flag, taking a Node out of service
 "baremetal:node:set_maintenance": "rule:is_admin"
 # Clear maintenance flag, placing the Node into service again
-"baremetal:node:clear_maintenance": "role:is_admin"
+"baremetal:node:clear_maintenance": "rule:is_admin"
 # Change Node boot device
 "baremetal:node:set_boot_device": "rule:is_admin"
 # Change Node power status

ironic/api/controllers/v1/node.py

@@ -775,8 +775,7 @@ class Node(base.APIBase):
         setattr(self, 'chassis_uuid', kwargs.get('chassis_id', wtypes.Unset))
 
     @staticmethod
-    def _convert_with_links(node, url, fields=None, show_password=True,
+    def _convert_with_links(node, url, fields=None, show_states_links=True):
-                            show_states_links=True):
         # NOTE(lucasagomes): Since we are able to return a specified set of
         # fields the "uuid" can be unset, so we need to save it in another
         # variable to use when building the links
@@ -797,10 +796,6 @@ class Node(base.APIBase):
                                                    node_uuid + "/states",
                                                    bookmark=True)]
 
-        if not show_password and node.driver_info != wtypes.Unset:
-            node.driver_info = strutils.mask_dict_password(node.driver_info,
-                                                           "******")
-
         # NOTE(lucasagomes): The numeric ID should not be exposed to
         #                    the user, it's internal only.
         node.chassis_id = wtypes.Unset
@@ -819,14 +814,35 @@ class Node(base.APIBase):
         if fields is not None:
             api_utils.check_for_invalid_fields(fields, node.as_dict())
 
+        cdict = pecan.request.context.to_dict()
+        # NOTE(deva): the 'show_password' policy setting name exists for legacy
+        #             purposes and can not be changed. Changing it will cause
+        #             upgrade problems for any operators who have customized
+        #             the value of this field
+        show_driver_secrets = policy.check("show_password", cdict, cdict)
+        show_instance_secrets = policy.check("show_instance_secrets",
+                                             cdict, cdict)
+
+        if not show_driver_secrets and node.driver_info != wtypes.Unset:
+            node.driver_info = strutils.mask_dict_password(
+                node.driver_info, "******")
+        if not show_instance_secrets and node.instance_info != wtypes.Unset:
+            node.instance_info = strutils.mask_dict_password(
+                node.instance_info, "******")
+            # NOTE(deva): agent driver may store a swift temp_url on the
+            # instance_info, which shouldn't be exposed to non-admin users.
+            # Now that ironic supports additional policies, we need to hide
+            # it here, based on this policy.
+            # Related to bug #1613903
+            if node.instance_info.get('image_url'):
+                node.instance_info['image_url'] = "******"
+
         update_state_in_older_versions(node)
         hide_fields_in_newer_versions(node)
-        show_password = pecan.request.context.show_password
         show_states_links = (
             api_utils.allow_links_node_states_and_driver_properties())
         return cls._convert_with_links(node, pecan.request.public_url,
                                        fields=fields,
-                                       show_password=show_password,
                                        show_states_links=show_states_links)
 
     @classmethod

ironic/api/hooks.py

@@ -80,13 +80,9 @@ class ContextHook(hooks.PecanHook):
             'is_public_api': is_public_api,
         }
 
-        # TODO(deva): refactor this so enforce is called directly at relevant
-        #             places in code, not globally and for every request
-        show_password = policy.check('show_password', creds, creds)
         is_admin = policy.check('is_admin', creds, creds)
 
         state.request.context = context.RequestContext(
-            show_password=show_password,
             is_admin=is_admin,
             **creds)
 

ironic/common/context.py

@@ -21,7 +21,7 @@ class RequestContext(context.RequestContext):
     def __init__(self, auth_token=None, domain_id=None, domain_name=None,
                  user=None, tenant=None, is_admin=False, is_public_api=False,
                  read_only=False, show_deleted=False, request_id=None,
-                 roles=None, show_password=True, overwrite=True):
+                 roles=None, overwrite=True):
         """Initialize the RequestContext
 
         :param auth_token: The authentication token of the current request.
@@ -37,8 +37,6 @@ class RequestContext(context.RequestContext):
         :param show_deleted: unused flag for Ironic.
         :param request_id: The UUID of the request.
         :param roles: List of user's roles if any.
-        :param show_password: Specifies whether passwords should be masked
-                              before sending back to API call.
         :param overwrite: Set to False to ensure that the greenthread local
                              copy of the index is not overwritten.
         """
@@ -52,7 +50,6 @@ class RequestContext(context.RequestContext):
         self.is_public_api = is_public_api
         self.domain_id = domain_id
         self.domain_name = domain_name
-        self.show_password = show_password
         # NOTE(dims): roles was added in context.RequestContext recently.
         # we should pass roles in __init__ above instead of setting the
         # value here once the minimum version of oslo.context is updated.
@@ -69,7 +66,6 @@ class RequestContext(context.RequestContext):
                 'domain_id': self.domain_id,
                 'roles': self.roles,
                 'domain_name': self.domain_name,
-                'show_password': self.show_password,
                 'is_public_api': self.is_public_api}
 
     @classmethod

ironic/common/policy.py

@@ -38,10 +38,19 @@ default_policies = [
     policy.RuleDefault('public_api',
                        'is_public_api:True',
                        description='Internal flag for public API routes'),
-    # Generic default to hide passwords
+    # Generic default to hide passwords in node driver_info
+    # NOTE(deva): the 'show_password' policy setting hides secrets in
+    #             driver_info. However, the name exists for legacy
+    #             purposes and can not be changed. Changing it will cause
+    #             upgrade problems for any operators who have customized
+    #             the value of this field
     policy.RuleDefault('show_password',
                        '!',
-                       description='Show or mask passwords in API responses'),
+                       description='Show or mask secrets within node driver information in API responses'),  # noqa
+    # Generic default to hide instance secrets
+    policy.RuleDefault('show_instance_secrets',
+                       '!',
+                       description='Show or mask secrets within instance information in API responses'),  # noqa
     # Roles likely to be overriden by operator
     policy.RuleDefault('is_member',
                        'tenant:demo or tenant:baremetal',

ironic/drivers/modules/agent_base_vendor.py

@@ -30,6 +30,7 @@ from ironic.api.controllers.v1 import ramdisk
 from ironic.common import boot_devices
 from ironic.common import exception
 from ironic.common.i18n import _, _LE, _LI, _LW
+from ironic.common import policy
 from ironic.common import states
 from ironic.common import utils
 from ironic.conductor import rpcapi
@@ -801,7 +802,9 @@ class BaseAgentVendor(AgentDeployMixin, base.VendorInterface):
                      'and waiting for commands'), node.uuid)
 
         ndict = node.as_dict()
-        if not context.show_password:
+        cdict = context.to_dict()
+        show_driver_secrets = policy.check('show_password', cdict, cdict)
+        if not show_driver_secrets:
             ndict['driver_info'] = strutils.mask_dict_password(
                 ndict['driver_info'], "******")
 

ironic/tests/unit/api/test_hooks.py

@@ -55,11 +55,10 @@ class FakeRequestState(object):
         is_admin = ('admin' in creds['roles'] or
                     'administrator' in creds['roles'])
         is_public_api = self.request.environ.get('is_public_api', False)
-        show_password = ('admin' in creds['tenant'])
 
         self.request.context = context.RequestContext(
             is_admin=is_admin, is_public_api=is_public_api,
-            show_password=show_password, **creds)
+            **creds)
 
 
 def fake_headers(admin=False):
@@ -227,7 +226,6 @@ class TestContextHook(base.BaseApiTest):
             domain_id=headers['X-User-Domain-Id'],
             domain_name=headers['X-User-Domain-Name'],
             is_public_api=False,
-            show_password=False,
             is_admin=False,
             roles=headers['X-Roles'].split(','))
 
@@ -245,7 +243,6 @@ class TestContextHook(base.BaseApiTest):
             domain_id=headers['X-User-Domain-Id'],
             domain_name=headers['X-User-Domain-Name'],
             is_public_api=False,
-            show_password=True,
             is_admin=True,
             roles=headers['X-Roles'].split(','))
 
@@ -264,7 +261,6 @@ class TestContextHook(base.BaseApiTest):
             domain_id=headers['X-User-Domain-Id'],
             domain_name=headers['X-User-Domain-Name'],
             is_public_api=True,
-            show_password=True,
             is_admin=True,
             roles=headers['X-Roles'].split(','))
 
@@ -282,7 +278,6 @@ class TestContextHook(base.BaseApiTest):
             domain_id=headers['X-User-Domain-Id'],
             domain_name=headers['X-User-Domain-Name'],
             is_public_api=False,
-            show_password=False,
             is_admin=False,
             roles=headers['X-Roles'].split(','))
 

ironic/tests/unit/api/v1/test_nodes.py

@@ -128,6 +128,10 @@ class TestListNodes(test_api_base.BaseApiTest):
         self.assertEqual('******', data['driver_info']['fake_password'])
         self.assertEqual('bar', data['driver_info']['foo'])
         self.assertIn('driver_internal_info', data)
+        self.assertIn('instance_info', data)
+        self.assertEqual('******', data['instance_info']['configdrive'])
+        self.assertEqual('******', data['instance_info']['image_url'])
+        self.assertEqual('bar', data['instance_info']['foo'])
         self.assertIn('extra', data)
         self.assertIn('properties', data)
         self.assertIn('chassis_uuid', data)

ironic/tests/unit/common/test_context.py

@@ -31,7 +31,6 @@ class RequestContextTestCase(tests_base.TestCase):
         self.assertFalse(test_context.is_public_api)
         self.assertIsNone(test_context.domain_id)
         self.assertIsNone(test_context.domain_name)
-        self.assertTrue(test_context.show_password)
         self.assertEqual([], test_context.roles)
 
     def test_from_dict(self):
@@ -41,7 +40,6 @@ class RequestContextTestCase(tests_base.TestCase):
             "is_public_api": True,
             "domain_id": "domain_id1",
             "domain_name": "domain_name1",
-            "show_password": False,
             "roles": None
         }
         ctx = context.RequestContext.from_dict(dict)
@@ -50,7 +48,6 @@ class RequestContextTestCase(tests_base.TestCase):
         self.assertTrue(ctx.is_public_api)
         self.assertEqual("domain_id1", ctx.domain_id)
         self.assertEqual("domain_name1", ctx.domain_name)
-        self.assertFalse(ctx.show_password)
         self.assertEqual([], ctx.roles)
 
     def test_to_dict(self):
@@ -65,7 +62,6 @@ class RequestContextTestCase(tests_base.TestCase):
             "is_public_api": True,
             "domain_id": "domain_id1",
             "domain_name": "domain_name1",
-            "show_password": False,
             "roles": None,
             "overwrite": True
         }
@@ -81,7 +77,6 @@ class RequestContextTestCase(tests_base.TestCase):
         self.assertIn('domain_id', ctx_dict)
         self.assertIn('roles', ctx_dict)
         self.assertIn('domain_name', ctx_dict)
-        self.assertIn('show_password', ctx_dict)
         self.assertIn('is_public_api', ctx_dict)
         self.assertNotIn('overwrite', ctx_dict)
 
@@ -95,7 +90,6 @@ class RequestContextTestCase(tests_base.TestCase):
         self.assertTrue(ctx_dict['is_public_api'])
         self.assertEqual('domain_id1', ctx_dict['domain_id'])
         self.assertEqual('domain_name1', ctx_dict['domain_name'])
-        self.assertFalse(ctx_dict['show_password'])
         self.assertEqual([], ctx_dict['roles'])
 
     def test_get_admin_context(self):

ironic/tests/unit/db/utils.py

@@ -193,7 +193,21 @@ def get_test_node(**kw):
         "local_gb": "10",
         "memory_mb": "4096",
     }
-    fake_info = {"foo": "bar", "fake_password": "fakepass"}
+    # NOTE(deva): API unit tests confirm that sensitive fields in instance_info
+    #             and driver_info will get scrubbed from the API response
+    #             but other fields (eg, 'foo') do not.
+    fake_instance_info = {
+        "configdrive": "TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQ=",
+        "image_url": "http://example.com/test_image_url",
+        "foo": "bar",
+    }
+    fake_driver_info = {
+        "foo": "bar",
+        "fake_password": "fakepass",
+    }
+    fake_internal_info = {
+        "private_state": "secret value"
+    }
     return {
         'id': kw.get('id', 123),
         'name': kw.get('name', None),
@@ -208,10 +222,11 @@ def get_test_node(**kw):
         'provision_updated_at': kw.get('provision_updated_at'),
         'last_error': kw.get('last_error'),
         'instance_uuid': kw.get('instance_uuid'),
-        'instance_info': kw.get('instance_info', fake_info),
+        'instance_info': kw.get('instance_info', fake_instance_info),
         'driver': kw.get('driver', 'fake'),
-        'driver_info': kw.get('driver_info', fake_info),
+        'driver_info': kw.get('driver_info', fake_driver_info),
-        'driver_internal_info': kw.get('driver_internal_info', fake_info),
+        'driver_internal_info': kw.get('driver_internal_info',
+                                       fake_internal_info),
         'clean_step': kw.get('clean_step'),
         'properties': kw.get('properties', properties),
         'reservation': kw.get('reservation'),

ironic/tests/unit/drivers/modules/test_agent.py

@@ -84,7 +84,7 @@ class TestAgentMethods(db_base.DbTestCase):
     @mock.patch.object(image_service, 'GlanceImageService', autospec=True)
     def test_build_instance_info_for_deploy_glance_partition_image(
             self, glance_mock, parse_instance_info_mock):
-        i_info = self.node.instance_info
+        i_info = {}
         i_info['image_source'] = '733d1c44-a2ea-414b-aca7-69decf20d810'
         i_info['kernel'] = '13ce5a56-1de3-4916-b8b2-be778645d003'
         i_info['ramdisk'] = 'a5a370a8-1b39-433f-be63-2c7d708e4b4e'
@@ -120,10 +120,8 @@ class TestAgentMethods(db_base.DbTestCase):
                            'ramdisk': 'ramdisk',
                            'image_type': 'partition',
                            'image_checksum': 'aa',
-                           'fake_password': 'fakepass',
                            'image_container_format': 'bare',
-                           'image_disk_format': 'qcow2',
+                           'image_disk_format': 'qcow2'}
-                           'foo': 'bar'}
         mgr_utils.mock_the_extension_manager(driver='fake_agent')
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
@@ -174,13 +172,14 @@ class TestAgentMethods(db_base.DbTestCase):
                        autospec=True)
     def test_build_instance_info_for_deploy_nonglance_partition_image(
             self, validate_href_mock, parse_instance_info_mock):
-        i_info = self.node.instance_info
+        i_info = {}
         driver_internal_info = self.node.driver_internal_info
         i_info['image_source'] = 'http://image-ref'
         i_info['kernel'] = 'http://kernel-ref'
         i_info['ramdisk'] = 'http://ramdisk-ref'
         i_info['image_checksum'] = 'aa'
         i_info['root_gb'] = 10
+        i_info['configdrive'] = 'configdrive'
         driver_internal_info['is_whole_disk_image'] = False
         self.node.instance_info = i_info
         self.node.driver_internal_info = driver_internal_info
@@ -199,8 +198,7 @@ class TestAgentMethods(db_base.DbTestCase):
                            'image_checksum': 'aa',
                            'root_gb': 10,
                            'swap_mb': 5,
-                           'fake_password': 'fakepass',
+                           'configdrive': 'configdrive'}
-                           'foo': 'bar'}
         with task_manager.acquire(
                 self.context, self.node.uuid, shared=False) as task:
 

ironic/tests/unit/drivers/modules/test_agent_base_vendor.py

@@ -94,10 +94,10 @@ class TestBaseAgentVendor(db_base.DbTestCase):
                               task.context,
                               **kwargs)
 
+    @mock.patch('ironic.common.policy.check', autospec=True)
     @mock.patch('ironic.drivers.modules.agent_base_vendor.BaseAgentVendor'
                 '._find_node_by_macs', autospec=True)
-    def _test_lookup_v2(self, find_mock, show_password=True):
+    def _test_lookup_v2(self, find_mock, check_mock, show_password=True):
-        self.context.show_password = show_password
         kwargs = {
             'version': '2',
             'inventory': {
@@ -116,7 +116,10 @@ class TestBaseAgentVendor(db_base.DbTestCase):
         }
         # NOTE(jroll) apparently as_dict() returns a dict full of references
         expected = copy.deepcopy(self.node.as_dict())
-        if not show_password:
+        if show_password:
+            check_mock.return_value = True
+        else:
+            check_mock.return_value = False
             expected['driver_info']['ipmi_password'] = '******'
 
         self.config(agent_backend='statsd', group='metrics')
@@ -171,8 +174,8 @@ class TestBaseAgentVendor(db_base.DbTestCase):
 
     @mock.patch.object(objects.Node, 'get_by_uuid')
     def test_lookup_v2_with_node_uuid(self, mock_get_node):
-        self.context.show_password = True
         expected = copy.deepcopy(self.node.as_dict())
+        expected['driver_info']['ipmi_password'] = '******'
         kwargs = {
             'version': '2',
             'node_uuid': 'fake-uuid',

ironic/tests/unit/objects/test_node.py

@@ -79,7 +79,7 @@ class TestNodeObject(base.DbTestCase):
                                    autospec=True) as mock_update_node:
 
                 n = objects.Node.get(self.context, uuid)
-                self.assertEqual({"foo": "bar", "fake_password": "fakepass"},
+                self.assertEqual({"private_state": "secret value"},
                                  n.driver_internal_info)
                 n.properties = {"fake": "property"}
                 n.driver = "fake-driver"

releasenotes/notes/mask-configdrive-contents-77fc557d6bc63b2b.yaml

@@ -0,0 +1,19 @@
+---
+features:
+  - This change adds a new policy rule that may be used to mask
+    instance-specific secrets, such as configdrive contents or the temp URL
+    used to store a configdrive or instance image.  This is similar to how
+    passwords are already masked.
+upgrade:
+  - After this change, instance secrets will, by default, be masked in API
+    responses.  Operators wishing to expose the configdrive or instance image
+    to specific users will need to update their policy.json file and grant the
+    relevant keystone roles.
+security:
+  - Configdrives often contain sensitive information. Users may upload their
+    own images, which could also contain sensitive information.  The Agent
+    drivers may store this information in a Swift temp URL to allow access from
+    the Agent ramdisk. These URLs are considered sensitive information because
+    they grant unauthenticated access to sensitive information.  With this
+    change, we being to only selectively expose this information to privileged
+    users, whereas previously it was exposed to all authenticated users.