Repository: salesforce/policy_sentry (python)

Issue:
when services is specified in 2 differents "wildcard-only", the former one is erased
# Description

It seems `service-*` get overridden when multiple actions is specified 

# How to reproduce
CRUD config:
```yaml
service-write:
- 'eks'
service-list:
- 'eks'
```
# Expected result
```json
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "MultMultNone",
            "Effect": "Allow",
            "Action": [
                "eks:CreateClusters",
                "eks:ListClusters"
            ],
            "Resource": [
                "*"
            ]
        }
    ]
}
```
# Actual result

```json
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "MultMultNone",
            "Effect": "Allow",
            "Action": [
                "eks:ListClusters"
            ],
            "Resource": [
                "*"
            ]
        }
    ]
}

```

Pull request description:
Fix issue where service wide wildcard-only actions were overwriting each other
Fixes #155 

FYI @lvincel

Patch:
diff --git a/.travis.yml b/.travis.yml
index 2a74dd55..757d9cb9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -10,6 +10,7 @@ install:
 - pipenv install --dev
 script:
 - invoke build.install-package
+- invoke test.format
 - invoke test.lint
 - invoke integration.clean
 - invoke integration.version
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 26d44b34..81d694a9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,9 @@
 ## Future release (Unreleased)
 * Add conditions support
 
