diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 022e762a9c..f29da0e3f1 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -42,6 +42,18 @@ Changed Fixed ~~~~~ +* Fixed a bug where secrets in pack configs weren't being masked. Recently we + introduced support for nested objects and arrays. Secret parameters within these + nested objects and arrays were not being masked. The fix involves us fully + traversing deeply nested objects and arrays and masking out any variables + marked as secret. This means we now support pack config JSON schemas with + ``type: object`` and its corresponding ``parameters: {}`` stanza, along with + ``type: array`` and its corresponding ``items: {}`` stanza. We still do NOT + support JSON schema combinations that includes the ``anyOf``, ``allOf``, + ``oneOf``, and ``not`` keywords. (bug fix) #4139 + + Contributed by Nick Maludy (Encore Technologies). + 2.7.2 - May 16, 2018 -------------------- diff --git a/st2client/tests/unit/test_inquiry.py b/st2client/tests/unit/test_inquiry.py index 9d28687600..09e3c09c3c 100644 --- a/st2client/tests/unit/test_inquiry.py +++ b/st2client/tests/unit/test_inquiry.py @@ -32,6 +32,11 @@ def _randomize_inquiry_id(inquiry): newinquiry = copy.deepcopy(inquiry) newinquiry['id'] = str(uuid.uuid4()) + # ID can't have '1440' in it, otherwise our `count()` fails + # when inspecting the inquiry list output for test: + # test_list_inquiries_limit() + while '1440' in newinquiry['id']: + newinquiry['id'] = str(uuid.uuid4()) return newinquiry diff --git a/st2common/st2common/models/db/execution.py b/st2common/st2common/models/db/execution.py index f8e55b88db..63b043f14f 100644 --- a/st2common/st2common/models/db/execution.py +++ b/st2common/st2common/models/db/execution.py @@ -120,9 +120,14 @@ def mask_secrets(self, value): # schema. # # To prevent leakage, we can just mask all response fields. + # + # Note: The 'string' type in secret_parameters doesn't matter, + # it's just a placeholder to tell mask_secret_parameters() + # that this parameter is indeed a secret parameter and to + # mask it. result['parameters']['response'] = mask_secret_parameters( parameters=liveaction['parameters']['response'], - secret_parameters=[p for p in liveaction['parameters']['response']] + secret_parameters={p: 'string' for p in liveaction['parameters']['response']} ) # TODO(mierdin): This logic should be moved to the dedicated Inquiry diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index aec6db8c48..af12aa92b6 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -27,36 +27,146 @@ def get_secret_parameters(parameters): """ - Filter the provided parameters dict and return a list of parameter names which are marked as - secret. + Filter the provided parameters dict and return a dict of parameters which are marked as + secret. Every key in the dict is the parameter name and values are the parameter type: + + >>> d = get_secret_parameters(params) + >>> d + { + "param_a": "string", + "param_b": "boolean", + "param_c": "integer" + } + + If a paramter is a dictionary or a list, then the value will be a nested dictionary + containing information about that sub-object: + + >>> d = get_secret_parameters(params) + >>> d + { + "param_dict": { + "nested_a": "boolean", + "nested_b": "string", + }, + "param_list": { + "nested_dict: { + "param_c": "integer" + } + } + } + + Note: in JSON Schema, we're assuming lists contain the same data type for every element + :param parameters: Dictionary with runner or action parameters schema specification. :type parameters: ``dict`` :rtype ``list`` """ - secret_parameters = [parameter for parameter, options in - six.iteritems(parameters) if options.get('secret', False)] + + secret_parameters = {} + parameters_type = parameters.get('type') + iterator = None + if parameters_type == 'object': + # if this is an object, then iterate over the properties within + # the object + # result = dict + iterator = six.iteritems(parameters.get('properties', {})) + elif parameters_type == 'array': + # if this is an array, then iterate over the items definition as a single + # property + # result = list + iterator = enumerate([parameters.get('items', {})]) + secret_parameters = [] + elif parameters_type in ['integer', 'number', 'boolean', 'null', 'string']: + # if this a "plain old datatype", then iterate over the properties set + # of the data type + # result = string (property type) + iterator = enumerate([parameters]) + else: + # otherwise, assume we're in an object's properties definition + # this is the default case for the "root" level for schema specs. + # result = dict + iterator = six.iteritems(parameters) + + # iterate over all of the parameters recursively + for parameter, options in iterator: + if not isinstance(options, dict): + continue + + parameter_type = options.get('type') + if parameter_type in ['object', 'array']: + sub_params = get_secret_parameters(options) + if sub_params: + if isinstance(secret_parameters, list): + secret_parameters.append(sub_params) + elif isinstance(secret_parameters, dict): + secret_parameters[parameter] = sub_params + else: + return sub_params + elif options.get('secret', False): + # if this parameter is secret, then add it our secret parameters + if isinstance(secret_parameters, list): + secret_parameters.append(parameter_type) + elif isinstance(secret_parameters, dict): + secret_parameters[parameter] = parameter_type + else: + return parameter_type return secret_parameters -def mask_secret_parameters(parameters, secret_parameters): +def mask_secret_parameters(parameters, secret_parameters, result=None): """ Introspect the parameters dict and return a new dict with masked secret parameters. - :param parameters: Parameters to process. - :type parameters: ``dict`` - - :param secret_parameters: List of parameter names which are secret. - :type secret_parameters: ``list`` + :type parameters: ``dict`` or ``list`` or ``string`` + + :param secret_parameters: Dict of parameter names which are secret. + The type must be the same type as ``parameters`` + (or at least behave in the same way), + so that they can be traversed in the same way as + recurse down into the structure. + :type secret_parameters: ``dict`` + + :param result: Deep copy of parameters so that parameters is not modified + in place. Default = None, meaning this function will make a + deep copy before starting. + :type result: ``dict`` or ``list`` or ``string`` """ - result = copy.deepcopy(parameters) - - for parameter in secret_parameters: - if parameter in result: - result[parameter] = MASKED_ATTRIBUTE_VALUE + # how we iterate depends on what data type was passed in + iterator = None + is_dict = isinstance(secret_parameters, dict) + is_list = isinstance(secret_parameters, list) + if is_dict: + iterator = six.iteritems(secret_parameters) + elif is_list: + iterator = enumerate(secret_parameters) + else: + return MASKED_ATTRIBUTE_VALUE + + # only create a deep copy of parameters on the first call + # all other recursive calls pass back referneces to this result object + # so we can reuse it, saving memory and CPU cycles + if result is None: + result = copy.deepcopy(parameters) + + # iterate over the secret parameters + for secret_param, secret_sub_params in iterator: + if is_dict: + if secret_param in result: + result[secret_param] = mask_secret_parameters(parameters[secret_param], + secret_sub_params, + result=result[secret_param]) + elif is_list: + # we're assuming lists contain the same data type for every element + for idx, value in enumerate(result): + result[idx] = mask_secret_parameters(parameters[idx], + secret_sub_params, + result=result[idx]) + else: + result[secret_param] = MASKED_ATTRIBUTE_VALUE return result diff --git a/st2common/tests/unit/test_util_secrets.py b/st2common/tests/unit/test_util_secrets.py new file mode 100644 index 0000000000..848b372574 --- /dev/null +++ b/st2common/tests/unit/test_util_secrets.py @@ -0,0 +1,671 @@ +# 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. + +from __future__ import absolute_import +import unittest2 + +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE +from st2common.util import secrets + +################################################################################ + +TEST_FLAT_SCHEMA = { + 'arg_required_no_default': { + 'description': 'Foo', + 'required': True, + 'type': 'string', + 'secret': False + }, + 'arg_optional_no_type_secret': { + 'description': 'Bar', + 'secret': True + }, + 'arg_optional_type_array': { + 'description': 'Who''s the fairest?', + 'type': 'array' + }, + 'arg_optional_type_object': { + 'description': 'Who''s the fairest of them?', + 'type': 'object' + }, +} + +TEST_FLAT_SECRET_PARAMS = { + 'arg_optional_no_type_secret': None +} + +################################################################################ + +TEST_NO_SECRETS_SCHEMA = { + 'arg_required_no_default': { + 'description': 'Foo', + 'required': True, + 'type': 'string', + 'secret': False + } +} + +TEST_NO_SECRETS_SECRET_PARAMS = {} + +################################################################################ + +TEST_NESTED_OBJECTS_SCHEMA = { + 'arg_string': { + 'description': 'Junk', + 'type': 'string', + }, + 'arg_optional_object': { + 'description': 'Mirror', + 'type': 'object', + 'properties': { + 'arg_nested_object': { + 'description': 'Mirror mirror', + 'type': 'object', + 'properties': { + 'arg_double_nested_secret': { + 'description': 'Deep, deep down', + 'type': 'string', + 'secret': True + } + } + }, + 'arg_nested_secret': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } +} + +TEST_NESTED_OBJECTS_SECRET_PARAMS = { + 'arg_optional_object': { + 'arg_nested_secret': 'string', + 'arg_nested_object': { + 'arg_double_nested_secret': 'string', + } + } +} + +################################################################################ + +TEST_ARRAY_SCHEMA = { + 'arg_optional_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'down', + 'type': 'string', + 'secret': True + } + } +} + +TEST_ARRAY_SECRET_PARAMS = { + 'arg_optional_array': [ + 'string' + ] +} + + +################################################################################ + +TEST_ROOT_ARRAY_SCHEMA = { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'down', + 'type': 'object', + 'properties': { + 'secret_field_in_object': { + 'type': 'string', + 'secret': True + } + } + } +} + +TEST_ROOT_ARRAY_SECRET_PARAMS = [ + { + 'secret_field_in_object': 'string' + } +] + +################################################################################ + +TEST_NESTED_ARRAYS_SCHEMA = { + 'arg_optional_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + }, + 'arg_optional_double_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + }, + 'arg_optional_tripple_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } + }, + 'arg_optional_quad_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } + } + } +} + +TEST_NESTED_ARRAYS_SECRET_PARAMS = { + 'arg_optional_array': [ + 'string' + ], + 'arg_optional_double_array': [ + [ + 'string' + ] + ], + 'arg_optional_tripple_array': [ + [ + [ + 'string' + ] + ] + ], + 'arg_optional_quad_array': [ + [ + [ + [ + 'string' + ] + ] + ] + ] +} + +################################################################################ + +TEST_NESTED_OBJECT_WITH_ARRAY_SCHEMA = { + 'arg_optional_object_with_array': { + 'description': 'Mirror', + 'type': 'object', + 'properties': { + 'arg_nested_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } + } +} + +TEST_NESTED_OBJECT_WITH_ARRAY_SECRET_PARAMS = { + 'arg_optional_object_with_array': { + 'arg_nested_array': [ + 'string' + ] + } +} + +################################################################################ + +TEST_NESTED_OBJECT_WITH_DOUBLE_ARRAY_SCHEMA = { + 'arg_optional_object_with_double_array': { + 'description': 'Mirror', + 'type': 'object', + 'properties': { + 'arg_double_nested_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } + } + } +} + +TEST_NESTED_OBJECT_WITH_DOUBLE_ARRAY_SECRET_PARAMS = { + 'arg_optional_object_with_double_array': { + 'arg_double_nested_array': [ + [ + 'string' + ] + ] + } +} + +################################################################################ + +TEST_NESTED_ARRAY_WITH_OBJECT_SCHEMA = { + 'arg_optional_array_with_object': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'Mirror', + 'type': 'object', + 'properties': { + 'arg_nested_secret': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } + } +} + +TEST_NESTED_ARRAY_WITH_OBJECT_SECRET_PARAMS = { + 'arg_optional_array_with_object': [ + { + 'arg_nested_secret': 'string' + } + ] +} + + +################################################################################ + + +class SecretUtilsTestCase(unittest2.TestCase): + + def test_get_secret_parameters_flat(self): + result = secrets.get_secret_parameters(TEST_FLAT_SCHEMA) + self.assertEqual(TEST_FLAT_SECRET_PARAMS, result) + + def test_get_secret_parameters_no_secrets(self): + result = secrets.get_secret_parameters(TEST_NO_SECRETS_SCHEMA) + self.assertEqual(TEST_NO_SECRETS_SECRET_PARAMS, result) + + def test_get_secret_parameters_nested_objects(self): + result = secrets.get_secret_parameters(TEST_NESTED_OBJECTS_SCHEMA) + self.assertEqual(TEST_NESTED_OBJECTS_SECRET_PARAMS, result) + + def test_get_secret_parameters_array(self): + result = secrets.get_secret_parameters(TEST_ARRAY_SCHEMA) + self.assertEqual(TEST_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_root_array(self): + result = secrets.get_secret_parameters(TEST_ROOT_ARRAY_SCHEMA) + self.assertEqual(TEST_ROOT_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_nested_arrays(self): + result = secrets.get_secret_parameters(TEST_NESTED_ARRAYS_SCHEMA) + self.assertEqual(TEST_NESTED_ARRAYS_SECRET_PARAMS, result) + + def test_get_secret_parameters_nested_object_with_array(self): + result = secrets.get_secret_parameters(TEST_NESTED_OBJECT_WITH_ARRAY_SCHEMA) + self.assertEqual(TEST_NESTED_OBJECT_WITH_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_nested_object_with_double_array(self): + result = secrets.get_secret_parameters(TEST_NESTED_OBJECT_WITH_DOUBLE_ARRAY_SCHEMA) + self.assertEqual(TEST_NESTED_OBJECT_WITH_DOUBLE_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_nested_array_with_object(self): + result = secrets.get_secret_parameters(TEST_NESTED_ARRAY_WITH_OBJECT_SCHEMA) + self.assertEqual(TEST_NESTED_ARRAY_WITH_OBJECT_SECRET_PARAMS, result) + + ############################################################################ + # TODO unit tests for mask_secret_parameters + + def test_mask_secret_parameters_flat(self): + parameters = { + 'arg_required_no_default': 'test', + 'arg_optional_no_type_secret': None + } + result = secrets.mask_secret_parameters(parameters, + TEST_FLAT_SECRET_PARAMS) + expected = { + 'arg_required_no_default': 'test', + 'arg_optional_no_type_secret': MASKED_ATTRIBUTE_VALUE + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_no_secrets(self): + parameters = {'arg_required_no_default': 'junk'} + result = secrets.mask_secret_parameters(parameters, + TEST_NO_SECRETS_SECRET_PARAMS) + expected = { + 'arg_required_no_default': 'junk' + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_nested_objects(self): + parameters = { + 'arg_optional_object': { + 'arg_nested_secret': 'nested Secret', + 'arg_nested_object': { + 'arg_double_nested_secret': 'double nested $ecret', + } + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_NESTED_OBJECTS_SECRET_PARAMS) + expected = { + 'arg_optional_object': { + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE, + 'arg_nested_object': { + 'arg_double_nested_secret': MASKED_ATTRIBUTE_VALUE, + } + } + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_array(self): + parameters = { + 'arg_optional_array': [ + '$ecret $tring 1', + '$ecret $tring 2', + '$ecret $tring 3' + ] + } + result = secrets.mask_secret_parameters(parameters, + TEST_ARRAY_SECRET_PARAMS) + expected = { + 'arg_optional_array': [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ] + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_root_array(self): + parameters = [ + { + 'secret_field_in_object': 'Secret $tr!ng' + }, + { + 'secret_field_in_object': 'Secret $tr!ng 2' + }, + { + 'secret_field_in_object': 'Secret $tr!ng 3' + }, + { + 'secret_field_in_object': 'Secret $tr!ng 4' + } + ] + + result = secrets.mask_secret_parameters(parameters, TEST_ROOT_ARRAY_SECRET_PARAMS) + expected = [ + { + 'secret_field_in_object': MASKED_ATTRIBUTE_VALUE + }, + { + 'secret_field_in_object': MASKED_ATTRIBUTE_VALUE + }, + { + 'secret_field_in_object': MASKED_ATTRIBUTE_VALUE + }, + { + 'secret_field_in_object': MASKED_ATTRIBUTE_VALUE + } + ] + self.assertEqual(expected, result) + + def test_mask_secret_parameters_nested_arrays(self): + parameters = { + 'arg_optional_array': [ + 'secret 1', + 'secret 2', + 'secret 3', + ], + 'arg_optional_double_array': [ + [ + 'secret 4', + 'secret 5', + 'secret 6', + ], + [ + 'secret 7', + 'secret 8', + 'secret 9', + ] + ], + 'arg_optional_tripple_array': [ + [ + [ + 'secret 10', + 'secret 11' + ], + [ + 'secret 12', + 'secret 13', + 'secret 14' + ] + ], + [ + [ + 'secret 15', + 'secret 16' + ] + ] + ], + 'arg_optional_quad_array': [ + [ + [ + [ + 'secret 17', + 'secret 18' + ], + [ + 'secret 19' + ] + ] + ] + ] + } + + result = secrets.mask_secret_parameters(parameters, + TEST_NESTED_ARRAYS_SECRET_PARAMS) + expected = { + 'arg_optional_array': [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], + 'arg_optional_double_array': [ + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ] + ], + 'arg_optional_tripple_array': [ + [ + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ], + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ] + ], + [ + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ] + ] + ], + 'arg_optional_quad_array': [ + [ + [ + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ], + [ + MASKED_ATTRIBUTE_VALUE + ] + ] + ] + ] + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_nested_object_with_array(self): + parameters = { + 'arg_optional_object_with_array': { + 'arg_nested_array': [ + 'secret array value 1', + 'secret array value 2', + 'secret array value 3', + ] + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_NESTED_OBJECT_WITH_ARRAY_SECRET_PARAMS) + expected = { + 'arg_optional_object_with_array': { + 'arg_nested_array': [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ] + } + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_nested_object_with_double_array(self): + parameters = { + 'arg_optional_object_with_double_array': { + 'arg_double_nested_array': [ + [ + 'secret 1', + 'secret 2', + 'secret 3' + ], + [ + 'secret 4', + 'secret 5', + 'secret 6', + ], + [ + 'secret 7', + 'secret 8', + 'secret 9', + 'secret 10', + ] + ] + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_NESTED_OBJECT_WITH_DOUBLE_ARRAY_SECRET_PARAMS) + expected = { + 'arg_optional_object_with_double_array': { + 'arg_double_nested_array': [ + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE + ], + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], + [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ] + ] + } + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_nested_array_with_object(self): + parameters = { + 'arg_optional_array_with_object': [ + { + 'arg_nested_secret': 'secret 1' + }, + { + 'arg_nested_secret': 'secret 2' + }, + { + 'arg_nested_secret': 'secret 3' + } + ] + } + result = secrets.mask_secret_parameters(parameters, + TEST_NESTED_ARRAY_WITH_OBJECT_SECRET_PARAMS) + expected = { + 'arg_optional_array_with_object': [ + { + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE + }, + { + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE + }, + { + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE + } + ] + } + self.assertEqual(expected, result)