From 1b315615e7dc61dbf845bd663560fc8d5a18fa09 Mon Sep 17 00:00:00 2001 From: Jay Faulkner Date: Thu, 10 Aug 2023 11:28:32 -0700 Subject: [PATCH] Only allow safe context fields in notifications Publishing a fully hydrated context object in a notification would give someone with access to that notification the ability to impersonate the original actor through inclusion of sensitive fields. Now, instead, we pare down the context object to the bare minimum before passing it for serialization in notification workflows. Related-bug: 2030976 Change-Id: Ic94323658c89df1c1ff32f511ca23502317d0f00 --- oslo_messaging/notify/notifier.py | 48 +++++- oslo_messaging/tests/notify/test_listener.py | 34 ++-- oslo_messaging/tests/notify/test_notifier.py | 166 ++++++++++++++++++- 3 files changed, 223 insertions(+), 25 deletions(-) diff --git a/oslo_messaging/notify/notifier.py b/oslo_messaging/notify/notifier.py index 02ff643ed..5fa0ebeeb 100644 --- a/oslo_messaging/notify/notifier.py +++ b/oslo_messaging/notify/notifier.py @@ -171,6 +171,47 @@ def get_notification_transport(conf, url=None, allowed_remote_exmods=None): transport_cls=msg_transport.NotificationTransport) +def _sanitize_context(ctxt): + # NOTE(JayF): The below values are in the same order they are in + # oslo_context.context.RequestContext.__init__() + safe_keys = ( + 'user_id', + 'project_id', + 'domain_id', + 'user_domain_id', + 'project_domain_id', + 'request_id', + 'roles', + 'user_name', + 'project_name', + 'domain_name', + 'user_domain_name', + 'project_domain_name', + 'service_user_id', + 'service_user_domain_id', + 'service_user_domain_name', + 'service_project_id', + 'service_project_name', + 'service_project_domain_id', + 'service_project_domain_name', + 'service_roles', + 'global_request_id', + 'system_scope', + # NOTE(JayF) These have been renamed but may show up in notifications + 'user', + 'domain', + 'user_domain', + 'project_domain', + ) + ctxt_dict = ctxt if isinstance(ctxt, dict) else ctxt.to_dict() + safe_dict = {k: v for k, v in ctxt_dict.items() + if k in safe_keys} + if ctxt_dict is ctxt: + return safe_dict + else: + return ctxt.__class__.from_dict(safe_dict) + + class Notifier(object): """Send notification messages. @@ -296,7 +337,12 @@ class Notifier(object): def _notify(self, ctxt, event_type, payload, priority, publisher_id=None, retry=None): payload = self._serializer.serialize_entity(ctxt, payload) - ctxt = self._serializer.serialize_context(ctxt) + + # NOTE(JayF): We must remove secure information from notification + # payloads, otherwise we risk sending sensitive creds + # to a notification bus. + safe_ctxt = _sanitize_context(ctxt) + ctxt = self._serializer.serialize_context(safe_ctxt) msg = dict(message_id=str(uuid.uuid4()), publisher_id=publisher_id or self.publisher_id, diff --git a/oslo_messaging/tests/notify/test_listener.py b/oslo_messaging/tests/notify/test_listener.py index f0cabaec7..60bbf042d 100644 --- a/oslo_messaging/tests/notify/test_listener.py +++ b/oslo_messaging/tests/notify/test_listener.py @@ -286,18 +286,18 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): listener_thread = self._setup_listener(transport, [endpoint], targets=targets) notifier = self._setup_notifier(transport, topics=['topic1']) - notifier.info({'ctxt': '1'}, 'an_event.start1', 'test') + notifier.info({'user_name': 'bob'}, 'an_event.start1', 'test') notifier = self._setup_notifier(transport, topics=['topic2']) - notifier.info({'ctxt': '2'}, 'an_event.start2', 'test') + notifier.info({'user_name': 'bob2'}, 'an_event.start2', 'test') self.wait_for_messages(2) self.assertFalse(listener_thread.stop()) endpoint.info.assert_has_calls([ - mock.call({'ctxt': '1'}, 'testpublisher', + mock.call({'user_name': 'bob'}, 'testpublisher', 'an_event.start1', 'test', {'timestamp': mock.ANY, 'message_id': mock.ANY}), - mock.call({'ctxt': '2'}, 'testpublisher', + mock.call({'user_name': 'bob2'}, 'testpublisher', 'an_event.start2', 'test', {'timestamp': mock.ANY, 'message_id': mock.ANY})], any_order=True) @@ -326,23 +326,23 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): transport._send_notification = mock.MagicMock( side_effect=side_effect) - notifier.info({'ctxt': '0'}, + notifier.info({'user_name': 'bob0'}, 'an_event.start', 'test message default exchange') mock_notifier_exchange('exchange1') - notifier.info({'ctxt': '1'}, + notifier.info({'user_name': 'bob1'}, 'an_event.start', 'test message exchange1') mock_notifier_exchange('exchange2') - notifier.info({'ctxt': '2'}, + notifier.info({'user_name': 'bob2'}, 'an_event.start', 'test message exchange2') self.wait_for_messages(2) self.assertFalse(listener_thread.stop()) endpoint.info.assert_has_calls([ - mock.call({'ctxt': '1'}, 'testpublisher', 'an_event.start', + mock.call({'user_name': 'bob1'}, 'testpublisher', 'an_event.start', 'test message exchange1', {'timestamp': mock.ANY, 'message_id': mock.ANY}), - mock.call({'ctxt': '2'}, 'testpublisher', 'an_event.start', + mock.call({'user_name': 'bob2'}, 'testpublisher', 'an_event.start', 'test message exchange2', {'timestamp': mock.ANY, 'message_id': mock.ANY})], any_order=True) @@ -414,8 +414,8 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): targets=targets, pool="pool2") notifier = self._setup_notifier(transport, topics=["topic"]) - notifier.info({'ctxt': '0'}, 'an_event.start', 'test message0') - notifier.info({'ctxt': '1'}, 'an_event.start', 'test message1') + notifier.info({'user_name': 'bob0'}, 'an_event.start', 'test message0') + notifier.info({'user_name': 'bob1'}, 'an_event.start', 'test message1') self.wait_for_messages(2, "pool1") self.wait_for_messages(2, "pool2") @@ -423,7 +423,7 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): self.assertFalse(listener1_thread.stop()) def mocked_endpoint_call(i): - return mock.call({'ctxt': '%d' % i}, 'testpublisher', + return mock.call({'user_name': 'bob%d' % i}, 'testpublisher', 'an_event.start', 'test message%d' % i, {'timestamp': mock.ANY, 'message_id': mock.ANY}) @@ -452,14 +452,14 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): targets=targets, pool="pool2") def mocked_endpoint_call(i): - return mock.call({'ctxt': '%d' % i}, 'testpublisher', + return mock.call({'user_name': 'bob%d' % i}, 'testpublisher', 'an_event.start', 'test message%d' % i, {'timestamp': mock.ANY, 'message_id': mock.ANY}) notifier = self._setup_notifier(transport, topics=["topic"]) mocked_endpoint1_calls = [] for i in range(0, 25): - notifier.info({'ctxt': '%d' % i}, 'an_event.start', + notifier.info({'user_name': 'bob%d' % i}, 'an_event.start', 'test message%d' % i) mocked_endpoint1_calls.append(mocked_endpoint_call(i)) @@ -467,7 +467,7 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): listener2_thread.stop() for i in range(0, 25): - notifier.info({'ctxt': '%d' % i}, 'an_event.start', + notifier.info({'user_name': 'bob%d' % i}, 'an_event.start', 'test message%d' % i) mocked_endpoint1_calls.append(mocked_endpoint_call(i)) @@ -476,7 +476,7 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): listener3_thread.stop() for i in range(0, 25): - notifier.info({'ctxt': '%d' % i}, 'an_event.start', + notifier.info({'user_name': 'bob%d' % i}, 'an_event.start', 'test message%d' % i) mocked_endpoint1_calls.append(mocked_endpoint_call(i)) @@ -484,7 +484,7 @@ class TestNotifyListener(test_utils.BaseTestCase, ListenerSetupMixin): listener3_thread.start() for i in range(0, 25): - notifier.info({'ctxt': '%d' % i}, 'an_event.start', + notifier.info({'user_name': 'bob%d' % i}, 'an_event.start', 'test message%d' % i) mocked_endpoint1_calls.append(mocked_endpoint_call(i)) diff --git a/oslo_messaging/tests/notify/test_notifier.py b/oslo_messaging/tests/notify/test_notifier.py index 330bdaba5..8a918fdcf 100644 --- a/oslo_messaging/tests/notify/test_notifier.py +++ b/oslo_messaging/tests/notify/test_notifier.py @@ -122,7 +122,7 @@ class TestMessagingNotifier(test_utils.BaseTestCase): ] _context = [ - ('ctxt', dict(ctxt={'user': 'bob'})), + ('ctxt', dict(ctxt={'user_name': 'bob'})), ] _retry = [ @@ -229,6 +229,157 @@ class TestMessagingNotifier(test_utils.BaseTestCase): TestMessagingNotifier.generate_scenarios() +class TestMessagingNotifierContextFiltering(test_utils.BaseTestCase): + + _v1 = [ + ('v1', dict(v1=True)), + ('not_v1', dict(v1=False)), + ] + + _v2 = [ + ('v2', dict(v2=True)), + ('not_v2', dict(v2=False)), + ] + + _publisher_id = [ + ('ctor_pub_id', dict(ctor_pub_id='test', + expected_pub_id='test')), + ('prep_pub_id', dict(prep_pub_id='test.localhost', + expected_pub_id='test.localhost')), + ('override', dict(ctor_pub_id='test', + prep_pub_id='test.localhost', + expected_pub_id='test.localhost')), + ] + + _topics = [ + ('no_topics', dict(topics=[])), + ('single_topic', dict(topics=['notifications'])), + ('multiple_topic2', dict(topics=['foo', 'bar'])), + ] + + _priority = [ + ('audit', dict(priority='audit')), + ('debug', dict(priority='debug')), + ('info', dict(priority='info')), + ('warn', dict(priority='warn')), + ('error', dict(priority='error')), + ('sample', dict(priority='sample')), + ('critical', dict(priority='critical')), + ] + + _payload = [ + ('payload', dict(payload={'foo': 'bar'})), + ] + + _context = [ + ('ctxt', dict(ctxt={'user_name': 'bob'})), + ] + + _retry = [ + ('unconfigured', dict()), + ('None', dict(retry=None)), + ('0', dict(retry=0)), + ('5', dict(retry=5)), + ] + + @classmethod + def generate_scenarios(cls): + cls.scenarios = testscenarios.multiply_scenarios(cls._v1, + cls._v2, + cls._publisher_id, + cls._topics, + cls._priority, + cls._payload, + cls._retry) + + def setUp(self): + super(TestMessagingNotifierContextFiltering, self).setUp() + + self.logger = self.useFixture(_ReRaiseLoggedExceptionsFixture()).logger + self.useFixture(fixtures.MockPatchObject( + messaging, 'LOG', self.logger)) + self.useFixture(fixtures.MockPatchObject( + msg_notifier, '_LOG', self.logger)) + + @mock.patch('oslo_utils.timeutils.utcnow') + def test_notifier(self, mock_utcnow): + ctxt = {'user_name': 'bob', 'secret_data': 'redact_me'} + safe_ctxt = {'user_name': 'bob'} + drivers = [] + if self.v1: + drivers.append('messaging') + if self.v2: + drivers.append('messagingv2') + + self.config(driver=drivers, + topics=self.topics, + group='oslo_messaging_notifications') + + transport = oslo_messaging.get_notification_transport(self.conf, + url='fake:') + + if hasattr(self, 'ctor_pub_id'): + notifier = oslo_messaging.Notifier(transport, + publisher_id=self.ctor_pub_id) + else: + notifier = oslo_messaging.Notifier(transport) + + prepare_kwds = {} + if hasattr(self, 'retry'): + prepare_kwds['retry'] = self.retry + if hasattr(self, 'prep_pub_id'): + prepare_kwds['publisher_id'] = self.prep_pub_id + if prepare_kwds: + notifier = notifier.prepare(**prepare_kwds) + + transport._send_notification = mock.Mock() + + message_id = uuid.uuid4() + uuid.uuid4 = mock.Mock(return_value=message_id) + + mock_utcnow.return_value = datetime.datetime.utcnow() + + message = { + 'message_id': str(message_id), + 'publisher_id': self.expected_pub_id, + 'event_type': 'test.notify', + 'priority': self.priority.upper(), + 'payload': self.payload, + 'timestamp': str(timeutils.utcnow()), + } + + sends = [] + if self.v1: + sends.append(dict(version=1.0)) + if self.v2: + sends.append(dict(version=2.0)) + + calls = [] + for send_kwargs in sends: + for topic in self.topics: + if hasattr(self, 'retry'): + send_kwargs['retry'] = self.retry + else: + send_kwargs['retry'] = -1 + target = oslo_messaging.Target(topic='%s.%s' % (topic, + self.priority)) + calls.append(mock.call(target, + safe_ctxt, + message, + **send_kwargs)) + + method = getattr(notifier, self.priority) + method(ctxt, 'test.notify', self.payload) + + uuid.uuid4.assert_called_once_with() + transport._send_notification.assert_has_calls(calls, any_order=True) + + self.assertTrue(notifier.is_enabled()) + + +TestMessagingNotifierContextFiltering.generate_scenarios() + + class TestMessagingNotifierRetry(test_utils.BaseTestCase): class TestingException(BaseException): @@ -328,12 +479,12 @@ class TestSerializer(test_utils.BaseTestCase): mock_utcnow.return_value = datetime.datetime.utcnow() serializer.serialize_context = mock.Mock() - serializer.serialize_context.return_value = dict(user='alice') + serializer.serialize_context.return_value = dict(user_name='alice') serializer.serialize_entity = mock.Mock() serializer.serialize_entity.return_value = 'sbar' - notifier.info(dict(user='bob'), 'test.notify', 'bar') + notifier.info(dict(user_name='bob'), 'test.notify', 'bar') message = { 'message_id': str(message_id), @@ -344,13 +495,14 @@ class TestSerializer(test_utils.BaseTestCase): 'timestamp': str(timeutils.utcnow()), } - self.assertEqual([(dict(user='alice'), message, 'INFO', -1)], + self.assertEqual([(dict(user_name='alice'), message, 'INFO', -1)], _impl_test.NOTIFICATIONS) uuid.uuid4.assert_called_once_with() - serializer.serialize_context.assert_called_once_with(dict(user='bob')) - serializer.serialize_entity.assert_called_once_with(dict(user='bob'), - 'bar') + serializer.serialize_context.assert_called_once_with( + dict(user_name='bob')) + serializer.serialize_entity.assert_called_once_with( + dict(user_name='bob'), 'bar') class TestNotifierTopics(test_utils.BaseTestCase):