From ee7947ac70872abc462b71fe8c035c8acfd04fd9 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 22 May 2018 15:33:02 -0400 Subject: [PATCH 1/7] Initial work on a fix for #4139 - Secrets not being masked in pack configs --- st2common/st2common/util/secrets.py | 78 +++++++++++++++++++++++++---- 1 file changed, 67 insertions(+), 11 deletions(-) diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index aec6db8c48..e76d6e48ff 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -27,16 +27,65 @@ 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)] + + # determine if this parameters set is an object definition + # if it is, then drill in and grab the properties from the object itself + parameters_type = parameters.get('type') + if parameters_type == 'object': + parameters = parameters.get('properties', {}) + elif parameters_type == 'array': + parameters = parameters.get('items', {}) + + # iterate over all of the parameters recursively + secret_parameters = {} + for parameter, options in six.iteritems(parameters): + # if parameter is a dict or a list, then we need to recurse into them + parameter_type = options.get('type') + if parameter_type == 'object': + sub_params = get_secret_parameters(options.get('properties', {})) + secret_parameters[parameter] = sub_params + elif parameter_type == 'array': + sub_params = get_secret_parameters(options.get('items', {})) + secret_parameters[parameter] = sub_params + elif options.get('secret', False): + # if this parameter is secret, then add it our secret parameters + secret_parameters[parameter] = parameter_type return secret_parameters @@ -49,15 +98,22 @@ def mask_secret_parameters(parameters, secret_parameters): :param parameters: Parameters to process. :type parameters: ``dict`` - :param secret_parameters: List of parameter names which are secret. - :type secret_parameters: ``list`` + :param secret_parameters: Dict of parameter names which are secret. + :type secret_parameters: ``dict`` """ result = copy.deepcopy(parameters) - - for parameter in secret_parameters: - if parameter in result: - result[parameter] = MASKED_ATTRIBUTE_VALUE - + for secret_param, secret_sub_params in six.iteritems(secret_parameters): + if secret_param in result: + if isinstance(result[secret_param], dict): + result[secret_param] = mask_secret_parameters(parameters[secret_param], + secret_sub_params) + elif isinstance(result[secret_param], list): + # we're assuming lists contain the same data type for every element + for idx, value in enumerate(result[secret_param]): + result[secret_param][idx] = mask_secret_parameters(parameters[secret_param][idx], + secret_sub_params) + else: + result[secret_param] = MASKED_ATTRIBUTE_VALUE return result From 7cdcfb6e397531f3a5834ac670667aeb883dd6d7 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 23 May 2018 11:38:42 -0400 Subject: [PATCH 2/7] Added unit tests and fixed several bugs found during testing --- st2common/st2common/util/secrets.py | 57 +++-- st2common/tests/unit/test_util_secrets.py | 291 ++++++++++++++++++++++ 2 files changed, 334 insertions(+), 14 deletions(-) create mode 100644 st2common/tests/unit/test_util_secrets.py diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index e76d6e48ff..1a2b28466f 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -64,28 +64,54 @@ def get_secret_parameters(parameters): :rtype ``list`` """ - # determine if this parameters set is an object definition - # if it is, then drill in and grab the properties from the object itself + secret_parameters = {} parameters_type = parameters.get('type') + iterator = None if parameters_type == 'object': - parameters = parameters.get('properties', {}) + # 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': - parameters = parameters.get('items', {}) + # 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 - secret_parameters = {} - for parameter, options in six.iteritems(parameters): - # if parameter is a dict or a list, then we need to recurse into them + for parameter, options in iterator: + if not isinstance(options, dict): + continue + parameter_type = options.get('type') - if parameter_type == 'object': - sub_params = get_secret_parameters(options.get('properties', {})) - secret_parameters[parameter] = sub_params - elif parameter_type == 'array': - sub_params = get_secret_parameters(options.get('items', {})) - secret_parameters[parameter] = sub_params + 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 - secret_parameters[parameter] = parameter_type + 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 @@ -101,6 +127,9 @@ def mask_secret_parameters(parameters, secret_parameters): :param secret_parameters: Dict of parameter names which are secret. :type secret_parameters: ``dict`` """ + + # TODO fix this to work with deeply nested arrays / objects + result = copy.deepcopy(parameters) for secret_param, secret_sub_params in six.iteritems(secret_parameters): if secret_param in 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..c1860596f3 --- /dev/null +++ b/st2common/tests/unit/test_util_secrets.py @@ -0,0 +1,291 @@ +# 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.util import secrets + + +TEST_SCHEMA_NO_SECRETS = { + 'arg_required_no_default': { + 'description': 'Foo', + 'required': True, + 'type': 'string', + 'secret': False + } +} + +TEST_SCHEMA_FLAT = { + '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_SCHEMA_NESTED_OBJECTS = { + '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_SCHEMA_ARRAY = { + 'arg_optional_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'description': 'down', + 'type': 'string', + 'secret': True + } + } +} + +TEST_SCHEMA_NESTED_ARRAYS = { + '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_SCHEMA_NESTED_OBJECT_WITH_ARRAY = { + '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_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY = { + '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_SCHEMA_NESTED_ARRAY_WITH_OBJECT = { + '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 + } + } + } + } +} + + +class SecretUtilsTestCase(unittest2.TestCase): + + def test_get_secret_parameters_flat(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_FLAT) + expected = {'arg_optional_no_type_secret': None} + self.assertEqual(expected, result) + + def test_get_secret_parameters_no_secrets(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NO_SECRETS) + expected = {} + self.assertEqual(expected, result) + + def test_get_secret_parameters_nested_objects(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECTS) + expected = { + 'arg_optional_object': { + 'arg_nested_secret': 'string', + 'arg_nested_object': { + 'arg_double_nested_secret': 'string', + } + } + } + self.assertEqual(expected, result) + + def test_get_secret_parameters_array(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_ARRAY) + expected = { + 'arg_optional_array': [ + 'string' + ] + } + self.assertEqual(expected, result) + + def test_get_secret_parameters_nested_arrays(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAYS) + expected = { + 'arg_optional_array': [ + 'string' + ], + 'arg_optional_double_array': [ [ + 'string' + ] ], + 'arg_optional_tripple_array': [ [ [ + 'string' + ] ] ], + 'arg_optional_quad_array': [ [ [ [ + 'string' + ] ] ] ], + } + self.assertEqual(expected, result) + + def test_get_secret_parameters_nested_object_with_array(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_ARRAY) + expected = { + 'arg_optional_object_with_array': { + 'arg_nested_array': [ + 'string' + ] + } + } + self.assertEqual(expected, result) + + def test_get_secret_parameters_nested_object_with_double_array(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY) + expected = { + 'arg_optional_object_with_double_array': { + 'arg_double_nested_array': [ [ + 'string' + ] ] + } + } + self.assertEqual(expected, result) + + def test_get_secret_parameters_nested_array_with_object(self): + result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAY_WITH_OBJECT) + expected = { + 'arg_optional_array_with_object': [ { + 'arg_nested_secret': 'string' + } ] + } + self.assertEqual(expected, result) + + # TODO unit tests for mask_secret_parameters From 6a2693ec98b579db7c0f2f91c6686a32503bca52 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 23 May 2018 16:09:25 -0400 Subject: [PATCH 3/7] Added a lot more unit tests. mask_secret_parameters is not working and fully tested --- st2common/st2common/util/secrets.py | 59 ++- st2common/tests/unit/test_util_secrets.py | 508 +++++++++++++++++++--- 2 files changed, 484 insertions(+), 83 deletions(-) diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index 1a2b28466f..b2cf5bb066 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -116,33 +116,54 @@ def get_secret_parameters(parameters): 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`` + :type parameters: ``dict`` or ``list`` or ``string`` :param secret_parameters: Dict of parameter names which are secret. - :type secret_parameters: ``dict`` - """ + :type secret_parameters: ``dict`` or ``list`` - # TODO fix this to work with deeply nested arrays / objects - - result = copy.deepcopy(parameters) - for secret_param, secret_sub_params in six.iteritems(secret_parameters): - if secret_param in result: - if isinstance(result[secret_param], 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`` + """ + # 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 not result: + 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) - elif isinstance(result[secret_param], list): - # we're assuming lists contain the same data type for every element - for idx, value in enumerate(result[secret_param]): - result[secret_param][idx] = mask_secret_parameters(parameters[secret_param][idx], - secret_sub_params) - else: - result[secret_param] = MASKED_ATTRIBUTE_VALUE + 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 index c1860596f3..2a51211ae4 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -16,19 +16,12 @@ from __future__ import absolute_import import unittest2 +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE from st2common.util import secrets +################################################################################ -TEST_SCHEMA_NO_SECRETS = { - 'arg_required_no_default': { - 'description': 'Foo', - 'required': True, - 'type': 'string', - 'secret': False - } -} - -TEST_SCHEMA_FLAT = { +TEST_FLAT_SCHEMA = { 'arg_required_no_default': { 'description': 'Foo', 'required': True, @@ -49,7 +42,26 @@ }, } -TEST_SCHEMA_NESTED_OBJECTS = { +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', @@ -78,7 +90,18 @@ } } -TEST_SCHEMA_ARRAY = { +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', @@ -90,7 +113,39 @@ } } -TEST_SCHEMA_NESTED_ARRAYS = { +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', @@ -147,7 +202,36 @@ } } -TEST_SCHEMA_NESTED_OBJECT_WITH_ARRAY = { +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', @@ -165,7 +249,17 @@ } } -TEST_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY = { +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', @@ -187,7 +281,19 @@ } } -TEST_SCHEMA_NESTED_ARRAY_WITH_OBJECT = { +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', @@ -195,7 +301,7 @@ 'description': 'Mirror', 'type': 'object', 'properties': { - 'arg_nested_secret': { + 'arg_nested_secret': { 'description': 'Deep down', 'type': 'string', 'secret': True @@ -205,87 +311,361 @@ } } +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_SCHEMA_FLAT) - expected = {'arg_optional_no_type_secret': None} - self.assertEqual(expected, result) + 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_SCHEMA_NO_SECRETS) - expected = {} - self.assertEqual(expected, result) + 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_SCHEMA_NESTED_OBJECTS) + 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': 'secret password' + } + 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': 'string', + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE, 'arg_nested_object': { - 'arg_double_nested_secret': 'string', + 'arg_double_nested_secret': MASKED_ATTRIBUTE_VALUE, } } } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_ARRAY) + 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': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE ] } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_arrays(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAYS) + 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': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, ], - 'arg_optional_double_array': [ [ - 'string' - ] ], - 'arg_optional_tripple_array': [ [ [ - 'string' - ] ] ], - 'arg_optional_quad_array': [ [ [ [ - 'string' - ] ] ] ], + '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) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_object_with_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_ARRAY) + 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_optional_object_with_array': { 'arg_nested_array': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, ] } } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_object_with_double_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY) + 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': [ [ - 'string' - ] ] + '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) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_array_with_object(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAY_WITH_OBJECT) + 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': 'string' - } ] + '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) - - # TODO unit tests for mask_secret_parameters + self.assertEqual(expected, result) From b030f47a4fd94a1d3c9c293587e84423ccdac529 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 23 May 2018 16:09:25 -0400 Subject: [PATCH 4/7] Added a lot more unit tests. mask_secret_parameters is not working and fully tested --- st2common/st2common/util/secrets.py | 59 ++- st2common/tests/unit/test_util_secrets.py | 508 +++++++++++++++++++--- 2 files changed, 484 insertions(+), 83 deletions(-) diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index 1a2b28466f..b2cf5bb066 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -116,33 +116,54 @@ def get_secret_parameters(parameters): 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`` + :type parameters: ``dict`` or ``list`` or ``string`` :param secret_parameters: Dict of parameter names which are secret. - :type secret_parameters: ``dict`` - """ + :type secret_parameters: ``dict`` or ``list`` - # TODO fix this to work with deeply nested arrays / objects - - result = copy.deepcopy(parameters) - for secret_param, secret_sub_params in six.iteritems(secret_parameters): - if secret_param in result: - if isinstance(result[secret_param], 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`` + """ + # 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 not result: + 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) - elif isinstance(result[secret_param], list): - # we're assuming lists contain the same data type for every element - for idx, value in enumerate(result[secret_param]): - result[secret_param][idx] = mask_secret_parameters(parameters[secret_param][idx], - secret_sub_params) - else: - result[secret_param] = MASKED_ATTRIBUTE_VALUE + 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 index c1860596f3..2a51211ae4 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -16,19 +16,12 @@ from __future__ import absolute_import import unittest2 +from st2common.constants.secrets import MASKED_ATTRIBUTE_VALUE from st2common.util import secrets +################################################################################ -TEST_SCHEMA_NO_SECRETS = { - 'arg_required_no_default': { - 'description': 'Foo', - 'required': True, - 'type': 'string', - 'secret': False - } -} - -TEST_SCHEMA_FLAT = { +TEST_FLAT_SCHEMA = { 'arg_required_no_default': { 'description': 'Foo', 'required': True, @@ -49,7 +42,26 @@ }, } -TEST_SCHEMA_NESTED_OBJECTS = { +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', @@ -78,7 +90,18 @@ } } -TEST_SCHEMA_ARRAY = { +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', @@ -90,7 +113,39 @@ } } -TEST_SCHEMA_NESTED_ARRAYS = { +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', @@ -147,7 +202,36 @@ } } -TEST_SCHEMA_NESTED_OBJECT_WITH_ARRAY = { +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', @@ -165,7 +249,17 @@ } } -TEST_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY = { +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', @@ -187,7 +281,19 @@ } } -TEST_SCHEMA_NESTED_ARRAY_WITH_OBJECT = { +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', @@ -195,7 +301,7 @@ 'description': 'Mirror', 'type': 'object', 'properties': { - 'arg_nested_secret': { + 'arg_nested_secret': { 'description': 'Deep down', 'type': 'string', 'secret': True @@ -205,87 +311,361 @@ } } +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_SCHEMA_FLAT) - expected = {'arg_optional_no_type_secret': None} - self.assertEqual(expected, result) + 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_SCHEMA_NO_SECRETS) - expected = {} - self.assertEqual(expected, result) + 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_SCHEMA_NESTED_OBJECTS) + 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': 'secret password' + } + 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': 'string', + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE, 'arg_nested_object': { - 'arg_double_nested_secret': 'string', + 'arg_double_nested_secret': MASKED_ATTRIBUTE_VALUE, } } } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_ARRAY) + 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': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE ] } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_arrays(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAYS) + 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': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, ], - 'arg_optional_double_array': [ [ - 'string' - ] ], - 'arg_optional_tripple_array': [ [ [ - 'string' - ] ] ], - 'arg_optional_quad_array': [ [ [ [ - 'string' - ] ] ] ], + '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) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_object_with_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_ARRAY) + 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_optional_object_with_array': { 'arg_nested_array': [ - 'string' + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, ] } } - self.assertEqual(expected, result) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_object_with_double_array(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_OBJECT_WITH_DOUBLE_ARRAY) + 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': [ [ - 'string' - ] ] + '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) + self.assertEqual(expected, result) - def test_get_secret_parameters_nested_array_with_object(self): - result = secrets.get_secret_parameters(TEST_SCHEMA_NESTED_ARRAY_WITH_OBJECT) + 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': 'string' - } ] + '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) - - # TODO unit tests for mask_secret_parameters + self.assertEqual(expected, result) From 2a2a8c86d08d06d40d7b114e49aef563dcd0b6e6 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 23 May 2018 19:31:15 -0400 Subject: [PATCH 5/7] Fixing unit tests for secret masking --- st2common/st2common/models/db/execution.py | 7 ++++++- st2common/st2common/util/secrets.py | 6 +++++- st2common/tests/unit/test_util_secrets.py | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/models/db/execution.py b/st2common/st2common/models/db/execution.py index 1012f2298b..81a3c19706 100644 --- a/st2common/st2common/models/db/execution.py +++ b/st2common/st2common/models/db/execution.py @@ -119,9 +119,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 b2cf5bb066..bad466edfc 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -124,7 +124,11 @@ def mask_secret_parameters(parameters, secret_parameters, result=None): :type parameters: ``dict`` or ``list`` or ``string`` :param secret_parameters: Dict of parameter names which are secret. - :type secret_parameters: ``dict`` or ``list`` + 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 diff --git a/st2common/tests/unit/test_util_secrets.py b/st2common/tests/unit/test_util_secrets.py index 2a51211ae4..848b372574 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -367,7 +367,7 @@ def test_get_secret_parameters_nested_array_with_object(self): def test_mask_secret_parameters_flat(self): parameters = { 'arg_required_no_default': 'test', - 'arg_optional_no_type_secret': 'secret password' + 'arg_optional_no_type_secret': None } result = secrets.mask_secret_parameters(parameters, TEST_FLAT_SECRET_PARAMS) From 78763fe236928ad9e645a9e0102d3f3587fbcfb6 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Wed, 23 May 2018 20:00:32 -0400 Subject: [PATCH 6/7] Fix random failures in test_inquiry unit test --- st2client/tests/unit/test_inquiry.py | 5 +++++ 1 file changed, 5 insertions(+) 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 From c793abfd54594650c1981a1077c669d8867d66e3 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Thu, 24 May 2018 09:32:40 -0400 Subject: [PATCH 7/7] Updated changelog for #4139 --- CHANGELOG.rst | 12 ++++++++++++ st2common/st2common/util/secrets.py | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) 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/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index bad466edfc..af12aa92b6 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -149,7 +149,7 @@ def mask_secret_parameters(parameters, secret_parameters, result=None): # 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 not result: + if result is None: result = copy.deepcopy(parameters) # iterate over the secret parameters