Internal checkov error on aws_network_acl but exits 0

[ThomasLachaux]

Describe the issue Checkov crashed before evaluating some rules based on aws_network_acl (CKV_AWS_232, CKV_AWS_231, CKV_AWS_230, CKV_AWS_229)

The crash occurs when rule_no is used with different types.

The big problem is checkov exits 0. I couldn't find a way to make it exit with an error code

Examples Please share an example code sample (in the IaC of your choice) + the expected outcomes.

On this example, having a string rule_no on the first ingress, and a number rule_no on the second will occur a crash

resource "aws_network_acl" "this" {
  vpc_id = var.hub_vpc.id

  ingress {
    protocol   = "-1"
    rule_no    = "800" # string
    action     = "deny"
    cidr_block = "10.0.0.0/8"
    from_port  = 0
    to_port    = 0
  }

  ingress {
    protocol   = "-1"
    rule_no    = 900 # int
    action     = "allow"
    cidr_block = "0.0.0.0/0"
    from_port  = 0
    to_port    = 0
  }
}

Command used : checkov -f test.tf --quiet --output cli --framework terraform

Exception Trace Please share the trace for the exception and all relevant output by checkov. To maximize the understanding, please run checkov with LOG_LEVEL set to debug as follows:

(Same with debug log)

The whole debug log for this example is available here debug.log

Where does this issue comes from ? It comes from Python sort method that cannot be called with mixed string and int arguments

Desktop (please complete the following information):

• OS: MacOs
• Checkov Version 3.1.40

[Saarett]

@ThomasLachaux Thanks for reaching out. I'm glad to see you've found the source of the issue. Would you like to contribute the fix? 🙂 That would be very helpful.

[ThomasLachaux]

hey @Saarett ! I can have a look and make a PR.

I would remove the sorting part to solve the problem. Do you see any issues on this ? I don't see the point of sorting for this check.

Btw, is it a normal behaviour that checkov exit 0 even if there is an internal error ?

[gruebel]

you can't remove the sorting block, because the rule_no sets evaluation order of the e/ingress rules. You would need to wrap the rule_no values with and int() cast.

We don't fail the scan, if one of the checks logic fails unexpectedly, because we can't validate the resource anyway. We failed the whole scan in the past, but this was more disturbing for users.

[ThomasLachaux]

Hey @gruebel,

Thanks for your answer. An int casting wouldn't work with dynamic blocks to solve the problem.

Take this code

resource "aws_network_acl" "these" {
  for_each = local.vpc_denies_other_vpc

  vpc_id     = each.key
  subnet_ids = local.vpc_nacl_subnets[each.key]

  dynamic "ingress" {
    for_each = each.value
    content {
      protocol   = "-1"
      rule_no    = 1 + ingress.key
      action     = "deny"
      cidr_block = ingress.value.cidr_block
      from_port  = 0
      to_port    = 0
    }
  }
  ingress {
    protocol   = "-1"
    rule_no    = 900
    action     = "allow"
    cidr_block = "0.0.0.0/0"
    from_port  = 0
    to_port    = 0
  }
}

We can see the variable is not evaluated, and the first rule_no is a string with 1 + ingress.key.

Therefore, we cannot correctly order the rules, that's why I suggested deleting the order. Another solution would be to try to cast the string to int, and if not, put it at the end., but there are chances it does follow the same ordering as it would be.

About the failing, I would have loved to have a flag to choose this behavior. It took a while on our side to detect the problem as the pipeline was not failing.

[gruebel]

Yeah, that's true. That's why we have a small utility function to enforce an int value force_int(), otherwise it will return None. And your dynamic example is a great one, which can be also used to setup multiple ports and then we wouldn't be able to validate anything too. In those cases it is always recommend to also scan your plan files.

The order is essential to understand what exactly is allowed or not and it was added to fix this use case #4518

[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 codifiedsecurity.slack.com Thanks!