-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(terraform): adding 3 policies & tests #7011
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Added some suggested changes. Also, in the description for this PR, please add a map of the CKV ID to Prisma Policy ID if these are translated policies. For example, CKV_AZURE_250
translates ddf89efb-979f-412d-8e62-5ffa8d388e2c
super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resources) | ||
|
||
def scan_resource_conf(self, conf: Dict[str, Any]) -> CheckResult: | ||
if "image_owner_alias" in conf or 'owner_id' in conf: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are Attribute Reference attributes, so they are not written by the user for the resource type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yet if they are specified (doesn't matter the actual value of it) its a best practice to avoid WhoAMI attack. So if it is declared it's enough in order to pass the policy. is that makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm saying that a user can't specify them in their Terraform code, so this would only apply to plan file scans. This check is effectively just looking for * in name and I don't think that is the intention.
categories = [CheckCategories.NETWORKING] | ||
super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resources) | ||
|
||
def scan_resource_conf(self, conf: Dict[str, Any]) -> CheckResult: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
from typing import Dict, Any | ||
|
||
from checkov.common.models.enums import CheckResult, CheckCategories | ||
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomerSegev241 users can't define owner in the resource type. I think you need a data type check instead.
@@ -0,0 +1,29 @@ | |||
# DataCatalogWithPublicAccess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this comment
checkov/terraform/checks/resource/azure/StorageSyncServicePermissiveAccess.py
Outdated
Show resolved
Hide resolved
checkov/terraform/checks/resource/oci/DataCatalogWithPublicAccess.py
Outdated
Show resolved
Hide resolved
else: | ||
return CheckResult.FAILED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary, but ok
checkov/terraform/checks/resource/azure/VMDiskWithPublicAccess.py
Outdated
Show resolved
Hide resolved
checkov/terraform/checks/resource/oci/DataCatalogWithPublicAccess.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Taylor <[email protected]>
…issiveAccess.py Co-authored-by: Taylor <[email protected]>
Co-authored-by: Taylor <[email protected]>
…ess.py Co-authored-by: Taylor <[email protected]>
Co-authored-by: Taylor <[email protected]>
…ess.py Co-authored-by: Taylor <[email protected]>
User description
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
New/Edited policies (Delete if not relevant)
Description
Include a description of what makes it a violation and any relevant external links.
Fix
How does someone fix the issue in code and/or in runtime?
Checklist:
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Implements three new security policies for Terraform resources and adds corresponding tests. The policies address potential vulnerabilities in AWS AMI configuration, Azure Storage Sync Service and Managed Disk network access, and OCI Data Catalog public access. New classes are created to perform these checks, along with test files and example configurations to validate the policies.
Modified files (4)
Latest Contributors(0)
Modified files (8)
Latest Contributors(0)