From e874a82755da1511bbbc1f447a276badc2c92928 Mon Sep 17 00:00:00 2001 From: Pat Myron Date: Thu, 4 Feb 2021 16:15:13 -0800 Subject: [PATCH] catch common resource schema issues in cfn validate (#675) * catching non-ASCII characters * hardcoded patterns/enums * lowercase properties * incorrect min/max constraints * invalid patterns --- setup.cfg | 2 +- setup.py | 1 + src/rpdk/core/data_loaders.py | 102 +++++++++++++++++- .../valid/invalid_lowercase_property.json | 14 +++ .../schema/valid/invalid_non_ascii_chars.json | 14 +++ .../valid/invalid_property_constraints.json | 17 +++ 6 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 tests/data/schema/valid/invalid_lowercase_property.json create mode 100644 tests/data/schema/valid/invalid_non_ascii_chars.json create mode 100644 tests/data/schema/valid/invalid_property_constraints.json diff --git a/setup.cfg b/setup.cfg index a33bff55..f3de31c4 100644 --- a/setup.cfg +++ b/setup.cfg @@ -29,7 +29,7 @@ include_trailing_comma = true combine_as_imports = True force_grid_wrap = 0 known_first_party = rpdk -known_third_party = boto3,botocore,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml +known_third_party = boto3,botocore,cfnlint,colorama,docker,hypothesis,jinja2,jsonschema,nested_lookup,ordered_set,pkg_resources,pytest,pytest_localserver,setuptools,yaml [tool:pytest] # can't do anything about 3rd part modules, so don't spam us diff --git a/setup.py b/setup.py index 14f94c51..aad7ddd0 100644 --- a/setup.py +++ b/setup.py @@ -52,6 +52,7 @@ def find_version(*file_paths): "docker>=4.3.1", "ordered-set>=4.0.2", "cfn-lint>=0.43.0", + "nested-lookup", ], entry_points={ "console_scripts": ["cfn-cli = rpdk.core.cli:main", "cfn = rpdk.core.cli:main"] diff --git a/src/rpdk/core/data_loaders.py b/src/rpdk/core/data_loaders.py index 8f10e794..12f8eff5 100644 --- a/src/rpdk/core/data_loaders.py +++ b/src/rpdk/core/data_loaders.py @@ -1,6 +1,7 @@ import json import logging import os +import re import shutil from io import TextIOWrapper from pathlib import Path @@ -9,9 +10,12 @@ import yaml from jsonschema import Draft7Validator, RefResolver from jsonschema.exceptions import RefResolutionError, ValidationError +from nested_lookup import nested_lookup from .exceptions import InternalError, SpecValidationError +from .jsonutils.flattener import JsonSchemaFlattener from .jsonutils.inliner import RefInliner +from .jsonutils.utils import FlatteningError LOG = logging.getLogger(__name__) @@ -123,7 +127,7 @@ def get_file_base_uri(file): return path.resolve().as_uri() -def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C901 +def load_resource_spec(resource_spec_file): # pylint: disable=R # noqa: C901 """Load a resource provider definition from a file, and validate it.""" try: resource_spec = json.load(resource_spec_file) @@ -141,6 +145,102 @@ def load_resource_spec(resource_spec_file): # pylint: disable=R0912 # noqa: C90 LOG.debug("Resource spec validation failed", exc_info=True) raise SpecValidationError(str(e)) from e + min_max_keywords = { + "minimum", + "maximum", + "minLength", + "maxLength", + "minProperties", + "maxProperties", + "minItems", + "maxItems", + "exclusiveMinimum", + "exclusiveMaximum", + } + try: # pylint: disable=R + for _key, schema in JsonSchemaFlattener(resource_spec).flatten_schema().items(): + for property_name, property_details in schema.get("properties", {}).items(): + if property_name[0].islower(): + LOG.warning( + "CloudFormation properties don't usually start with lowercase letters: %s", + property_name, + ) + try: + property_type = property_details["type"] + property_keywords = property_details.keys() + for types, allowed_keywords in [ + ( + {"integer", "number"}, + { + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + }, + ), + ( + {"string"}, + { + "minLength", + "maxLength", + }, + ), + ( + {"object"}, + { + "minProperties", + "maxProperties", + }, + ), + ( + {"array"}, + { + "minItems", + "maxItems", + }, + ), + ]: + if ( + property_type in types + and min_max_keywords - allowed_keywords & property_keywords + ): + LOG.warning( + "Incorrect min/max JSON schema keywords for type: %s for property: %s", + property_type, + property_name, + ) + except (KeyError, TypeError): + pass + except FlatteningError: + pass + + for pattern in nested_lookup("pattern", resource_spec): + if "arn:aws:" in pattern: + LOG.warning( + "Don't hardcode the aws partition in ARN patterns: %s", + pattern, + ) + try: + re.compile(pattern) + except re.error: + LOG.warning("Could not validate regular expression: %s", pattern) + + for enum in nested_lookup("enum", resource_spec): + if len(enum) > 15: + LOG.warning( + "Consider not manually maintaining large constantly evolving enums like \ +instance types, lambda runtimes, partitions, regions, availability zones, etc. that get outdated quickly: %s", + enum, + ) + + non_ascii_chars = re.findall( + r"[^ -~]", json.dumps(resource_spec, ensure_ascii=False) + ) + if non_ascii_chars: + LOG.warning( + "non-ASCII characters found in resource schema: %s", non_ascii_chars + ) + list_options = { "maxresults", "maxrecords", diff --git a/tests/data/schema/valid/invalid_lowercase_property.json b/tests/data/schema/valid/invalid_lowercase_property.json new file mode 100644 index 00000000..e2b018a4 --- /dev/null +++ b/tests/data/schema/valid/invalid_lowercase_property.json @@ -0,0 +1,14 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "", + "additionalProperties" : false, + "properties" : { + "property" : { + "type" : "string" + } + }, + "readOnlyProperties": [ + "/properties/property" + ], + "primaryIdentifier" : [ "/properties/property" ] +} diff --git a/tests/data/schema/valid/invalid_non_ascii_chars.json b/tests/data/schema/valid/invalid_non_ascii_chars.json new file mode 100644 index 00000000..109018aa --- /dev/null +++ b/tests/data/schema/valid/invalid_non_ascii_chars.json @@ -0,0 +1,14 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "🤠", + "additionalProperties" : false, + "properties" : { + "Property" : { + "type" : "string" + } + }, + "readOnlyProperties": [ + "/properties/Property" + ], + "primaryIdentifier" : [ "/properties/Property" ] +} diff --git a/tests/data/schema/valid/invalid_property_constraints.json b/tests/data/schema/valid/invalid_property_constraints.json new file mode 100644 index 00000000..786e1681 --- /dev/null +++ b/tests/data/schema/valid/invalid_property_constraints.json @@ -0,0 +1,17 @@ +{ + "typeName" : "AWS::Service::Type", + "description" : "", + "additionalProperties" : false, + "properties" : { + "Property" : { + "type" : "string", + "pattern" : "arn:aws:[A-Za-z0-9]+{1,255}", + "enum" : [ "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z" ], + "minimum" : 0 + } + }, + "readOnlyProperties": [ + "/properties/Property" + ], + "primaryIdentifier" : [ "/properties/Property" ] +}