feat(misconf): Filtering findings for Terraform modules based on attributes

[simar7]

TODO:

• Trivy should support accessing individual blocks in the case of for_each or count variables. This would allow to target specific resources such as described in the example below.
• In order to specify more granular ignores for the above attributes, Trivy should allow such a configuration to be passed in via the .trivyignore.yaml format for ease of use and readability.
• While displaying the misconfigurations for such cases, we should expand the blocks to specifically mention which iterated item is being shown.

Discussed in #7157

^{Originally posted by acdha July 12, 2024}

Description

Like many people, I use the terraform-aws-modules/vpc/aws module and that module creates a variety of resources using large lists for things like ACLs:

module "vpc" {
  source                 = "terraform-aws-modules/vpc/aws"
  version                = "~> 5.8"
…
  public_inbound_acl_rules = concat(
    [
      for i in range(length(local.default_ingress_acls)) :
      merge(
        {
          rule_number = 1000 + i,
          rule_action = local.default_ingress_acls[i]["action"]
        },
        local.default_ingress_acls[i]
      )
    ],
…

This creates a problem on every project when using Trivy because you'll get findings for the resources created using count or for_each blocks inside the module based on those lists:

CRITICAL: Network ACL rule allows ingress from public internet.

Opening up ACLs to the public internet is potentially dangerous. You should restrict access to IP addresses or ranges that explicitly require it where possible.

See https://avd.aquasec.com/misconfig/avd-aws-0105

 terraform-aws-modules/vpc/aws/.terraform/modules/vpc/main.tf:204
   via terraform-aws-modules/vpc/aws/.terraform/modules/vpc/main.tf:191-206 (aws_network_acl_rule.public_inbound[13])
    via vpc.tf:55-250 (module.vpc)

 191   resource "aws_network_acl_rule" "public_inbound" {
 ...   
 204 [   cidr_block      = lookup(var.public_inbound_acl_rules[count.index], "cidr_block", null)
 ...   
 206   }

(this also repeats for AVD-AWS-102, etc.)

I think Trivy needs explicit support for filtering things like this safely. The current design for attribute filtering does not support indexed resources (as every input to that module uses) and since these rules are usually false-positives (most people use AWS to host public applications so they'll need to support ingress HTTP, HTTPS, ICMP for Path MTU discovery, etc.), it's really tempting to just toss #trivy:ignore:avd-aws-0102 trivy:ignore:avd-aws-0105 into that file — greatly increasing the odds of Trivy not catching an actual security problem when someone adds another rule – and even if indexed access were possible, it'd be risky because the indexes are not stable (e.g. in my example above, some of the rules come from a different module which has common firewall rules and an update could easily shift all of the index numbers).

Since Trivy has full knowledge of the resources created in a module, it seems like it should be possible to have some way to use that hierarchy to select a resource which would otherwise not be in scope at this point:

# #trivy:ignore:avd-aws-0102[module_resource_name=aws_network_acl_rule.public_inbound,type=ingress,protocol=1]
module "vpc" {
  source                 = "terraform-aws-modules/vpc/aws"
…

I don't love that syntax and it seems like it might be preferable to say that complex filtering has to be done in a .trivyignore.yaml file where it could look more like this:

- id: AVD-AWS-0102
  statement: ICMP Path MTU discovery
  paths:
    - vpc.tf
  resources:
    - path: module.vpc.aws_network_acl_rule.public_inbound
      attributes:
        type: ingress
        protocol: 1
- id: AVD-AWS-0105
  statement: Public web app ingress HTTPS
  paths:
    - vpc.tf
  resources:
    - path: module.vpc.aws_network_acl_rule.public_inbound
      attributes:
        type: ingress
        protocol: 6
        from_port: 443
        to_port: 443

One other thought which came out of this is that the error messages could be improved somewhat to list the attributes of the resource in question. When you have large lists being fed into a loop it'd be really nice to be able to see cidr_block=0.0.0.0/0 from_port=22 to_port=22 so you could easily know which source definitely it refers to rather than having to count them.

Target

None

Scanner

Misconfiguration

[nikpivkin]

@simar7 I'll divide this into 3 tasks.

1. I implemented filtering by count and each objects, but we decided not to add that. It would be easy to add it. 835351d
2. I think we need to have a better discussion about the file scheme. We need to somehow distinguish resources from different projects. For example, there could be a case where the scan target is a directory containing unrelated root modules foo and bar, which have resources with the same logical path. Maybe we should add some kind of entrypoint field? The path field for a resource makes no sense if it is in a remote module. Only the logical path is enough.
3. Right now we don't display the actual value of attributes in the report. What would it look like? Now for displaying misconfig we just read the file and display a piece of code according to the location of the finding. In order to display the expanded block in the report, we need to modify the HCL file, overwrite the file in the file system, and update the finding metadata to refer to the new location.