From 4dd8e1d68cf3d05ece446d4fa53dd5cd4b8e91a6 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 10 Jul 2018 12:30:55 -0400 Subject: [PATCH 1/4] Fix bug where objects and arrays marked as secret weren't being masked --- CHANGELOG.rst | 9 +++ st2common/st2common/util/secrets.py | 18 +++--- st2common/tests/unit/test_util_secrets.py | 70 ++++++++++++++++++++++- 3 files changed, 87 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ff3f22e160..54ff8b3382 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -33,6 +33,15 @@ Changed Contributed by Nick Maludy (Encore Technologies). + +Fixed +~~~~~ + +* Fixed a bug where ``secret: true`` was not applying to full object and array trees. (bugfix) #4234 + Reported by @jjm + + Contributed by Nick Maludy (Encore Technologies). + 2.8.0 - July 10, 2018 --------------------- diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index af12aa92b6..d605cd4e83 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -95,7 +95,15 @@ def get_secret_parameters(parameters): continue parameter_type = options.get('type') - if parameter_type in ['object', 'array']: + if 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 + elif parameter_type in ['object', 'array']: sub_params = get_secret_parameters(options) if sub_params: if isinstance(secret_parameters, list): @@ -104,14 +112,6 @@ def get_secret_parameters(parameters): 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 diff --git a/st2common/tests/unit/test_util_secrets.py b/st2common/tests/unit/test_util_secrets.py index 848b372574..0faef33523 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -319,9 +319,34 @@ ] } +################################################################################ + +TEST_SECRET_ARRAY_SCHEMA = { + 'arg_secret_array': { + 'description': 'Mirror', + 'type': 'array', + 'secret': True, + } +} + +TEST_SECRET_ARRAY_SECRET_PARAMS = { + 'arg_secret_array': 'array' +} ################################################################################ +TEST_SECRET_OBJECT_SCHEMA = { + 'arg_secret_object': { + 'type': 'object', + 'secret': True, + } +} + +TEST_SECRET_OBJECT_SECRET_PARAMS = { + 'arg_secret_object': 'object' +} + +################################################################################ class SecretUtilsTestCase(unittest2.TestCase): @@ -361,8 +386,15 @@ 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) + def test_get_secret_parameters_secret_array(self): + result = secrets.get_secret_parameters(TEST_SECRET_ARRAY_SCHEMA) + self.assertEqual(TEST_SECRET_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_secret_object(self): + result = secrets.get_secret_parameters(TEST_SECRET_OBJECT_SCHEMA) + self.assertEqual(TEST_SECRET_OBJECT_SECRET_PARAMS, result) + ############################################################################ - # TODO unit tests for mask_secret_parameters def test_mask_secret_parameters_flat(self): parameters = { @@ -669,3 +701,39 @@ def test_mask_secret_parameters_nested_array_with_object(self): ] } self.assertEqual(expected, result) + + def test_mask_secret_array(self): + parameters = { + 'arg_secret_array': [ + "abc", + 123, + True + ] + } + result = secrets.mask_secret_parameters(parameters, + TEST_SECRET_ARRAY_SECRET_PARAMS) + expected = { + 'arg_secret_array': MASKED_ATTRIBUTE_VALUE + } + self.assertEqual(expected, result) + + def test_mask_secret_object(self): + parameters = { + 'arg_secret_object': + { + "abc": 123, + "key": "value", + "bool": True, + "array": ["x", "y", "z"], + "obj": + { + "x": "deep" + } + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_SECRET_OBJECT_SECRET_PARAMS) + expected = { + 'arg_secret_object': MASKED_ATTRIBUTE_VALUE + } + self.assertEqual(expected, result) From f19176f0cd4820e2565d6bb5516e1c7f0bd01c2e Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 10 Jul 2018 12:44:10 -0400 Subject: [PATCH 2/4] Added comments to code about object/array secret masking --- st2common/st2common/util/secrets.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index d605cd4e83..fbe566dbe1 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -96,7 +96,13 @@ def get_secret_parameters(parameters): parameter_type = options.get('type') if options.get('secret', False): - # if this parameter is secret, then add it our secret parameters + # If this parameter is secret, then add it our secret parameters + # + # **This causes the _full_ object / array tree to be secret.** + # + # **Important** that we do this check first, so in case this parameter + # is an `object` or `array`, and the user wants the full thing + # to be secret, that it is marked as secret. if isinstance(secret_parameters, list): secret_parameters.append(parameter_type) elif isinstance(secret_parameters, dict): @@ -104,6 +110,8 @@ def get_secret_parameters(parameters): else: return parameter_type elif parameter_type in ['object', 'array']: + # otherwise recursively dive into the `object`/`array` and + # find individual parameters marked as secret sub_params = get_secret_parameters(options) if sub_params: if isinstance(secret_parameters, list): From cd0389111a72569cf2e368e0bf0f39e80e6c4480 Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 10 Jul 2018 13:27:38 -0400 Subject: [PATCH 3/4] Added a ton more tests for nested secrets objects and arrays --- st2common/st2common/util/secrets.py | 14 +- st2common/tests/unit/test_util_secrets.py | 363 +++++++++++++++++++++- 2 files changed, 374 insertions(+), 3 deletions(-) diff --git a/st2common/st2common/util/secrets.py b/st2common/st2common/util/secrets.py index fbe566dbe1..57d8a0b08f 100644 --- a/st2common/st2common/util/secrets.py +++ b/st2common/st2common/util/secrets.py @@ -66,6 +66,17 @@ def get_secret_parameters(parameters): secret_parameters = {} parameters_type = parameters.get('type') + # If the parameter itself is secret, then skip all processing below it + # and return the type of this parameter. + # + # **This causes the _full_ object / array tree to be secret (no children will be shown).** + # + # **Important** that we do this check first, so in case this parameter + # is an `object` or `array`, and the user wants the full thing + # to be secret, that it is marked as secret. + if parameters.get('secret', False): + return parameters_type + iterator = None if parameters_type == 'object': # if this is an object, then iterate over the properties within @@ -98,7 +109,8 @@ def get_secret_parameters(parameters): if options.get('secret', False): # If this parameter is secret, then add it our secret parameters # - # **This causes the _full_ object / array tree to be secret.** + # **This causes the _full_ object / array tree to be secret + # (no children will be shown)** # # **Important** that we do this check first, so in case this parameter # is an `object` or `array`, and the user wants the full thing diff --git a/st2common/tests/unit/test_util_secrets.py b/st2common/tests/unit/test_util_secrets.py index 0faef33523..8f7cf4cdaf 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -145,6 +145,32 @@ ################################################################################ +TEST_ROOT_OBJECT_SCHEMA = { + 'description': 'root', + 'type': 'object', + 'properties': { + 'arg_level_one': { + 'description': 'down', + 'type': 'object', + 'properties': { + 'secret_field_in_object': { + 'type': 'string', + 'secret': True + } + } + } + } +} + +TEST_ROOT_OBJECT_SECRET_PARAMS = { + 'arg_level_one': + { + 'secret_field_in_object': 'string' + } +} + +################################################################################ + TEST_NESTED_ARRAYS_SCHEMA = { 'arg_optional_array': { 'description': 'Mirror', @@ -346,6 +372,182 @@ 'arg_secret_object': 'object' } + +################################################################################ + +TEST_SECRET_ROOT_ARRAY_SCHEMA = { + 'description': 'secret array', + 'type': 'array', + 'secret': True, + 'items': { + 'description': 'down', + 'type': 'object', + 'properties': { + 'secret_field_in_object': { + 'type': 'string', + 'secret': True + } + } + } +} + +TEST_SECRET_ROOT_ARRAY_SECRET_PARAMS = 'array' + +################################################################################ + +TEST_SECRET_ROOT_OBJECT_SCHEMA = { + 'description': 'secret object', + 'type': 'object', + 'secret': True, + 'proeprteis': { + 'arg_level_one': { + 'description': 'down', + 'type': 'object', + 'properties': { + 'secret_field_in_object': { + 'type': 'string', + 'secret': True + } + } + } + } +} + +TEST_SECRET_ROOT_OBJECT_SECRET_PARAMS = 'object' + +################################################################################ + +TEST_SECRET_NESTED_OBJECTS_SCHEMA = { + 'arg_object': { + 'description': 'Mirror', + 'type': 'object', + 'properties': { + 'arg_nested_object': { + 'description': 'Mirror mirror', + 'type': 'object', + 'secret': True, + 'properties': { + 'arg_double_nested_secret': { + 'description': 'Deep, deep down', + 'type': 'string', + 'secret': True + } + } + }, + 'arg_nested_secret': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + }, + 'arg_secret_object': { + 'description': 'Mirror', + 'type': 'object', + 'secret': True, + 'properties': { + 'arg_nested_object': { + 'description': 'Mirror mirror', + 'type': 'object', + 'secret': True, + 'properties': { + 'arg_double_nested_secret': { + 'description': 'Deep, deep down', + 'type': 'string', + 'secret': True + } + } + }, + 'arg_nested_secret': { + 'description': 'Deep down', + 'type': 'string', + 'secret': True + } + } + } +} + +TEST_SECRET_NESTED_OBJECTS_SECRET_PARAMS = { + 'arg_object': { + 'arg_nested_secret': 'string', + 'arg_nested_object': 'object' + }, + 'arg_secret_object': 'object' +} + + +################################################################################ + +TEST_SECRET_NESTED_ARRAYS_SCHEMA = { + 'arg_optional_array': { + 'description': 'Mirror', + 'type': 'array', + 'secret': True, + 'items': { + 'description': 'Deep down', + 'type': 'string' + } + }, + 'arg_optional_double_array': { + 'description': 'Mirror', + 'type': 'array', + 'secret': True, + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + } + } + }, + 'arg_optional_tripple_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'type': 'array', + 'secret': True, + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + } + } + } + }, + 'arg_optional_quad_array': { + 'description': 'Mirror', + 'type': 'array', + 'items': { + 'type': 'array', + 'items': { + 'type': 'array', + 'secret': True, + 'items': { + 'type': 'array', + 'items': { + 'description': 'Deep down', + 'type': 'string', + } + } + } + } + } +} + +TEST_SECRET_NESTED_ARRAYS_SECRET_PARAMS = { + 'arg_optional_array': 'array', + 'arg_optional_double_array': 'array', + 'arg_optional_tripple_array': [ + 'array' + ], + 'arg_optional_quad_array': [ + [ + 'array' + ] + ] +} + ################################################################################ class SecretUtilsTestCase(unittest2.TestCase): @@ -370,6 +572,10 @@ 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_root_object(self): + result = secrets.get_secret_parameters(TEST_ROOT_OBJECT_SCHEMA) + self.assertEqual(TEST_ROOT_OBJECT_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) @@ -394,6 +600,22 @@ def test_get_secret_parameters_secret_object(self): result = secrets.get_secret_parameters(TEST_SECRET_OBJECT_SCHEMA) self.assertEqual(TEST_SECRET_OBJECT_SECRET_PARAMS, result) + def test_get_secret_parameters_secret_root_array(self): + result = secrets.get_secret_parameters(TEST_SECRET_ROOT_ARRAY_SCHEMA) + self.assertEqual(TEST_SECRET_ROOT_ARRAY_SECRET_PARAMS, result) + + def test_get_secret_parameters_secret_root_object(self): + result = secrets.get_secret_parameters(TEST_SECRET_ROOT_OBJECT_SCHEMA) + self.assertEqual(TEST_SECRET_ROOT_OBJECT_SECRET_PARAMS, result) + + def test_get_secret_parameters_secret_nested_arrays(self): + result = secrets.get_secret_parameters(TEST_SECRET_NESTED_ARRAYS_SCHEMA) + self.assertEqual(TEST_SECRET_NESTED_ARRAYS_SECRET_PARAMS, result) + + def test_get_secret_parameters_secret_nested_objects(self): + result = secrets.get_secret_parameters(TEST_SECRET_NESTED_OBJECTS_SCHEMA) + self.assertEqual(TEST_SECRET_NESTED_OBJECTS_SECRET_PARAMS, result) + ############################################################################ def test_mask_secret_parameters_flat(self): @@ -491,6 +713,23 @@ def test_mask_secret_parameters_root_array(self): ] self.assertEqual(expected, result) + def test_mask_secret_parameters_root_object(self): + parameters = { + 'arg_level_one': + { + 'secret_field_in_object': 'Secret $tr!ng' + } + } + + result = secrets.mask_secret_parameters(parameters, TEST_ROOT_OBJECT_SECRET_PARAMS) + expected = { + 'arg_level_one': + { + 'secret_field_in_object': MASKED_ATTRIBUTE_VALUE + } + } + self.assertEqual(expected, result) + def test_mask_secret_parameters_nested_arrays(self): parameters = { 'arg_optional_array': [ @@ -702,7 +941,7 @@ def test_mask_secret_parameters_nested_array_with_object(self): } self.assertEqual(expected, result) - def test_mask_secret_array(self): + def test_mask_secret_parameters_secret_array(self): parameters = { 'arg_secret_array': [ "abc", @@ -717,7 +956,7 @@ def test_mask_secret_array(self): } self.assertEqual(expected, result) - def test_mask_secret_object(self): + def test_mask_secret_parameters_secret_object(self): parameters = { 'arg_secret_object': { @@ -737,3 +976,123 @@ def test_mask_secret_object(self): 'arg_secret_object': MASKED_ATTRIBUTE_VALUE } self.assertEqual(expected, result) + + def test_mask_secret_parameters_secret_root_array(self): + parameters = [ + "abc", + 123, + True + ] + result = secrets.mask_secret_parameters(parameters, + TEST_SECRET_ROOT_ARRAY_SECRET_PARAMS) + expected = MASKED_ATTRIBUTE_VALUE + self.assertEqual(expected, result) + + def test_mask_secret_parameters_secret_root_object(self): + parameters = { + 'arg_level_one': + { + 'secret_field_in_object': 'Secret $tr!ng' + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_SECRET_ROOT_OBJECT_SECRET_PARAMS) + expected = MASKED_ATTRIBUTE_VALUE + self.assertEqual(expected, result) + + def test_mask_secret_parameters_secret_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_SECRET_NESTED_ARRAYS_SECRET_PARAMS) + + expected = { + 'arg_optional_array': MASKED_ATTRIBUTE_VALUE, + 'arg_optional_double_array': MASKED_ATTRIBUTE_VALUE, + 'arg_optional_tripple_array': [ + MASKED_ATTRIBUTE_VALUE, + MASKED_ATTRIBUTE_VALUE, + ], + 'arg_optional_quad_array': [ + [ + MASKED_ATTRIBUTE_VALUE, + ] + ] + } + self.assertEqual(expected, result) + + def test_mask_secret_parameters_secret_nested_objects(self): + parameters = { + 'arg_object': { + 'arg_nested_secret': 'nested Secret', + 'arg_nested_object': { + 'arg_double_nested_secret': 'double nested $ecret', + } + }, + 'arg_secret_object': { + 'arg_nested_secret': 'secret data', + 'arg_nested_object': { + 'arg_double_nested_secret': 'double nested $ecret', + } + } + } + result = secrets.mask_secret_parameters(parameters, + TEST_SECRET_NESTED_OBJECTS_SECRET_PARAMS) + expected = { + 'arg_object': { + 'arg_nested_secret': MASKED_ATTRIBUTE_VALUE, + 'arg_nested_object': MASKED_ATTRIBUTE_VALUE, + }, + 'arg_secret_object': MASKED_ATTRIBUTE_VALUE, + } + self.assertEqual(expected, result) From f4dbebe95ca61df25c768e703b411f95c2d0a25b Mon Sep 17 00:00:00 2001 From: Nick Maludy Date: Tue, 10 Jul 2018 13:31:25 -0400 Subject: [PATCH 4/4] Fix flake8 for unit tests --- st2common/tests/unit/test_util_secrets.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/st2common/tests/unit/test_util_secrets.py b/st2common/tests/unit/test_util_secrets.py index 8f7cf4cdaf..b110492180 100644 --- a/st2common/tests/unit/test_util_secrets.py +++ b/st2common/tests/unit/test_util_secrets.py @@ -550,6 +550,7 @@ ################################################################################ + class SecretUtilsTestCase(unittest2.TestCase): def test_get_secret_parameters_flat(self): @@ -978,7 +979,7 @@ def test_mask_secret_parameters_secret_object(self): self.assertEqual(expected, result) def test_mask_secret_parameters_secret_root_array(self): - parameters = [ + parameters = [ "abc", 123, True