From efb321c71a709a6f5b33d9de62587117f0c29ba3 Mon Sep 17 00:00:00 2001 From: Zhenzan Zhou Date: Wed, 28 Jan 2015 13:10:02 +0800 Subject: [PATCH] Add policy show_password to mask passwords in driver_info Ironic API already enforces admin role to run node-show. So a new policy show_password is added to control if plain text passwords in driver_info should be masked or not before sending back to API calls. The default is masking password for all cases. Change-Id: Icd3e6be049376bf7b4468f0c149a72a06643da32 Closes-Bug: #1406191 --- etc/ironic/policy.json | 1 + ironic/api/controllers/v1/node.py | 11 ++++++-- ironic/api/hooks.py | 2 ++ ironic/common/context.py | 6 ++++- ironic/tests/api/test_hooks.py | 45 ++++++++++++++++++++++++++++++- ironic/tests/api/v1/test_nodes.py | 2 ++ ironic/tests/db/utils.py | 2 +- ironic/tests/fake_policy.py | 3 ++- ironic/tests/objects/test_node.py | 3 ++- ironic/tests/test_policy.py | 8 ++++++ 10 files changed, 76 insertions(+), 7 deletions(-) diff --git a/etc/ironic/policy.json b/etc/ironic/policy.json index 05b3994764..f7726778ed 100644 --- a/etc/ironic/policy.json +++ b/etc/ironic/policy.json @@ -1,4 +1,5 @@ { "admin_api": "role:admin or role:administrator", + "show_password": "!", "default": "rule:admin_api" } diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index ff0dd1aec9..29b771b667 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import ast import datetime from oslo.config import cfg @@ -34,6 +35,7 @@ from ironic.common import states as ir_states from ironic.common import utils from ironic import objects from ironic.openstack.common import log +from ironic.openstack.common import strutils CONF = cfg.CONF @@ -512,12 +514,16 @@ class Node(base.APIBase): setattr(self, 'chassis_uuid', kwargs.get('chassis_id', wtypes.Unset)) @staticmethod - def _convert_with_links(node, url, expand=True): + def _convert_with_links(node, url, expand=True, show_password=True): if not expand: except_list = ['instance_uuid', 'maintenance', 'power_state', 'provision_state', 'uuid'] node.unset_fields_except(except_list) else: + if not show_password: + node.driver_info = ast.literal_eval(strutils.mask_password( + node.driver_info, + "******")) node.ports = [link.Link.make_link('self', url, 'nodes', node.uuid + "/ports"), link.Link.make_link('bookmark', url, 'nodes', @@ -542,7 +548,8 @@ class Node(base.APIBase): assert_juno_provision_state_name(node) hide_driver_internal_info(node) return cls._convert_with_links(node, pecan.request.host_url, - expand) + expand, + pecan.request.context.show_password) @classmethod def sample(cls, expand=True): diff --git a/ironic/api/hooks.py b/ironic/api/hooks.py index 63e46fffaf..653c8ea2ff 100644 --- a/ironic/api/hooks.py +++ b/ironic/api/hooks.py @@ -82,10 +82,12 @@ class ContextHook(hooks.PecanHook): is_admin = (policy.enforce('admin_api', creds, creds) or policy.enforce('admin', creds, creds)) is_public_api = state.request.environ.get('is_public_api', False) + show_password = policy.enforce('show_password', creds, creds) state.request.context = context.RequestContext( is_admin=is_admin, is_public_api=is_public_api, + show_password=show_password, **creds) diff --git a/ironic/common/context.py b/ironic/common/context.py index 52885e6290..26ca85d567 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): + roles=None, show_password=True): """Stores several additional request parameters: :param domain_id: The ID of the domain. @@ -29,12 +29,15 @@ class RequestContext(context.RequestContext): :param is_public_api: Specifies whether the request should be processed without authentication. :param roles: List of user's roles if any. + :param show_password: Specifies whether passwords should be masked + before sending back to API call. """ self.is_public_api = is_public_api self.domain_id = domain_id self.domain_name = domain_name self.roles = roles or [] + self.show_password = show_password super(RequestContext, self).__init__(auth_token=auth_token, user=user, tenant=tenant, @@ -54,6 +57,7 @@ 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/tests/api/test_hooks.py b/ironic/tests/api/test_hooks.py index a4f73396c9..d7cd58df95 100644 --- a/ironic/tests/api/test_hooks.py +++ b/ironic/tests/api/test_hooks.py @@ -54,9 +54,11 @@ 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, **creds) + is_admin=is_admin, is_public_api=is_public_api, + show_password=show_password, **creds) def fake_headers(admin=False): @@ -85,6 +87,8 @@ def fake_headers(admin=False): 'X-Project-Name': 'admin', 'X-Role': '_member_,admin', 'X-Roles': '_member_,admin', + 'X-Tenant': 'admin', + 'X-Tenant-Name': 'admin', }) else: headers.update({ @@ -183,6 +187,7 @@ class TestContextHook(base.FunctionalTest): 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(',')) @@ -199,6 +204,7 @@ class TestContextHook(base.FunctionalTest): 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(',')) @@ -216,6 +222,7 @@ class TestContextHook(base.FunctionalTest): 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(',')) @@ -226,6 +233,42 @@ class TestContextHookCompatJuno(TestContextHook): self.policy = self.useFixture( policy_fixture.PolicyFixture(compat='juno')) + # override two cases because Juno has no "show_password" policy + @mock.patch.object(context, 'RequestContext') + def test_context_hook_admin(self, mock_ctx): + headers = fake_headers(admin=True) + reqstate = FakeRequestState(headers=headers) + context_hook = hooks.ContextHook(None) + context_hook.before(reqstate) + mock_ctx.assert_called_with( + auth_token=headers['X-Auth-Token'], + user=headers['X-User'], + tenant=headers['X-Tenant'], + domain_id=headers['X-User-Domain-Id'], + domain_name=headers['X-User-Domain-Name'], + is_public_api=False, + show_password=False, + is_admin=True, + roles=headers['X-Roles'].split(',')) + + @mock.patch.object(context, 'RequestContext') + def test_context_hook_public_api(self, mock_ctx): + headers = fake_headers(admin=True) + env = {'is_public_api': True} + reqstate = FakeRequestState(headers=headers, environ=env) + context_hook = hooks.ContextHook(None) + context_hook.before(reqstate) + mock_ctx.assert_called_with( + auth_token=headers['X-Auth-Token'], + user=headers['X-User'], + tenant=headers['X-Tenant'], + domain_id=headers['X-User-Domain-Id'], + domain_name=headers['X-User-Domain-Name'], + is_public_api=True, + show_password=False, + is_admin=True, + roles=headers['X-Roles'].split(',')) + class TestTrustedCallHook(base.FunctionalTest): def test_trusted_call_hook_not_admin(self): diff --git a/ironic/tests/api/v1/test_nodes.py b/ironic/tests/api/v1/test_nodes.py index e5d89e7e94..0aeb4b6cf5 100644 --- a/ironic/tests/api/v1/test_nodes.py +++ b/ironic/tests/api/v1/test_nodes.py @@ -123,6 +123,8 @@ class TestListNodes(test_api_base.FunctionalTest): self.assertEqual(node.uuid, data['uuid']) self.assertIn('driver', data) self.assertIn('driver_info', data) + self.assertEqual('******', data['driver_info']['fake_password']) + self.assertEqual('bar', data['driver_info']['foo']) self.assertIn('driver_internal_info', data) self.assertIn('extra', data) self.assertIn('properties', data) diff --git a/ironic/tests/db/utils.py b/ironic/tests/db/utils.py index 89a063dd5f..3326f0927a 100644 --- a/ironic/tests/db/utils.py +++ b/ironic/tests/db/utils.py @@ -167,7 +167,7 @@ def get_test_node(**kw): "local_gb": "10", "memory_mb": "4096", } - fake_info = {"foo": "bar"} + fake_info = {"foo": "bar", "fake_password": "fakepass"} return { 'id': kw.get('id', 123), 'uuid': kw.get('uuid', '1be26c0b-03f2-4d2e-ae87-c02d7f33c123'), diff --git a/ironic/tests/fake_policy.py b/ironic/tests/fake_policy.py index eb723ff710..66f600845f 100644 --- a/ironic/tests/fake_policy.py +++ b/ironic/tests/fake_policy.py @@ -18,7 +18,8 @@ policy_data = """ "admin_api": "role:admin or role:administrator", "public_api": "is_public_api:True", "trusted_call": "rule:admin_api or rule:public_api", - "default": "rule:trusted_call" + "default": "rule:trusted_call", + "show_password": "tenant:admin" } """ diff --git a/ironic/tests/objects/test_node.py b/ironic/tests/objects/test_node.py index c3ab6c4465..579d9ee929 100644 --- a/ironic/tests/objects/test_node.py +++ b/ironic/tests/objects/test_node.py @@ -63,7 +63,8 @@ class TestNodeObject(base.DbTestCase): autospec=True) as mock_update_node: n = objects.Node.get(self.context, uuid) - self.assertEqual({"foo": "bar"}, n.driver_internal_info) + self.assertEqual({"foo": "bar", "fake_password": "fakepass"}, + n.driver_internal_info) n.properties = {"fake": "property"} n.driver = "fake-driver" n.save() diff --git a/ironic/tests/test_policy.py b/ironic/tests/test_policy.py index 1f9955dc06..dd3a71ab68 100644 --- a/ironic/tests/test_policy.py +++ b/ironic/tests/test_policy.py @@ -43,6 +43,10 @@ class PolicyTestCase(base.TestCase): for c in creds: self.assertTrue(policy.enforce('trusted_call', c, c)) + def test_show_password(self): + creds = {'roles': [u'admin'], 'tenant': 'admin'} + self.assertTrue(policy.enforce('show_password', creds, creds)) + class PolicyTestCaseNegative(base.TestCase): """Tests whether the configuration of the policy engine is corect.""" @@ -64,3 +68,7 @@ class PolicyTestCaseNegative(base.TestCase): for c in creds: self.assertFalse(policy.enforce('trusted_call', c, c)) + + def test_show_password(self): + creds = {'roles': [u'admin'], 'tenant': 'demo'} + self.assertFalse(policy.enforce('show_password', creds, creds))