openstack/ironic
Code Issues Proposed changes

Merge "Mask instance secrets in API responses"

Browse Source
This commit is contained in:
Jenkins 2016-08-29 14:05:41 +00:00 committed by Gerrit Code Review
commit 1d73c10cad
14 changed files with 99 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
etc/ironic/policy.json.sample
View File

@ -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

32
ironic/api/controllers/v1/node.py
View File

@ -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

4
ironic/api/hooks.py
View File

@ -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)

6
ironic/common/context.py
View File

@ -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

13
ironic/common/policy.py
View File

@ -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',

5
ironic/drivers/modules/agent_base_vendor.py
View File

@ -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'], "******")

7
ironic/tests/unit/api/test_hooks.py
View File

@ -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(','))

4
ironic/tests/unit/api/v1/test_nodes.py
View File

@ -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)

6
ironic/tests/unit/common/test_context.py
View File

@ -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):

23
ironic/tests/unit/db/utils.py
View File

@ -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'),

12
ironic/tests/unit/drivers/modules/test_agent.py
View File

@ -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:

11
ironic/tests/unit/drivers/modules/test_agent_base_vendor.py
View File

@ -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',

2
ironic/tests/unit/objects/test_node.py
View File

@ -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"

19
releasenotes/notes/mask-configdrive-contents-77fc557d6bc63b2b.yaml Normal file
View File

@ -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.