From dece1e86641605a3d2de0653a611ffe1508d4a24 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 10:15:34 +0100 Subject: [PATCH 1/6] Make sure we don't log API key inside st2api log file if API key is provided using "?st2-api-key" query parameter. --- st2common/st2common/middleware/logging.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/middleware/logging.py b/st2common/st2common/middleware/logging.py index e4d30a1e7d..2a51a3edf9 100644 --- a/st2common/st2common/middleware/logging.py +++ b/st2common/st2common/middleware/logging.py @@ -19,11 +19,19 @@ import itertools from st2common.constants.api import REQUEST_ID_HEADER +from st2common.constants.auth import QUERY_PARAM_ATTRIBUTE_NAME +from st2common.constants.auth import QUERY_PARAM_API_KEY_ATTRIBUTE_NAME +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE from st2common import log as logging from st2common.router import Request, NotFoundException LOG = logging.getLogger(__name__) +SECRET_QUERY_PARAMS = [ + QUERY_PARAM_ATTRIBUTE_NAME, + QUERY_PARAM_API_KEY_ATTRIBUTE_NAME +] + try: clock = time.perf_counter except AttributeError: @@ -46,12 +54,19 @@ def __call__(self, environ, start_response): request = Request(environ) + query_params = request.GET.dict_of_lists() + + # Mask secret / sensitive query params + for param_name in SECRET_QUERY_PARAMS: + if param_name in query_params: + query_params[param_name] = MASKED_ATTRIBUTE_VALUE + # Log the incoming request values = { 'method': request.method, 'path': request.path, 'remote_addr': request.remote_addr, - 'query': request.GET.dict_of_lists(), + 'query': query_params, 'request_id': request.headers.get(REQUEST_ID_HEADER, None) } From 2dd806d38ffa19d087fe1e83f175bcda018ec948 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 10:28:09 +0100 Subject: [PATCH 2/6] Also mask query parameters which are defined in MASKED_ATTRIBUTES_BLACKLIST constant and log.mask_secrets_blacklist config option. --- st2common/st2common/middleware/logging.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/st2common/st2common/middleware/logging.py b/st2common/st2common/middleware/logging.py index 2a51a3edf9..9eeaa4cfeb 100644 --- a/st2common/st2common/middleware/logging.py +++ b/st2common/st2common/middleware/logging.py @@ -14,14 +14,18 @@ # limitations under the License. from __future__ import absolute_import + import time import types import itertools +from oslo_config import cfg + from st2common.constants.api import REQUEST_ID_HEADER from st2common.constants.auth import QUERY_PARAM_ATTRIBUTE_NAME from st2common.constants.auth import QUERY_PARAM_API_KEY_ATTRIBUTE_NAME from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE +from st2common.constants.secrets import MASKED_ATTRIBUTES_BLACKLIST from st2common import log as logging from st2common.router import Request, NotFoundException @@ -30,7 +34,7 @@ SECRET_QUERY_PARAMS = [ QUERY_PARAM_ATTRIBUTE_NAME, QUERY_PARAM_API_KEY_ATTRIBUTE_NAME -] +] + MASKED_ATTRIBUTES_BLACKLIST try: clock = time.perf_counter @@ -57,7 +61,8 @@ def __call__(self, environ, start_response): query_params = request.GET.dict_of_lists() # Mask secret / sensitive query params - for param_name in SECRET_QUERY_PARAMS: + secret_query_params = SECRET_QUERY_PARAMS + cfg.CONF.log.mask_secrets_blacklist + for param_name in secret_query_params: if param_name in query_params: query_params[param_name] = MASKED_ATTRIBUTE_VALUE From 69ad33e03263a7bcb4323460302e2716c34891e3 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 12:30:25 +0100 Subject: [PATCH 3/6] Add a test case for masking secret values in API log messages. --- .../tests/unit/test_logging_middleware.py | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 st2common/tests/unit/test_logging_middleware.py diff --git a/st2common/tests/unit/test_logging_middleware.py b/st2common/tests/unit/test_logging_middleware.py new file mode 100644 index 0000000000..885fea95b3 --- /dev/null +++ b/st2common/tests/unit/test_logging_middleware.py @@ -0,0 +1,65 @@ +# Licensed to the StackStorm, Inc ('StackStorm') under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import mock +import unittest2 + +from st2common.middleware.logging import LoggingMiddleware +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE + +__all__ = [ + 'LoggingMiddlewareTestCase' +] + + +class LoggingMiddlewareTestCase(unittest2.TestCase): + @mock.patch('st2common.middleware.logging.LOG') + @mock.patch('st2common.middleware.logging.Request') + def test_secret_parameters_are_masked_in_log_message(self, mock_request, mock_log): + + def app(environ, custom_start_response): + custom_start_response(status='200 OK', headers=[('Content-Length', 100)]) + return [None] + + router = mock.Mock() + endpoint = mock.Mock() + router.match.return_value = (endpoint, None) + middleware = LoggingMiddleware(app=app, router=router) + + environ = {} + mock_request.return_value.GET.dict_of_lists.return_value = { + 'foo': 'bar', + 'bar': 'baz', + 'x-auth-token': 'secret', + 'st2-api-key': 'secret', + 'password': 'secret', + 'st2_auth_token': 'secret', + 'token': 'secret' + } + middleware(environ=environ, start_response=mock.Mock()) + + expected_query = { + 'foo': 'bar', + 'bar': 'baz', + 'x-auth-token': MASKED_ATTRIBUTE_VALUE, + 'st2-api-key': MASKED_ATTRIBUTE_VALUE, + 'password': MASKED_ATTRIBUTE_VALUE, + 'token': MASKED_ATTRIBUTE_VALUE, + 'st2_auth_token': MASKED_ATTRIBUTE_VALUE + } + + call_kwargs = mock_log.info.call_args_list[0][1] + query = call_kwargs['extra']['query'] + self.assertEqual(query, expected_query) From 1dc54dd963b2647fede0b728105e3c3dd4238a93 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 12:31:12 +0100 Subject: [PATCH 4/6] Add changelog entry and fix formatting for existing changelog entries (use consistent 100 characters column length, remove extra whitespace). --- CHANGELOG.rst | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 499a76d654..d558cb121d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -30,25 +30,27 @@ Changed to make it easier for developers to understand. (improvement) * Update Python runner code so it prioritizes libraries from pack virtual environment over StackStorm system dependencies. - + For example, if pack depends on ``six==1.11.0`` in pack ``requirements.txt``, but StackStorm depends on ``six==1.10.0``, ``six==1.11.0`` will be used when running Python actions from that pack. - + Keep in mind that will not work correctly if pack depends on a library which brakes functionality used by Python action wrapper code. - + Contributed by Hiroyasu OHYAMA (@userlocalhost). #4571 Fixed ~~~~~ -* Refactored orquesta execution graph to fix performance issue for workflows with many - references to non-join tasks. st2workflowengine and DB models are refactored accordingly. - (improvement) StackStorm/orquesta#122. -* Fix orquesta workflow stuck in running status when one or more items failed execution for a - with items task. (bug fix) #4523 -* Fix orquesta workflow bug where context variables are being overwritten on task join. - (bug fix) StackStorm/orquesta#112 +* Refactored orquesta execution graph to fix performance issue for workflows with many references + to non-join tasks. st2workflowengine and DB models are refactored accordingly. (improvement) + StackStorm/orquesta#122. +* Fix orquesta workflow stuck in running status when one or more items failed execution for a with + items task. (bug fix) #4523 +* Fix orquesta workflow bug where context variables are being overwritten on task join. (bug fix) + StackStorm/orquesta#112 +* Make sure we don't log auth token and api key inside st2api log file if those values are provided + via query parameter and not header (``?x-auth-token=foo``, ``?st2-api-key=bar``). (bug fix) #4592 2.10.3 - March 06, 2019 ----------------------- @@ -60,7 +62,7 @@ Fixed with ``null`` for the ``Access-Control-Allow-Origin`` header. The fix returns the first of our allowed origins if the requesting origin is not a supported origin. Reported by Barak Tawily. (bug fix) - + 2.9.3 - March 06, 2019 ----------------------- @@ -134,7 +136,7 @@ Fixed Reported by @johandahlberg (bug fix) #4533 * Fix ``core.sendmail`` action so it specifies ``charset=UTF-8`` in the ``Content-Type`` email header. This way it works correctly when an email subject and / or body contains unicode data. - + Reported by @johandahlberg (bug fix) #4533 4534 * Fix CLI ``st2 apikey load`` not being idempotent and API endpoint ``/api/v1/apikeys`` not From 32751b2dddde7c6c4ca677231075b3d8b4aeeaa1 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 14:08:24 +0100 Subject: [PATCH 5/6] Update changelog. --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d558cb121d..e3282ad2ba 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -51,6 +51,7 @@ Fixed StackStorm/orquesta#112 * Make sure we don't log auth token and api key inside st2api log file if those values are provided via query parameter and not header (``?x-auth-token=foo``, ``?st2-api-key=bar``). (bug fix) #4592 + #4589 2.10.3 - March 06, 2019 ----------------------- From b20104275cbe49059d53692095656c30a3fe4128 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 13 Mar 2019 14:09:58 +0100 Subject: [PATCH 6/6] Update a test case to verify that custom attributes which are defined in the config are also masked. --- st2common/tests/unit/test_logging_middleware.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/st2common/tests/unit/test_logging_middleware.py b/st2common/tests/unit/test_logging_middleware.py index 885fea95b3..014634e414 100644 --- a/st2common/tests/unit/test_logging_middleware.py +++ b/st2common/tests/unit/test_logging_middleware.py @@ -16,6 +16,8 @@ import mock import unittest2 +from oslo_config import cfg + from st2common.middleware.logging import LoggingMiddleware from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE @@ -38,6 +40,9 @@ def app(environ, custom_start_response): router.match.return_value = (endpoint, None) middleware = LoggingMiddleware(app=app, router=router) + cfg.CONF.set_override(group='log', name='mask_secrets_blacklist', + override=['blacklisted_4', 'blacklisted_5']) + environ = {} mock_request.return_value.GET.dict_of_lists.return_value = { 'foo': 'bar', @@ -46,7 +51,9 @@ def app(environ, custom_start_response): 'st2-api-key': 'secret', 'password': 'secret', 'st2_auth_token': 'secret', - 'token': 'secret' + 'token': 'secret', + 'blacklisted_4': 'super secret', + 'blacklisted_5': 'super secret', } middleware(environ=environ, start_response=mock.Mock()) @@ -57,7 +64,9 @@ def app(environ, custom_start_response): 'st2-api-key': MASKED_ATTRIBUTE_VALUE, 'password': MASKED_ATTRIBUTE_VALUE, 'token': MASKED_ATTRIBUTE_VALUE, - 'st2_auth_token': MASKED_ATTRIBUTE_VALUE + 'st2_auth_token': MASKED_ATTRIBUTE_VALUE, + 'blacklisted_4': MASKED_ATTRIBUTE_VALUE, + 'blacklisted_5': MASKED_ATTRIBUTE_VALUE, } call_kwargs = mock_log.info.call_args_list[0][1]