[RFC] Block Evaluation

[johnttompkins]

Issue #, if available: #105, #79, #45

Description of changes: This request for comments details proposed enhancements of block evaluations, allowing rule authors to make more composable, less redundant, and unambiguous rules using named rules, type blocks, and conditional blocks. Full details on the proposal are in the below file.

All feedback is welcome on these proposed features, particularly on type blocks and the divergence in behavior detailed below on setting the root object for evaluation to the parent of the properties object of a resource. (Previously was the properties object)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[benbridts]

Would it make sense to allow setting strict checking in a block?

Being able to specify that a certain block needs strict checking (or doesn't need it), would ensure that rules are always evaluated the same way.
And if strict is always determined by the person writing the rules, it should probably be removed as an option that can be specified on the command line.

Some other remarks related to rule meta-data:

• Would an (optional) ID next to a rule name be helpful for the output, or would that make it more confusing?
• How about a "level" (warn vs enforce), If Guard at some point in the future can autodetect rule files, it might be nice to have both of them in a single file (for now we can have a warnings ruleset and an errors ruleset and only stop the deploy if the latter finds something)

[PatMyron]

If Guard at some point in the future can autodetect rule files

think finalizing ruleset file extensions is the key to this, along with autocompletion / syntax highlighting / validating rulesets

[PatMyron]

[copied from our previous discussion] think there's multiple sides to this. Lines being their own rules that can be evaluated independently and being able to sort and autocomplete rules more easily is arguably simpler

[PatMyron]

Personally would prefer not having to specify the top-level Properties in almost every rule

[jeromevdl]

Why not using the spring boot yaml format or sort of, with indentation for each sub-level. The Properties would be visible once, and same for each sub-level. Last sample could be simplified like this:

AWS::EC2::Instance {                      
    Properties {
        BlockDeviceMappings {
           *.Ebs != null
           *.device_name == /^\/dev\/ebs-/
           *.Ebs.encrypted == true
           *.Ebs.delete_on_termination == true
        }
    }
}

Not sure however if the * could be used for a block...

[johnttompkins]

I like this way of specifying a lot to reduce repetition. Let me see how we can incorporate this into the block evaluation.

[shreyasdamle]

Is it possible to add details on rule suppression by resource in this RFC? In some cases, security engineers would like to give developers an option to skip a certain rule on a specific resource. For example, if a developer has two S3 buckets (bucket1 and bucket2) in their CFN stack and they would like to enforce bucket encryption only on bucket1, there should be a way to suppress a check for bucket2. This can be implemented by reading a metadata field discussed in #105

On the contrary, does it make sense to add an optional condition to the rule grammar for cases where rule suppression should not be allowed? For example, if some organization wants to enforce a policy that S3 buckets must never allow world read/write, so in this case, a rule block should allow a field like suppression_allowed=false so that developers can never suppress this check.

[johnttompkins]

Is it possible to add details on rule suppression by resource in this RFC? In some cases, security engineers would like to give developers an option to skip a certain rule on a specific resource. For example, if a developer has two S3 buckets (bucket1 and bucket2) in their CFN stack and they would like to enforce bucket encryption only on bucket1, there should be a way to suppress a check for bucket2. This can be implemented by reading a metadata field discussed in #105

On the contrary, does it make sense to add an optional condition to the rule grammar for cases where rule suppression should not be allowed? For example, if some organization wants to enforce a policy that S3 buckets must never allow world read/write, so in this case, a rule block should allow a field like suppression_allowed=false so that developers can never suppress this check.

i think you should be able to write something of the following where either the check is satisfied or the rule is explicitly suppressed.

For example, with the following template:

Resources:
    BucketRuleNotEnforced:
        Type: AWS::S3::Bucket
        Properties:
            AccessControl: public-read-write
        Metadata:
            cfn-guard:
                SuppressedRules:
                - public_read_write_rule
    BucketRuleEnforced:
        Type: AWS::S3::Bucket
        Properties:
            AccessControl: public-read-write

one could author a ruleset that can apply a rule named public_read_write_rule only if it is not suppressed

rule public_read_write_rule {
    AWS::S3::Bucket {
        Properties.AccessControl != "public-read-write" or
        Metadata.cfn-guard.SuppressedRules[*] == "public_read_write_rule"
    }
}

This would have to be done for every rule so it isn't streamlined right now, but it makes it explicity clear what the rule block allows. No magic variables or anything; if you don't want to allow suppression on the rule, you'd just remove the Metadata check line

[johnttompkins]

Would it make sense to allow setting strict checking in a block?

Being able to specify that a certain block needs strict checking (or doesn't need it), would ensure that rules are always evaluated the same way.
And if strict is always determined by the person writing the rules, it should probably be removed as an option that can be specified on the command line.

Some other remarks related to rule meta-data:

* Would an (optional) ID next to a rule name be helpful for the output, or would that make it more confusing?

* How about a "level" (warn vs enforce), If Guard at some point in the future can autodetect rule files, it might be nice to have both of them in a single file (for now we can have a warnings ruleset and an errors ruleset and only stop the deploy if the latter finds something)

Would it make sense to allow setting strict checking in a block?

Being able to specify that a certain block needs strict checking (or doesn't need it), would ensure that rules are always evaluated the same way.
And if strict is always determined by the person writing the rules, it should probably be removed as an option that can be specified on the command line.

Some other remarks related to rule meta-data:

* Would an (optional) ID next to a rule name be helpful for the output, or would that make it more confusing?

* How about a "level" (warn vs enforce), If Guard at some point in the future can autodetect rule files, it might be nice to have both of them in a single file (for now we can have a warnings ruleset and an errors ruleset and only stop the deploy if the latter finds something)

I think as far as levels go, a way to achieve this with the current setup is to group rules of a certain level together in a block which would represent the overall status for said level.

For example:

rule rule_warn_1 {...}
rule rule_warn_2 {...}
rule rule_error_a {...}
rule rule_error_b {...}

rule evaluate_warn {
    rule_warn_1
    rule_warn_2
}

rule evaluate_error {
    rule_error_1
    rule_error_2
}

In the output for evaluation, as per the RFC, there will be some output of the form:

Ruleset RULESET_FILE_NAME: Overall Status FAILED
... # information on each rule result
/evaluate_warn: PASSED
/evaluate_warn/rule_warn_1: PASSED
/evaluate_warn/rule_warn_2: PASSED
/evaluate_error: FAILED
/evaluate_error/rule_error_1: PASSED
/evaluate_error/rule_error_2: FAILED
...

If we also offer multiple evaluation result formats like JSON with this enhanced output, end users could define their own rule block "levels" and choose which rule evaluations to consider when running guard.

[benbridts]

I think both my remark about levels and @shreyasdamle's about supression, point to a similar (potential) feature.

It does not have to be part of this RFC, but in both cases, we're looking at adding metadata to rules, so that cfn-guard can treat failures differently depending on the metadata.

It's worth considering how the language could support those cases, even when not implemented right away (if ever).

Additionally, I'd argue that to meet all the tenants, strict checking should be removed as a cli option and added to the rules themself (and it makes sense to do that on block-level, or globally)

[epsstan]

I'd rather see the rule syntax support parenthesis. The CNF form, the explicit OR vs implicit AND etc are new rules that developers have to learn. Yes it can be abused to write complex rules. But that should be caught by normal s/w sdlc, code reviews etc. I personally prefer this construction.

rule example_rule when stage == 'prod' {
        let ec2_instance_types := [/^t*/, /^m*/]  
	
	dependent_rule
	AND
        AWS::EC2::Instance InstanceType IN %ec2_instance_types 
	AND 
	(
		AWS::EC2::Instance {                         
			xxxxx 
		}
		OR		
		AWS::EC2::Instance {        
                        yyyyy    
    		}
	)
}

[johnttompkins]

so with this construction, you would have to specify AND and couldn't implicitly rely on the next rule being ANDed, right? I think as long as it is explicit blocks being ANDed makes sense. this seems a repetitive, but with the added benefit of being explicit as possible.

[johnttompkins]

I think both my remark about levels and @shreyasdamle's about supression, point to a similar (potential) feature.

It does not have to be part of this RFC, but in both cases, we're looking at adding metadata to rules, so that cfn-guard can treat failures differently depending on the metadata.

It's worth considering how the language could support those cases, even when not implemented right away (if ever).

Additionally, I'd argue that to meet all the tenants, strict checking should be removed as a cli option and added to the rules themself (and it makes sense to do that on block-level, or globally)

I agree with removing strict checks as a cli option. Without it, rulesets are not deteministic. Planning on following up with another RFC more related to enhancing property access in templates and introducing more operations like existence checks (AWS::S3::Bucket PropertyA EXISTS/ AWS::S3::Bucket PropertyA != null). If we have those, ruleset authors should be able to craft rules that achieve parity with existing strict checked rulesets.

[GriffinMB]

Some other points I'd love to see addressed (though perhaps not part of this RFC), are:

• Existential checks, as in "There exists at least one X with Y property". For instance, to validate that S3/Dynamodb vpc endpoints have been created.
• Dynamic variables so that you can make complex rules, such as "This instance is in a subnet, which has a route table that is not associated with an internet gateway"

[johnttompkins]

Some other points I'd love to see addressed (though perhaps not part of this RFC), are:

* Existential checks, as in "There exists at least one X with Y property". For instance, to validate that S3/Dynamodb vpc endpoints have been created.

* Dynamic variables so that you can make complex rules, such as "This instance is in a subnet, which has a route table that is not associated with an internet gateway"

I think #115 should address some of these concerns such as existence checks and dynamic variables. We are planning on introducing advanced filtering to select certain items in a template and then do checks on said items after filtering.

[priyap286]

All issues linked in this RFC PR have been addressed. I am merging this RFC to the Guard1.0 branch.