openstack/oslo.messaging
Code Issues Proposed changes

Only allow safe context fields in notifications

Browse Source 
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
This commit is contained in:
Jay Faulkner 2023-08-10 11:28:32 -07:00
parent ec1c99d562
commit 1b315615e7
3 changed files with 223 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
oslo_messaging/notify/notifier.py
View File

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

34
oslo_messaging/tests/notify/test_listener.py
View File

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

166
oslo_messaging/tests/notify/test_notifier.py
View File

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