[GENERAL ISSUE] Question: how to filter by logical ID

[polothy]

Describe the issue

Attempting to author a rule that verifies that a AWS::EC2::FlowLog has been created for every AWS::EC2::VPC. See the below example, but the problem is likely in the rule where I have let flow_log defined. I'm trying to lookup the FlowLog by the logical ID of the VPC. I have seen examples of these logical ID lookups, but in the reverse order (EG: loop through FlowLogs and find VPCs).

Any examples

Guard rule:

#
# Select all VPCs
#
let vpcs = Resources.*[ Type == 'AWS::EC2::VPC' 
  Metadata.guard.SuppressedRules not exists or 
  Metadata.guard.SuppressedRules.* != "VPC_FLOW_LOGS_ENABLED"
]

rule VPC_FLOW_LOGS_ENABLED when %vpcs !empty {
  let flow_log = Resources.*[ Type == 'AWS::EC2::FlowLog'
    some Properties.ResourceId.Ref == %vpcs
  ]
  %flow_log !empty
  <<
  	Violation: VPC must have flow logs enabled
  	Fix: create a AWS::EC2::FlowLog for the VPC
  >>
}

Tests:

###
# VPC_FLOW_LOGS_ENABLED tests
###
---
- name: Empty, SKIP
  input: {}
  expectations:
    rules:
      VPC_FLOW_LOGS_ENABLED: SKIP

- name: No resources, SKIP
  input:
    Resources: {}
  expectations:
    rules:
      VPC_FLOW_LOGS_ENABLED: SKIP

- name: VPC and Flow Logs resource, PASS
  input:
    Resources:
      Vpc:
        Type: AWS::EC2::VPC
        Properties:
          CidrBlock: 10.0.0.0/16
          EnableDnsHostnames: true
          EnableDnsSupport: true
          InstanceTenancy: default
      FlowLog:
        Type: AWS::EC2::FlowLog
        Properties:
          ResourceId:
            Ref: Vpc
          ResourceType: VPC
          TrafficType: ALL
          DeliverLogsPermissionArn:
            Fn::GetAtt:
              - IamRole
              - Arn
          LogDestinationType: cloud-watch-logs
          LogGroupName:
            Ref: LogGroup
  expectations:
    rules:
      VPC_FLOW_LOGS_ENABLED: PASS

- name: VPC with Flow Logs resource missing, FAIL
  input:
    Resources:
      Vpc:
        Type: AWS::EC2::VPC
        Properties:
          CidrBlock: 10.0.0.0/16
          EnableDnsHostnames: true
          EnableDnsSupport: true
          InstanceTenancy: default
  expectations:
    rules:
      VPC_FLOW_LOGS_ENABLED: FAIL

Operating System:
MacOS

OS Version
Monterey

Additional context
None.

[polothy]

Hey @akshayrane - just noticed that you assigned this to yourself. Is there a quick response to this question? Just a "yes, there is a way" or "no, this requires (big/small) changes to Guard"? Been evaluating Guard and cdk-nag for compliance enforcement/checking and I ran into this issue.

[akshayrane]

Hi @polothy

Thanks for reaching out. Short answer is yes, there is a way to extract a resource using its logical name and add checks on it. However, we are evaluating at the moment how can it be extended to meet your exact use case.

Here's a sample rule to extract a resource with known pattern (this could be a regular expression keys == /<your-regex-here>/) and add check for a specific property, rule.guard:

let flow_logs = Resources[ keys == /FlowLog/ ]
rule ensure_logs_have_some_props when %flow_logs !empty {
    %flow_logs.Properties{
        TrafficType == 'ALL'
        <<
            Violation: TrafficType should be ALL
        >>
    }
}

Corresponding data template, template.yml

Resources:
  Vpc:
    Type: AWS::EC2::VPC
    Properties:
      CidrBlock: 10.0.0.0/16
      EnableDnsHostnames: true
      EnableDnsSupport: true
      InstanceTenancy: default
  FlowLog:
    Type: AWS::EC2::FlowLog
    Properties:
      ResourceId:
        Ref: Vpc
      ResourceType: VPC
      TrafficType: ALL
      DeliverLogsPermissionArn:
        Fn::GetAtt:
          - IamRole
          - Arn
      LogDestinationType: cloud-watch-logs
      LogGroupName:
        Ref: LogGroup

Command

cfn-guard validate \
	-d template.yml \
	-r rule.guard \
 	--show-summary all

Output

template.yml Status = PASS
PASS rules
rule.guard/ensure_logs_have_some_props    PASS

If you see a way you can re-use this functionality and solve your problem do let us know how your final rule looks like.

HTH,
Akshay

[polothy]

Hey @akshayrane - thanks for commenting back, very much appreciated 😄

Hrm, I'm not sure how this would help. I cannot make any assumptions about the logical ID because we have a wide range of CFN templates and we also use CDK (logical IDs can be pretty wild). In addition, I'd like to make sure that the VPC and the flow log are connected (the logical ID of the VPC is in the definition of the flow log).

