diff --git a/etc/ironic/policy.json.sample b/etc/ironic/policy.json.sample index 888c0f0c0c..31b923ce81 100644 --- a/etc/ironic/policy.json.sample +++ b/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 diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index f7f7fd35ea..aaffad4b25 100644 --- a/ironic/api/controllers/v1/node.py +++ b/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, - show_states_links=True): + def _convert_with_links(node, url, fields=None, 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 diff --git a/ironic/api/hooks.py b/ironic/api/hooks.py index f25a05be92..655773c65f 100644 --- a/ironic/api/hooks.py +++ b/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) diff --git a/ironic/common/context.py b/ironic/common/context.py index 131a942322..6e9c7e34b1 100644 --- a/ironic/common/context.py +++ b/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 diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 9237d267a5..2ff00fb786 100644 --- a/ironic/common/policy.py +++ b/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', diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index 0d8afd7b04..f2d7156825 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/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'], "******") diff --git a/ironic/tests/unit/api/test_hooks.py b/ironic/tests/unit/api/test_hooks.py index c1a9392156..88eaada516 100644 --- a/ironic/tests/unit/api/test_hooks.py +++ b/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(',')) diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index ac88537121..e69d4e151d 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/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) diff --git a/ironic/tests/unit/common/test_context.py b/ironic/tests/unit/common/test_context.py index a20e01ada3..cf9286b66e 100644 --- a/ironic/tests/unit/common/test_context.py +++ b/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): diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 2d8b4c9e87..6d2b300fdf 100644 --- a/ironic/tests/unit/db/utils.py +++ b/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_internal_info': kw.get('driver_internal_info', fake_info), + 'driver_info': kw.get('driver_info', fake_driver_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'), diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index 0e3dad2dbd..ca8cb474b7 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/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', - 'foo': 'bar'} + 'image_disk_format': 'qcow2'} 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', - 'foo': 'bar'} + 'configdrive': 'configdrive'} with task_manager.acquire( self.context, self.node.uuid, shared=False) as task: diff --git a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py index 8276de38bb..bbbb9dd5f0 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/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): - self.context.show_password = show_password + def _test_lookup_v2(self, find_mock, check_mock, show_password=True): 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', diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index f439d88601..5fcaeb9194 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/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" diff --git a/releasenotes/notes/mask-configdrive-contents-77fc557d6bc63b2b.yaml b/releasenotes/notes/mask-configdrive-contents-77fc557d6bc63b2b.yaml new file mode 100644 index 0000000000..beb7681612 --- /dev/null +++ b/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.