feedback

Signed-off-by: Sammy Abed <sammy.abed@digitalasset.com>

content/file/errors.go

@@ -23,6 +23,7 @@ var (
 	ErrPathTraversalDisallowed = errors.New("path traversal disallowed")
 	ErrOverwriteDisallowed     = errors.New("overwrite disallowed")
 	ErrStoreClosed             = errors.New("store already closed")
+	ErrRestorePermissions      = errors.New("failed to restore permission during unpacking")
 )
 
 var errSkipUnnamed = errors.New("unnamed descriptor skipped")

content/file/file.go

@@ -108,8 +108,9 @@ type Store struct {
 	// value overrides the [AnnotationUnpack].
 	// Default value: false.
 	SkipUnpack bool
-	// PreserveModeBits controls whether to preserve file permissions when unpacking
-	PreserveModeBits bool
+	// PreservePermission controls whether to preserve file permissions when unpacking,
+	// disregarding the active umask, similar to tar's `--preserve-permissions`
+	PreservePermission bool
 
 	workingDir   string   // the working directory of the file store
 	closed       int32    // if the store is closed - 0: false, 1: true.
@@ -501,7 +502,7 @@ func (s *Store) pushDir(name, target string, expected ocispec.Descriptor, conten
 	checksum := expected.Annotations[AnnotationDigest]
 	buf := bufPool.Get().(*[]byte)
 	defer bufPool.Put(buf)
-	if err := extractTarGzip(target, name, gzPath, checksum, *buf, s.PreserveModeBits); err != nil {
+	if err := extractTarGzip(target, name, gzPath, checksum, *buf, s.PreservePermission); err != nil {
 		return fmt.Errorf("failed to extract tar to %s: %w", target, err)
 	}
 	return nil

content/file/utils.go

@@ -113,7 +113,7 @@ func tarDirectory(ctx context.Context, root, prefix string, w io.Writer, removeT
 
 // extractTarGzip decompresses the gzip
 // and extracts tar file to a directory specified by the `dir` parameter.
-func extractTarGzip(dirPath, dirName, gzPath, checksum string, buf []byte, preserveModeBits bool) (err error) {
+func extractTarGzip(dirPath, dirName, gzPath, checksum string, buf []byte, preservePermission bool) (err error) {
 	fp, err := os.Open(gzPath)
 	if err != nil {
 		return err
@@ -144,7 +144,7 @@ func extractTarGzip(dirPath, dirName, gzPath, checksum string, buf []byte, prese
 			r = io.TeeReader(r, verifier)
 		}
 	}
-	if err := extractTarDirectory(dirPath, dirName, r, buf, preserveModeBits); err != nil {
+	if err := extractTarDirectory(dirPath, dirName, r, buf, preservePermission); err != nil {
 		return err
 	}
 	if verifier != nil && !verifier.Verified() {
@@ -156,7 +156,7 @@ func extractTarGzip(dirPath, dirName, gzPath, checksum string, buf []byte, prese
 // extractTarDirectory extracts tar file to a directory specified by the `dir`
 // parameter. The file name prefix is ensured to be the string specified by the
 // `prefix` parameter and is trimmed.
-func extractTarDirectory(dirPath, dirName string, r io.Reader, buf []byte, preserveModeBits bool) error {
+func extractTarDirectory(dirPath, dirName string, r io.Reader, buf []byte, preservePermission bool) error {
 	tr := tar.NewReader(r)
 	for {
 		header, err := tr.Next()
@@ -217,8 +217,10 @@ func extractTarDirectory(dirPath, dirName string, r io.Reader, buf []byte, prese
 		_ = os.Chtimes(filePath, header.AccessTime, header.ModTime)
 
 		// Restore full mode bits (error ignored)
-		if preserveModeBits && (header.Typeflag == tar.TypeReg || header.Typeflag == tar.TypeDir) {
-			_ = os.Chmod(filePath, os.FileMode(header.Mode))
+		if preservePermission && (header.Typeflag == tar.TypeReg || header.Typeflag == tar.TypeDir) {
+			if err := os.Chmod(filePath, os.FileMode(header.Mode)); err != nil {
+				return fmt.Errorf("%w: %w", ErrRestorePermissions, err)
+			}
 		}
 	}
 }

content/file/utils_test.go

@@ -266,20 +266,6 @@ func Test_extractTarDirectory(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		{
-			name: "preserve file mode",
-			tarData: createTar(t, []tarEntry{
-				{name: "base/", mode: os.ModeDir | 0777},
-				{name: "base/test.txt", content: "hello world", mode: 0771},
-				{name: "base/file_symlink", linkname: "test.txt", mode: os.ModeSymlink | 0771},
-			}),
-			wantFiles: map[string]string{
-				"base/test.txt":     "hello world",
-				"base/file_symlink": "hello world",
-			},
-			wantErr:          false,
-			wantPreserveMode: true,
-		},
 		{
 			name: "non-regular files",
 			tarData: createTar(t, []tarEntry{
@@ -319,7 +305,6 @@ func Test_extractTarDirectory(t *testing.T) {
 				t.Fatalf("extractTarDirectory() error = %v, wantErr %v", err, tt.wantErr)
 			}
 			if !tt.wantErr {
-				headers := extractHeaders(tt.tarData)
 
 				for path, wantContent := range tt.wantFiles {
 					filePath := filepath.Join(tempDir, path)
@@ -344,31 +329,47 @@ func Test_extractTarDirectory(t *testing.T) {
 					if string(gotContent) != wantContent {
 						t.Errorf("file content = %s, want %s", gotContent, wantContent)
 					}
-
-					header := headers[path]
-					if tt.wantPreserveMode &&
-						fi.Mode() != os.FileMode(header.Mode) &&
-						(header.Typeflag == tar.TypeDir || header.Typeflag == tar.TypeReg) {
-						t.Errorf("file %q mode = %s, want %s", fi.Name(), fi.Mode(), os.FileMode(header.Mode))
-					}
 				}
 			}
 		})
 	}
 }
 
-func extractHeaders(tarData []byte) map[string]*tar.Header {
-	m := map[string]*tar.Header{}
-	r := tar.NewReader(bytes.NewBuffer(tarData))
-	for {
-		header, err := r.Next()
-		if err != nil {
-			break
-		}
+func Test_extractTarDirectory_PreservePermissions(t *testing.T) {
+	tarData := createTar(t, []tarEntry{
+		{name: "base/", mode: os.ModeDir | 0777},
+		{name: "base/test.txt", content: "hello world", mode: 0771},
+		{name: "base/file_symlink", linkname: "test.txt", mode: os.ModeSymlink | 0771},
+	})
+
+	tempDir := t.TempDir()
+	dirName := "base"
+	dirPath := filepath.Join(tempDir, dirName)
+	fileContent := "hello world"
+	fileMode := os.FileMode(0771)
+	buf := make([]byte, 1024)
+
+	if err := extractTarDirectory(dirPath, dirName, bytes.NewReader(tarData), buf, true); err != nil {
+		t.Fatalf("extractTarDirectory() error = %v", err)
+	}
+
+	filePath := filepath.Join(dirPath, "test.txt")
+	fi, err := os.Lstat(filePath)
+	if err != nil {
+		t.Fatalf("failed to stat file %s: %v", filePath, err)
+	}
+
+	gotContent, err := os.ReadFile(filePath)
+	if err != nil {
+		t.Fatalf("failed to read file %s: %v", filePath, err)
+	}
+	if string(gotContent) != fileContent {
+		t.Errorf("file content = %s, want %s", gotContent, fileContent)
+	}
 
-		m[header.Name] = header
+	if fi.Mode() != fileMode {
+		t.Errorf("file %q mode = %s, want %s", fi.Name(), fi.Mode(), fileMode)
 	}
-	return m
 }
 
 func Test_extractTarDirectory_HardLink(t *testing.T) {