Repository: salesforce/policy_sentry
Pull request #123 (2020-02-10 22:35:10)

Fix/ignore empty crud sections
Instead of throwing an error, if the user did not modify the pre-existing empty string in the CRUD YAML template, just ignore it.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c5b929e6..14e005b7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,9 @@ Other Pending:
 * Terraform module updates are currently pending
 * Update the GIF and the Medium blog
 
+## 0.7.0.2 (2020-02-10)
+* Ignore empty entries in the yaml template
+
 ## 0.7.0.1 (2020-02-07)
 * Quick fix for @jlongman's issue - #118
 * Fix the overly verbose logs for #119
diff --git a/examples/terraform/modules/generate-policy_sentry-yml/main.tf b/examples/terraform/modules/generate-policy_sentry-yml/main.tf
index c9424dc3..cf751068 100644
--- a/examples/terraform/modules/generate-policy_sentry-yml/main.tf
+++ b/examples/terraform/modules/generate-policy_sentry-yml/main.tf
@@ -19,7 +19,7 @@ resource "local_file" "policy_sentry_acl_yml_file" {
   filename = "${var.yml_file_destination_folder}/${var.role_name}.yml"
 
   content = templatefile(
-    "${path.module}/templates/policy_sentry-template.yml", {
+    "${path.module}/templates/policy-sentry-template.yml", {
       //      policy_sentry_role_name                   = yamlencode(local.policy_sentry_yaml_role_name)
       //      policy_sentry_role_description            = yamlencode(local.policy_sentry_yaml_role_description)
       //      policy_sentry_role_arn                    = yamlencode(local.policy_sentry_yaml_role_name)
diff --git a/examples/terraform/modules/generate-policy_sentry-yml/templates/vibranium-template.yml b/examples/terraform/modules/generate-policy_sentry-yml/templates/policy-sentry-template.yml
similarity index 100%
rename from examples/terraform/modules/generate-policy_sentry-yml/templates/vibranium-template.yml
rename to examples/terraform/modules/generate-policy_sentry-yml/templates/policy-sentry-template.yml
diff --git a/policy_sentry/bin/policy_sentry b/policy_sentry/bin/policy_sentry
index 11955e53..a8764309 100755
--- a/policy_sentry/bin/policy_sentry
+++ b/policy_sentry/bin/policy_sentry
@@ -2,7 +2,7 @@
 """
     Policy Sentry is a tool for generating least-privilege IAM Policies.
 """
-__version__ = '0.7.0.1'
+__version__ = '0.7.0.2'
 import click
 from policy_sentry import command
 
diff --git a/policy_sentry/writing/sid_group.py b/policy_sentry/writing/sid_group.py
index 782ac33a..10b9991a 100644
--- a/policy_sentry/writing/sid_group.py
+++ b/policy_sentry/writing/sid_group.py
@@ -293,29 +293,32 @@ class SidGroup:
                                     db_session, verified_wildcard_actions
                                 )
                     if "read" in cfg.keys():
-                        if cfg["read"] is not None:
+                        if cfg["read"] is not None and cfg["read"][0] != "":
                             self.add_by_arn_and_access_level(
                                 db_session, cfg["read"], "Read"
                             )
                     if "write" in cfg.keys():
-                        if cfg["write"] is not None:
+                        if cfg["write"] is not None and cfg["write"][0] != "":
                             self.add_by_arn_and_access_level(
                                 db_session, cfg["write"], "Write"
                             )
                     if "list" in cfg.keys():
-                        if cfg["list"] is not None:
+                        if cfg["list"] is not None and cfg["list"][0] != "":
                             self.add_by_arn_and_access_level(
                                 db_session, cfg["list"], "List"
                             )
                     if "permissions-management" in cfg.keys():
-                        if cfg["permissions-management"] is not None:
+                        if (
+                            cfg["permissions-management"] is not None
+                            and cfg["permissions-management"][0] != ""
+                        ):
                             self.add_by_arn_and_access_level(
                                 db_session,
                                 cfg["permissions-management"],
                                 "Permissions management",
                             )
                     if "tagging" in cfg.keys():
-                        if cfg["tagging"] is not None:
+                        if cfg["tagging"] is not None and cfg["tagging"][0] != "":
                             self.add_by_arn_and_access_level(
                                 db_session, cfg["tagging"], "Tagging"
                             )
@@ -420,7 +423,15 @@ def remove_actions_that_are_not_wildcard_arn_only(db_session, actions_list):
     actions_list_placeholder = []
 
     for action in actions_list:
-        service_name, action_name = action.split(":")
+        try:
+            service_name, action_name = action.split(":")
+        except ValueError as v_e:
+            # We will skip the action because this likely means that the wildcard action provided is not valid.
+            logger.debug(v_e)
+            logger.debug(
+                "The value provided in wildcard-only section is not formatted properly."
+            )
+            continue
         rows = get_actions_that_support_wildcard_arns_only(db_session, service_name)
         for row in rows:
             if row == action:


Test patch:
diff --git a/test/command/test_write_policy.py b/test/command/test_write_policy.py
index ae84d896..1318e8fa 100644
--- a/test/command/test_write_policy.py
+++ b/test/command/test_write_policy.py
@@ -64,15 +64,17 @@ class WritePolicyPreventWildcardEscalation(unittest.TestCase):
         self.assertDictEqual(desired_output, output)
 
 
-class TaggingTestCase(unittest.TestCase):
-    def test_write_tagging_only_policy(self):
-        """test_write_tagging_only_policy: We'd never write a policy like this IRL but doing this as a quality check against how it handles the database"""
-        policy_file_path = abspath(
-            join(
-                dirname(__file__), pardir + "/" + pardir + "/examples/yml/tagging.yml",
-            )
-        )
-        cfg = read_yaml_file(policy_file_path)
+# class TaggingTestCase(unittest.TestCase):
+#     def test_write_tagging_only_policy(self):
+#         """test_write_tagging_only_policy: We'd never write a policy like this IRL but doing this as a quality check against how it handles the database"""
+#         policy_file_path = abspath(
+#             join(
+#                 dirname(__file__), pardir + "/" + pardir + "/examples/yml/tagging.yml",
+#             )
+#         )
+#         cfg = read_yaml_file(policy_file_path)
+#
+#         policy = write_policy_with_template(db_session, cfg)
+#         print(policy)
+
 
-        policy = write_policy_with_template(db_session, cfg)
-        print(policy)
diff --git a/test/writing/test_yaml.py b/test/writing/test_yaml.py
index e63d9387..8e5cd1aa 100644
--- a/test/writing/test_yaml.py
+++ b/test/writing/test_yaml.py
@@ -130,7 +130,7 @@ class YamlValidationCrudTestCase(unittest.TestCase):
 
     def test_empty_strings_in_access_level_categories(self):
         """
-        test_allow_empty_access_level_categories_in_cfg: If the content of a list is an empty string, it should sysexit
+        test_empty_strings_in_access_level_categories: If the content of a list is an empty string, it should NOT sysexit
         :return:
         """
         crud_file_input = {
@@ -144,9 +144,42 @@ class YamlValidationCrudTestCase(unittest.TestCase):
             "tagging": [""],
             "permissions-management": [""],
         }
-        with self.assertRaises(Exception):
-            result = write_policy_with_template(db_session, crud_file_input)
-            # print(json.dumps(result, indent=4))
+        desired_output = {
+            "Version": "2012-10-17",
+            "Statement": [
+                {
+                    "Sid": "SsmReadParameter",
+                    "Effect": "Allow",
+                    "Action": [
+                        "ssm:getparameter",
+                        "ssm:getparameterhistory",
+                        "ssm:getparameters",
+                        "ssm:getparametersbypath",
+                        "ssm:listtagsforresource"
+                    ],
+                    "Resource": [
+                        "arn:aws:ssm:us-east-1:123456789012:parameter/test"
+                    ]
+                },
+                {
+                    "Sid": "SsmWriteParameter",
+                    "Effect": "Allow",
+                    "Action": [
+                        "ssm:deleteparameter",
+                        "ssm:deleteparameters",
+                        "ssm:labelparameterversion",
+                        "ssm:putparameter"
+                    ],
+                    "Resource": [
+                        "arn:aws:ssm:us-east-1:123456789012:parameter/test"
+                    ]
+                }
+            ]
+        }
+        # with self.assertRaises(Exception):
+        result = write_policy_with_template(db_session, crud_file_input)
+        print(json.dumps(result, indent=4))
+        self.assertDictEqual(desired_output, result)
 
 
 class YamlValidationActionsTestCase(unittest.TestCase):