From 435d8bf601a99624de016a523050d4d49486117f Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Fri, 23 Apr 2021 15:18:45 -0500 Subject: [PATCH 1/7] fix: Fail when Intrinsics are in SourceVPC list instead of IntrinsicsSourceVPC --- samtranslator/swagger/swagger.py | 51 ++++++++++--------- ...rror_api_invalid_source_vpc_blacklist.yaml | 31 +++++++++++ ...rror_api_invalid_source_vpc_whitelist.yaml | 31 +++++++++++ ...rror_api_invalid_source_vpc_blacklist.json | 3 ++ ...rror_api_invalid_source_vpc_whitelist.json | 3 ++ 5 files changed, 95 insertions(+), 24 deletions(-) create mode 100644 tests/translator/input/error_api_invalid_source_vpc_blacklist.yaml create mode 100644 tests/translator/input/error_api_invalid_source_vpc_whitelist.yaml create mode 100644 tests/translator/output/error_api_invalid_source_vpc_blacklist.json create mode 100644 tests/translator/output/error_api_invalid_source_vpc_whitelist.json diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index d6aa36c2f..db87376ba 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -892,6 +892,8 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): ip_range_blacklist = resource_policy.get("IpRangeBlacklist") source_vpc_whitelist = resource_policy.get("SourceVpcWhitelist") source_vpc_blacklist = resource_policy.get("SourceVpcBlacklist") + + # Intrinsic's supported in these properties source_vpc_intrinsic_whitelist = resource_policy.get("IntrinsicVpcWhitelist") source_vpce_intrinsic_whitelist = resource_policy.get("IntrinsicVpceWhitelist") source_vpc_intrinsic_blacklist = resource_policy.get("IntrinsicVpcBlacklist") @@ -913,31 +915,32 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_ip_resource_policy_for_method(ip_range_blacklist, "IpAddress", resource_list) - if ( - (source_vpc_blacklist is not None) - or (source_vpc_intrinsic_blacklist is not None) - or (source_vpce_intrinsic_blacklist is not None) - ): - blacklist_dict = { - "StringEndpointList": source_vpc_blacklist, - "IntrinsicVpcList": source_vpc_intrinsic_blacklist, - "IntrinsicVpceList": source_vpce_intrinsic_blacklist, - } - resource_list = self._get_method_path_uri_list(path, api_id, stage) - self._add_vpc_resource_policy_for_method(blacklist_dict, "StringEquals", resource_list) + # if ( + # (source_vpc_blacklist is not None) + # or (source_vpc_intrinsic_blacklist is not None) + # or (source_vpce_intrinsic_blacklist is not None) + # ): + if source_vpc_blacklist is not None and not all(isinstance(x, string_types) for x in source_vpc_blacklist): + raise InvalidDocumentException([InvalidTemplateException("SourceVpcBlacklist must be a list of strings. Use IntrinsicVpcBlacklist instead for values that use Intrinsic Functions")]) + + blacklist_dict = { + "StringEndpointList": source_vpc_blacklist, + "IntrinsicVpcList": source_vpc_intrinsic_blacklist, + "IntrinsicVpceList": source_vpce_intrinsic_blacklist, + } + resource_list = self._get_method_path_uri_list(path, api_id, stage) + self._add_vpc_resource_policy_for_method(blacklist_dict, "StringEquals", resource_list) - if ( - (source_vpc_whitelist is not None) - or (source_vpc_intrinsic_whitelist is not None) - or (source_vpce_intrinsic_whitelist is not None) - ): - whitelist_dict = { - "StringEndpointList": source_vpc_whitelist, - "IntrinsicVpcList": source_vpc_intrinsic_whitelist, - "IntrinsicVpceList": source_vpce_intrinsic_whitelist, - } - resource_list = self._get_method_path_uri_list(path, api_id, stage) - self._add_vpc_resource_policy_for_method(whitelist_dict, "StringNotEquals", resource_list) + if source_vpc_whitelist is not None and not all(isinstance(x, string_types) for x in source_vpc_whitelist): + raise InvalidDocumentException([InvalidTemplateException("SourceVpcWhitelist must be a list of strings. Use IntrinsicVpcWhitelist instead for values that use Intrinsic Functions")]) + + whitelist_dict = { + "StringEndpointList": source_vpc_whitelist, + "IntrinsicVpcList": source_vpc_intrinsic_whitelist, + "IntrinsicVpceList": source_vpce_intrinsic_whitelist, + } + resource_list = self._get_method_path_uri_list(path, api_id, stage) + self._add_vpc_resource_policy_for_method(whitelist_dict, "StringNotEquals", resource_list) self._doc[self._X_APIGW_POLICY] = self.resource_policy diff --git a/tests/translator/input/error_api_invalid_source_vpc_blacklist.yaml b/tests/translator/input/error_api_invalid_source_vpc_blacklist.yaml new file mode 100644 index 000000000..27eadd1fe --- /dev/null +++ b/tests/translator/input/error_api_invalid_source_vpc_blacklist.yaml @@ -0,0 +1,31 @@ +Globals: + Api: + Auth: + ResourcePolicy: + SourceVpcBlacklist: [{"Ref":"SomeParameter"}] + +Parameters: + SomeParameter: + Type: String + Default: param + +Resources: + MyFunction: + Type: AWS::Serverless::Function + Properties: + InlineCode: | + exports.handler = async (event) => { + const response = { + statusCode: 200, + body: JSON.stringify('Hello from Lambda!'), + }; + return response; + }; + Handler: index.handler + Runtime: nodejs12.x + Events: + Api: + Type: Api + Properties: + Method: Put + Path: /get \ No newline at end of file diff --git a/tests/translator/input/error_api_invalid_source_vpc_whitelist.yaml b/tests/translator/input/error_api_invalid_source_vpc_whitelist.yaml new file mode 100644 index 000000000..99ba49f2d --- /dev/null +++ b/tests/translator/input/error_api_invalid_source_vpc_whitelist.yaml @@ -0,0 +1,31 @@ +Globals: + Api: + Auth: + ResourcePolicy: + SourceVpcWhitelist: [{"Ref":"SomeParameter"}] + +Parameters: + SomeParameter: + Type: String + Default: param + +Resources: + MyFunction: + Type: AWS::Serverless::Function + Properties: + InlineCode: | + exports.handler = async (event) => { + const response = { + statusCode: 200, + body: JSON.stringify('Hello from Lambda!'), + }; + return response; + }; + Handler: index.handler + Runtime: nodejs12.x + Events: + Api: + Type: Api + Properties: + Method: Put + Path: /get \ No newline at end of file diff --git a/tests/translator/output/error_api_invalid_source_vpc_blacklist.json b/tests/translator/output/error_api_invalid_source_vpc_blacklist.json new file mode 100644 index 000000000..cc7d2188a --- /dev/null +++ b/tests/translator/output/error_api_invalid_source_vpc_blacklist.json @@ -0,0 +1,3 @@ +{ + "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. SourceVpcBlacklist must be a list of strings. Use IntrinsicVpcBlacklist instead for values that use Intrinsic Functions" +} diff --git a/tests/translator/output/error_api_invalid_source_vpc_whitelist.json b/tests/translator/output/error_api_invalid_source_vpc_whitelist.json new file mode 100644 index 000000000..5ce6cf3fd --- /dev/null +++ b/tests/translator/output/error_api_invalid_source_vpc_whitelist.json @@ -0,0 +1,3 @@ +{ + "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. SourceVpcWhitelist must be a list of strings. Use IntrinsicVpcWhitelist instead for values that use Intrinsic Functions" +} From 7a5fc1df48f39d7cfa651dfc1d02085f4f30fa94 Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Fri, 23 Apr 2021 15:24:00 -0500 Subject: [PATCH 2/7] remove commented out code --- samtranslator/swagger/swagger.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index db87376ba..be1931644 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -915,11 +915,6 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_ip_resource_policy_for_method(ip_range_blacklist, "IpAddress", resource_list) - # if ( - # (source_vpc_blacklist is not None) - # or (source_vpc_intrinsic_blacklist is not None) - # or (source_vpce_intrinsic_blacklist is not None) - # ): if source_vpc_blacklist is not None and not all(isinstance(x, string_types) for x in source_vpc_blacklist): raise InvalidDocumentException([InvalidTemplateException("SourceVpcBlacklist must be a list of strings. Use IntrinsicVpcBlacklist instead for values that use Intrinsic Functions")]) From 9628882f1b5667fd0f91648d0db7431a683ef986 Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Fri, 23 Apr 2021 15:33:20 -0500 Subject: [PATCH 3/7] remove duplicated call --- samtranslator/swagger/swagger.py | 1 - 1 file changed, 1 deletion(-) diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index be1931644..53f824ea3 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -934,7 +934,6 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): "IntrinsicVpcList": source_vpc_intrinsic_whitelist, "IntrinsicVpceList": source_vpce_intrinsic_whitelist, } - resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_vpc_resource_policy_for_method(whitelist_dict, "StringNotEquals", resource_list) self._doc[self._X_APIGW_POLICY] = self.resource_policy From 9fd307733348fc6a879e1046b9d6d0a9ce130dee Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Fri, 23 Apr 2021 15:52:05 -0500 Subject: [PATCH 4/7] run black --- samtranslator/swagger/swagger.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index 53f824ea3..0618ef362 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -916,7 +916,13 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): self._add_ip_resource_policy_for_method(ip_range_blacklist, "IpAddress", resource_list) if source_vpc_blacklist is not None and not all(isinstance(x, string_types) for x in source_vpc_blacklist): - raise InvalidDocumentException([InvalidTemplateException("SourceVpcBlacklist must be a list of strings. Use IntrinsicVpcBlacklist instead for values that use Intrinsic Functions")]) + raise InvalidDocumentException( + [ + InvalidTemplateException( + "SourceVpcBlacklist must be a list of strings. Use IntrinsicVpcBlacklist instead for values that use Intrinsic Functions" + ) + ] + ) blacklist_dict = { "StringEndpointList": source_vpc_blacklist, @@ -927,7 +933,13 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): self._add_vpc_resource_policy_for_method(blacklist_dict, "StringEquals", resource_list) if source_vpc_whitelist is not None and not all(isinstance(x, string_types) for x in source_vpc_whitelist): - raise InvalidDocumentException([InvalidTemplateException("SourceVpcWhitelist must be a list of strings. Use IntrinsicVpcWhitelist instead for values that use Intrinsic Functions")]) + raise InvalidDocumentException( + [ + InvalidTemplateException( + "SourceVpcWhitelist must be a list of strings. Use IntrinsicVpcWhitelist instead for values that use Intrinsic Functions" + ) + ] + ) whitelist_dict = { "StringEndpointList": source_vpc_whitelist, From cbc060b42ed0e0e91985b9bedaf7abfd9c4f5a46 Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Mon, 10 May 2021 09:45:35 -0500 Subject: [PATCH 5/7] Refactor to common method and add more tests --- samtranslator/swagger/swagger.py | 17 +++++++++++++++-- tests/swagger/test_swagger.py | 14 ++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index 0618ef362..87ddf901c 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -915,7 +915,7 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_ip_resource_policy_for_method(ip_range_blacklist, "IpAddress", resource_list) - if source_vpc_blacklist is not None and not all(isinstance(x, string_types) for x in source_vpc_blacklist): + if not self._validate_list_property_is_resolved(source_vpc_blacklist): raise InvalidDocumentException( [ InvalidTemplateException( @@ -932,7 +932,7 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_vpc_resource_policy_for_method(blacklist_dict, "StringEquals", resource_list) - if source_vpc_whitelist is not None and not all(isinstance(x, string_types) for x in source_vpc_whitelist): + if not self._validate_list_property_is_resolved(source_vpc_whitelist): raise InvalidDocumentException( [ InvalidTemplateException( @@ -950,6 +950,19 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): self._doc[self._X_APIGW_POLICY] = self.resource_policy + def _validate_list_property_is_resolved(self, property_list): + """ + + :param property_list: + :return: + """ + + if property_list is not None and not all(isinstance(x, string_types) for x in property_list): + return False + + return True + + def add_custom_statements(self, custom_statements): self._add_custom_statement(custom_statements) diff --git a/tests/swagger/test_swagger.py b/tests/swagger/test_swagger.py index fa3f8e580..c95dc2bd1 100644 --- a/tests/swagger/test_swagger.py +++ b/tests/swagger/test_swagger.py @@ -1181,6 +1181,20 @@ def test_must_add_vpc_allow_string_only(self): self.assertEqual(deep_sort_lists(expected), deep_sort_lists(self.editor.swagger[_X_POLICY])) + @parameterized.expand( + [ + param("SourceVpcWhitelist"), + param("SourceVpcBlacklist"), + ] + ) + def test_must_fail_when_vpc_whitelist_is_non_string(self, resource_policy_key): + resource_policy = { + resource_policy_key: [{"sub": "somevalue"}] + } + + with self.assertRaises(InvalidDocumentException): + self.editor.add_resource_policy(resource_policy, "/foo", "123", "prod") + def test_must_add_vpc_deny_string_only(self): resourcePolicy = { From 8db75eb9ad5754b0dabc0070012a6a1b4a0e3993 Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Mon, 10 May 2021 09:48:33 -0500 Subject: [PATCH 6/7] Refactored to staticmethod and add doc strings --- samtranslator/swagger/swagger.py | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/samtranslator/swagger/swagger.py b/samtranslator/swagger/swagger.py index 87ddf901c..36c9f5d18 100644 --- a/samtranslator/swagger/swagger.py +++ b/samtranslator/swagger/swagger.py @@ -915,7 +915,7 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_ip_resource_policy_for_method(ip_range_blacklist, "IpAddress", resource_list) - if not self._validate_list_property_is_resolved(source_vpc_blacklist): + if not SwaggerEditor._validate_list_property_is_resolved(source_vpc_blacklist): raise InvalidDocumentException( [ InvalidTemplateException( @@ -932,7 +932,7 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): resource_list = self._get_method_path_uri_list(path, api_id, stage) self._add_vpc_resource_policy_for_method(blacklist_dict, "StringEquals", resource_list) - if not self._validate_list_property_is_resolved(source_vpc_whitelist): + if not SwaggerEditor._validate_list_property_is_resolved(source_vpc_whitelist): raise InvalidDocumentException( [ InvalidTemplateException( @@ -950,19 +950,6 @@ def add_resource_policy(self, resource_policy, path, api_id, stage): self._doc[self._X_APIGW_POLICY] = self.resource_policy - def _validate_list_property_is_resolved(self, property_list): - """ - - :param property_list: - :return: - """ - - if property_list is not None and not all(isinstance(x, string_types) for x in property_list): - return False - - return True - - def add_custom_statements(self, custom_statements): self._add_custom_statement(custom_statements) @@ -1290,3 +1277,17 @@ def safe_compare_regex_with_string(regex, data): def get_path_without_trailing_slash(path): # convert greedy paths to such as {greedy+}, {proxy+} to "*" return re.sub(r"{([a-zA-Z0-9._-]+|[a-zA-Z0-9._-]+\+|proxy\+)}", "*", path) + + @staticmethod + def _validate_list_property_is_resolved(property_list): + """ + Validate if the values of a Property List are all of type string + + :param property_list: Value of a Property List + :return bool: True if the property_list is all of type string otherwise False + """ + + if property_list is not None and not all(isinstance(x, string_types) for x in property_list): + return False + + return True From 8ba5ef878437dc40e3a037b5a4df815b739e70cb Mon Sep 17 00:00:00 2001 From: Jacob Fuss Date: Mon, 10 May 2021 09:49:00 -0500 Subject: [PATCH 7/7] run make black --- tests/swagger/test_swagger.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/swagger/test_swagger.py b/tests/swagger/test_swagger.py index c95dc2bd1..e6381d526 100644 --- a/tests/swagger/test_swagger.py +++ b/tests/swagger/test_swagger.py @@ -1188,9 +1188,7 @@ def test_must_add_vpc_allow_string_only(self): ] ) def test_must_fail_when_vpc_whitelist_is_non_string(self, resource_policy_key): - resource_policy = { - resource_policy_key: [{"sub": "somevalue"}] - } + resource_policy = {resource_policy_key: [{"sub": "somevalue"}]} with self.assertRaises(InvalidDocumentException): self.editor.add_resource_policy(resource_policy, "/foo", "123", "prod")