diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ec63ecc90e..7d2fd46715 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,8 @@ Changed * Allow the orquesta st2kv function to return default for nonexistent key. (improvement) #4678 * Update requests library to latest version (2.22.0) in requirements. (improvement) #4680 +* Disallow "decrypt_kv" filter to be specified in the config for values that are marked as + "secret: True" in the schema. (improvement) #4709 Fixed ~~~~~ diff --git a/st2api/st2api/controllers/v1/pack_configs.py b/st2api/st2api/controllers/v1/pack_configs.py index 90a9f9a14d..bf6e8c640d 100644 --- a/st2api/st2api/controllers/v1/pack_configs.py +++ b/st2api/st2api/controllers/v1/pack_configs.py @@ -107,6 +107,8 @@ def put(self, pack_config_content, pack_ref, requester_user, show_secrets=False) config_api.validate(validate_against_schema=True) except jsonschema.ValidationError as e: raise ValueValidationException(six.text_type(e)) + except ValueValidationException as e: + raise ValueValidationException(six.text_type(e)) self._dump_config_to_disk(config_api) diff --git a/st2api/tests/unit/controllers/v1/test_pack_configs.py b/st2api/tests/unit/controllers/v1/test_pack_configs.py index dbbe93a991..719bf5c34b 100644 --- a/st2api/tests/unit/controllers/v1/test_pack_configs.py +++ b/st2api/tests/unit/controllers/v1/test_pack_configs.py @@ -18,7 +18,6 @@ from st2tests.api import FunctionalTest from st2api.controllers.v1.pack_configs import PackConfigsController - from st2tests.fixturesloader import get_fixtures_packs_base_path __all__ = [ @@ -79,3 +78,16 @@ def test_put_pack_config(self): get_resp.json['values'], expect_errors=True) self.assertEqual(put_resp.status_int, 200) self.assertEqual(get_resp.json, put_resp_undo.json) + + @mock.patch.object(PackConfigsController, '_dump_config_to_disk', mock.MagicMock()) + def test_put_invalid_pack_config(self): + get_resp = self.app.get('/v1/configs/dummy_pack_11', params={'show_secrets': True}, + expect_errors=True) + config = copy.copy(get_resp.json['values']) + put_resp = self.app.put_json('/v1/configs/dummy_pack_11', config, expect_errors=True) + self.assertEqual(put_resp.status_int, 400) + expected_msg = ('Values specified as "secret: True" in config schema are automatically ' + 'decrypted by default. Use of "decrypt_kv" jinja filter is not allowed ' + 'for such values. Please check the specified values in the config or ' + 'the default values in the schema.') + self.assertTrue(expected_msg in put_resp.json['faultstring']) diff --git a/st2common/st2common/util/pack.py b/st2common/st2common/util/pack.py index 1eb6b5acc0..dce3e5723f 100644 --- a/st2common/st2common/util/pack.py +++ b/st2common/st2common/util/pack.py @@ -25,6 +25,8 @@ from st2common.constants.pack import PACK_REF_WHITELIST_REGEX from st2common.content.loader import MetaLoader from st2common.persistence.pack import Pack +from st2common.exceptions.apivalidation import ValueValidationException +from st2common.util import jinja as jinja_utils __all__ = [ 'get_pack_ref_from_metadata', @@ -110,6 +112,14 @@ def validate_config_against_schema(config_schema, config_object, config_path, cleaned = util_schema.validate(instance=instance, schema=schema, cls=util_schema.CustomValidator, use_default=True, allow_default_none=True) + for key in cleaned: + if (jinja_utils.is_jinja_expression(value=cleaned.get(key)) and + "decrypt_kv" in cleaned.get(key) and config_schema.get(key).get('secret')): + raise ValueValidationException('Values specified as "secret: True" in config ' + 'schema are automatically decrypted by default. Use ' + 'of "decrypt_kv" jinja filter is not allowed for ' + 'such values. Please check the specified values in ' + 'the config or the default values in the schema.') except jsonschema.ValidationError as e: attribute = getattr(e, 'path', []) diff --git a/st2common/tests/unit/test_configs_registrar.py b/st2common/tests/unit/test_configs_registrar.py index 7fc72f44b0..f8c63a9dfe 100644 --- a/st2common/tests/unit/test_configs_registrar.py +++ b/st2common/tests/unit/test_configs_registrar.py @@ -36,6 +36,8 @@ PACK_1_PATH = os.path.join(fixturesloader.get_fixtures_packs_base_path(), 'dummy_pack_1') PACK_6_PATH = os.path.join(fixturesloader.get_fixtures_packs_base_path(), 'dummy_pack_6') PACK_19_PATH = os.path.join(fixturesloader.get_fixtures_packs_base_path(), 'dummy_pack_19') +PACK_11_PATH = os.path.join(fixturesloader.get_fixtures_packs_base_path(), 'dummy_pack_11') +PACK_22_PATH = os.path.join(fixturesloader.get_fixtures_packs_base_path(), 'dummy_pack_22') class ConfigsRegistrarTestCase(CleanDbTestCase): @@ -148,3 +150,63 @@ def test_register_all_configs_with_config_schema_validation_validation_failure_2 self.assertRaisesRegexp(ValueError, expected_msg, registrar.register_from_packs, base_dirs=packs_base_paths) + + def test_register_all_configs_with_config_schema_validation_validation_failure_3(self): + # This test checks for values containing "decrypt_kv" jinja filter in the config + # object where keys have "secret: True" set in the schema. + + # Verify DB is empty + pack_dbs = Pack.get_all() + config_dbs = Config.get_all() + + self.assertEqual(len(pack_dbs), 0) + self.assertEqual(len(config_dbs), 0) + + registrar = ConfigsRegistrar(use_pack_cache=False, fail_on_failure=True, + validate_configs=True) + registrar._pack_loader.get_packs = mock.Mock() + registrar._pack_loader.get_packs.return_value = {'dummy_pack_11': PACK_11_PATH} + + # Register ConfigSchema for pack + registrar._register_pack_db = mock.Mock() + registrar._register_pack(pack_name='dummy_pack_11', pack_dir=PACK_11_PATH) + packs_base_paths = content_utils.get_packs_base_paths() + + expected_msg = ('Values specified as "secret: True" in config schema are automatically ' + 'decrypted by default. Use of "decrypt_kv" jinja filter is not allowed ' + 'for such values. Please check the specified values in the config or ' + 'the default values in the schema.') + + self.assertRaisesRegexp(ValueError, expected_msg, + registrar.register_from_packs, + base_dirs=packs_base_paths) + + def test_register_all_configs_with_config_schema_validation_validation_failure_4(self): + # This test checks for default values containing "decrypt_kv" jinja filter for + # keys which have "secret: True" set. + + # Verify DB is empty + pack_dbs = Pack.get_all() + config_dbs = Config.get_all() + + self.assertEqual(len(pack_dbs), 0) + self.assertEqual(len(config_dbs), 0) + + registrar = ConfigsRegistrar(use_pack_cache=False, fail_on_failure=True, + validate_configs=True) + registrar._pack_loader.get_packs = mock.Mock() + registrar._pack_loader.get_packs.return_value = {'dummy_pack_22': PACK_22_PATH} + + # Register ConfigSchema for pack + registrar._register_pack_db = mock.Mock() + registrar._register_pack(pack_name='dummy_pack_22', pack_dir=PACK_22_PATH) + packs_base_paths = content_utils.get_packs_base_paths() + + expected_msg = ('Values specified as "secret: True" in config schema are automatically ' + 'decrypted by default. Use of "decrypt_kv" jinja filter is not allowed ' + 'for such values. Please check the specified values in the config or ' + 'the default values in the schema.') + + self.assertRaisesRegexp(ValueError, expected_msg, + registrar.register_from_packs, + base_dirs=packs_base_paths) diff --git a/st2tests/st2tests/fixtures/packs/configs/dummy_pack_11.yaml b/st2tests/st2tests/fixtures/packs/configs/dummy_pack_11.yaml new file mode 100644 index 0000000000..cd478da2de --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/configs/dummy_pack_11.yaml @@ -0,0 +1,2 @@ +--- + api_key: "{{st2kv.user.api_key | decrypt_kv}}" diff --git a/st2tests/st2tests/fixtures/packs/configs/dummy_pack_22.yaml b/st2tests/st2tests/fixtures/packs/configs/dummy_pack_22.yaml new file mode 100644 index 0000000000..bd7d065f8a --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/configs/dummy_pack_22.yaml @@ -0,0 +1,3 @@ +--- + key_with_no_secret_and_no_default: "Any Value" + diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_11/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_11/config.schema.yaml new file mode 100644 index 0000000000..7cbb5cd8be --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_11/config.schema.yaml @@ -0,0 +1,5 @@ +--- + api_key: + type: "string" + secret: true + required: true diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_11/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_11/pack.yaml new file mode 100644 index 0000000000..cbf3207ff0 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_11/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_11 +description : dummy pack +version : 0.1.0 +author : st2-dev +email : info@stackstorm.com diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_22/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_22/config.schema.yaml new file mode 100644 index 0000000000..19551a7e5b --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_22/config.schema.yaml @@ -0,0 +1,9 @@ +--- + wrong_key_with_invalid_default_value: + type: "string" + secret: true + required: true + default: "{{st2kv.user.api_key | decrypt_kv}}" + key_with_no_secret_and_no_default: + type: "string" + diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_22/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_22/pack.yaml new file mode 100644 index 0000000000..05d2011e33 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_22/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_22 +description : dummy pack +version : 0.1.0 +author : st2-dev +email : info@stackstorm.com