diff --git a/CHANGELOG.rst b/CHANGELOG.rst index bc9f7f8001..4a6ae0d5b6 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,12 @@ Fixed * Fix redis SSL problems with sentinel #5660 +* Fix a bug in the pack config loader so that objects covered by an ``patternProperties`` schema + or arrays using ``additionalItems`` schema(s) can use encrypted datastore keys and have their + default values applied correctly. #5321 + + Contributed by @cognifloyd. + Added ~~~~~ diff --git a/st2common/st2common/util/config_loader.py b/st2common/st2common/util/config_loader.py index 657d209d82..42f750e1fc 100644 --- a/st2common/st2common/util/config_loader.py +++ b/st2common/st2common/util/config_loader.py @@ -15,6 +15,7 @@ from __future__ import absolute_import import copy +import re import six @@ -99,21 +100,118 @@ def _get_values_for_config(self, config_schema_db, config_db): return config @staticmethod - def _get_object_property_schema(object_schema, additional_properties_keys=None): + def _get_object_properties_schema(object_schema, object_keys=None): """ - Create a schema for an object property using both additionalProperties and properties. + Create a schema for an object property using all of: properties, + patternProperties, and additionalProperties. + + This 'flattens' properties, patternProperties, and additionalProperties + so that we can handle patternProperties and additionalProperties + as if they were defined in properties. + So, every key in object_keys will be assigned a schema + from properties, patternProperties, or additionalProperties. + + NOTE: order of precedence: properties, patternProperties, additionalProperties + So, the additionalProperties schema is only used for keys that are not in + properties and that do not match any of the patterns in patternProperties. + And, patternProperties schemas only apply to keys missing from properties. :rtype: ``dict`` """ - property_schema = {} + # preserve the order in object_keys + flattened_properties_schema = {key: {} for key in object_keys} + + # properties takes precedence over patternProperties and additionalProperties + properties_schema = object_schema.get("properties", {}) + flattened_properties_schema.update(properties_schema) + + # extra_keys has keys that may use patternProperties or additionalProperties + # we remove keys when they have been assigned a schema + extra_keys = set(object_keys) - set(properties_schema.keys()) + + if not extra_keys: + # nothing to check. Don't look at patternProperties or additionalProperties. + return flattened_properties_schema + + # match each key against patternPropetties + pattern_properties = object_schema.get("patternProperties", {}) + # patternProperties should be a dict if defined + if pattern_properties and isinstance(pattern_properties, dict): + # we need to match all extra_keys against all patterns + # and then compose the per-property schema from all + # the matched patterns' properties. + pattern_properties = { + re.compile(raw_pattern): pattern_schema + for raw_pattern, pattern_schema in pattern_properties.items() + } + for key in list(extra_keys): + key_schemas = [] + for pattern, pattern_schema in pattern_properties.items(): + if pattern.search(key): + key_schemas.append(pattern_schema) + if key_schemas: + # This naive schema composition approximates allOf. + # We can improve this later if someone provides examples that need + # a better allOf schema implementation for patternProperties. + composed_schema = {} + for schema in key_schemas: + composed_schema.update(schema) + # update matched key + flattened_properties_schema[key] = composed_schema + # don't overwrite matched key's schema with additionalProperties + extra_keys.remove(key) + + if not extra_keys: + # nothing else to check. Don't look at additionalProperties. + return flattened_properties_schema + + # fill in any remaining keys with additionalProperties additional_properties = object_schema.get("additionalProperties", {}) # additionalProperties can be a boolean or a dict if additional_properties and isinstance(additional_properties, dict): # ensure that these keys are present in the object - for key in additional_properties_keys: - property_schema[key] = additional_properties - property_schema.update(object_schema.get("properties", {})) - return property_schema + for key in extra_keys: + flattened_properties_schema[key] = additional_properties + + return flattened_properties_schema + + @staticmethod + def _get_array_items_schema(array_schema, items_count=0): + """ + Create a schema for array items using both additionalItems and items. + + This 'flattens' items and additionalItems so that we can handle additionalItems + as if each additional item was defined in items. + + The additionalItems schema will only be used if the items schema is shorter + than items_count. So, when additionalItems is defined, the items schema will be + extended to be at least as long as items_count. + + :rtype: ``list`` + """ + flattened_items_schema = [] + items_schema = array_schema.get("items", []) + if isinstance(items_schema, dict): + # with only one schema for all items, additionalItems will be ignored. + flattened_items_schema.extend([items_schema] * items_count) + else: + # items is a positional array of schemas + flattened_items_schema.extend(items_schema) + + flattened_items_schema_count = len(flattened_items_schema) + if flattened_items_schema_count >= items_count: + # no additional items to account for. + return flattened_items_schema + + additional_items = array_schema.get("additionalItems", {}) + # additionalItems can be a boolean or a dict + if additional_items and isinstance(additional_items, dict): + # ensure that these indexes are present in the array + flattened_items_schema.extend( + [additional_items] * (items_count - flattened_items_schema_count) + ) + + return flattened_items_schema def _assign_dynamic_config_values(self, schema, config, parent_keys=None): """ @@ -137,7 +235,13 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): if config_is_dict: # different schema for each key/value pair schema_item = schema.get(config_item_key, {}) - if config_is_list: + if config_is_list and isinstance(schema, list): + # positional schema for list items + try: + schema_item = schema[config_item_key] + except IndexError: + schema_item = {} + elif config_is_list: # same schema is shared between every item in the list schema_item = schema @@ -149,19 +253,23 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None): # Inspect nested object properties if is_dictionary: - property_schema = self._get_object_property_schema( + properties_schema = self._get_object_properties_schema( schema_item, - additional_properties_keys=config_item_value.keys(), + object_keys=config_item_value.keys(), ) self._assign_dynamic_config_values( - schema=property_schema, + schema=properties_schema, config=config[config_item_key], parent_keys=current_keys, ) # Inspect nested list items elif is_list: + items_schema = self._get_array_items_schema( + schema_item, + items_count=len(config[config_item_key]), + ) self._assign_dynamic_config_values( - schema=schema_item.get("items", {}), + schema=items_schema, config=config[config_item_key], parent_keys=current_keys, ) @@ -193,35 +301,72 @@ def _assign_default_values(self, schema, config): Note: This method mutates config argument in place. - :rtype: ``dict`` + :rtype: ``dict|list`` """ - for schema_item_key, schema_item in six.iteritems(schema): + schema_is_dict = isinstance(schema, dict) + iterator = schema.items() if schema_is_dict else enumerate(schema) + + # _get_*_schema ensures that schema_item is always a dict + for schema_item_key, schema_item in iterator: has_default_value = "default" in schema_item - has_config_value = schema_item_key in config + if isinstance(config, dict): + has_config_value = schema_item_key in config + else: + has_config_value = schema_item_key < len(config) default_value = schema_item.get("default", None) - is_object = schema_item.get("type", None) == "object" - has_properties = schema_item.get("properties", None) - has_additional_properties = schema_item.get("additionalProperties", None) - if has_default_value and not has_config_value: # Config value is not provided, but default value is, use a default value config[schema_item_key] = default_value - # Inspect nested object properties - if is_object and (has_properties or has_additional_properties): - if not config.get(schema_item_key, None): - config[schema_item_key] = {} + try: + config_value = config[schema_item_key] + except (KeyError, IndexError): + config_value = None - property_schema = self._get_object_property_schema( - schema_item, - additional_properties_keys=config[schema_item_key].keys(), - ) + schema_item_type = schema_item.get("type", None) - self._assign_default_values( - schema=property_schema, config=config[schema_item_key] + if schema_item_type == "object": + has_properties = schema_item.get("properties", None) + has_pattern_properties = schema_item.get("patternProperties", None) + has_additional_properties = schema_item.get( + "additionalProperties", None ) + # Inspect nested object properties + if ( + has_properties + or has_pattern_properties + or has_additional_properties + ): + if not config_value: + config_value = config[schema_item_key] = {} + + properties_schema = self._get_object_properties_schema( + schema_item, + object_keys=config_value.keys(), + ) + + self._assign_default_values( + schema=properties_schema, config=config_value + ) + elif schema_item_type == "array": + has_items = schema_item.get("items", None) + has_additional_items = schema_item.get("additionalItems", None) + + # Inspect nested array items + if has_items or has_additional_items: + if not config_value: + config_value = config[schema_item_key] = [] + + items_schema = self._get_array_items_schema( + schema_item, + items_count=len(config_value), + ) + self._assign_default_values( + schema=items_schema, config=config_value + ) + return config def _get_datastore_value_for_expression(self, key, value, config_schema_item=None): diff --git a/st2common/tests/unit/test_config_loader.py b/st2common/tests/unit/test_config_loader.py index d8616a3b44..3f8e23d5be 100644 --- a/st2common/tests/unit/test_config_loader.py +++ b/st2common/tests/unit/test_config_loader.py @@ -575,6 +575,245 @@ def test_get_config_dynamic_config_item_under_additional_properties(self): config_db.delete() + def test_get_config_dynamic_config_item_under_pattern_properties(self): + pack_name = "dummy_pack_schema_with_pattern_properties_1" + loader = ContentPackConfigLoader(pack_name=pack_name) + + encrypted_value = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v1_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k1_encrypted", value=encrypted_value, secret=True) + ) + + #################### + # values in objects under an object with patternProperties + values = { + "profiles": { + "dev": { + # no host or port to test default value + "token": "hard-coded-secret", + }, + "prod": { + "host": "127.1.2.7", + "port": 8282, + # encrypted in datastore + "token": "{{st2kv.system.k1_encrypted}}", + # schema declares `secret: true` which triggers auto-decryption. + # If this were not encrypted, it would try to decrypt it and fail. + }, + } + } + config_db = ConfigDB(pack=pack_name, values=values) + config_db = Config.add_or_update(config_db) + + config_rendered = loader.get_config() + + self.assertEqual( + config_rendered, + { + "region": "us-east-1", + "profiles": { + "dev": { + "host": "127.0.0.3", + "port": 8080, + "token": "hard-coded-secret", + }, + "prod": { + "host": "127.1.2.7", + "port": 8282, + "token": "v1_encrypted", + }, + }, + }, + ) + + config_db.delete() + + def test_get_config_dynamic_config_item_properties_order_of_precedence(self): + pack_name = "dummy_pack_schema_with_pattern_and_additional_properties_1" + loader = ContentPackConfigLoader(pack_name=pack_name) + + encrypted_value_1 = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v1_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k1_encrypted", value=encrypted_value_1, secret=True) + ) + encrypted_value_2 = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v2_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k2_encrypted", value=encrypted_value_2, secret=True) + ) + encrypted_value_3 = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v3_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k3_encrypted", value=encrypted_value_3, secret=True) + ) + + #################### + # values in objects under an object with additionalProperties + values = { + "profiles": { + # properties + "foo": { + "domain": "foo.example.com", + "token": "hard-coded-secret", + }, + "bar": { + "domain": "bar.example.com", + "token": "{{st2kv.system.k1_encrypted}}", + }, + # patternProperties start with env- + "env-dev": { + "host": "127.0.0.127", + "token": "hard-coded-secret", + }, + "env-prod": { + "host": "127.1.2.7", + "port": 8282, + # encrypted in datastore + "token": "{{st2kv.system.k2_encrypted}}", + # schema declares `secret: true` which triggers auto-decryption. + # If this were not encrypted, it would try to decrypt it and fail. + }, + # additionalProperties + "dev": { + "url": "https://example.com", + "token": "hard-coded-secret", + }, + "prod": { + "url": "https://other.example.com", + "port": 2345, + "token": "{{st2kv.system.k3_encrypted}}", + }, + } + } + config_db = ConfigDB(pack=pack_name, values=values) + config_db = Config.add_or_update(config_db) + + config_rendered = loader.get_config() + + self.assertEqual( + config_rendered, + { + "region": "us-east-1", + "profiles": { + "foo": { + "domain": "foo.example.com", + "token": "hard-coded-secret", + }, + "bar": { + "domain": "bar.example.com", + "token": "v1_encrypted", + }, + "env-dev": { + "host": "127.0.0.127", + "port": 8080, + "token": "hard-coded-secret", + }, + "env-prod": { + "host": "127.1.2.7", + "port": 8282, + "token": "v2_encrypted", + }, + "dev": { + "url": "https://example.com", + "port": 1234, + "token": "hard-coded-secret", + }, + "prod": { + "url": "https://other.example.com", + "port": 2345, + "token": "v3_encrypted", + }, + }, + }, + ) + + config_db.delete() + + def test_get_config_dynamic_config_item_under_additional_items(self): + pack_name = "dummy_pack_schema_with_additional_items_1" + loader = ContentPackConfigLoader(pack_name=pack_name) + + encrypted_value = crypto.symmetric_encrypt( + KeyValuePairAPI.crypto_key, "v1_encrypted" + ) + KeyValuePair.add_or_update( + KeyValuePairDB(name="k1_encrypted", value=encrypted_value, secret=True) + ) + + #################### + # values in objects under an object with additionalProperties + values = { + "profiles": [ + { + # no host or port to test default value + "token": "hard-coded-secret", + }, + { + "host": "127.1.2.7", + "port": 8282, + # encrypted in datastore + "token": "{{st2kv.system.k1_encrypted}}", + # schema declares `secret: true` which triggers auto-decryption. + # If this were not encrypted, it would try to decrypt it and fail. + }, + ], + # foobar has additionalItems: true + "foobar": [ + # there are no types to validate here + 5, + "a string", + { + # there are no defaults to interpolate here + "token": "hard-coded-secret", + }, + { + # nothing is marked `secret: true` so no auto-decryption occurs. + "token": "{{st2kv.system.k1_encrypted|decrypt_kv}}", + }, + ], + } + config_db = ConfigDB(pack=pack_name, values=values) + config_db = Config.add_or_update(config_db) + + config_rendered = loader.get_config() + + self.assertEqual( + config_rendered, + { + "region": "us-east-1", + "profiles": [ + { + "host": "127.0.0.3", + "port": 8080, + "token": "hard-coded-secret", + }, + { + "host": "127.1.2.7", + "port": 8282, + "token": "v1_encrypted", + }, + ], + "foobar": [ + 5, + "a string", + { + "token": "hard-coded-secret", + }, + { + "token": "v1_encrypted", + }, + ], + }, + ) + + config_db.delete() + def test_empty_config_object_in_the_database(self): pack_name = "dummy_pack_empty_config" diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/config.schema.yaml new file mode 100644 index 0000000000..88c8b1c5d7 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/config.schema.yaml @@ -0,0 +1,28 @@ +--- + region: + type: "string" + required: false + default: "us-east-1" + profiles: + type: "array" + required: false + additionalItems: + type: object + additionalProperties: false + properties: + host: + type: "string" + required: false + default: "127.0.0.3" + port: + type: "integer" + required: false + default: 8080 + token: + type: "string" + required: true + secret: true + foobar: + type: "array" + required: false + additionalItems: true diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/pack.yaml new file mode 100644 index 0000000000..172b029d27 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_additional_items_1/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_schema_with_additional_items_1 +description : dummy pack with nested objects under additionalItems +version : 0.1.0 +author : st2-dev +email : info@stackstorm.com diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/config.schema.yaml new file mode 100644 index 0000000000..eb60d04cb0 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/config.schema.yaml @@ -0,0 +1,61 @@ +--- + region: + type: "string" + required: false + default: "us-east-1" + profiles: + type: "object" + required: false + # order of precedence: properties, patternProperties, additionalProperties + properties: + foo: + type: object + properties: + domain: + type: "string" + required: true + token: + type: "string" + required: true + secret: true + bar: + type: object + properties: + domain: + type: "string" + required: true + token: + type: "string" + required: true + secret: true + patternProperties: + "^env-\\w+$": + type: object + additionalProperties: false + properties: + host: + type: "string" + required: true + port: + type: "integer" + required: false + default: 8080 + token: + type: "string" + required: true + secret: true + additionalProperties: + type: object + additionalProperties: false + properties: + url: + type: "string" + required: true + port: + type: "integer" + required: false + default: 1234 + token: + type: "string" + required: true + secret: true diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/pack.yaml new file mode 100644 index 0000000000..5861470ddd --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_and_additional_properties_1/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_schema_with_pattern_and_additional_properties_1 +description : dummy pack with nested objects under patternProperties and additionalProperties +version : 0.1.0 +author : st2-dev +email : info@stackstorm.com diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/config.schema.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/config.schema.yaml new file mode 100644 index 0000000000..237507215f --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/config.schema.yaml @@ -0,0 +1,25 @@ +--- + region: + type: "string" + required: false + default: "us-east-1" + profiles: + type: "object" + required: false + patternProperties: + "^\\w+$": + type: object + additionalProperties: false + properties: + host: + type: "string" + required: false + default: "127.0.0.3" + port: + type: "integer" + required: false + default: 8080 + token: + type: "string" + required: true + secret: true diff --git a/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/pack.yaml b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/pack.yaml new file mode 100644 index 0000000000..b0816cf159 --- /dev/null +++ b/st2tests/st2tests/fixtures/packs/dummy_pack_schema_with_pattern_properties_1/pack.yaml @@ -0,0 +1,6 @@ +--- +name : dummy_pack_schema_with_pattern_properties_1 +description : dummy pack with nested objects under patternProperties +version : 0.1.0 +author : st2-dev +email : info@stackstorm.com