fix: bug fixes in api resource policies (#1395)

Author: praneetap

examples/2016-10-31/api_resource_policy/template.yaml

@@ -1,22 +1,33 @@
 AWSTemplateFormatVersion: '2010-09-09'
 Transform: AWS::Serverless-2016-10-31
+Conditions:
+  C1:
+    Fn::Equals:
+      - true
+      - true
 Globals:
   Api:
     Auth:
       ResourcePolicy:
-        CustomStatements: [{
-            "Effect": "Allow",
-            "Principal": "*",
-            "Action": "execute-api:Invoke",
-            "Resource": "execute-api:/Prod/PUT/get",
-            "Condition": {
-                "IpAddress": {
-                    "aws:SourceIp": "1.2.3.4"
-                }
-            }
-        }]
-        # OR you can use the following, they both do the same thing
-        IpRangeBlacklist: ['1.2.3.4']
+        CustomStatements:
+          Fn::If:
+            - C1
+            - Principal: '*'
+              Action: execute-api:Invoke
+              Resource: 
+                - execute-api:/Prod/PUT/get
+              Condition: 
+                IpAddress:
+                  aws:SourceIp: 1.2.3.4
+            - Principal: '*'
+              Action: execute-api:Invoke
+              Resource: 
+                - execute-api:/Prod/PUT/get
+              Condition: 
+                IpAddress:
+                  aws:SourceIp: 5.6.7.8
+        # OR you can use the following
+        # IpRangeBlacklist: ['1.2.3.4']
 Resources:
   MyFunction:
     Type: AWS::Serverless::Function

samtranslator/model/api/api_generator.py

@@ -535,6 +535,8 @@ def _add_auth(self):
                 swagger_editor.add_resource_policy(
                     auth_properties.ResourcePolicy, path, self.logical_id, self.stage_name
                 )
+            if auth_properties.ResourcePolicy.get("CustomStatements"):
+                swagger_editor.add_custom_statements(auth_properties.ResourcePolicy.get("CustomStatements"))
 
         self.definition_body = self._openapi_postprocess(swagger_editor.swagger)
 

samtranslator/model/eventsources/push.py

@@ -545,6 +545,7 @@ def to_cloudformation(self, **kwargs):
         resources = []
 
         function = kwargs.get("function")
+        intrinsics_resolver = kwargs.get("intrinsics_resolver")
 
         if not function:
             raise TypeError("Missing required keyword argument: function")
@@ -557,7 +558,7 @@ def to_cloudformation(self, **kwargs):
 
         explicit_api = kwargs["explicit_api"]
         if explicit_api.get("__MANAGE_SWAGGER"):
-            self._add_swagger_integration(explicit_api, function)
+            self._add_swagger_integration(explicit_api, function, intrinsics_resolver)
 
         return resources
 
@@ -600,7 +601,7 @@ def _get_permission(self, resources_to_link, stage, suffix):
 
         return self._construct_permission(resources_to_link["function"], source_arn=source_arn, suffix=suffix)
 
-    def _add_swagger_integration(self, api, function):
+    def _add_swagger_integration(self, api, function, intrinsics_resolver):
         """Adds the path and method for this Api event source to the Swagger body for the provided RestApi.
 
         :param model.apigateway.ApiGatewayRestApi rest_api: the RestApi to which the path and method should be added.
@@ -639,6 +640,7 @@ def _add_swagger_integration(self, api, function):
         if self.Auth:
             method_authorizer = self.Auth.get("Authorizer")
             api_auth = api.get("Auth")
+            api_auth = intrinsics_resolver.resolve_parameter_refs(api_auth)
 
             if method_authorizer:
                 api_authorizers = api_auth and api_auth.get("Authorizers")

samtranslator/model/sam_resources.py

@@ -168,7 +168,11 @@ def to_cloudformation(self, **kwargs):
 
         try:
             resources += self._generate_event_resources(
-                lambda_function, execution_role, kwargs["event_resources"], lambda_alias=lambda_alias
+                lambda_function,
+                execution_role,
+                kwargs["event_resources"],
+                intrinsics_resolver,
+                lambda_alias=lambda_alias,
             )
         except InvalidEventException as e:
             raise InvalidResourceException(self.logical_id, e.message)
@@ -563,7 +567,9 @@ def order_events(event):
             return logical_id
         return event_dict.get("Properties", {}).get("Path", logical_id)
 
-    def _generate_event_resources(self, lambda_function, execution_role, event_resources, lambda_alias=None):
+    def _generate_event_resources(
+        self, lambda_function, execution_role, event_resources, intrinsics_resolver, lambda_alias=None
+    ):
         """Generates and returns the resources associated with this function's events.
 
         :param model.lambda_.LambdaFunction lambda_function: generated Lambda function
@@ -591,6 +597,7 @@ def _generate_event_resources(self, lambda_function, execution_role, event_resou
                     # When Alias is provided, connect all event sources to the alias and *not* the function
                     "function": lambda_alias or lambda_function,
                     "role": execution_role,
+                    "intrinsics_resolver": intrinsics_resolver,
                 }
 
                 for name, resource in event_resources[logical_id].items():
@@ -773,7 +780,6 @@ def to_cloudformation(self, **kwargs):
         self.BinaryMediaTypes = intrinsics_resolver.resolve_parameter_refs(self.BinaryMediaTypes)
         self.Domain = intrinsics_resolver.resolve_parameter_refs(self.Domain)
         self.Auth = intrinsics_resolver.resolve_parameter_refs(self.Auth)
-
         redeploy_restapi_parameters = kwargs.get("redeploy_restapi_parameters")
 
         api_generator = ApiGenerator(

samtranslator/swagger/swagger.py

@@ -3,8 +3,8 @@
 import re
 from six import string_types
 
-from samtranslator.model.intrinsics import ref
-from samtranslator.model.intrinsics import make_conditional, fnSub
+from samtranslator.model.intrinsics import ref, is_intrinsic_no_value
+from samtranslator.model.intrinsics import make_conditional, fnSub, is_intrinsic_if
 from samtranslator.model.exceptions import InvalidDocumentException, InvalidTemplateException
 
 
@@ -804,7 +804,6 @@ 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")
-        custom_statements = resource_policy.get("CustomStatements")
 
         if aws_account_whitelist is not None:
             resource_list = self._get_method_path_uri_list(path, api_id, stage)
@@ -824,16 +823,16 @@ def add_resource_policy(self, resource_policy, path, api_id, stage):
 
         if source_vpc_whitelist is not None:
             resource_list = self._get_method_path_uri_list(path, api_id, stage)
-            for endpoint in source_vpc_whitelist:
-                self._add_vpc_resource_policy_for_method(endpoint, "StringNotEquals", resource_list)
+            self._add_vpc_resource_policy_for_method(source_vpc_whitelist, "StringNotEquals", resource_list)
 
         if source_vpc_blacklist is not None:
             resource_list = self._get_method_path_uri_list(path, api_id, stage)
-            for endpoint in source_vpc_blacklist:
-                self._add_vpc_resource_policy_for_method(endpoint, "StringEquals", resource_list)
+            self._add_vpc_resource_policy_for_method(source_vpc_blacklist, "StringEquals", resource_list)
 
-        if custom_statements is not None:
-            self._add_custom_statement(custom_statements)
+        self._doc[self._X_APIGW_POLICY] = self.resource_policy
+
+    def add_custom_statements(self, custom_statements):
+        self._add_custom_statement(custom_statements)
 
         self._doc[self._X_APIGW_POLICY] = self.resource_policy
 
@@ -932,24 +931,33 @@ def _add_ip_resource_policy_for_method(self, ip_list, conditional, resource_list
                 statement.extend([deny_statement])
             self.resource_policy["Statement"] = statement
 
-    def _add_vpc_resource_policy_for_method(self, vpc, conditional, resource_list):
+    def _add_vpc_resource_policy_for_method(self, endpoint_list, conditional, resource_list):
         """
         This method generates a policy statement to grant/deny specific VPC/VPCE access to the API method and
         appends it to the swagger under `x-amazon-apigateway-policy`
         :raises ValueError: If the conditional passed in does not match the allowed values.
         """
-        if not vpc:
+        if not endpoint_list:
             return
 
         if conditional not in ["StringNotEquals", "StringEquals"]:
             raise ValueError("Conditional must be one of {}".format(["StringNotEquals", "StringEquals"]))
 
         vpce_regex = r"^vpce-"
-        if not re.match(vpce_regex, vpc):
-            endpoint = "aws:SourceVpc"
-        else:
-            endpoint = "aws:SourceVpce"
-
+        vpc_regex = r"^vpc-"
+        vpc_list = []
+        vpce_list = []
+        for endpoint in endpoint_list:
+            if re.match(vpce_regex, endpoint):
+                vpce_list.append(endpoint)
+            if re.match(vpc_regex, endpoint):
+                vpc_list.append(endpoint)
+
+        condition = {}
+        if vpc_list:
+            condition["aws:SourceVpc"] = vpc_list
+        if vpce_list:
+            condition["aws:SourceVpce"] = vpce_list
         self.resource_policy["Version"] = "2012-10-17"
         allow_statement = {}
         allow_statement["Effect"] = "Allow"
@@ -962,7 +970,7 @@ def _add_vpc_resource_policy_for_method(self, vpc, conditional, resource_list):
         deny_statement["Action"] = "execute-api:Invoke"
         deny_statement["Resource"] = resource_list
         deny_statement["Principal"] = "*"
-        deny_statement["Condition"] = {conditional: {endpoint: vpc}}
+        deny_statement["Condition"] = {conditional: condition}
 
         if self.resource_policy.get("Statement") is None:
             self.resource_policy["Statement"] = [allow_statement, deny_statement]
@@ -980,16 +988,17 @@ def _add_custom_statement(self, custom_statements):
         if custom_statements is None:
             return
 
-        if not isinstance(custom_statements, list):
-            custom_statements = [custom_statements]
-
         self.resource_policy["Version"] = "2012-10-17"
         if self.resource_policy.get("Statement") is None:
             self.resource_policy["Statement"] = custom_statements
         else:
+            if not isinstance(custom_statements, list):
+                custom_statements = [custom_statements]
+
             statement = self.resource_policy["Statement"]
             if not isinstance(statement, list):
                 statement = [statement]
+
             for s in custom_statements:
                 if s not in statement:
                     statement.append(s)

tests/swagger/test_swagger.py

@@ -995,7 +995,7 @@ def test_must_add_custom_statements(self):
             ]
         }
 
-        self.editor.add_resource_policy(resourcePolicy, "/foo", "123", "prod")
+        self.editor.add_custom_statements(resourcePolicy.get("CustomStatements"))
 
         expected = {
             "Version": "2012-10-17",
@@ -1007,6 +1007,33 @@ def test_must_add_custom_statements(self):
 
         self.assertEqual(deep_sort_lists(expected), deep_sort_lists(self.editor.swagger[_X_POLICY]))
 
+    def test_must_add_fn_if_custom_statements(self):
+
+        resourcePolicy = {
+            "CustomStatements": {
+                "Fn::If": [
+                    "condition",
+                    {"Action": "execute-api:Invoke", "Resource": ["execute-api:/*/*/*"]},
+                    {"Action": "execute-api:blah", "Resource": ["execute-api:/*/*/*"]},
+                ],
+            }
+        }
+
+        self.editor.add_custom_statements(resourcePolicy.get("CustomStatements"))
+
+        expected = {
+            "Version": "2012-10-17",
+            "Statement": {
+                "Fn::If": [
+                    "condition",
+                    {"Action": "execute-api:Invoke", "Resource": ["execute-api:/*/*/*"]},
+                    {"Action": "execute-api:blah", "Resource": ["execute-api:/*/*/*"]},
+                ]
+            },
+        }
+
+        self.assertEqual(deep_sort_lists(expected), deep_sort_lists(self.editor.swagger[_X_POLICY]))
+
     def test_must_add_iam_allow(self):
         ## fails
         resourcePolicy = {"AwsAccountWhitelist": ["123456"]}
@@ -1140,17 +1167,7 @@ def test_must_add_vpc_allow(self):
                         {"Fn::Sub": ["execute-api:/${__Stage__}/GET/foo", {"__Stage__": "prod"}]},
                     ],
                     "Effect": "Deny",
-                    "Condition": {"StringNotEquals": {"aws:SourceVpc": "vpc-123"}},
-                    "Principal": "*",
-                },
-                {
-                    "Action": "execute-api:Invoke",
-                    "Resource": [
-                        {"Fn::Sub": ["execute-api:/${__Stage__}/PUT/foo", {"__Stage__": "prod"}]},
-                        {"Fn::Sub": ["execute-api:/${__Stage__}/GET/foo", {"__Stage__": "prod"}]},
-                    ],
-                    "Effect": "Deny",
-                    "Condition": {"StringNotEquals": {"aws:SourceVpce": "vpce-345"}},
+                    "Condition": {"StringNotEquals": {"aws:SourceVpc": ["vpc-123"], "aws:SourceVpce": ["vpce-345"]}},
                     "Principal": "*",
                 },
             ],
@@ -1183,7 +1200,7 @@ def test_must_add_vpc_deny(self):
                         {"Fn::Sub": ["execute-api:/${__Stage__}/GET/foo", {"__Stage__": "prod"}]},
                     ],
                     "Effect": "Deny",
-                    "Condition": {"StringEquals": {"aws:SourceVpc": "vpc-123"}},
+                    "Condition": {"StringEquals": {"aws:SourceVpc": ["vpc-123"]}},
                     "Principal": "*",
                 },
             ],
@@ -1201,6 +1218,7 @@ def test_must_add_iam_allow_and_custom(self):
         }
 
         self.editor.add_resource_policy(resourcePolicy, "/foo", "123", "prod")
+        self.editor.add_custom_statements(resourcePolicy.get("CustomStatements"))
 
         expected = {
             "Version": "2012-10-17",

tests/translator/input/api_with_if_conditional_with_resource_policy.yaml

@@ -0,0 +1,54 @@
+Conditions:
+  C1:
+    Fn::Equals:
+      - true
+      - true
+Resources:
+  ExplicitApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      Auth:
+        ResourcePolicy:
+          CustomStatements: 
+            Fn::If: [
+              C1,
+              {
+                Action: 'execute-api:Invoke', 
+                Resource: ['execute-api:/*/*/*']
+              },
+              {
+                Ref: 'AWS::NoValue'
+              },
+            ]
+
+  ExplicitApiFunction:
+    Type: AWS::Serverless::Function
+    Properties:
+      CodeUri: s3://sam-demo-bucket/member_portal.zip
+      Handler: index.gethtml
+      Runtime: nodejs12.x
+      Events:
+        GetHtml:
+          Type: Api
+          Properties:
+            RestApiId:
+              Ref: ExplicitApi
+            Path: /one
+            Method: get
+        PostHtml:
+          Type: Api
+          Properties:
+            RestApiId:
+              Ref: ExplicitApi
+            Path: /two
+            Method: post
+        PutHtml:
+          Type: Api
+          Properties:
+            RestApiId:
+              Ref: ExplicitApi
+            Path: /three
+            Method: put
+
+