Repository: bridgecrewio/checkov
Pull request: Pass ECR Policy with conditional constraint
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Fixes #1582 

Issue 1582 - Allow an AWS ECR Policy to pass with a wildcarded Prinicipal if there is a Principal Org ID constraint conditional statement.


Problem statement:
BC_AWS_PUBLIC_1 does not take Conditions into account
**Describe the bug**
BC_AWS_PUBLIC_1 seems to only look for the principal in a ECR Resource Policy. If you want to control access to a repo based on the pricipals AWS Organization ID, you have to do this using a condition instead of a Principal. It seems like checkov does not take this into account.

Example:

``` yaml
  Repository:
    Type: AWS::ECR::Repository
    Properties:
      RepositoryName: !Ref RepoName
      RepositoryPolicyText:
        Version: "2012-10-17"
        Statement:
          - Sid: OrgRead
            Effect: Allow
            Principal: "*"
            Action:
              - ecr:GetAuthorizationToken
              - ecr:BatchCheckLayerAvailability
              - ecr:GetDownloadUrlForLayer
              - ecr:GetRepositoryPolicy
              - ecr:DescribeRepositories
              - ecr:ListImages
              - ecr:DescribeImages
              - ecr:BatchGetImage
              - ecr:DescribeImageScanFindings
            Condition:
              ForAllValues:StringEquals:
                aws:PrincipalOrgID: "o-12345678"
```  

**To Reproduce**
Create the above mentioned resource in cloudformation and scan it with checkov

**Expected behavior**
I expect checkov to look for the usage of a PrincipalOrgID or similar global condition keys and not fail the check when they are used to make the repository non public.

**Desktop (please complete the following information):**
 - OS: macOS 11.5.2
 - Checkov Version 2.0.390

Hints from issue discussion:

Commit message: Pass ECR Policy with conditional constraint

Patch to review:
diff --git a/checkov/cloudformation/checks/resource/aws/ECRPolicy.py b/checkov/cloudformation/checks/resource/aws/ECRPolicy.py
index 3b4e2f216e..bcaaa76ba8 100644
--- a/checkov/cloudformation/checks/resource/aws/ECRPolicy.py
+++ b/checkov/cloudformation/checks/resource/aws/ECRPolicy.py
@@ -31,9 +31,22 @@ def scan_resource_conf(self, conf):
                     for statement_index, statement in enumerate(policy_text['Statement']):
                         if 'Principal' in statement.keys():
                             for principal_index, principal in enumerate(statement['Principal']):
-                                if principal == "*":
+                                if principal == "*" and self.check_for_constrained_condition(statement) == False:
                                     self.evaluated_keys = [f"Properties/RepositoryPolicyText/Statement/[{statement_index}]/Principal/[{principal_index}]"]
                                     return CheckResult.FAILED
         return CheckResult.PASSED
 
+    def check_for_constrained_condition(self, statement):
+        """
+        Checks to see if there is a constraint on a a wildcarded principal
+        :param statement: statement from aws_repository_configuration
+        :return: true if there is a constraint
+        """
+        if 'Condition' in statement.keys():
+          condition = statement['Condition']
+          if 'ForAllValues:StringEquals' in condition.keys():
+            if 'aws:PrincipalOrgID' in condition['ForAllValues:StringEquals'].keys():
+              return True
+        return False
+
 check = ECRPolicy()
diff --git a/checkov/terraform/checks/resource/aws/ECRPolicy.py b/checkov/terraform/checks/resource/aws/ECRPolicy.py
index a42c26ed0b..0760bb628b 100644
--- a/checkov/terraform/checks/resource/aws/ECRPolicy.py
+++ b/checkov/terraform/checks/resource/aws/ECRPolicy.py
@@ -27,7 +27,7 @@ def scan_resource_conf(self, conf):
                 statement = policy['Statement'][0]
                 if statement['Principal'] and type(statement['Principal']) is str:
                     principal = statement['Principal']
-                    if principal == "*":
+                    if principal == "*"  and self.check_for_constrained_condition(statement) == False:
                         self.evaluated_keys = ["policy/Statement/Principal"]
                         return CheckResult.FAILED
         return CheckResult.PASSED
@@ -35,5 +35,17 @@ def scan_resource_conf(self, conf):
     def get_evaluated_keys(self) -> List[str]:
         return ['policy']
 
+    def check_for_constrained_condition(self, statement):
+        """
+        Checks to see if there is a constraint on a a wildcarded principal
+        :param statement: statement from aws_repository_configuration
+        :return: True if there is a constraint
+        """
+        if statement['Condition'] and type(statement['Condition']) is dict:
+          condition = statement['Condition']
+          if condition['ForAllValues:StringEquals'] and type(condition['ForAllValues:StringEquals']) is dict:
+            if 'aws:PrincipalOrgID' in condition['ForAllValues:StringEquals'].keys():
+              return True
+        return False
 
 check = ECRPolicy()