+## 0.8.0.1 (Unreleased)
+* Fix issue where service-wide wildcard-only actions were overwriting each other (#155)
+
 ## 0.8.0 (2020-04-09)
 * Policy Sentry is now 10x faster!!
 * Refactored database approach - not using SQLite Database anymore.
diff --git a/policy_sentry/writing/sid_group.py b/policy_sentry/writing/sid_group.py
index dc339c1d..99cd31a6 100644
--- a/policy_sentry/writing/sid_group.py
+++ b/policy_sentry/writing/sid_group.py
@@ -22,6 +22,7 @@ from policy_sentry.util.actions import get_lowercase_action_list
 logger = logging.getLogger(__name__)
 
 
+# pylint: disable=too-many-instance-attributes
 class SidGroup:
     """
     This class is critical to the creation of least privilege policies.
@@ -49,6 +50,13 @@ class SidGroup:
         self.sids = {}
         self.universal_conditions = {}
         self.overrides = []
+        self.wildcard_only_single_actions = []
+        # When a user requests all wildcard-only actions available under a service at a specific access level
+        self.wildcard_only_service_read = []
+        self.wildcard_only_service_write = []
+        self.wildcard_only_service_list = []
+        self.wildcard_only_service_tagging = []
+        self.wildcard_only_service_permissions_management = []
 
     def get_sid_group(self):
         """
@@ -91,6 +99,42 @@ class SidGroup:
         else:
             raise Exception("Please provide 'overrides' as a list of IAM actions.")
 
+    def add_requested_service_wide(self, service_prefixes, access_level):
+        """
+        When a user requests all wildcard-only actions available under a service at a specific access level
+
+        :param service_prefixes: A list of service prefixes
+        :param access_level: The requested access level
+        """
+        if access_level == "Read":
+            self.wildcard_only_service_read = service_prefixes
+        elif access_level == "Write":
+            self.wildcard_only_service_write = service_prefixes
+        elif access_level == "List":
+            self.wildcard_only_service_list = service_prefixes
+        elif access_level == "Tagging":
+            self.wildcard_only_service_tagging = service_prefixes
+        elif access_level == "Permissions management":
+            self.wildcard_only_service_permissions_management = service_prefixes
+
+    def process_wildcard_only_actions(self):
+        """
+        After (1) the list of wildcard-only single actions have been added and (2) the list of wildcard-only service-wide actions have been added, process them and store them under the proper SID.
+        :return:
+        """
+        provided_wildcard_actions = []
+        provided_wildcard_actions = (
+            self.wildcard_only_single_actions
+            + get_wildcard_only_actions_matching_services_and_access_level(self.wildcard_only_service_read, "Read")
+            + get_wildcard_only_actions_matching_services_and_access_level(self.wildcard_only_service_list, "List")
+            + get_wildcard_only_actions_matching_services_and_access_level(self.wildcard_only_service_write, "Write")
+            + get_wildcard_only_actions_matching_services_and_access_level(self.wildcard_only_service_tagging, "Tagging")
+            + get_wildcard_only_actions_matching_services_and_access_level(self.wildcard_only_service_permissions_management, "Permissions management")
+        )
+        self.add_wildcard_only_actions(
+            provided_wildcard_actions
+        )
+
     def get_rendered_policy(self, minimize=None):
         """
         Get the JSON rendered policy
@@ -261,23 +305,8 @@ class SidGroup:
                             "access_level": row["access_level"],
                             "action": row["action"],
                         }
-                        # [row["resource_arn_format"], row["access_level"], row["action"]])
                     )
 
-        # arns_matching_supplied_actions = [{
-        #         "resource_arn_format": "*",
-        #         "access_level": "Write",
-        #         "action": "kms:createcustomkeystore"
-        #     },{
-        #         "resource_arn_format": "arn:${Partition}:kms:${Region}:${Account}:key/${KeyId}",
-        #         "access_level": "Permissions management",
-        #         "action": "kms:creategrant"
-        #     },{
-        #         "resource_arn_format": "*",
-        #         "access_level": "Permissions management",
-        #         "action": "kms:creategrant"
-        # }]
-
         # Identify the actions that do not support resource constraints
         # If that's the case, add it to the wildcard namespace. Otherwise, don't add it.
 
@@ -343,9 +372,7 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(provided_wildcard_actions)}"
                                 )
-                                self.add_wildcard_only_actions(
-                                    provided_wildcard_actions
-                                )
+                                self.wildcard_only_single_actions = provided_wildcard_actions
                     if "service-read" in cfg["wildcard-only"]:
                         if cfg["wildcard-only"]["service-read"]:
                             if cfg["wildcard-only"]["service-read"][0] != "":
@@ -353,9 +380,7 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(service_read)}"
                                 )
-                                self.add_wildcard_only_actions_matching_services_and_access_level(
-                                    service_read, "Read"
-                                )
+                                self.wildcard_only_service_read = service_read
                     if "service-write" in cfg["wildcard-only"]:
                         if cfg["wildcard-only"]["service-write"]:
                             if cfg["wildcard-only"]["service-write"][0] != "":
@@ -363,9 +388,7 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(service_write)}"
                                 )
-                                self.add_wildcard_only_actions_matching_services_and_access_level(
-                                    service_write, "Write"
-                                )
+                                self.wildcard_only_service_write = service_write
                     if "service-list" in cfg["wildcard-only"]:
                         if cfg["wildcard-only"]["service-list"]:
                             if cfg["wildcard-only"]["service-list"][0] != "":
@@ -373,9 +396,7 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(service_list)}"
                                 )
-                                self.add_wildcard_only_actions_matching_services_and_access_level(
-                                    service_list, "List"
-                                )
+                                self.wildcard_only_service_list = service_list
                     if "service-tagging" in cfg["wildcard-only"]:
                         if cfg["wildcard-only"]["service-tagging"]:
                             if cfg["wildcard-only"]["service-tagging"][0] != "":
@@ -385,9 +406,7 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(service_tagging)}"
                                 )
-                                self.add_wildcard_only_actions_matching_services_and_access_level(
-                                    service_tagging, "Tagging"
-                                )
+                                self.wildcard_only_service_tagging = service_tagging
                     if "service-permissions-management" in cfg["wildcard-only"]:
                         if cfg["wildcard-only"]["service-permissions-management"]:
                             if (
@@ -403,10 +422,8 @@ class SidGroup:
                                 logger.debug(
                                     f"Requested wildcard-only actions: {str(service_permissions_management)}"
                                 )
-                                self.add_wildcard_only_actions_matching_services_and_access_level(
-                                    service_permissions_management,
-                                    "Permissions management",
-                                )
+                                self.wildcard_only_service_permissions_management = service_permissions_management
+                    self.process_wildcard_only_actions()
                 if "read" in cfg.keys():
                     if cfg["read"] is not None and cfg["read"][0] != "":
                         logger.debug(f"Requested access to arns: {str(cfg['read'])}")
@@ -497,16 +514,21 @@ class SidGroup:
         :param actions_to_keep: A list of actions to leave in the policy. All actions not in this list are removed.
         """
         actions_to_keep = get_lowercase_action_list(actions_to_keep)
+        actions_deleted = []
         for sid in self.sids:
             placeholder_actions_list = []
             for action in self.sids[sid]["actions"]:
                 # if the action is not in the list of selected actions, don't copy it to the placeholder list
                 if action.lower() in actions_to_keep:
                     placeholder_actions_list.append(action)
+                elif action.lower() not in actions_to_keep:
+                    logger.debug("%s not found in list of actions to keep: %s", action.lower(), actions_to_keep)
+                    actions_deleted.append(action)
             # Clear the list and then extend it to include the updated actions only
             self.sids[sid]["actions"].clear()
             self.sids[sid]["actions"].extend(placeholder_actions_list.copy())
-
+        # Highlight the actions that you remove
+        logger.debug("Actions deleted: %s", str(actions_deleted))
         # Now that we've removed a bunch of actions, if there are SID groups without any actions,
         # remove them so we don't get SIDs with empty action lists
         self.remove_sids_with_empty_action_lists()
@@ -593,6 +615,21 @@ def remove_actions_that_are_not_wildcard_arn_only(actions_list):
     return actions_list_placeholder
 
 
+def get_wildcard_only_actions_matching_services_and_access_level(services, access_level):
+    """
+    :param services: A list of AWS services
+    :param access_level: An access level as it is written in the database, such as 'Read', 'Write', 'List',
+    'Permissions management', or 'Tagging'
+    """
+    wildcard_only_actions_to_add = []
+    for service in services:
+        actions = get_actions_at_access_level_that_support_wildcard_arns_only(
+            service, access_level
+        )
+        wildcard_only_actions_to_add.extend(actions)
+    return wildcard_only_actions_to_add
+
+
 def create_policy_sid_namespace(
     service, access_level, resource_type_name, condition_block=None
 ):
diff --git a/tasks.py b/tasks.py
index 32f4b8b3..84531e7c 100755
--- a/tasks.py
+++ b/tasks.py
@@ -224,12 +224,25 @@ def security_scan(c):
         sys.exit(1)
 
 
+# TEST - format
+@task
+def format(c):
+    """Auto format code with Python `black`"""
+    try:
+        c.run("black policy_sentry/")
+    except UnexpectedExit as u_e:
+        logger.critical(f"FAIL! UnexpectedExit: {u_e}")
+        sys.exit(1)
+    except Failure as f_e:
+        logger.critical(f"FAIL: Failure: {f_e}")
+        sys.exit(1)
+
+
 # TEST - LINT
 @task
 def run_linter(c):
     """Linting with `pylint` and `black`"""
     try:
-        c.run('black policy_sentry/')
         c.run('pylint policy_sentry/', warn=False)
     except UnexpectedExit as u_e:
         logger.critical(f"FAIL! UnexpectedExit: {u_e}")
@@ -292,6 +305,7 @@ docs.add_task(open_html_docs, 'open-html')
 docs.add_task(download_latest_aws_docs, 'download_latest_aws_docs')
 
 # test.add_task(run_full_test_suite, 'all')
+test.add_task(format, 'format')
 test.add_task(run_linter, 'lint')
 test.add_task(security_scan, 'security')
 


Test patch:
diff --git a/test/command/test_write_policy.py b/test/command/test_write_policy.py
index cc2f850d..8b957af5 100644
--- a/test/command/test_write_policy.py
+++ b/test/command/test_write_policy.py
@@ -90,7 +90,6 @@ class WildcardOnlyServiceLevelTestCase(unittest.TestCase):
         cfg = read_yaml_file(policy_file_path)
 
         output = write_policy_with_template(cfg)
-        print(json.dumps(output, indent=4))
         desired_output = {
             "Version": "2012-10-17",
             "Statement": [
@@ -98,6 +97,9 @@ class WildcardOnlyServiceLevelTestCase(unittest.TestCase):
                     "Sid": "MultMultNone",
                     "Effect": "Allow",
                     "Action": [
+                        "ram:EnableSharingWithAwsOrganization",
+                        "ram:GetResourcePolicies",
+                        "secretsmanager:CreateSecret",
                         "ecr:GetAuthorizationToken",
                         "s3:GetAccessPoint",
                         "s3:GetAccountPublicAccessBlock",
@@ -127,6 +129,37 @@ class WildcardOnlyServiceLevelTestCase(unittest.TestCase):
         self.maxDiff = None
         self.assertDictEqual(output, desired_output)
 
+    def test_eks_gh_155(self):
+        """test_eks_gh_155: Test EKS issue raised in GH-155"""
+        template_file_path = os.path.abspath(
+            os.path.join(
+                os.path.dirname(__file__),
+                os.path.pardir,
+                "files",
+                "eks-service-wide.yml",
+            )
+        )
+        cfg = read_yaml_file(template_file_path)
+        result = write_policy_with_template(cfg)
+        print(json.dumps(result, indent=4))
+        expected_results = {
+            "Version": "2012-10-17",
+            "Statement": [
+                {
+                    "Sid": "MultMultNone",
+                    "Effect": "Allow",
+                    "Action": [
+                        "eks:ListClusters",
+                        "eks:CreateCluster"
+                    ],
+                    "Resource": [
+                        "*"
+                    ]
+                }
+            ]
+        }
+        self.assertDictEqual(result, expected_results)
+
 
 class RdsWritingTestCase(unittest.TestCase):
     def test_rds_policy_read_only(self):
diff --git a/test/files/eks-service-wide.yml b/test/files/eks-service-wide.yml
new file mode 100644
index 00000000..4685364f
--- /dev/null
+++ b/test/files/eks-service-wide.yml
@@ -0,0 +1,7 @@
+mode: crud
+name: 'GH-155'
+wildcard-only:
+    service-write:
+    - 'eks'
+    service-list:
+    - 'eks'