Merge pull request #1094 from rashedkvm/executable-permission

storage: set `0o744` for files with exec mode set

Author: hiddeco

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