Skip to content

feat(misconf): support symlinks inside of Helm archives#6621

Merged
simar7 merged 6 commits into
aquasecurity:mainfrom
nikpivkin:helm-tar-symlinks
May 7, 2024
Merged

feat(misconf): support symlinks inside of Helm archives#6621
simar7 merged 6 commits into
aquasecurity:mainfrom
nikpivkin:helm-tar-symlinks

Conversation

@nikpivkin

@nikpivkin nikpivkin commented May 3, 2024

Copy link
Copy Markdown
Contributor

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

@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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
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