Created new GenericCrossAccountTrustRule (#208)

* stopped using _statement_as_list() in CrossAccountCheckingRule in favor of statement_as_list()

* created GenericCrossAccountTrustRule

* added tests for the new rule

* updated changelog and version

* updated documentation regarding future deprecations

* fix changelog wrong date for 1.4.2

* covering cases for OpenSearch Domain

* covering cases for ElasticSearch Domain

* covering cases for ksm key - skipped tests pending pycfmodel 0.17.0

* covering cases for S3 buckets

* covering cases for KMS Keys by bumping pycfmodel to 0.17.0

* move GenericCrossAccountTrustRule tests into its own file

* update documentation

* test one side only

* updated cross account documentation

* fix typo

* add test to check that the new generic rule ignores roles

* parametrize tests

* PR suggestions

* improve test imports

Co-authored-by: Ramon <ramon.guimera@skyscanner.net>

CHANGELOG.md

@@ -1,7 +1,17 @@
 # Changelog
 All notable changes to this project will be documented in this file.
 
-## [1.4.2] - 2022-2-24
+## [1.5.0]
+### Updates
+- Created `GenericCrossAccountTrustRule` in order to check for CrossAccount issues for generic resources.
+- Added documentation regarding the deprecation of `S3CrossAccountTrustRule`, `KMSKeyCrossAccountTrustRule`, `ElasticsearchDomainCrossAccountTrustRule` and `OpenSearchDomainCrossAccountTrustRule`.
+- Covering cases for already mapped models in rules inherited from `CrossAccountCheckingRule` with the new `GenericCrossAccountTrustRule`.
+### Improvements
+- Bump `pycfmodel` to `0.17.0`
+### Fixes
+- Stopped using `_statement_as_list()` when retrieving statements in `CrossAccountCheckingRule` in favor of `statement_as_list()`.
+
+## [1.4.2] - 2022-2-28
 ### Fixes
 - Fix how `make install-dev` works, it will install dependencies from `make install` first.
 ### Improvements

cfripper/__version__.py

@@ -1,3 +1,3 @@
-VERSION = (1, 4, 2)
+VERSION = (1, 5, 0)
 
 __version__ = ".".join(map(str, VERSION))

cfripper/rules/__init__.py

@@ -5,6 +5,7 @@
     CrossAccountCheckingRule,
     CrossAccountTrustRule,
     ElasticsearchDomainCrossAccountTrustRule,
+    GenericCrossAccountTrustRule,
     KMSKeyCrossAccountTrustRule,
     OpenSearchDomainCrossAccountTrustRule,
     S3CrossAccountTrustRule,
@@ -55,6 +56,7 @@
         EC2SecurityGroupOpenToWorldRule,
         ElasticsearchDomainCrossAccountTrustRule,
         FullWildcardPrincipalRule,
+        GenericCrossAccountTrustRule,
         HardcodedRDSPasswordRule,
         IAMRolesOverprivilegedRule,
         IAMRoleWildcardActionOnPolicyRule,

cfripper/rules/cross_account_trust.py

@@ -2,6 +2,7 @@
     "CrossAccountCheckingRule",
     "CrossAccountTrustRule",
     "ElasticsearchDomainCrossAccountTrustRule",
+    "GenericCrossAccountTrustRule",
     "KMSKeyCrossAccountTrustRule",
     "OpenSearchDomainCrossAccountTrustRule",
     "S3CrossAccountTrustRule",
@@ -53,7 +54,7 @@ def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
                 properties = resource.Properties
                 policy_document = getattr(properties, self.PROPERTY_WITH_POLICYDOCUMENT)
                 if policy_document:
-                    for statement in policy_document._statement_as_list():
+                    for statement in policy_document.statement_as_list():
                         filters_available_context = {
                             "config": self._config,
                             "extras": extras,
@@ -105,6 +106,52 @@ def _do_statement_check(
                         )
 
 
+class GenericCrossAccountTrustRule(CrossAccountCheckingRule):
+    """
+    Checks if the trust policy of every resource grants permissions to principals from other accounts.
+    Do not use whole accounts as principals.
+    It doesn't check if policies allow permissions to assume roles in other accounts.
+
+    Risk:
+        It might allow other AWS identities to escalate privileges.
+
+    Fix:
+        If cross account permissions are required, the stack should be added to the allowlist for this rule.
+        Otherwise, the access should be removed from the CloudFormation definition.
+
+    Filters context:
+        | Parameter   | Type        | Description                                                    |
+        |:-----------:|:-----------:|:--------------------------------------------------------------:|
+        |`config`     | `str`       | `config` variable available inside the rule                    |
+        |`extras`     | `str`       | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`       | ID used in CloudFormation to refer the resource being analysed |
+        |`resource`   | `Generic`   | Resource that is being addressed                               |
+        |`statement`  | `Statement` | Statement being checked found in the Resource                  |
+        |`principal`  | `str`       | AWS Principal being checked found in the statement             |
+        |`account_id` | `str`       | Account ID found in the principal                              |
+    """
+
+    REASON = "{} has forbidden cross-account with {}"
+
+    def invoke(self, cfmodel: CFModel, extras: Optional[Dict] = None) -> Result:
+        result = Result()
+        for logical_id, resource in cfmodel.Resources.items():
+            policy_documents = resource.policy_documents
+            if policy_documents:
+                for document in policy_documents:
+                    for statement in document.policy_document.statement_as_list():
+                        filters_available_context = {
+                            "config": self._config,
+                            "extras": extras,
+                            "logical_id": logical_id,
+                            "resource": resource,
+                            "statement": statement,
+                        }
+                        self._do_statement_check(result, logical_id, statement, filters_available_context)
+
+        return result
+
+
 class CrossAccountTrustRule(CrossAccountCheckingRule):
     """
     Checks if the trust policy of a role grants permissions to principals from other accounts.
@@ -120,9 +167,9 @@ class CrossAccountTrustRule(CrossAccountCheckingRule):
     Filters context:
         | Parameter   | Type        | Description                                                    |
         |:-----------:|:-----------:|:--------------------------------------------------------------:|
-        |`config`     | str         | `config` variable available inside the rule                    |
-        |`extras`     | str         | `extras` variable available inside the rule                    |
-        |`logical_id` | str         | ID used in Cloudformation to refer the resource being analysed |
+        |`config`     | `str`       | `config` variable available inside the rule                    |
+        |`extras`     | `str`       | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`       | ID used in CloudFormation to refer the resource being analysed |
         |`resource`   | `IAMRole`   | Resource that is being addressed                               |
         |`statement`  | `Statement` | Statement being checked found in the Resource                  |
         |`principal`  | `str`       | AWS Principal being checked found in the statement             |
@@ -136,6 +183,8 @@ class CrossAccountTrustRule(CrossAccountCheckingRule):
 
 class S3CrossAccountTrustRule(CrossAccountCheckingRule):
     """
+    To be replaced by GenericCrossAccountTrustRule.
+
     Check for cross account access in S3 bucket policies. Cross account access by default should not be allowed.
 
     Risk:
@@ -148,9 +197,9 @@ class S3CrossAccountTrustRule(CrossAccountCheckingRule):
     Filters context:
         | Parameter   | Type             | Description                                                    |
         |:-----------:|:----------------:|:--------------------------------------------------------------:|
-        |`config`     | str              | `config` variable available inside the rule                    |
-        |`extras`     | str              | `extras` variable available inside the rule                    |
-        |`logical_id` | str              | ID used in Cloudformation to refer the resource being analysed |
+        |`config`     | `str`            | `config` variable available inside the rule                    |
+        |`extras`     | `str`            | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`            | ID used in CloudFormation to refer the resource being analysed |
         |`resource`   | `S3BucketPolicy` | Resource that is being addressed                               |
         |`statement`  | `Statement`      | Statement being checked found in the Resource                  |
         |`principal`  | `str`            | AWS Principal being checked found in the statement             |
@@ -164,6 +213,8 @@ class S3CrossAccountTrustRule(CrossAccountCheckingRule):
 
 class KMSKeyCrossAccountTrustRule(CrossAccountCheckingRule):
     """
+    To be replaced by GenericCrossAccountTrustRule.
+
     Checks for KMS keys that allow cross-account principals to get access to the key.
 
     Risk:
@@ -176,9 +227,9 @@ class KMSKeyCrossAccountTrustRule(CrossAccountCheckingRule):
     Filters context:
         | Parameter   | Type        | Description                                                    |
         |:-----------:|:-----------:|:--------------------------------------------------------------:|
-        |`config`     | str         | `config` variable available inside the rule                    |
-        |`extras`     | str         | `extras` variable available inside the rule                    |
-        |`logical_id` | str         | ID used in Cloudformation to refer the resource being analysed |
+        |`config`     | `str`       | `config` variable available inside the rule                    |
+        |`extras`     | `str`       | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`       | ID used in CloudFormation to refer the resource being analysed |
         |`resource`   | `KMSKey`    | Resource that is being addressed                               |
         |`statement`  | `Statement` | Statement being checked found in the Resource                  |
         |`principal`  | `str`       | AWS Principal being checked found in the statement             |
@@ -192,6 +243,8 @@ class KMSKeyCrossAccountTrustRule(CrossAccountCheckingRule):
 
 class ElasticsearchDomainCrossAccountTrustRule(CrossAccountCheckingRule):
     """
+    To be replaced by GenericCrossAccountTrustRule.
+
     Checks for Elasticsearch domains that allow cross-account principals to get access.
 
     Risk:
@@ -204,9 +257,9 @@ class ElasticsearchDomainCrossAccountTrustRule(CrossAccountCheckingRule):
     Filters context:
         | Parameter   | Type        | Description                                                    |
         |:-----------:|:-----------:|:--------------------------------------------------------------:|
-        |`config`     | str         | `config` variable available inside the rule                    |
-        |`extras`     | str         | `extras` variable available inside the rule                    |
-        |`logical_id` | str         | ID used in Cloudformation to refer the resource being analysed |
+        |`config`     | `str`       | `config` variable available inside the rule                    |
+        |`extras`     | `str`       | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`       | ID used in CloudFormation to refer the resource being analysed |
         |`resource`   | `ESDomain`  | Resource that is being addressed                               |
         |`statement`  | `Statement` | Statement being checked found in the Resource                  |
         |`principal`  | `str`       | AWS Principal being checked found in the statement             |
@@ -220,6 +273,8 @@ class ElasticsearchDomainCrossAccountTrustRule(CrossAccountCheckingRule):
 
 class OpenSearchDomainCrossAccountTrustRule(CrossAccountCheckingRule):
     """
+    To be replaced by GenericCrossAccountTrustRule.
+
     Checks for OpenSearch domains that allow cross-account principals to get access.
 
     Risk:
@@ -232,9 +287,9 @@ class OpenSearchDomainCrossAccountTrustRule(CrossAccountCheckingRule):
     Filters context:
         | Parameter   | Type               | Description                                                    |
         |:-----------:|:------------------:|:--------------------------------------------------------------:|
-        |`config`     | str                | `config` variable available inside the rule                    |
-        |`extras`     | str                | `extras` variable available inside the rule                    |
-        |`logical_id` | str                | ID used in Cloudformation to refer the resource being analysed |
+        |`config`     | `str`              | `config` variable available inside the rule                    |
+        |`extras`     | `str`              | `extras` variable available inside the rule                    |
+        |`logical_id` | `str`              | ID used in CloudFormation to refer the resource being analysed |
         |`resource`   | `OpenSearchDomain` | Resource that is being addressed                               |
         |`statement`  | `Statement`        | Statement being checked found in the Resource                  |
         |`principal`  | `str`              | AWS Principal being checked found in the statement             |

requirements.txt

@@ -10,7 +10,7 @@ cfn-flip==1.3.0
 click==7.1.2
 jmespath==0.10.0
 pluggy==0.13.1
-pycfmodel==0.16.3
+pycfmodel==0.17.0
 pydantic==1.9.0
 pydash==4.7.6
 python-dateutil==2.8.2

setup.py

@@ -11,7 +11,7 @@
     "cfn_flip>=1.2.0",
     "click~=7.1.1",
     "pluggy~=0.13.1",
-    "pycfmodel>=0.16.3",
+    "pycfmodel>=0.17.0",
     "pydash~=4.7.6",
     "PyYAML>=4.2b1",
 ]