From e07f04b4c33eea5830864a70b45b3ae6fb3e37fd Mon Sep 17 00:00:00 2001 From: potato Date: Thu, 2 Jul 2020 10:27:08 +0200 Subject: [PATCH 1/4] Remove authentication headers from webhook payloads Co-authored-by: Krisztian Nagy --- st2api/st2api/controllers/v1/webhooks.py | 6 ++++++ .../unit/controllers/v1/test_webhooks.py | 20 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/st2api/st2api/controllers/v1/webhooks.py b/st2api/st2api/controllers/v1/webhooks.py index 9add551ed9..b204424d1b 100644 --- a/st2api/st2api/controllers/v1/webhooks.py +++ b/st2api/st2api/controllers/v1/webhooks.py @@ -18,6 +18,7 @@ urljoin = urlparse.urljoin from st2common import log as logging +from st2common.constants.auth import HEADER_API_KEY_ATTRIBUTE_NAME, HEADER_ATTRIBUTE_NAME from st2common.constants.triggers import WEBHOOK_TRIGGER_TYPES from st2common.models.api.trace import TraceContext from st2common.models.api.trigger import TriggerAPI @@ -125,6 +126,7 @@ def post(self, hook, webhook_body_api, headers, requester_user): permission_type=permission_type) headers = self._get_headers_as_dict(headers) + headers = self._filter_authentication_headers(headers) # If webhook contains a trace-tag use that else create create a unique trace-tag. trace_context = self._create_trace_context(trace_tag=headers.pop(TRACE_TAG_HEADER, None), @@ -218,6 +220,10 @@ def _get_headers_as_dict(self, headers): headers_dict[key] = value return headers_dict + def _filter_authentication_headers(self, headers): + auth_headers = [HEADER_API_KEY_ATTRIBUTE_NAME, HEADER_ATTRIBUTE_NAME, 'Cookie'] + return {key: value for key, value in headers.items() if key not in auth_headers} + def _log_request(self, msg, headers, body, log_method=LOG.debug): headers = self._get_headers_as_dict(headers) body = str(body) diff --git a/st2api/tests/unit/controllers/v1/test_webhooks.py b/st2api/tests/unit/controllers/v1/test_webhooks.py index 4ab73feebc..71700cf7a6 100644 --- a/st2api/tests/unit/controllers/v1/test_webhooks.py +++ b/st2api/tests/unit/controllers/v1/test_webhooks.py @@ -319,6 +319,26 @@ def get_webhook_trigger(name, url): self.assertTrue(controller._is_valid_hook('with_leading_trailing_slash')) self.assertTrue(controller._is_valid_hook('with/mixed/slash')) + @mock.patch.object(TriggerInstancePublisher, 'publish_trigger', mock.MagicMock( + return_value=True)) + @mock.patch.object(WebhooksController, '_is_valid_hook', mock.MagicMock( + return_value=True)) + @mock.patch.object(HooksHolder, 'get_triggers_for_hook', mock.MagicMock( + return_value=[DUMMY_TRIGGER_DICT])) + @mock.patch('st2common.transport.reactor.TriggerDispatcher.dispatch') + def test_authentication_headers_should_be_removed(self, dispatch_mock): + headers = { + 'Content-Type': 'application/x-www-form-urlencoded', + 'St2-Api-_Key': 'foobar', + 'X-Auth-Token': 'deadbeaf', + 'Cookie': 'foo=bar' + } + + self.app.post('/v1/webhooks/git', WEBHOOK_1, headers=headers) + self.assertNotIn('St2-Api-Key', dispatch_mock.call_args[1]['payload']['headers']) + self.assertNotIn('X-Auth-Token', dispatch_mock.call_args[1]['payload']['headers']) + self.assertNotIn('Cookie', dispatch_mock.call_args[1]['payload']['headers']) + def __do_post(self, hook, webhook, expect_errors=False, headers=None): return self.app.post_json('/v1/webhooks/' + hook, params=webhook, From 8320dec1c3a098bd33d61817ddb5e2aa1d806400 Mon Sep 17 00:00:00 2001 From: Laszlo Hammerl Date: Thu, 2 Jul 2020 14:25:10 +0200 Subject: [PATCH 2/4] Update st2api/tests/unit/controllers/v1/test_webhooks.py Co-authored-by: Eugen C. --- st2api/tests/unit/controllers/v1/test_webhooks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/st2api/tests/unit/controllers/v1/test_webhooks.py b/st2api/tests/unit/controllers/v1/test_webhooks.py index 71700cf7a6..c6d2523603 100644 --- a/st2api/tests/unit/controllers/v1/test_webhooks.py +++ b/st2api/tests/unit/controllers/v1/test_webhooks.py @@ -329,7 +329,7 @@ def get_webhook_trigger(name, url): def test_authentication_headers_should_be_removed(self, dispatch_mock): headers = { 'Content-Type': 'application/x-www-form-urlencoded', - 'St2-Api-_Key': 'foobar', + 'St2-Api-Key': 'foobar', 'X-Auth-Token': 'deadbeaf', 'Cookie': 'foo=bar' } From a264010a1093e7d945869a0364a03d450eb5ade0 Mon Sep 17 00:00:00 2001 From: potato Date: Thu, 2 Jul 2020 14:35:06 +0200 Subject: [PATCH 3/4] Updated CHANGELOG.rst Co-authored-by: Krisztian Nagy --- CHANGELOG.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 474ce77cc8..d96fac536f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -33,6 +33,10 @@ Fixed up correctly, specifically when specifying a bastion host / jump box. (bug fix) #4973 Contributed by Nick Maludy (@nmaludy Encore Technologies) + +* Fixed a bug where authentication headers sent to a webhook trigger got stored in the database. + + Contributed by @potato and @knagy 3.2.0 - April 27, 2020 From 74cecf63cfdd5ae5b7e2ba92054d4c07f60a7921 Mon Sep 17 00:00:00 2001 From: Eugen C Date: Tue, 29 Sep 2020 20:15:08 +0100 Subject: [PATCH 4/4] Update Changelog for the #4983 fix --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3c045b24f1..43d317b942 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -49,7 +49,7 @@ Changed validation to use the new value when checking RBAC configuration. Removed other references to enterprise for RBAC related contents. (improvement) * Remove authentication headers ``St2-Api-Key``, ``X-Auth-Token`` and ``Cookie`` from webhook payloads to - prevent them from being stored in the database. (security bug fix) 4983 + prevent them from being stored in the database. (security bug fix) #4983 Contributed by @potato and @knagy