Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(parser): support nameless tf resources #6510

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

liorj-orca
Copy link
Contributor

@liorj-orca liorj-orca commented Jul 11, 2023

this PR solves the fatal error we are getting when scanning a TF resources without a name. for exmple for the following, we were getting the following fatal error:

resource "aws_lb" {
  name               = "test-lb-tf"
  internal           = false
  load_balancer_type = "network"
  subnets            = [for subnet in aws_subnet.public : subnet.id]

  enable_deletion_protection = true

  tags = {
    Environment = "production"
  }
}

resource "aws_lb" {
  name               = "test-lb-tf"
  internal           = false
  load_balancer_type = "network"
  subnets            = [for subnet in aws_subnet.public : subnet.id]

  enable_deletion_protection = true

  tags = {
    Environment = "production"
  }
}

the error we are getting:

panic: interface conversion: interface {} is []interface {}, not model.Document

goroutine 13 [running]:
github.com/Checkmarx/kics/pkg/parser/terraform.processResources(0x1e43a80?, {0x4000230f40, 0xa})
	/app/pkg/parser/terraform/terraform.go:75 +0x1d8
github.com/Checkmarx/kics/pkg/parser/terraform.addExtraInfo({0x400012e250?, 0x1, 0x1}, {0x4000230f40, 0xa})
	/app/pkg/parser/terraform/terraform.go:93 +0xac
github.com/Checkmarx/kics/pkg/parser/terraform.(*Parser).Parse(0x4000f1d300, {0x4000230f40, 0xa}, {0x4000c426c0, 0x223, 0x240})
	/app/pkg/parser/terraform/terraform.go:133 +0x188
github.com/Checkmarx/kics/pkg/parser.(*Parser).Parse(0x4000f255c0, {0x4000230f40, 0xa}, {0x4000c426c0, 0x223, 0x240})
	/app/pkg/parser/parser.go:128 +0x120
github.com/Checkmarx/kics/pkg/kics.(*Service).sink(0x40005e9730, {0x2765410, 0x400013a000}, {0x4000230f40, 0xa}, {0x210a8c2, 0x7}, {0x2748400, 0x400012e040}, {0x4001200000, ...})
	/app/pkg/kics/sink.go:42 +0x134
github.com/Checkmarx/kics/pkg/kics.(*Service).PrepareSources.func1({0x2765410, 0x400013a000}, {0x4000230f40, 0xa}, {0x2753608?, 0x400012e040})
	/app/pkg/kics/service.go:71 +0xa8
github.com/Checkmarx/kics/pkg/engine/provider.(*FileSystemSourceProvider).GetSources(0x40001284c0, {0x2765410, 0x400013a000}, 0x87354?, 0x40004c0dc0, 0x0?)
	/app/pkg/engine/provider/filesystem.go:129 +0x140
github.com/Checkmarx/kics/pkg/kics.(*Service).PrepareSources(0x40005e9730, {0x2765410, 0x400013a000}, {0x210a8c2, 0x7}, 0x4000cf8120?, 0x4000cf8180?)
	/app/pkg/kics/service.go:67 +0x1c0
created by github.com/Checkmarx/kics/pkg/scanner.PrepareAndScan
	/app/pkg/scanner/scanner.go:24 +0xb4

overall, tf resources withtout a name are not valid, but I belive it would be better inspecting those resources anyway and be able to indicate other issues we have on them other than completly fail the scan

Proposed Changes

I submit this contribution under the Apache-2.0 license.

@liorj-orca liorj-orca force-pushed the nameless_tf_resources branch 2 times, most recently from 4be00c5 to 0cb7fd8 Compare July 11, 2023 13:01
@lior-orca
Copy link

@cxMiguelSilva any chance you look on this one?

Copy link
Collaborator

@cxMiguelSilva cxMiguelSilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @liorj-orca, sorry for the late response,
The proposed changes to the Terraform parser LGTM. Would you just be able to add a unit test to ensure that this issue will be validated from other changes from now onward?

@kaplanlior kaplanlior added the community Community contribution label Jul 24, 2023
@github-actions github-actions bot added the terraform Terraform query label Aug 15, 2023
@liorj-orca liorj-orca force-pushed the nameless_tf_resources branch 2 times, most recently from 54a3054 to 374124d Compare August 15, 2023 14:02
@liorj-orca
Copy link
Contributor Author

liorj-orca commented Aug 16, 2023

Hi @cxMiguelSilva, i added a simple test for that, can we push it forward?
cc: @gabriel-cx @lior-orca @kaplanlior

Copy link
Collaborator

@cxMiguelSilva cxMiguelSilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cxMiguelSilva cxMiguelSilva added the QA Pending QA Validation label Aug 17, 2023
@cxMiguelSilva
Copy link
Collaborator

Hi @cxMiguelSilva, i added a simple test for that, can we push it forward? cc: @gabriel-cx @lior-orca @kaplanlior

Hi @liorj-orca, I have made the initial code review, and now it will be submitted to QA analysis. Once we have the ok we will merge it. Thank you so much for this contribution to this missing case on the Terraform parser 😀

@github-actions github-actions bot added the aws PR related with AWS Cloud label Aug 20, 2023
@pereiramarco011 pereiramarco011 merged commit c1518e6 into Checkmarx:master Aug 29, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws PR related with AWS Cloud community Community contribution QA Pending QA Validation terraform Terraform query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants