Make sure we don't log sensitive query params in the st2api log file

CHANGELOG.rst

@@ -30,25 +30,28 @@ 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
+  #4589
 
 2.10.3 - March 06, 2019
 -----------------------
@@ -60,7 +63,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 +137,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

st2common/st2common/middleware/logging.py

@@ -14,16 +14,28 @@
 # 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
 
 LOG = logging.getLogger(__name__)
 
+SECRET_QUERY_PARAMS = [
+    QUERY_PARAM_ATTRIBUTE_NAME,
+    QUERY_PARAM_API_KEY_ATTRIBUTE_NAME
+] + MASKED_ATTRIBUTES_BLACKLIST
+
 try:
     clock = time.perf_counter
 except AttributeError:
@@ -46,12 +58,20 @@ def __call__(self, environ, start_response):
 
         request = Request(environ)
 
+        query_params = request.GET.dict_of_lists()
+
+        # Mask secret / sensitive 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
+
         # 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)
         }
 

st2common/tests/unit/test_logging_middleware.py

@@ -0,0 +1,74 @@
+# 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 oslo_config import cfg
+
+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)
+
+        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',
+            'bar': 'baz',
+            'x-auth-token': 'secret',
+            'st2-api-key': 'secret',
+            'password': 'secret',
+            'st2_auth_token': 'secret',
+            'token': 'secret',
+            'blacklisted_4': 'super secret',
+            'blacklisted_5': 'super 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,
+            'blacklisted_4': MASKED_ATTRIBUTE_VALUE,
+            'blacklisted_5': MASKED_ATTRIBUTE_VALUE,
+        }
+
+        call_kwargs = mock_log.info.call_args_list[0][1]
+        query = call_kwargs['extra']['query']
+        self.assertEqual(query, expected_query)