Add canonical ids of elasticache (#74)

* add service accounts

* fix code style

* update tests and rules

* Update changelog

* extract valid_principals to a shared method

* add cache

* Add principals checking tule

* update changelog

* update

* fix imports

* fixes

* add python typing

* add tests

* update S3CrossAccountTrustRule.

* update changelog

* update tests

* update changelog

* update variables name

* update changelog

* update tests

* fix tests

CHANGELOG.md

@@ -3,12 +3,14 @@ All notable changes to this project will be documented in this file.
 
 ## [0.10.2] - 2019-11-20
 ### Added
-- Added `PrincipalCheckingRule`, it has a property called `valid_principals`. It's a list with all allowed principals. 
+- Added `PrincipalCheckingRule`, it has a property called `valid_principals`. It's a list with all allowed principals. This list can be customized using `_get_whitelist_from_config()`.
+- Added `AWS_ELASTICACHE_BACKUP_CANONICAL_IDS` which contains the aws canonical ids used for backups.
 ### Changed
 - `CrossAccountTrustRule` outputs warning log message if the AWS Account ID is not present in the config.
 - `HardcodedRDSPasswordRule` updated to check for both RDS Clusters and RDS Instances, and reduce false positives on valid instances.
 - `CrossAccountTrustRule`, `GenericWildcardPrincipalRule`, `S3BucketPolicyPrincipalRule`, `S3BucketPolicyPrincipalRule` and `S3CrossAccountTrustRule` now check the account against a list.
   The list is composed of AWS service accounts, configured AWS principals and the account id where the event came from.
+- Rename `AWS_ELB_ACCOUNT_IDS` to `AWS_ELB_LOGS_ACCOUNT_IDS`
 
 ## [0.10.1] - 2019-11-14
 ### Added

cfripper/config/config.py

@@ -15,7 +15,7 @@
 import re
 from typing import List
 
-from .whitelist import AWS_ELB_ACCOUNT_IDS
+from .whitelist import AWS_ELASTICACHE_BACKUP_CANONICAL_IDS, AWS_ELB_LOGS_ACCOUNT_IDS
 from .whitelist import rule_to_action_whitelist as default_rule_to_action_whitelist
 from .whitelist import rule_to_resource_whitelist as default_rule_to_resource_whitelist
 from .whitelist import stack_whitelist as default_stack_whitelist
@@ -117,7 +117,13 @@ def __init__(
             rule_to_resource_whitelist if rule_to_resource_whitelist is not None else default_rule_to_resource_whitelist
         )
         self.stack_whitelist = stack_whitelist if stack_whitelist is not None else default_stack_whitelist
-        self.aws_service_accounts = aws_service_accounts if aws_service_accounts is not None else AWS_ELB_ACCOUNT_IDS
+        if aws_service_accounts is None:
+            self.aws_service_accounts = {
+                "elb_logs_account_ids": AWS_ELB_LOGS_ACCOUNT_IDS,
+                "elasticache_backup_canonical_ids": AWS_ELASTICACHE_BACKUP_CANONICAL_IDS,
+            }
+        else:
+            self.aws_service_accounts = aws_service_accounts
 
         if self.stack_name:
             whitelisted_rules = self.get_whitelisted_rules()

cfripper/config/whitelist.py

@@ -104,7 +104,7 @@
 rule_to_action_whitelist = {}
 
 
-AWS_ELB_ACCOUNT_IDS = [
+AWS_ELB_LOGS_ACCOUNT_IDS = [
     # From https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/enable-access-logs.html
     "009996457667",  # Elastic Load Balancing Account ID - eu-west-3
     "027434742980",  # Elastic Load Balancing Account ID - us-west-1
@@ -130,3 +130,11 @@
     "897822967062",  # Elastic Load Balancing Account ID - eu-north-1
     "985666609251",  # Elastic Load Balancing Account ID - ca-central-1
 ]
+
+
+AWS_ELASTICACHE_BACKUP_CANONICAL_IDS = [
+    # From https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/backups-exporting.html
+    "b14d6a125bdf69854ed8ef2e71d8a20b7c490f252229b806e514966e490b8d83",  # China (Beijing) and China (Ningxia) Regions
+    "40fa568277ad703bd160f66ae4f83fc9dfdfd06c2f1b5060ca22442ac3ef8be6",  # AWS GovCloud (US-West) Region
+    "540804c33a284a299d2547575ce1010f2312ef3da9b3a053c8bc45bf233e4353",  # All other AWS Regions
+]

cfripper/model/principal_checking_rule.py

@@ -12,7 +12,7 @@
 CONDITIONS OF ANY KIND, either express or implied. See the License for the
 specific language governing permissions and limitations under the License.
 """
-from typing import Set
+from typing import List, Set
 
 from cfripper.model.rule import Rule
 
@@ -22,12 +22,21 @@ def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self._valid_principals = None
 
+    def _get_whitelist_from_config(self, services: List[str] = None) -> Set[str]:
+        if services is None:
+            services = self._config.aws_service_accounts.keys()
+
+        unique_list = set()
+        for service in services:
+            unique_list |= set(self._config.aws_service_accounts[service])
+        return unique_list
+
     @property
     def valid_principals(self) -> Set[str]:
         if self._valid_principals is None:
             self._valid_principals = {
-                *self._config.aws_service_accounts,
                 *self._config.aws_principals,
                 self._config.aws_account_id,
+                *self._get_whitelist_from_config(),
             }
         return self._valid_principals

cfripper/rules/S3CrossAccountTrustRule.py

@@ -35,9 +35,6 @@ def invoke(self, cfmodel):
                     if statement.Effect == "Allow":
                         for principal in statement.get_principal_list():
                             account_id = get_account_id_from_principal(principal)
-                            if account_id in self._config.aws_service_accounts:
-                                # It's ok to allow access to AWS service accounts
-                                continue
                             if account_id not in self.valid_principals:
                                 if statement.Condition and statement.Condition.dict():
                                     logger.warning(

tests/model/test_principal_checking_rule.py

@@ -0,0 +1,148 @@
+"""
+Copyright 2018-2019 Skyscanner Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License"); you may not use
+this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software distributed
+under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+CONDITIONS OF ANY KIND, either express or implied. See the License for the
+specific language governing permissions and limitations under the License.
+"""
+import pytest
+from pycfmodel.model.cf_model import CFModel
+
+from cfripper.config.config import Config
+from cfripper.model.principal_checking_rule import PrincipalCheckingRule
+from cfripper.model.result import Result
+
+
+class FakePrincipalCheckingRule(PrincipalCheckingRule):
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+    def invoke(self, cfmodel: CFModel):
+        pass
+
+
+@pytest.mark.parametrize(
+    "rule, params, expected_output",
+    [
+        (
+            FakePrincipalCheckingRule(config=Config(aws_service_accounts=None), result=Result()),
+            None,
+            {
+                "009996457667",
+                "027434742980",
+                "033677994240",
+                "037604701340",
+                "048591011584",
+                "054676820928",
+                "076674570225",
+                "114774131450",
+                "127311923021",
+                "156460612806",
+                "190560391635",
+                "383597477331",
+                "40fa568277ad703bd160f66ae4f83fc9dfdfd06c2f1b5060ca22442ac3ef8be6",
+                "507241528517",
+                "540804c33a284a299d2547575ce1010f2312ef3da9b3a053c8bc45bf233e4353",
+                "582318560864",
+                "600734575887",
+                "638102146993",
+                "652711504416",
+                "718504428378",
+                "754344448648",
+                "783225319266",
+                "797873946194",
+                "897822967062",
+                "985666609251",
+                "b14d6a125bdf69854ed8ef2e71d8a20b7c490f252229b806e514966e490b8d83",
+            },
+        ),
+        (
+            FakePrincipalCheckingRule(config=Config(aws_service_accounts=None), result=Result()),
+            ["elb_logs_account_ids", "elasticache_backup_canonical_ids"],
+            {
+                "009996457667",
+                "027434742980",
+                "033677994240",
+                "037604701340",
+                "048591011584",
+                "054676820928",
+                "076674570225",
+                "114774131450",
+                "127311923021",
+                "156460612806",
+                "190560391635",
+                "383597477331",
+                "40fa568277ad703bd160f66ae4f83fc9dfdfd06c2f1b5060ca22442ac3ef8be6",
+                "507241528517",
+                "540804c33a284a299d2547575ce1010f2312ef3da9b3a053c8bc45bf233e4353",
+                "582318560864",
+                "600734575887",
+                "638102146993",
+                "652711504416",
+                "718504428378",
+                "754344448648",
+                "783225319266",
+                "797873946194",
+                "897822967062",
+                "985666609251",
+                "b14d6a125bdf69854ed8ef2e71d8a20b7c490f252229b806e514966e490b8d83",
+            },
+        ),
+        (
+            FakePrincipalCheckingRule(config=Config(aws_service_accounts=None), result=Result()),
+            ["elasticache_backup_canonical_ids"],
+            {
+                "40fa568277ad703bd160f66ae4f83fc9dfdfd06c2f1b5060ca22442ac3ef8be6",
+                "540804c33a284a299d2547575ce1010f2312ef3da9b3a053c8bc45bf233e4353",
+                "b14d6a125bdf69854ed8ef2e71d8a20b7c490f252229b806e514966e490b8d83",
+            },
+        ),
+        (
+            FakePrincipalCheckingRule(config=Config(aws_service_accounts={"A": ["a", "b", "c"]}), result=Result()),
+            None,
+            {"a", "b", "c"},
+        ),
+        (
+            FakePrincipalCheckingRule(
+                config=Config(aws_service_accounts={"A": ["a", "b", "c"], "B": ["d", "e", "f"]}), result=Result()
+            ),
+            None,
+            {"a", "b", "c", "d", "e", "f"},
+        ),
+        (
+            FakePrincipalCheckingRule(
+                config=Config(aws_service_accounts={"A": ["a", "b", "c"], "B": ["d", "a", "b"]}), result=Result()
+            ),
+            None,
+            {"a", "b", "c", "d"},
+        ),
+        (
+            FakePrincipalCheckingRule(config=Config(aws_service_accounts={"A": ["a", "b", "c"]}), result=Result()),
+            ["A"],
+            {"a", "b", "c"},
+        ),
+        (
+            FakePrincipalCheckingRule(
+                config=Config(aws_service_accounts={"A": ["a", "b", "c"], "B": ["d", "e", "f"]}), result=Result()
+            ),
+            ["A", "B"],
+            {"a", "b", "c", "d", "e", "f"},
+        ),
+        (
+            FakePrincipalCheckingRule(
+                config=Config(aws_service_accounts={"A": ["a", "b", "c"], "B": ["d", "a", "b"]}), result=Result()
+            ),
+            ["A", "B"],
+            {"a", "b", "c", "d"},
+        ),
+    ],
+)
+def test_get_whitelist_from_config(rule, params, expected_output):
+    assert rule._get_whitelist_from_config(params) == expected_output

tests/model/test_result.py

@@ -1,3 +1,17 @@
+"""
+Copyright 2018-2019 Skyscanner Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License"); you may not use
+this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software distributed
+under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
+CONDITIONS OF ANY KIND, either express or implied. See the License for the
+specific language governing permissions and limitations under the License.
+"""
 from cfripper.model.enums import RuleGranularity, RuleMode, RuleRisk
 from cfripper.model.result import Result