From 426a306fb580762e97ada04e1253dedd9b64d410 Mon Sep 17 00:00:00 2001 From: Devananda van der Veen Date: Fri, 10 Jun 2016 17:24:53 -0700 Subject: [PATCH] Mask password on agent lookup according to policy We currently mask passwords in driver_info for all API responses, except for agent lookups. This is because driver_vendor_passthru just expects a dict to return as JSON in the response, and doesn't modify it at all. Change lookup to follow the defined policy when returning the node object in the response, the same way we do for other API responses. Co-authored-by: Jim Rollenhagen Change-Id: Ib19d2f86d3527333e905fdbf1f09fc3dc8b8c5a7 Closes-Bug: #1572796 --- ironic/drivers/modules/agent_base_vendor.py | 8 ++++++- ironic/tests/unit/db/utils.py | 1 + .../drivers/modules/test_agent_base_vendor.py | 24 +++++++++++++++---- .../fix-cve-2016-4985-b62abae577025365.yaml | 11 +++++++++ 4 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/fix-cve-2016-4985-b62abae577025365.yaml diff --git a/ironic/drivers/modules/agent_base_vendor.py b/ironic/drivers/modules/agent_base_vendor.py index 56a40ce126..caf66720e8 100644 --- a/ironic/drivers/modules/agent_base_vendor.py +++ b/ironic/drivers/modules/agent_base_vendor.py @@ -16,6 +16,7 @@ # License for the specific language governing permissions and limitations # under the License. +import ast import collections import time @@ -582,9 +583,14 @@ class BaseAgentVendor(base.VendorInterface): LOG.info(_LI('Initial lookup for node %s succeeded, agent is running ' 'and waiting for commands'), node.uuid) + ndict = node.as_dict() + if not context.show_password: + ndict['driver_info'] = ast.literal_eval( + strutils.mask_password(ndict['driver_info'], "******")) + return { 'heartbeat_timeout': CONF.agent.heartbeat_timeout, - 'node': node.as_dict() + 'node': ndict, } def _get_completed_cleaning_command(self, task, commands): diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index e336967034..0513def1c3 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -152,6 +152,7 @@ def get_test_agent_driver_info(): return { 'deploy_kernel': 'glance://deploy_kernel_uuid', 'deploy_ramdisk': 'glance://deploy_ramdisk_uuid', + 'ipmi_password': 'foo', } 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 4aee02f4dc..a1a18743dc 100644 --- a/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py +++ b/ironic/tests/unit/drivers/modules/test_agent_base_vendor.py @@ -15,6 +15,7 @@ # License for the specific language governing permissions and limitations # under the License. +import copy import time import types @@ -91,7 +92,8 @@ class TestBaseAgentVendor(db_base.DbTestCase): @mock.patch('ironic.drivers.modules.agent_base_vendor.BaseAgentVendor' '._find_node_by_macs', autospec=True) - def test_lookup_v2(self, find_mock): + def _test_lookup_v2(self, find_mock, show_password=True): + self.context.show_password = show_password kwargs = { 'version': '2', 'inventory': { @@ -108,10 +110,20 @@ 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: + expected['driver_info']['ipmi_password'] = '******' find_mock.return_value = self.node with task_manager.acquire(self.context, self.node.uuid) as task: node = self.passthru.lookup(task.context, **kwargs) - self.assertEqual(self.node.as_dict(), node['node']) + self.assertEqual(expected, node['node']) + + def test_lookup_v2_show_password(self): + self._test_lookup_v2(show_password=True) + + def test_lookup_v2_hide_password(self): + self._test_lookup_v2(show_password=False) def test_lookup_v2_missing_inventory(self): with task_manager.acquire(self.context, self.node.uuid) as task: @@ -136,9 +148,11 @@ 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()) kwargs = { 'version': '2', - 'node_uuid': 'fake uuid', + 'node_uuid': 'fake-uuid', 'inventory': { 'interfaces': [ { @@ -156,8 +170,8 @@ class TestBaseAgentVendor(db_base.DbTestCase): mock_get_node.return_value = self.node with task_manager.acquire(self.context, self.node.uuid) as task: node = self.passthru.lookup(task.context, **kwargs) - self.assertEqual(self.node.as_dict(), node['node']) - mock_get_node.assert_called_once_with(mock.ANY, 'fake uuid') + self.assertEqual(expected, node['node']) + mock_get_node.assert_called_once_with(mock.ANY, 'fake-uuid') @mock.patch.object(objects.port.Port, 'get_by_address', spec_set=types.FunctionType) diff --git a/releasenotes/notes/fix-cve-2016-4985-b62abae577025365.yaml b/releasenotes/notes/fix-cve-2016-4985-b62abae577025365.yaml new file mode 100644 index 0000000000..6127f61e1a --- /dev/null +++ b/releasenotes/notes/fix-cve-2016-4985-b62abae577025365.yaml @@ -0,0 +1,11 @@ +--- +security: + - A critical security vulnerability (CVE-2016-4985) was fixed in this + release. Previously, a client with network access to the ironic-api service + was able to bypass Keystone authentication and retrieve all information + about any Node registered with Ironic, if they knew (or were able to guess) + the MAC address of a network card belonging to that Node, by sending a + crafted POST request to the /v1/drivers/$DRIVER_NAME/vendor_passthru + resource. Ironic's policy.json configuration is now respected when + responding to this request such that, if passwords should be masked for + other requests, they are also masked for this request.