diff --git a/tests/cloudformation/checks/resource/aws/example_ECRPolicy/ECRPolicy-PASSED-2.yaml b/tests/cloudformation/checks/resource/aws/example_ECRPolicy/ECRPolicy-PASSED-2.yaml
new file mode 100644
index 0000000000..30076915c3
--- /dev/null
+++ b/tests/cloudformation/checks/resource/aws/example_ECRPolicy/ECRPolicy-PASSED-2.yaml
@@ -0,0 +1,28 @@
+AWSTemplateFormatVersion: "2010-09-09"
+Resources: 
+  MyRepository: 
+    Type: AWS::ECR::Repository
+    Properties: 
+      RepositoryName: "test-repository"
+      RepositoryPolicyText: 
+        Version: "2012-10-17"
+        Statement: 
+          - 
+            Sid: AllowPushPull
+            Effect: Allow
+            Principal: "*"
+            #Principal: 
+            #   AWS: 
+            #     - "arn:aws:iam::123456789012:user/Bob"
+            #     - "arn:aws:iam::123456789012:user/Alice"
+            Action: 
+              - "ecr:GetDownloadUrlForLayer"
+              - "ecr:BatchGetImage"
+              - "ecr:BatchCheckLayerAvailability"
+              - "ecr:PutImage"
+              - "ecr:InitiateLayerUpload"
+              - "ecr:UploadLayerPart"
+              - "ecr:CompleteLayerUpload"
+            Condition:
+              ForAllValues:StringEquals:
+                aws:PrincipalOrgID: "o-12345678"
\ No newline at end of file


Code review comments:
diff_hunk: @@ -27,13 +27,25 @@ def scan_resource_conf(self, conf):
                 statement = policy['Statement'][0]
                 if statement['Principal'] and type(statement['Principal']) is str:
                     principal = statement['Principal']
-                    if principal == "*":
+                    if principal == "*"  and self.check_for_constrained_condition(statement) == False:
path: checkov/terraform/checks/resource/aws/ECRPolicy.py
text: @user1:
```suggestion
                    if principal == "*"  and not self.check_for_constrained_condition(statement):
```

@author:
Done

diff_hunk: @@ -27,13 +27,25 @@ def scan_resource_conf(self, conf):
                 statement = policy['Statement'][0]
                 if statement['Principal'] and type(statement['Principal']) is str:
                     principal = statement['Principal']
-                    if principal == "*":
+                    if principal == "*"  and self.check_for_constrained_condition(statement) == False:
                         self.evaluated_keys = ["policy/Statement/Principal"]
                         return CheckResult.FAILED
         return CheckResult.PASSED
 
     def get_evaluated_keys(self) -> List[str]:
         return ['policy']
 
+    def check_for_constrained_condition(self, statement):
+        """
+        Checks to see if there is a constraint on a a wildcarded principal
+        :param statement: statement from aws_repository_configuration
+        :return: True if there is a constraint
+        """
+        if statement['Condition'] and type(statement['Condition']) is dict:
+          condition = statement['Condition']
+          if condition['ForAllValues:StringEquals'] and type(condition['ForAllValues:StringEquals']) is dict:
path: checkov/terraform/checks/resource/aws/ECRPolicy.py
text: @user1:
same here, check if the key exists and then check if it is an instance of `dict`

@author:
Done

diff_hunk: @@ -0,0 +1,28 @@
+AWSTemplateFormatVersion: "2010-09-09"
+Resources: 
+  MyRepository: 
+    Type: AWS::ECR::Repository
+    Properties: 
+      RepositoryName: "test-repository"
+      RepositoryPolicyText: 
+        Version: "2012-10-17"
+        Statement: 
+          - 
+            Sid: AllowPushPull
+            Effect: Allow
+            Principal: "*"
+            #Principal: 
+            #   AWS: 
+            #     - "arn:aws:iam::123456789012:user/Bob"
+            #     - "arn:aws:iam::123456789012:user/Alice"
path: tests/cloudformation/checks/resource/aws/example_ECRPolicy/ECRPolicy-PASSED-2.yaml
text: @user1:
just remove the comment, not needed anymore 🙂 

@author:
Done

diff_hunk: @@ -27,13 +27,25 @@ def scan_resource_conf(self, conf):
                 statement = policy['Statement'][0]
                 if statement['Principal'] and type(statement['Principal']) is str:
                     principal = statement['Principal']
-                    if principal == "*":
+                    if principal == "*"  and self.check_for_constrained_condition(statement) == False:
                         self.evaluated_keys = ["policy/Statement/Principal"]
                         return CheckResult.FAILED
         return CheckResult.PASSED
 
     def get_evaluated_keys(self) -> List[str]:
         return ['policy']
 
+    def check_for_constrained_condition(self, statement):
+        """
+        Checks to see if there is a constraint on a a wildcarded principal
+        :param statement: statement from aws_repository_configuration
+        :return: True if there is a constraint
+        """
+        if statement['Condition'] and type(statement['Condition']) is dict:
path: checkov/terraform/checks/resource/aws/ECRPolicy.py
text: @user1:
```suggestion
        if 'Condition' in statement and isinstance(statement['Condition'], dict):
```

@author:
Done

diff_hunk: @@ -31,9 +31,22 @@ def scan_resource_conf(self, conf):
                     for statement_index, statement in enumerate(policy_text['Statement']):
                         if 'Principal' in statement.keys():
                             for principal_index, principal in enumerate(statement['Principal']):
-                                if principal == "*":
+                                if principal == "*" and self.check_for_constrained_condition(statement) == False:
path: checkov/cloudformation/checks/resource/aws/ECRPolicy.py
text: @user1:
```suggestion
                                if principal == "*" and not self.check_for_constrained_condition(statement):
```

@author:
Done