The rule I'm trying to write would cover this control: https://docs.aws.amazon.com/securityhub/latest/userguide/securityhub-cis-controls.html#securityhub-cis-controls-2.9

I think the problem I'm having would also be experienced when trying to write this rule. It's a common pattern in CFN where you define a resource and then "attach it" to another resource. So, solving this sort of tracing/linking problem would help with a lot of use cases as you build out the rules registry.

Cheers and thanks for your continued help on this!

[polothy]

Found another example that I think would be resolvable if this issue was fixed: aws-cloudformation/aws-guard-rules-registry#240

[polothy]

@akshayrane hello, with v3 released, would this issue be addressed soon'ish?

[akshayrane]

Hi @polothy

This should be possible to do, referring to a logical ID from a different resource and checking properties for this linked resource. But we would need to address the possibility of reference NOT being used elsewhere, for example, some Flow Logs may not have a reference to another VPC but rather a hard-coded string name of another VPC resource (this use case isn't handled in the following example).

The rule would look like:

let flow_logs = Resources.*[ Type == 'AWS::EC2::FlowLog' ]

rule ensure_logs_have_some_props when %flow_logs !empty {
    let vpc_logical_id = %flow_logs.Properties.ResourceId.Ref

    when %vpc_logical_id exists {
        let vpc = Resources[ keys == %vpc_logical_id ]

        %vpc.Properties.EnableDnsHostnames == true
        <<
            Violation: EnableDnsHostnames should be false
        >>
    }

}

The template is:

Resources:
  Vpc:
    Type: AWS::EC2::VPC
    Properties:
      CidrBlock: 10.0.0.0/16
      EnableDnsHostnames: true
      EnableDnsSupport: true
      InstanceTenancy: default
  FlowLog:
    Type: AWS::EC2::FlowLog
    Properties:
      ResourceId:
        Ref: Vpc
      ResourceType: VPC
      TrafficType: ALL
      DeliverLogsPermissionArn:
        Fn::GetAtt:
          - IamRole
          - Arn
      LogDestinationType: cloud-watch-logs
      LogGroupName:
        Ref: LogGroup
  FlowLog2:
    Type: AWS::EC2::FlowLog
    Properties:
      ResourceId:
        !Ref Vpc
      ResourceType: VPC
      TrafficType: ALL
      DeliverLogsPermissionArn:
        Fn::GetAtt:
          - IamRole
          - Arn
      LogDestinationType: cloud-watch-logs
      LogGroupName:
        Ref: LogGroup

Output:

$ cfn-guard validate -d data/issue-267.yml -r rules/issue-267.guard -S all
issue-267.yml Status = PASS
PASS rules
issue-267.guard/ensure_logs_have_some_props    PASS
---

Please let us know if it's okay to mark the issue as resolved.

HTH,
Akshay

[polothy]

Hey @akshayrane - thank you so much for the followup!

The security control that we are enforcing says that all VPCs must have flow logs. The above example would pass if I just had a VPC defined but no flow logs defined. We use the same Guard file across all our microservices. Most microservices do not have a VPC defined, so we cannot assume there is always at least one VPC in the template.

Same exact scenario with the security control where S3 bucket must only accept SSL connections. For each S3 bucket, we must look up the bucket policy resource and see if there is a statement enforcing SSL.

This pattern repeats a lot throughout CloudFormation.

[joshfried-aws]

Hi @polothy does this better handle your use case? I built upon what my colleague @akshayrane had posted before.

This handles all the test cases in your provided example. If vpcs are present, flow logs must be present as well. It also ensures that every referenced VPC logical id for each flow log is unique meaning 2 flow logs cannot reference the same VPC (not sure if this was a requirement i have left a comment in the rule to help you remove it if need be). This also means that the number of Flow Log resource types == the number of VPC resource types. We are able to do this by querying for all the ref'd VPCs and then ensuring that they're only present one time in our query (using the new builtin count function). We then loop over each defined Flow Log resource and ensure that the we're able to map the referenced logical id, we then validate the property as shown below.

Please note since this does use the new count function this will only work on cfn-guard v3.x.x and above

let vpcs = Resources.*[ Type == "AWS::EC2::VPC" ]

rule VPC_FLOW_LOGS_ENABLED when %vpcs !empty {
    let flow_logs = Resources.*[ Type == 'AWS::EC2::FlowLog' ]
    %flow_logs !empty
    
    let res = Resources # have to do this because of the loop below, will lose the 'root context'

    %flow_logs.Properties.ResourceId.Ref exists

    let refs = %flow_logs.Properties.ResourceId.Ref

    # this also makes sure that all refs are unique logical ids, can omit if this is not a necessary requirement
    %refs {
        let curr_ref = this 
        let check = %refs[ this == %curr_ref ] 

        let num = count(%check)

        %num == 1
    }

    %flow_logs {
        let vpc_logical_id = this.Properties.ResourceId.Ref
        %vpc_logical_id exists

        let vpc = %res[ keys == %vpc_logical_id ]
        
        %vpc.Properties.EnableDnsHostnames == true
        <<
            Violation: EnableDnsHostnames should be false
        >>
    }
}

Please let me know if this answer was sufficient.

Thanks,

[polothy]

Hey Josh, thanks for the reply. It does not appear to attempt to create a 1 to 1 relationship. For example, this does not pass:

- name: Two VPCc and only one Flow Logs resource, FAIL
  input:
    Resources:
      Vpc:
        Type: AWS::EC2::VPC
        Properties:
          CidrBlock: 10.0.0.0/16
          EnableDnsHostnames: true
          EnableDnsSupport: true
          InstanceTenancy: default
      Vpc2:
        Type: AWS::EC2::VPC
        Properties:
          CidrBlock: 10.0.0.0/16
          EnableDnsHostnames: true
          EnableDnsSupport: true
          InstanceTenancy: default
      FlowLog:
        Type: AWS::EC2::FlowLog
        Properties:
          ResourceId:
            Ref: Vpc2
          ResourceType: VPC
          TrafficType: ALL
          DeliverLogsPermissionArn:
            Fn::GetAtt:
              - IamRole
              - Arn
          LogDestinationType: cloud-watch-logs
          LogGroupName:
            Ref: LogGroup
  expectations:
    rules:
      VPC_FLOW_LOGS_ENABLED: FAIL

2 VPCs, but only 1 flow logs.

Just a general feedback type of thing, this pattern repeats throughout CloudFormation a lot. So, while a workaround is nice, a concise way to express/enforce this pattern would be very helpful because this will be repeated a lot across many rules.

Cheers and thanks!

[joshfried-aws]

Hey @polothy a fix to that would just be to run a count on both queries for the resource types and ensure they're equal.

That said I do see your point about this being a common pattern. Would love to hear any suggestions you might have as a way to add support for something like this within the DSL.

[polothy]

First off, I'm not good at Guard syntax, I mostly copy/paste/modify :) But let's take this for an example because this is what I was trying to do originally:

#
# Select all VPCs
#
let vpcs = Resources.*[ Type == 'AWS::EC2::VPC' 
  Metadata.guard.SuppressedRules not exists or 
  Metadata.guard.SuppressedRules.* != "VPC_FLOW_LOGS_ENABLED"
]

rule VPC_FLOW_LOGS_ENABLED when %vpcs !empty {
  %vpcs {
    let flow_log = Resources.*[ Type == 'AWS::EC2::FlowLog'
      some Properties.ResourceId.Ref == KEY_OF_CURRENT_VPC
    ]
    %flow_log !empty
    <<
    	Violation: VPC must have flow logs enabled
    	Fix: create a AWS::EC2::FlowLog for the VPC
    >>
  }
}

But, I believe this is looping right?

%vpcs {
   # I'm now in a loops of vpcs and `this` refers to current VPC in the iteration
}

So, we just need a backwards compatible way to get the key of this where I have KEY_OF_CURRENT_VPC in the above rule. I'm not sure what this would be... like [key this] or this[ key ].

But, the example hopefully shows the intent. I select a bunch of resources and for each, I want to enforce a rule. In this case, I need to query another Resource type based on the key (AKA Logical ID) of the original resource.

Another way would be to get the keys from a list, EG:

#
# Select all VPCs
#
let vpcs = Resources.*[ Type == 'AWS::EC2::VPC' 
  Metadata.guard.SuppressedRules not exists or 
  Metadata.guard.SuppressedRules.* != "VPC_FLOW_LOGS_ENABLED"
]

rule VPC_FLOW_LOGS_ENABLED when %vpcs !empty {
  let vpcids = keys %vpcs
  %vpcids {
    let flow_log = Resources.*[ Type == 'AWS::EC2::FlowLog'
      some Properties.ResourceId.Ref == this
    ]
    %flow_log !empty
    <<
    	Violation: VPC must have flow logs enabled
    	Fix: create a AWS::EC2::FlowLog for the VPC
    >>
  }
}

Where keys %vpcs is the example of the new syntax that would be whatever you like, but basically returns a list of strings of the Logical IDs of all the VPCs.

Either of these would get the job done, but in the end, might want both options.

Let me know if this makes any sense at all, I appreciate your help!

[polothy]

Hello, do my suggestions make any sense? Would love to have this resolved early this year.

[joshfried-aws]

Hey @polothy sorry for the delayed response, wish I could have gotten back to you sooner.

If i understand the ask correctly, you would like for us to introduce a functionality that would retrieve the parent of any given node?

[polothy]

Well, it's the key right? An associative array where the keys are the Logical ID and the values are the resources. Right now, no way to get the key (AKA logical ID).

[joshfried-aws]

Yeah my bad. Key is a better way to describe it. To be clear this will not be logical id specific, and just would be a way for a user to get the key.

[joshfried-aws]

@polothy would you mind if i close out this issue, and create a new enhancement issue for this feature?

[polothy]

Yup, not a problem 👍

[polothy]

Yeah my bad. Key is a better way to describe it. To be clear this will not be logical id specific, and just would be a way for a user to get the key.

Yes, totally onboard with that!

[joshfried-aws]

Hey @polothy I have now created #476 to track this enhancement request, please feel free to provide any more context you'd like / let me know if theres anything i missed!