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

WCOW: local and tar exporter to exclude known non-exportable files/dirs #5011

Open
profnandaa opened this issue Jun 10, 2024 · 2 comments
Open

Comments

@profnandaa
Copy link
Collaborator

profnandaa commented Jun 10, 2024

This is as a follow-up of #4994

Need to find the best way of excluding these files in the export, without touching fsutil iself.

Here is the change when you change from fsutil instead:

diff --git a/vendor/github.com/tonistiigi/fsutil/metadata_unix.go b/vendor/github.com/tonistiigi/fsutil/metadata_unix.go
new file mode 100644
index 000000000..8fd5c1e38
--- /dev/null
+++ b/vendor/github.com/tonistiigi/fsutil/metadata_unix.go
@@ -0,0 +1,9 @@
+//go:build !windows
+// +build !windows
+
+package fsutil
+
+// no special files on unix
+func isMetadataFile(_ string) bool {
+       return false
+}
diff --git a/vendor/github.com/tonistiigi/fsutil/metadata_windows.go b/vendor/github.com/tonistiigi/fsutil/metadata_windows.go
new file mode 100644
index 000000000..c6c86a0d0
--- /dev/null
+++ b/vendor/github.com/tonistiigi/fsutil/metadata_windows.go
@@ -0,0 +1,26 @@
+//go:build windows
+// +build windows
+
+package fsutil
+
+import "strings"
+
+// there are some metadata files/directories that are not
+// useful to containers and can be skipped
+// they also require extra security privileges to read.
+// lowercase for any future case changes
+var metadataFiles = map[string]bool{
+	"\\system volume information": true,
+	"\\wcsandboxstate":            true,
+	"\\programdata\\microsoft":    true,
+}
+
+func isMetadataFile(path string) bool {
+	// normalize path
+	// assumption: filepaths already in Windows format
+	if path[0] != '\\' {
+		path = "\\" + path
+	}
+	path = strings.ToLower(path)
+	return metadataFiles[path]
+}
diff --git a/vendor/github.com/tonistiigi/fsutil/send.go b/vendor/github.com/tonistiigi/fsutil/send.go
index e4a315638..9feb29bd8 100644
--- a/vendor/github.com/tonistiigi/fsutil/send.go
+++ b/vendor/github.com/tonistiigi/fsutil/send.go
@@ -149,6 +149,10 @@ func (s *sender) sendFile(h *sendHandle) error {
 func (s *sender) walk(ctx context.Context) error {
 	var i uint32 = 0
 	err := s.fs.Walk(ctx, "/", func(path string, entry os.DirEntry, err error) error {
+		// skip metadata files on Windows, no check cost on Unix
+		if isMetadataFile(path) {
+			return filepath.SkipDir
+		}
 		if err != nil {
 			return err
 		}
diff --git a/vendor/github.com/tonistiigi/fsutil/tarwriter.go b/vendor/github.com/tonistiigi/fsutil/tarwriter.go
index 06b7bda9b..8da2a97dd 100644
--- a/vendor/github.com/tonistiigi/fsutil/tarwriter.go
+++ b/vendor/github.com/tonistiigi/fsutil/tarwriter.go
@@ -16,6 +16,11 @@ import (
 func WriteTar(ctx context.Context, fs FS, w io.Writer) error {
 	tw := tar.NewWriter(w)
 	err := fs.Walk(ctx, "/", func(path string, entry os.DirEntry, err error) error {
+		// skip metadata files on Windows, no check cost on Unix
+		if isMetadataFile(path) {
+			return filepath.SkipDir
+		}
+
 		if err != nil && !errors.Is(err, os.ErrNotExist) {
 			return err
 		}


Sure, I can add that the exclusion, the team was okay with skipping that, was listed among what's skipped in their layer exports:

"\\System Volume Information",
"\\ProgramData\\Microsoft\\Diagnosis",
"\\WcSandboxState",

Do you suggest adding that in the fsutil side or sending it from the caller?

Originally posted by @profnandaa in #4994 (comment)

@profnandaa
Copy link
Collaborator Author

cross-ref #4985

profnandaa added a commit to profnandaa/buildkit that referenced this issue Jun 10, 2024
TODO: need to cross-check that there is no way the
SeBackupPrivilege can be abused/exploited.

WIP: how best to handle the files to be exclused
without touching `fsutil` - see moby#5011

Signed-off-by: Anthony Nandaa <profnandaa@gmail.com>
@profnandaa
Copy link
Collaborator Author

Exporting via buildctl v0.14.1 (and not via remote builder node in Docker Desktop v4.31.1) does indeed work in this example as well as in my own project.

Would it be possible to not include system files when using scratch as a base for artifacts target to match Linux experience?

moving comment from #4866 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant