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(misconf): support symlinks inside of Helm archives #6621

Merged
merged 6 commits into from
May 7, 2024

Conversation

nikpivkin
Copy link
Contributor

@nikpivkin nikpivkin commented May 3, 2024

Description

Since symbolic links are not supported in the virtual file system, the files referenced are copied to FS.

Related issues

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).

Sorry, something went wrong.

nikpivkin added 2 commits May 3, 2024 17:30
@nikpivkin nikpivkin force-pushed the helm-tar-symlinks branch from c840605 to 9841b99 Compare May 3, 2024 13:18
@nikpivkin nikpivkin marked this pull request as ready for review May 6, 2024 08:14
@nikpivkin nikpivkin requested a review from simar7 as a code owner May 6, 2024 08:14
Comment on lines +82 to +85
case tar.TypeSymlink:
if path.IsAbs(link) {
p.debug.Log("Symlink %s is absolute, skipping", link)
continue
Copy link
Member

Choose a reason for hiding this comment

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

In your test case, can we add a symlink that can exercise this path? An assertion for that could be the absence of the absolute symlink so that we know it's not included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such a file would be missing anyway, since it is not in the virtual file system.

Comment on lines +137 to +140
b, err := io.ReadAll(src)
if err != nil {
return fmt.Errorf("read error: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

should we add a buffered reader incase we encounter huge files again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file write method accepts bytes, so we still have to read the whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simar7 used a lazy file write cedb039

break
}
return nil, fmt.Errorf("failed to copy: %w", err)
func copySymlink(fsys *memoryfs.FS, src, dst string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Should we take into account recursive symlinks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already being handled. Added a test case e0bc1eb

Comment on lines 117 to 123
file, err := os.Open(testPath)
defer func() { _ = file.Close() }()
require.NoError(t, err)

assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file))
func() {
file, err := os.Open(testPath)
require.NoError(t, err)
defer file.Close()

_ = file.Close()
assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file))
}()
Copy link
Member

Choose a reason for hiding this comment

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

How about this to make them table driven? We could do it for the rest of the tests in this file since we are here.

diff --git a/pkg/iac/scanners/helm/test/parser_test.go b/pkg/iac/scanners/helm/test/parser_test.go
index 989590d42..68cc050e4 100644
--- a/pkg/iac/scanners/helm/test/parser_test.go
+++ b/pkg/iac/scanners/helm/test/parser_test.go
@@ -111,16 +111,16 @@ func Test_tar_is_chart(t *testing.T) {
 	}
 
 	for _, test := range tests {
+		t.Run(test.testName, func(t *testing.T) {
+			t.Logf("Running test: %s", test.testName)
+			testPath := filepath.Join("testdata", test.archiveFile)
 
-		t.Logf("Running test: %s", test.testName)
-		testPath := filepath.Join("testdata", test.archiveFile)
-		func() {
 			file, err := os.Open(testPath)
 			require.NoError(t, err)
 			defer file.Close()
 
 			assert.Equal(t, test.isHelmChart, detection.IsHelmChartArchive(test.archiveFile, file))
-		}()
+		})
 	}
 }
 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 5155dbd

@nikpivkin nikpivkin requested a review from simar7 May 7, 2024 05:54
@simar7 simar7 added this pull request to the merge queue May 7, 2024
Merged via the queue into aquasecurity:main with commit 4eae37c May 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(misconf): Support symlinks inside of tar archives
2 participants