Create a tunable "too open ingress" rule / Community rules collections

[KevinHock]

I want e.g. the SecurityGroupUnrestrictedIngress22 check, but on any port, and I want to pass in as an argument a list of CIDRs that I think are too open, aside from just 0.0.0.0/0.

(I can imagine folks wanting a "no bigger than /N" rule too, where N could be e.g. a /32.)

[KevinHock]

Here is some sample code, you could also make it tunable for egress.

from checkov.common.models.enums import CheckResult, CheckCategories
from checkov.common.util.type_forcers import force_list
from checkov.terraform.checks.resource.base_resource_check import BaseResourceCheck

# TODO: Make this a CLI argument
TOO_OPEN_CIDRS = (
    "0.0.0.0/0",
    "...",
)

class SecurityGroupTooOpen(BaseResourceCheck):
    def __init__(self):
        name = "Ensure every security group ingress is not too open"
        id = "CKV_AWS_???"
        supported_resource = [
            'aws_security_group',
            'aws_security_group_rule',
            'aws_db_security_group',
            'aws_elasticache_security_group',
            'aws_redshift_security_group',
        ]
        categories = [CheckCategories.NETWORKING]
        super().__init__(name=name, id=id, categories=categories, supported_resources=supported_resource)

    def scan_resource_conf(self, conf):
        """
        Looks at configurations for security group ingress:
            https://www.terraform.io/docs/providers/aws/r/security_group.html
            https://www.terraform.io/docs/providers/aws/r/security_group_rule.html
            https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_security_group
            https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/elasticache_security_group
            https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/redshift_security_group

        :param conf: a security group (any type) or security rule configuration
        :return: <CheckResult>
        """
        result = CheckResult.PASSED

        # This means it's a security group type resource with ingress block(s)
        if 'ingress' in conf:
            result = self.analyze_security_group_ingress_conf(conf['ingress'])

        # This means it's a 'aws_security_group_rule' resource.
        if 'type' in conf:
            result = self.analyze_security_group_rule(conf)

        return result

    def analyze_security_group_ingress_conf(self, ingress_conf):
        for ingress_rule in ingress_conf:
            # There can be
            #   resource "aws_security_group" "inline_rules" {
            #       ingress = [
            #       {
            # vs.
            #   resource "aws_security_group" "bar-sg" {
            #       ingress {
            # This covers the 1st case
            if isinstance(ingress_rule, list):
                assert len(ingress_rule) == 1
                ingress_rule = ingress_rule[0]
            if (
                not isinstance(ingress_rule, dict)
                or
                # Not checking 'ipv6_cidr_blocks' or 'prefix_list_ids'
                'cidr_blocks' not in ingress_rule.keys()
            ):
                continue

            ingress_rules = force_list(ingress_rule)
            for rule in ingress_rules:
                if isinstance(rule, dict):
                    if self.contains_violation(rule):
                        return CheckResult.FAILED

        return CheckResult.PASSED

    def analyze_security_group_rule(self, conf):
        rule_type = force_list(conf['type'])[0]
        if rule_type == 'ingress':
            return (
                CheckResult.FAILED
                if self.contains_violation(conf)
                else CheckResult.PASSED
            )
        return CheckResult.UNKNOWN

    def contains_violation(self, conf):
        cidr_blocks = force_list(conf.get('cidr_blocks', [[]])[0])
        return any(
            too_open_cidr in cidr_blocks
            for too_open_cidr in TOO_OPEN_CIDRS
        )

check = SecurityGroupTooOpen()

[schosterbarak]

might be easier to implement TOO_OPEN_CIDRS as an environment variable.

@KevinHock it seems that you have a lot of good usecases for "custom" checks. maybe we should create a sister repo to checkov with some of those advanced checks? another option is to have it as a directory within checkov, under /docs as set of examples of custom checks?

cc: @robeden @metahertz WDYT?

[robeden]

What about something like this?...

First step would be to allow for overriding a method on the check so a custom check could be written to validate a range. Something like:

def is_range_acceptable(self, range: str) -> bool:
    return range != “0.0.0.0/0”

(Haven’t looked, but I’m assuming the data is strings right now.)

Then I could definitely see a need for customize the behavior of certain checks. We might want to make a somewhat predicable scheme for doing so. Maybe something like -Xaws123.largest_mask=16 (off the top of my head, not tied to that exact syntax).

I like the idea of “extra” or “opt-in” checks, although I think this one could take the place of the “open ingress” check if the mask size defaulted to “0”. If we do want an optional check, we could control what’s auto-run by the ID (hacky, but...). Something like “CKV_AWS_EXT_123”? 🤷‍♂️

[KevinHock]

I like the idea of extra opt-in checks too, maybe enabled via an args.flag, like Parliament https://github.com/duo-labs/parliament/tree/main/parliament/community_auditors

I kind of lean into keeping these in the checkov repo, but disabled by default, because tests need to be written for the custom checks, and the test might become out-of-sync later rather than sooner, if the custom checks are in a separate repo.

[robeden]

Related to this: I've been going through our old tool and moving stuff over to Checkov, and suddenly I have quite a few candidates for the kind of checks we're talking about: where they might not be security per-se and might not be desired by everyone, but are definitely useful. Some random examples:

• RDS: allow minor version upgrades
• RDS: deny major version upgrade
• RDS: copy tags to snapshot
• RDS: allowed instance types check (this would be good with the parameters we've been discussing)
• Lambda: has accompanying CloudWatch groups

[metahertz]

On @KevinHock 's point:

I like the idea of extra opt-in checks too, maybe enabled via an args.flag, like Parliament https://github.com/duo-labs/parliament/tree/main/parliament/community_auditors

I kind of lean into keeping these in the checkov repo, but disabled by default, because tests need to be written for the custom checks, and the test might become out-of-sync later rather than sooner, if the custom checks are in a separate repo.

We'd need to look at CI, to prevent commits to that dir spitting out a new version of chekov for addition of custom checks. Would you see these custom ones consumed via the --external-checks style git pull? or baked into checkov as per regular checks?

[stale[bot]]

Thanks for contributing to Checkov! We've automatically marked this issue as stale to keep our issues list tidy, because it has not had any activity for 6 months. It will be closed in 14 days if no further activity occurs. Commenting on this issue will remove the stale tag. If you want to talk through the issue or help us understand the priority and context, feel free to add a comment or join us in the Checkov slack channel at https://slack.bridgecrew.io Thanks!

[stale[bot]]

Closing issue due to inactivity. If you feel this is in error, please re-open, or reach out to the community via slack: https://slack.bridgecrew.io Thanks!