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

feat(engine): ignore terraform cache folders #6240

Merged
merged 26 commits into from
Feb 19, 2024

Conversation

dim-ops
Copy link
Contributor

@dim-ops dim-ops commented Mar 21, 2023

Closes #6217
Proposed Changes

  • To improve scan Kics can ignore terraform cache folders;

I submit this contribution under the Apache-2.0 license.

@github-advanced-security
Copy link

You have successfully added a new gosec configuration .github/workflows/go-ci.yml:security-scan. As part of the setup process, we have scanned this repository and found 19 existing alerts. Please check the repository Security tab to see all alerts.

@kaplanlior kaplanlior added the community Community contribution label Mar 22, 2023
@kaplanlior
Copy link
Contributor

@dim-ops don't forget to remove the draft status for this PR so we can review it. If you need any help before that - ping me.

pkg/scan/scan.go Outdated Show resolved Hide resolved
@dim-ops dim-ops force-pushed the feat/ignore-terraform-cahce-files branch from 2fc3f90 to c7dd33a Compare March 28, 2023 17:17
@dim-ops
Copy link
Contributor Author

dim-ops commented Mar 28, 2023

It works for me, it ignores all .terra folders in current path or in subdirectories. I setup terra to work with terraform and terragrunt cache

@dim-ops dim-ops marked this pull request as ready for review March 28, 2023 17:23
@dim-ops dim-ops requested a review from kaplanlior March 28, 2023 17:23
@dim-ops dim-ops force-pushed the feat/ignore-terraform-cahce-files branch from 0b2902b to d4f77d5 Compare April 2, 2023 18:41
@cxMiguelSilva
Copy link
Collaborator

Hi @dim-ops,
Thank you for another great contribution! 🚀
From an initial overview, the changes make sense to me. I would only suggest adding some tests to the filesystem_test.go just to make sure the behavior from now onwards works as respected.
Let us know in case you need any help!

@dim-ops dim-ops marked this pull request as draft April 5, 2023 15:15
@dim-ops dim-ops force-pushed the feat/ignore-terraform-cahce-files branch from 6944800 to 307353c Compare April 5, 2023 15:29
@@ -310,7 +315,7 @@ func TestFileSystemSourceProvider_checkConditions(t *testing.T) {
path: filepath.FromSlash("assets/queries"),
},
want: want{
got: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this test because I didn't understand why "true" was expected. During the test I passed in this line:
_, err := os.Stat(filepath.Join(path, "Chart.yaml"))

Which returned true, nil because I had no Chart.yaml file in my path, so I understood if there is no Chart.yaml file it should be ignored and I don't understand why.

Can you explain me? @cxMiguelSilva

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @dim-ops,
That check is in place to ignore all folders of Helm Charts to later make a scan to the Helm templates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand the interest of _, err := os.Stat(filepath.Join(path, "Chart.yaml"))but I don't understand why true is returned when Chart.yaml doesn't exist. If I understand well true is when folder must be skipped, if Chart.yaml is not present folder should be parsed.

It's why I modify this check

		_, err := os.Stat(filepath.Join(path, "Chart.yaml"))
		if errors.Is(err, os.ErrNotExist) {
			return false, nil
		} else if err != nil || resolved {
			log.Error().Msgf("failed to check helm: %s", err)
			return true, nil
		}

@dim-ops dim-ops force-pushed the feat/ignore-terraform-cahce-files branch 2 times, most recently from f16217e to 371eb3e Compare April 11, 2023 19:00
@dim-ops dim-ops marked this pull request as ready for review April 11, 2023 19:00
@kaplanlior kaplanlior requested review from cxMiguelSilva and removed request for kaplanlior April 17, 2023 10:13
@dim-ops
Copy link
Contributor Author

dim-ops commented Apr 27, 2023

Can I have more information @gabriel-cx ?

@dim-ops
Copy link
Contributor Author

dim-ops commented Feb 2, 2024

Up @gabriel-cx please 🙏

@gabriel-cx
Copy link
Contributor

Hi @dim-ops ,

Sorry for the late reply!
We plan to give you a proper feedback during next week.

@github-actions github-actions bot added feature request Community: new feature request terraform Terraform query labels Feb 2, 2024
@gabriel-cx gabriel-cx changed the title feat: ignore terraform cache folders feat(engine): ignore terraform cache folders Feb 2, 2024
Copy link
Contributor

@JoaoAtGit JoaoAtGit left a comment

Choose a reason for hiding this comment

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

Hi @dim-ops , how are you?
So I was looking into your code and made some changes,
first I changed the detection of .terra* to use regex, to cover all the cases present on the issue;
second I removed the logic that you put for the chart.yaml, because I ran regressions, and the files that should scanned with the helm resolver were being treated by the normal yaml resolver and were giving exceptions with this we were losing results.
Let me know what you think.
Br,
Joao Martins

@dim-ops
Copy link
Contributor Author

dim-ops commented Feb 8, 2024

Thanks @JoaoCxMartins it seems better 👍

@JoaoAtGit
Copy link
Contributor

Thanks @JoaoCxMartins it seems better 👍

TY for your help improving Kics :)

JulioSCX
JulioSCX previously approved these changes Feb 8, 2024
@asofsilva asofsilva merged commit dfd15fc into Checkmarx:master Feb 19, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution feature request Community: new feature request terraform Terraform query
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore .terra* cache directories by default globally
7 participants