feat: actionable error on non-ARN policy

samtranslator/model/role_utils/role_constructor.py

@@ -13,7 +13,7 @@ def _get_managed_policy_arn(
     name: str,
     managed_policy_map: Optional[Dict[str, str]],
     get_managed_policy_map: Optional[GetManagedPolicyMap],
-) -> str:
+) -> Optional[str]:
     """
     Get the ARN of a AWS managed policy name. Used in Policies property of
     AWS::Serverless::Function and AWS::Serverless::StateMachine.
@@ -57,7 +57,7 @@ def _get_managed_policy_arn(
             if arn:
                 return arn
 
-    return name
+    return None
 
 
 def construct_role_for_resource(  # type: ignore[no-untyped-def] # noqa: too-many-arguments
@@ -146,6 +146,11 @@ def construct_role_for_resource(  # type: ignore[no-untyped-def] # noqa: too-man
                     managed_policy_map,
                     get_managed_policy_map,
                 )
+                if not policy_arn:
+                    raise InvalidResourceException(
+                        resource_logical_id,
+                        f"Invalid policy '{policy_entry.data}'; make sure the value is a valid AWS managed policy ARN.",
+                    )
 
             # De-Duplicate managed policy arns before inserting. Mainly useful
             # when customer specifies a managed policy which is already inserted

tests/translator/input/error_managed_policies_function.yaml

@@ -0,0 +1,9 @@
+Resources:
+  MyFunction:
+    Type: AWS::Serverless::Function
+    Properties:
+      Runtime: python3.8
+      Handler: foo
+      InlineCode: bar
+      Policies:
+      - AnyNonOfficialManagedPolicy

tests/translator/input/error_managed_policies_state_machine.yaml

@@ -0,0 +1,7 @@
+Resources:
+  MyStateMachine:
+    Type: AWS::Serverless::StateMachine
+    Properties:
+      DefinitionUri: s3://foo/bar
+      Policies:
+      - AnyNonOfficialManagedPolicy

tests/translator/input/managed_policies_minimal.yaml

@@ -6,13 +6,19 @@ Resources:
       Handler: foo
       InlineCode: bar
       Policies:
-      - AnyNonOfficialManagedPolicy
+      - arn:aws:iam::aws:policy/AmazonSQSFullAccess
+      - arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess
+      - arn:looks:like::an/arn
       - AmazonS3FullAccess
+      - AmazonAPIGatewayPushToCloudWatchLogs
 
   MyStateMachine:
     Type: AWS::Serverless::StateMachine
     Properties:
       DefinitionUri: s3://foo/bar
       Policies:
-      - AnyNonOfficialManagedPolicy
+      - arn:aws:iam::aws:policy/AmazonSQSFullAccess
+      - arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess
+      - arn:looks:like::an/arn
       - AmazonS3FullAccess
+      - AmazonAPIGatewayPushToCloudWatchLogs

tests/translator/output/aws-cn/managed_policies_minimal.json

@@ -42,8 +42,11 @@
         },
         "ManagedPolicyArns": [
           "arn:aws-cn:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole",
-          "AnyNonOfficialManagedPolicy",
-          "arn:aws-cn:iam::aws:policy/AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws-cn:iam::aws:policy/AmazonS3FullAccess",
+          "arn:aws-cn:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs"
         ],
         "Tags": [
           {
@@ -94,8 +97,11 @@
           "Version": "2012-10-17"
         },
         "ManagedPolicyArns": [
-          "AnyNonOfficialManagedPolicy",
-          "arn:aws-cn:iam::aws:policy/AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws-cn:iam::aws:policy/AmazonS3FullAccess",
+          "arn:aws-cn:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs"
         ],
         "Tags": [
           {

tests/translator/output/aws-us-gov/managed_policies_minimal.json

@@ -42,8 +42,11 @@
         },
         "ManagedPolicyArns": [
           "arn:aws-us-gov:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole",
-          "AnyNonOfficialManagedPolicy",
-          "AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws-us-gov:iam::aws:policy/AmazonS3FullAccess-mock-from-fallback-policy-loader",
+          "arn:aws-us-gov:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs-mock-from-fallback-policy-loader"
         ],
         "Tags": [
           {
@@ -94,8 +97,11 @@
           "Version": "2012-10-17"
         },
         "ManagedPolicyArns": [
-          "AnyNonOfficialManagedPolicy",
-          "AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws-us-gov:iam::aws:policy/AmazonS3FullAccess-mock-from-fallback-policy-loader",
+          "arn:aws-us-gov:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs-mock-from-fallback-policy-loader"
         ],
         "Tags": [
           {

tests/translator/output/aws-us-gov/schema_validation_1.json

@@ -60,7 +60,7 @@
           "Version": "2012-10-17"
         },
         "ManagedPolicyArns": [
-          "AWSXrayWriteOnlyAccess",
+          "arn:aws-us-gov:iam::aws:policy/AWSXrayWriteOnlyAccess-mock-from-fallback-policy-loader",
           "arn:aws-us-gov:iam::aws:policy/AWSXRayDaemonWriteAccess",
           "arn:aws-us-gov:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole"
         ],
@@ -170,7 +170,7 @@
           "Version": "2012-10-17"
         },
         "ManagedPolicyArns": [
-          "AWSXrayWriteOnlyAccess",
+          "arn:aws-us-gov:iam::aws:policy/AWSXrayWriteOnlyAccess-mock-from-fallback-policy-loader",
           "arn:aws-us-gov:iam::aws:policy/AWSXRayDaemonWriteAccess"
         ],
         "Policies": [

tests/translator/output/error_managed_policies_function.json

@@ -0,0 +1,9 @@
+{
+  "_autoGeneratedBreakdownErrorMessage": [
+    "Invalid Serverless Application Specification document. ",
+    "Number of errors found: 1. ",
+    "Resource with id [MyFunction] is invalid. ",
+    "Invalid policy 'AnyNonOfficialManagedPolicy'; make sure the value is a valid AWS managed policy ARN."
+  ],
+  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [MyFunction] is invalid. Invalid policy 'AnyNonOfficialManagedPolicy'; make sure the value is a valid AWS managed policy ARN."
+}

tests/translator/output/error_managed_policies_state_machine.json

@@ -0,0 +1,9 @@
+{
+  "_autoGeneratedBreakdownErrorMessage": [
+    "Invalid Serverless Application Specification document. ",
+    "Number of errors found: 1. ",
+    "Resource with id [MyStateMachine] is invalid. ",
+    "Invalid policy 'AnyNonOfficialManagedPolicy'; make sure the value is a valid AWS managed policy ARN."
+  ],
+  "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 1. Resource with id [MyStateMachine] is invalid. Invalid policy 'AnyNonOfficialManagedPolicy'; make sure the value is a valid AWS managed policy ARN."
+}

tests/translator/output/managed_policies_minimal.json

@@ -42,8 +42,11 @@
         },
         "ManagedPolicyArns": [
           "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole",
-          "AnyNonOfficialManagedPolicy",
-          "arn:aws:iam::aws:policy/AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws:iam::aws:policy/AmazonS3FullAccess",
+          "arn:aws:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs"
         ],
         "Tags": [
           {
@@ -94,8 +97,11 @@
           "Version": "2012-10-17"
         },
         "ManagedPolicyArns": [
-          "AnyNonOfficialManagedPolicy",
-          "arn:aws:iam::aws:policy/AmazonS3FullAccess"
+          "arn:aws:iam::aws:policy/AmazonSQSFullAccess",
+          "arn:aws-cn:iam::aws:policy/AmazonRedshiftFullAccess",
+          "arn:looks:like::an/arn",
+          "arn:aws:iam::aws:policy/AmazonS3FullAccess",
+          "arn:aws:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs"
         ],
         "Tags": [
           {

tests/translator/test_translator.py

@@ -175,6 +175,21 @@ def _compare_transform(self, manifest, expected, partition, region):
                     "AWSXRayDaemonWriteAccess"
                 ] = f"arn:{partition}:iam::aws:policy/AWSXRayDaemonWriteAccess"
 
+            # For the managed_policies_minimal.yaml transform test.
+            # For aws and aws-cn, these policies are cached (see https://github.com/aws/serverless-application-model/pull/2839),
+            # however we don't bundle managed policies in aws-us-gov, so instead we simulate passing the
+            # fallback policy loader
+            if partition == "aws-us-gov":
+                mock_policy_loader.load.return_value[
+                    "AmazonS3FullAccess"
+                ] = "arn:aws-us-gov:iam::aws:policy/AmazonS3FullAccess-mock-from-fallback-policy-loader"
+                mock_policy_loader.load.return_value[
+                    "AWSXrayWriteOnlyAccess"
+                ] = "arn:aws-us-gov:iam::aws:policy/AWSXrayWriteOnlyAccess-mock-from-fallback-policy-loader"
+                mock_policy_loader.load.return_value[
+                    "AmazonAPIGatewayPushToCloudWatchLogs"
+                ] = "arn:aws-us-gov:iam::aws:policy/service-role/AmazonAPIGatewayPushToCloudWatchLogs-mock-from-fallback-policy-loader"
+
             output_fragment = transform(manifest, parameter_values, mock_policy_loader)
 
         print(json.dumps(output_fragment, indent=2))
@@ -485,6 +500,7 @@ def test_transform_invalid_document(testcase):
     expected = json.load(open(os.path.join(OUTPUT_FOLDER, testcase + ".json")))
 
     mock_policy_loader = MagicMock()
+    mock_policy_loader.load.return_value = None
     parameter_values = get_template_parameter_values()
 
     with pytest.raises(InvalidDocumentException) as e:
@@ -759,7 +775,6 @@ class TestTemplateValidation(TestCase):
                     "InlineCode": "bar",
                     "Policies": [
                         "foo",
-                        "bar",
                     ],
                 },
             },
@@ -769,7 +784,6 @@ class TestTemplateValidation(TestCase):
                     "DefinitionUri": "s3://foo/bar",
                     "Policies": [
                         "foo",
-                        "bar",
                     ],
                 },
             },
@@ -786,7 +800,6 @@ class TestTemplateValidation(TestCase):
             (None, None, {"foo": "a3"}, "a3"),
             (None, {"foo": "a2"}, None, "a2"),
             ({"foo": "a1"}, None, None, "a1"),
-            (None, None, None, "foo"),
         ]
     )
     @patch("boto3.session.Session.region_name", "ap-southeast-1")
@@ -827,6 +840,35 @@ def get_managed_policy_map():
         self.assertEqual(function_arn, expected_arn)
         self.assertEqual(sfn_arn, expected_arn)
 
+    @parameterized.expand(
+        [
+            (None, None, None, "foo"),
+        ]
+    )
+    @patch("boto3.session.Session.region_name", "ap-southeast-1")
+    @patch("botocore.client.ClientEndpointBridge._check_default_region", mock_get_region)
+    def test_managed_policies_translator_translate_no_match(
+        self,
+        managed_policy_map,
+        bundled_managed_policy_map,
+        get_managed_policy_map_value,
+        expected_arn,
+    ):
+        def get_managed_policy_map():
+            return get_managed_policy_map_value
+
+        with patch(
+            "samtranslator.internal.managed_policies._BUNDLED_MANAGED_POLICIES",
+            {"aws": bundled_managed_policy_map},
+        ):
+            parameters = {}
+            with self.assertRaises(InvalidDocumentException):
+                Translator(managed_policy_map, Parser()).translate(
+                    self._MANAGED_POLICIES_TEMPLATE,
+                    parameters,
+                    get_managed_policy_map=get_managed_policy_map,
+                )
+
     # test to make sure with arn it doesnt load, with non-arn it does
     @parameterized.expand(
         [
@@ -850,7 +892,13 @@ def __init__(self):
 
             def load(self):
                 self.call_count += 1
-                return {}
+                return {
+                    "": "arn:",
+                    "SomeNonArnThing": "arn:SomeNonArnThing",
+                    "AnotherNonArnThing": "arn:AnotherNonArnThing",
+                    "aws:looks:like:an:ARN:but-not-really": "arn:aws:looks:like:an:ARN:but-not-really",
+                    "Mixing_things_v2": "arn:Mixing_things_v2",
+                }
 
         managed_policy_loader = ManagedPolicyLoader()
 
@@ -888,7 +936,6 @@ def load(self):
             ({"foo": "a1"}, {"foo": "a2"}, "a2"),
             ({"foo": "a1"}, None, "a1"),
             (None, {"foo": "a2"}, "a2"),
-            (None, None, "foo"),
         ]
     )
     @patch("boto3.session.Session.region_name", "ap-southeast-1")
@@ -925,6 +972,35 @@ def load(self):
         self.assertEqual(function_arn, expected_arn)
         self.assertEqual(sfn_arn, expected_arn)
 
+    @parameterized.expand(
+        [
+            (None, None, "foo"),
+        ]
+    )
+    @patch("boto3.session.Session.region_name", "ap-southeast-1")
+    @patch("botocore.client.ClientEndpointBridge._check_default_region", mock_get_region)
+    def test_managed_policies_transform_no_match(
+        self,
+        managed_policy_map,
+        bundled_managed_policy_map,
+        expected_arn,
+    ):
+        class ManagedPolicyLoader:
+            def load(self):
+                return managed_policy_map
+
+        with patch(
+            "samtranslator.internal.managed_policies._BUNDLED_MANAGED_POLICIES",
+            {"aws": bundled_managed_policy_map},
+        ):
+            parameters = {}
+            with self.assertRaises(InvalidDocumentException):
+                transform(
+                    self._MANAGED_POLICIES_TEMPLATE,
+                    parameters,
+                    ManagedPolicyLoader(),
+                )
+
     @patch("boto3.session.Session.region_name", "ap-southeast-1")
     @patch("botocore.client.ClientEndpointBridge._check_default_region", mock_get_region)
     def test_managed_policies_transform_policies_loaded_once(self):
@@ -938,7 +1014,12 @@ def __init__(self):
 
             def load(self):
                 self.call_count += 1
-                return {}
+                return {
+                    "foo": "arn:foo",
+                    "bar": "arn:bar",
+                    "egg": "arn:egg",
+                    "baz": "arn:baz",
+                }
 
         managed_policy_loader = ManagedPolicyLoader()