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
dependents making use of the controller to facilitate artifact
acquisitions for their (CI/CD) software suite.

Co-authored-by: Hidde Beydals <[email protected]>
Signed-off-by: Rashed Kamal <[email protected]>

Author: rashedkvm

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
+	}
+}

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