From dc3482da5315d0afeece9fb13a7f8e4284f5ac94 Mon Sep 17 00:00:00 2001 From: Rashed Kamal Date: Thu, 11 May 2023 16:33:59 -0400 Subject: [PATCH] storage: set `0o744` for files with exec mode set This commit ensures that files with exec permissions set continue to be executable by the user extracting the archive. This is not of use to any of Flux itself, but does help downstream dependencies making use of the controller to facilitate artifact acquisitions for their (CI/CD) software suite. Co-authored-by: Hidde Beydals Signed-off-by: Rashed Kamal --- internal/controller/storage.go | 60 +++++++++++----- internal/controller/storage_test.go | 103 +++++++++++++++++++--------- 2 files changed, 113 insertions(+), 50 deletions(-) diff --git a/internal/controller/storage.go b/internal/controller/storage.go index ef1ac7978..15fe93b36 100644 --- a/internal/controller/storage.go +++ b/internal/controller/storage.go @@ -48,10 +48,12 @@ import ( const GarbageCountLimit = 1000 const ( - // defaultFileMode is the permission mode applied to all files inside an artifact archive. + // defaultFileMode is the permission mode applied to files inside an artifact archive. defaultFileMode int64 = 0o644 // defaultDirMode is the permission mode applied to all directories inside an artifact archive. defaultDirMode int64 = 0o755 + // defaultExeFileMode is the permission mode applied to executable files inside an artifact archive. + defaultExeFileMode int64 = 0o744 ) // Storage manages artifacts @@ -424,6 +426,7 @@ func (s Storage) Archive(artifact *v1.Artifact, dir string, filter ArchiveFileFi if err != nil { return err } + // The name needs to be modified to maintain directory structure // as tar.FileInfoHeader only has access to the base name of the file. // Ref: https://golang.org/src/archive/tar/common.go?#L626 @@ -434,21 +437,7 @@ func (s Storage) Archive(artifact *v1.Artifact, dir string, filter ArchiveFileFi return err } } - header.Name = relFilePath - - // We want to remove any environment specific data as well, this - // ensures the checksum is purely content based. - header.Gid = 0 - header.Uid = 0 - header.Uname = "" - header.Gname = "" - header.ModTime = time.Time{} - header.AccessTime = time.Time{} - header.ChangeTime = time.Time{} - header.Mode = defaultFileMode - if fi.Mode().IsDir() { - header.Mode = defaultDirMode - } + sanitizeHeader(relFilePath, header) if err := tw.WriteHeader(header); err != nil { return err @@ -689,3 +678,42 @@ func (wc *writeCounter) Write(p []byte) (int, error) { wc.written += int64(n) return n, nil } + +// sanitizeHeader modifies the tar.Header to be relative to the root of the +// archive and removes any environment specific data. +func sanitizeHeader(relP string, h *tar.Header) { + // Modify the name to be relative to the root of the archive, + // this ensures we maintain the same structure when extracting. + h.Name = relP + + // We want to remove any environment specific data as well, this + // ensures the checksum is purely content based. + h.Gid = 0 + h.Uid = 0 + h.Uname = "" + h.Gname = "" + h.ModTime = time.Time{} + h.AccessTime = time.Time{} + h.ChangeTime = time.Time{} + + // Override the mode to be the default for the type of file. + setDefaultMode(h) +} + +// setDefaultMode sets the default mode for the given header. +func setDefaultMode(h *tar.Header) { + if h.FileInfo().IsDir() { + h.Mode = defaultDirMode + return + } + + if h.FileInfo().Mode().IsRegular() { + mode := h.FileInfo().Mode() + if mode&os.ModeType == 0 && mode&0o111 != 0 { + h.Mode = defaultExeFileMode + return + } + h.Mode = defaultFileMode + return + } +} diff --git a/internal/controller/storage_test.go b/internal/controller/storage_test.go index 4d624e9f5..8501093bb 100644 --- a/internal/controller/storage_test.go +++ b/internal/controller/storage_test.go @@ -109,9 +109,14 @@ func TestStorage_Archive(t *testing.T) { t.Fatalf("error while bootstrapping storage: %v", err) } - createFiles := func(files map[string][]byte) (dir string, err error) { + type dummyFile struct { + content []byte + mode int64 + } + + createFiles := func(files map[string]dummyFile) (dir string, err error) { dir = t.TempDir() - for name, b := range files { + for name, df := range files { absPath := filepath.Join(dir, name) if err = os.MkdirAll(filepath.Dir(absPath), 0o750); err != nil { return @@ -120,18 +125,24 @@ func TestStorage_Archive(t *testing.T) { if err != nil { return "", fmt.Errorf("could not create file %q: %w", absPath, err) } - if n, err := f.Write(b); err != nil { + if n, err := f.Write(df.content); err != nil { f.Close() return "", fmt.Errorf("could not write %d bytes to file %q: %w", n, f.Name(), err) } f.Close() + + if df.mode != 0 { + if err = os.Chmod(absPath, os.FileMode(df.mode)); err != nil { + return "", fmt.Errorf("could not chmod file %q: %w", absPath, err) + } + } } return } - matchFiles := func(t *testing.T, storage *Storage, artifact sourcev1.Artifact, files map[string][]byte, dirs []string) { + matchFiles := func(t *testing.T, storage *Storage, artifact sourcev1.Artifact, files map[string]dummyFile, dirs []string) { t.Helper() - for name, b := range files { + for name, df := range files { mustExist := !(name[0:1] == "!") if !mustExist { name = name[1:] @@ -140,7 +151,7 @@ func TestStorage_Archive(t *testing.T) { if err != nil { t.Fatalf("failed reading tarball: %v", err) } - if bs := int64(len(b)); s != bs { + if bs := int64(len(df.content)); s != bs { t.Fatalf("%q size %v != %v", name, s, bs) } if exist != mustExist { @@ -150,8 +161,12 @@ func TestStorage_Archive(t *testing.T) { t.Errorf("tarball contained excluded file %q", name) } } - if exist && m != defaultFileMode { - t.Fatalf("%q mode %v != %v", name, m, defaultFileMode) + expectMode := df.mode + if expectMode == 0 { + expectMode = defaultFileMode + } + if exist && m != expectMode { + t.Fatalf("%q mode %v != %v", name, m, expectMode) } } for _, name := range dirs { @@ -179,62 +194,62 @@ func TestStorage_Archive(t *testing.T) { tests := []struct { name string - files map[string][]byte + files map[string]dummyFile filter ArchiveFileFilter - want map[string][]byte + want map[string]dummyFile wantDirs []string wantErr bool }{ { name: "no filter", - files: map[string][]byte{ - ".git/config": nil, - "file.jpg": []byte(`contents`), - "manifest.yaml": nil, + files: map[string]dummyFile{ + ".git/config": {}, + "file.jpg": {content: []byte(`contents`)}, + "manifest.yaml": {}, }, filter: nil, - want: map[string][]byte{ - ".git/config": nil, - "file.jpg": []byte(`contents`), - "manifest.yaml": nil, + want: map[string]dummyFile{ + ".git/config": {}, + "file.jpg": {content: []byte(`contents`)}, + "manifest.yaml": {}, }, }, { name: "exclude VCS", - files: map[string][]byte{ - ".git/config": nil, - "manifest.yaml": nil, + files: map[string]dummyFile{ + ".git/config": {}, + "manifest.yaml": {}, }, wantDirs: []string{ "!.git", }, filter: SourceIgnoreFilter(nil, nil), - want: map[string][]byte{ - "!.git/config": nil, - "manifest.yaml": nil, + want: map[string]dummyFile{ + "!.git/config": {}, + "manifest.yaml": {}, }, }, { name: "custom", - files: map[string][]byte{ - ".git/config": nil, - "custom": nil, - "horse.jpg": nil, + files: map[string]dummyFile{ + ".git/config": {}, + "custom": {}, + "horse.jpg": {}, }, filter: SourceIgnoreFilter([]gitignore.Pattern{ gitignore.ParsePattern("custom", nil), }, nil), - want: map[string][]byte{ - "!git/config": nil, - "!custom": nil, - "horse.jpg": nil, + want: map[string]dummyFile{ + "!git/config": {}, + "!custom": {}, + "horse.jpg": {}, }, wantErr: false, }, { name: "including directories", - files: map[string][]byte{ - "test/.gitkeep": nil, + files: map[string]dummyFile{ + "test/.gitkeep": {}, }, filter: SourceIgnoreFilter([]gitignore.Pattern{ gitignore.ParsePattern("custom", nil), @@ -244,6 +259,26 @@ func TestStorage_Archive(t *testing.T) { }, wantErr: false, }, + { + name: "sets default file modes", + files: map[string]dummyFile{ + "test/file": { + mode: 0o666, + }, + "test/executable": { + mode: 0o777, + }, + }, + want: map[string]dummyFile{ + "test/file": { + mode: defaultFileMode, + }, + "test/executable": { + mode: defaultExeFileMode, + }, + }, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {