feat(misconf): Support relative paths in terraform config #3991

simar7 · 2023-04-05T05:41:06Z

Description

Currently, if a terraform deployment contains modules that are defined with relative file paths, trivy silently ignores them and does not scan them.

This PR also eliminates unnecessary scan for config types that are not found when evaluating.

Related issues

Close trivy config on terraform skips local modules #3575

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: Simar [email protected]

Currently, if a terraform deployment contains modules that are defined with relative file paths, trivy silently ignores them and does not scan them. This PR also eliminates unnecessary scan for config types that are not found when evaluating. Signed-off-by: Simar <[email protected]>

Signed-off-by: Simar <[email protected]>

simar7 · 2023-04-06T06:23:01Z

pkg/misconf/scanner.go

@@ -50,6 +51,7 @@ func NewScanner(filePatterns []string, opt config.ScannerOption) (Scanner, error
 	opts := []options.ScannerOption{
 		options.ScannerWithSkipRequiredCheck(true),
 		options.ScannerWithEmbeddedPolicies(!opt.DisableEmbeddedPolicies),
+		options.ScannerWithDebug(os.Stdout),


TODO: Remove prior to merge

Signed-off-by: Simar <[email protected]>

simar7 · 2023-04-14T00:14:05Z

pkg/fanal/artifact/local/fs_test.go

-				ID:   "sha256:b3ae72efb468a0e17551fa4067c1f9d9dff9a1e520b9f8191f48829ab6e8356d",
-				BlobIDs: []string{
-					"sha256:b3ae72efb468a0e17551fa4067c1f9d9dff9a1e520b9f8191f48829ab6e8356d",
-				},


note: I've removed these assertions as the key is calculated over everything (including the file paths in the blob), which results in the key being different on different OS's (unix vs windows).

Arguably the content is more important in these tests, which is still asserted.

simar7 · 2023-04-14T00:16:27Z

hi @knqyf263 could you review these as you are the code owner and your review is needed to merge it in.

knqyf263 · 2023-04-17T13:16:56Z

Sure. I cut v0.40.0 yesterday and have time to review it now.

knqyf263 · 2023-04-18T12:13:27Z

pkg/misconf/scanner.go

+					rootDir = strings.TrimPrefix(filepath.ToSlash(rootDir), volume+"/")
+				}
+
+				results, err = scanner.ScanFS(ctx, extrafs.OSDir("/"), rootDir)


extrafs.OSDir says

Go does not currently support symlinks in io/fs

https://github.com/aquasecurity/defsec/blob/a071f4812c575431b38ce741c2138c40cfccc423/pkg/extrafs/extrafs.go#L9-L12

Is it correct? DirFS seems to follow symlinks. Am I missing something?
https://pkg.go.dev/os#DirFS

And it breaks container/VM image scanning since os.DirFS refers to the local filesystem. Putting Terraform files into container/VM images is rare, but we have actually seen it in the past. That is why we use memoryfs instead of the OS filesystem.

IIUC, the problem here is memoryfs and os.DirFS don't allow accessing outside the root directory. I have a plan to replace memoryfs with mapfs. What if allowing mapfs to access the outside file?

I'll raise a PR shortly. We can discuss the solution after that.

extrafs.OSDir says

Go does not currently support symlinks in io/fs

https://github.com/aquasecurity/defsec/blob/a071f4812c575431b38ce741c2138c40cfcc423/pkg/extrafs/extrafs.go#L9-L12

https://github.com/aquasecurity/defsec/blob/a071f4812c575431b38ce741c2138c40cfccc423/pkg/extrafs/extrafs.go#L9-L12

Is it correct? DirFS seems to follow symlinks. Am I missing something? https://pkg.go.dev/os#DirFS

Hmm... I don't know the origin of that comment but I do see os.DirFS does follow symlinks as you said. Maybe that comment/commit was added pre go 1.16 and isn't relevant anymore? 🤷🏼

And it breaks container/VM image scanning since os.DirFS refers to the local filesystem. Putting Terraform files into container/VM images is rare, but we have actually seen it in the past. That is why we use memoryfs instead of the OS filesystem.

Oh interesting. We should have a test for this. Do we have any integration tests that test out container/VM images? Maybe we can add a symlinked terraform file in there to have this covered.

IIUC, the problem here is memoryfs and os.DirFS don't allow accessing outside the root directory. I have a plan to replace memoryfs with mapfs. What if allowing mapfs to access the outside file?

Yes that's the problem. I assume you are talking this mapfs right? Does it support symlinks today? I only see that it implements these https://github.com/aquasecurity/trivy/blob/main/pkg/mapfs/fs.go#L18-L22 - I assumed you would need to implement something like this: https://github.com/aquasecurity/defsec/blob/a071f4812c575431b38ce741c2138c40cfccc423/pkg/extrafs/extrafs.go#L48-L54?

I'll raise a PR shortly. We can discuss the solution after that.

Sure, add me to the review so I'll take a look at it as well.

Do we have any integration tests that test out container/VM images?

No, but you're right. We should have it. It was not a common case, so we were lazy 😄 .

@simar7 Can you add a test container image for Terraform here? Also, I assigned you another issue. It would be great if you take care of them.
aquasecurity/trivy-test-images#28
aquasecurity/trivy-test-images#27

Does it support symlinks today?

No, but it would be easy to support. I'll implement it.

Does it support symlinks today?

It uses os.Open() under the hood, which follows symlinks. What else do we need?

trivy/pkg/mapfs/file.go

Lines 68 to 69 in cc18f92

default: // Real file

return os.Open(f.path)

In theory, tthat should be all. The key thing would be to be able to support paths outside the memfs, including relative paths, which is what this PR tries to do with extrafs.

pkg/fanal/handler/misconf/misconf.go

Co-authored-by: Teppei Fukuda <[email protected]>

knqyf263 · 2023-04-21T11:10:21Z

pkg/fanal/artifact/local/testdata/misconfig/terraform/relative-paths/src/child/main.tf

Where is this file used? I didn't find the test case.

simar7 · 2023-04-24T21:28:20Z

Superseeded by #4094

simar7 mentioned this pull request Apr 5, 2023

fix(misconf): Handle relative directories for modules #3944

Closed

6 tasks

simar7 force-pushed the fix-relative-paths-last-good branch from fb55191 to 6a9e717 Compare April 5, 2023 05:57

Merge branch 'main' into fix-relative-paths-last-good

0337b65

Signed-off-by: Simar <[email protected]>

simar7 force-pushed the fix-relative-paths-last-good branch from 6a9e717 to 0337b65 Compare April 5, 2023 06:00

simar7 added 3 commits April 4, 2023 23:18

fix lint

453c519

Signed-off-by: Simar <[email protected]>

fix tests

80c4e43

Signed-off-by: Simar <[email protected]>

support windows

6a435df

Signed-off-by: Simar <[email protected]>

simar7 force-pushed the fix-relative-paths-last-good branch from 535f19c to 6a435df Compare April 6, 2023 02:18

sort misconfs

c5869b5

Signed-off-by: Simar <[email protected]>

simar7 force-pushed the fix-relative-paths-last-good branch 2 times, most recently from cbc439e to adeeec3 Compare April 6, 2023 06:22

simar7 commented Apr 6, 2023

View reviewed changes

simar7 force-pushed the fix-relative-paths-last-good branch 2 times, most recently from 499a3ac to 1baf04c Compare April 7, 2023 00:03

simar7 marked this pull request as ready for review April 7, 2023 00:54

simar7 requested a review from knqyf263 as a code owner April 7, 2023 00:54

add a fix for windows volumes

18c7925

Signed-off-by: Simar <[email protected]>

simar7 force-pushed the fix-relative-paths-last-good branch from 1baf04c to 18c7925 Compare April 7, 2023 01:00

simar7 mentioned this pull request Apr 8, 2023

fix(terraform): Use absolute path for filesystem to load tfvars aquasecurity/defsec#1271

Merged

simar7 requested a review from giorod3 April 11, 2023 00:11

giorod3 approved these changes Apr 13, 2023

View reviewed changes

simar7 commented Apr 14, 2023

View reviewed changes

knqyf263 assigned simar7 Apr 17, 2023

knqyf263 reviewed Apr 18, 2023

View reviewed changes

Update pkg/fanal/handler/misconf/misconf.go

3c10db3

Co-authored-by: Teppei Fukuda <[email protected]>

knqyf263 mentioned this pull request Apr 20, 2023

feat: allow root break for mapfs #4094

Merged

6 tasks

knqyf263 reviewed Apr 21, 2023

View reviewed changes

simar7 closed this Apr 24, 2023

knqyf263 deleted the fix-relative-paths-last-good branch June 6, 2023 06:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(misconf): Support relative paths in terraform config #3991

feat(misconf): Support relative paths in terraform config #3991

simar7 commented Apr 5, 2023

simar7 Apr 6, 2023

simar7 Apr 14, 2023 •

edited

Loading

simar7 commented Apr 14, 2023

knqyf263 commented Apr 17, 2023

knqyf263 Apr 18, 2023 •

edited

Loading

knqyf263 Apr 18, 2023

simar7 Apr 19, 2023

knqyf263 Apr 19, 2023

knqyf263 Apr 19, 2023

simar7 Apr 20, 2023

knqyf263 Apr 21, 2023

simar7 commented Apr 24, 2023

feat(misconf): Support relative paths in terraform config #3991

feat(misconf): Support relative paths in terraform config #3991

Conversation

simar7 commented Apr 5, 2023

Description

Related issues

Checklist

simar7 Apr 6, 2023

Choose a reason for hiding this comment

simar7 Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

simar7 commented Apr 14, 2023

knqyf263 commented Apr 17, 2023

knqyf263 Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

knqyf263 Apr 18, 2023

Choose a reason for hiding this comment

simar7 Apr 19, 2023

Choose a reason for hiding this comment

knqyf263 Apr 19, 2023

Choose a reason for hiding this comment

knqyf263 Apr 19, 2023

Choose a reason for hiding this comment

simar7 Apr 20, 2023

Choose a reason for hiding this comment

knqyf263 Apr 21, 2023

Choose a reason for hiding this comment

simar7 commented Apr 24, 2023

simar7 Apr 14, 2023 •

edited

Loading

knqyf263 Apr 18, 2023 •

edited

Loading