Fixed symlink restore behavior on macOS (#673)

* restore: use symlink-specific APIs instead of chmod, chown and chtimes

* upload: fix updating directory modtime for symlinks

* cli: plumbed through flags to restore to control new behaviors

* localfs: use Lstat() instead of Stat() in Child() method

* testing: added restore tests for new flags

cli/command_restore.go

@@ -46,14 +46,18 @@ directory ID and optionally a sub-directory path. For example,
 )
 
 var (
-	restoreCommand              = app.Command("restore", restoreCommandHelp)
-	restoreSourceID             = ""
-	restoreTargetPath           = ""
-	restoreOverwriteDirectories = true
-	restoreOverwriteFiles       = true
-	restoreConsistentAttributes = false
-	restoreMode                 = restoreModeAuto
-	restoreParallel             = 8
+	restoreCommand                = app.Command("restore", restoreCommandHelp)
+	restoreSourceID               = ""
+	restoreTargetPath             = ""
+	restoreOverwriteDirectories   = true
+	restoreOverwriteFiles         = true
+	restoreConsistentAttributes   = false
+	restoreMode                   = restoreModeAuto
+	restoreParallel               = 8
+	restoreIgnorePermissionErrors = true
+	restoreSkipTimes              = false
+	restoreSkipOwners             = false
+	restoreSkipPermissions        = false
 )
 
 const (
@@ -73,6 +77,10 @@ func addRestoreFlags(cmd *kingpin.CmdClause) {
 	cmd.Flag("consistent-attributes", "When multiple snapshots match, fail if they have inconsistent attributes").Envar("KOPIA_RESTORE_CONSISTENT_ATTRIBUTES").BoolVar(&restoreConsistentAttributes)
 	cmd.Flag("mode", "Override restore mode").EnumVar(&restoreMode, restoreModeAuto, restoreModeLocal, restoreModeZip, restoreModeZipNoCompress, restoreModeTar, restoreModeTgz)
 	cmd.Flag("parallel", "Restore parallelism (1=disable)").IntVar(&restoreParallel)
+	cmd.Flag("skip-owners", "Skip owners during restore").BoolVar(&restoreSkipOwners)
+	cmd.Flag("skip-permissions", "Skip permissions during restore").BoolVar(&restoreSkipPermissions)
+	cmd.Flag("skip-times", "Skip times during restore").BoolVar(&restoreSkipTimes)
+	cmd.Flag("ignore-permission-errors", "Ignore permission errors").BoolVar(&restoreIgnorePermissionErrors)
 }
 
 func restoreOutput(ctx context.Context) (restore.Output, error) {
@@ -85,9 +93,13 @@ func restoreOutput(ctx context.Context) (restore.Output, error) {
 	switch m {
 	case restoreModeLocal:
 		return &restore.FilesystemOutput{
-			TargetPath:           p,
-			OverwriteDirectories: restoreOverwriteDirectories,
-			OverwriteFiles:       restoreOverwriteFiles,
+			TargetPath:             p,
+			OverwriteDirectories:   restoreOverwriteDirectories,
+			OverwriteFiles:         restoreOverwriteFiles,
+			IgnorePermissionErrors: restoreIgnorePermissionErrors,
+			SkipOwners:             restoreSkipOwners,
+			SkipPermissions:        restoreSkipPermissions,
+			SkipTimes:              restoreSkipTimes,
 		}, nil
 
 	case restoreModeZip, restoreModeZipNoCompress:

fs/entry.go

@@ -80,11 +80,12 @@ type EntryWithError struct {
 
 // DirectorySummary represents summary information about a directory.
 type DirectorySummary struct {
-	TotalFileSize    int64     `json:"size"`
-	TotalFileCount   int64     `json:"files"`
-	TotalDirCount    int64     `json:"dirs"`
-	MaxModTime       time.Time `json:"maxTime"`
-	IncompleteReason string    `json:"incomplete,omitempty"`
+	TotalFileSize     int64     `json:"size"`
+	TotalFileCount    int64     `json:"files"`
+	TotalSymlinkCount int64     `json:"symlinks"`
+	TotalDirCount     int64     `json:"dirs"`
+	MaxModTime        time.Time `json:"maxTime"`
+	IncompleteReason  string    `json:"incomplete,omitempty"`
 
 	// number of failed files
 	NumFailed int `json:"numFailed"`

fs/localfs/local_fs.go

@@ -105,7 +105,7 @@ func (fsd *filesystemDirectory) Size() int64 {
 func (fsd *filesystemDirectory) Child(ctx context.Context, name string) (fs.Entry, error) {
 	fullPath := fsd.fullPath()
 
-	st, err := os.Stat(filepath.Join(fullPath, name))
+	st, err := os.Lstat(filepath.Join(fullPath, name))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, fs.ErrEntryNotFound

snapshot/restore/local_fs_output.go

@@ -15,6 +15,8 @@ import (
 	"github.com/kopia/kopia/fs/localfs"
 )
 
+const modBits = os.ModePerm | os.ModeSetgid | os.ModeSetuid | os.ModeSticky
+
 // FilesystemOutput contains the options for outputting a file system tree.
 type FilesystemOutput struct {
 	// TargetPath for restore.
@@ -27,6 +29,18 @@ type FilesystemOutput struct {
 	// the copier does not modify already existing files and returns an error
 	// instead.
 	OverwriteFiles bool
+
+	// IgnorePermissionErrors causes restore to ignore errors due to invalid permissions.
+	IgnorePermissionErrors bool
+
+	// SkipOwners when set to true causes restore to skip restoring owner information.
+	SkipOwners bool
+
+	// SkipPermissions when set to true causes restore to skip restoring permission information.
+	SkipPermissions bool
+
+	// SkipTimes when set to true causes restore to skip restoring modification times.
+	SkipTimes bool
 }
 
 // Parallelizable implements restore.Output interface.
@@ -48,7 +62,7 @@ func (o *FilesystemOutput) BeginDirectory(ctx context.Context, relativePath stri
 // FinishDirectory implements restore.Output interface.
 func (o *FilesystemOutput) FinishDirectory(ctx context.Context, relativePath string, e fs.Directory) error {
 	path := filepath.Join(o.TargetPath, filepath.FromSlash(relativePath))
-	if err := o.setAttributes(ctx, path, e); err != nil {
+	if err := o.setAttributes(path, e); err != nil {
 		return errors.Wrap(err, "error setting attributes")
 	}
 
@@ -69,7 +83,7 @@ func (o *FilesystemOutput) WriteFile(ctx context.Context, relativePath string, f
 		return errors.Wrap(err, "error creating directory")
 	}
 
-	if err := o.setAttributes(ctx, path, f); err != nil {
+	if err := o.setAttributes(path, f); err != nil {
 		return errors.Wrap(err, "error setting attributes")
 	}
 
@@ -91,7 +105,7 @@ func (o *FilesystemOutput) CreateSymlink(ctx context.Context, relativePath strin
 		return errors.Wrap(err, "error creating symlink")
 	}
 
-	if err := o.setAttributes(ctx, path, e); err != nil {
+	if err := o.setAttributes(path, e); err != nil {
 		return errors.Wrap(err, "error setting attributes")
 	}
 
@@ -99,43 +113,42 @@ func (o *FilesystemOutput) CreateSymlink(ctx context.Context, relativePath strin
 }
 
 // set permission, modification time and user/group ids on targetPath.
-func (o *FilesystemOutput) setAttributes(ctx context.Context, targetPath string, e fs.Entry) error {
-	const modBits = os.ModePerm | os.ModeSetgid | os.ModeSetuid | os.ModeSticky
-
+func (o *FilesystemOutput) setAttributes(targetPath string, e fs.Entry) error {
 	le, err := localfs.NewEntry(targetPath)
 	if err != nil {
 		return errors.Wrap(err, "could not create local FS entry for "+targetPath)
 	}
 
+	var (
+		osChmod   = os.Chmod
+		osChown   = os.Chown
+		osChtimes = os.Chtimes
+	)
+
+	// symbolic links require special handling that is OS-specific and sometimes unsupported
+	// os.* functions change the target of the symlink and not the symlink itself.
+	if isSymlink(e) {
+		osChmod, osChown, osChtimes = symlinkChmod, symlinkChown, symlinkChtimes
+	}
+
 	// Set owner user and group from e
 	// On Windows Chown is not supported. fs.OwnerInfo collected on Windows will always
 	// be zero-value for UID and GID, so the Chown operation is not performed.
-	if le.Owner() != e.Owner() && runtime.GOOS != "windows" {
-		if err = os.Chown(targetPath, int(e.Owner().UserID), int(e.Owner().GroupID)); err != nil && !os.IsPermission(err) {
+	if o.shouldUpdateOwner(le, e) {
+		if err = o.maybeIgnorePermissionError(osChown(targetPath, int(e.Owner().UserID), int(e.Owner().GroupID))); err != nil {
 			return errors.Wrap(err, "could not change owner/group for "+targetPath)
 		}
 	}
 
 	// Set file permissions from e
-	if (le.Mode() & modBits) != (e.Mode() & modBits) {
-		if err = os.Chmod(targetPath, e.Mode()&modBits); err != nil && !os.IsPermission(err) {
+	if o.shouldUpdatePermissions(le, e) {
+		if err = o.maybeIgnorePermissionError(osChmod(targetPath, e.Mode()&modBits)); err != nil {
 			return errors.Wrap(err, "could not change permissions on "+targetPath)
 		}
 	}
 
-	// Set mod time from e
-	if le.ModTime().Equal(e.ModTime()) {
-		return nil
-	}
-
-	if _, isSymlink := e.(fs.Symlink); isSymlink {
-		// symbolic links require special handling that is OS-specific and sometimes unsupported.
-		if err = symlinkChtimes(ctx, targetPath, e.ModTime(), e.ModTime()); err != nil && !os.IsPermission(err) {
-			return errors.Wrap(err, "could not change mod time on "+targetPath)
-		}
-	} else {
-		// Note: Set atime to ModTime as well
-		if err = os.Chtimes(targetPath, e.ModTime(), e.ModTime()); err != nil && !os.IsPermission(err) {
+	if o.shouldUpdateTimes(le, e) {
+		if err = o.maybeIgnorePermissionError(osChtimes(targetPath, e.ModTime(), e.ModTime())); err != nil {
 			return errors.Wrap(err, "could not change mod time on "+targetPath)
 		}
 	}
@@ -143,6 +156,51 @@ func (o *FilesystemOutput) setAttributes(ctx context.Context, targetPath string,
 	return nil
 }
 
+func isSymlink(e fs.Entry) bool {
+	_, ok := e.(fs.Symlink)
+	return ok
+}
+
+func (o *FilesystemOutput) maybeIgnorePermissionError(err error) error {
+	if o.IgnorePermissionErrors && os.IsPermission(err) {
+		return nil
+	}
+
+	return err
+}
+
+func (o *FilesystemOutput) shouldUpdateOwner(local, remote fs.Entry) bool {
+	if o.SkipOwners {
+		return false
+	}
+
+	if isWindows() {
+		return false
+	}
+
+	return local.Owner() != remote.Owner()
+}
+
+func (o *FilesystemOutput) shouldUpdatePermissions(local, remote fs.Entry) bool {
+	if o.SkipPermissions {
+		return false
+	}
+
+	return (local.Mode() & modBits) != (remote.Mode() & modBits)
+}
+
+func (o *FilesystemOutput) shouldUpdateTimes(local, remote fs.Entry) bool {
+	if o.SkipTimes {
+		return false
+	}
+
+	return !local.ModTime().Equal(remote.ModTime())
+}
+
+func isWindows() bool {
+	return runtime.GOOS == "windows"
+}
+
 func (o *FilesystemOutput) createDirectory(ctx context.Context, path string) error {
 	switch stat, err := os.Stat(path); {
 	case os.IsNotExist(err):

snapshot/restore/local_fs_output_linux.go

@@ -0,0 +1,24 @@
+package restore
+
+import (
+	"os"
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+func symlinkChown(path string, uid, gid int) error {
+	return unix.Lchown(path, uid, gid)
+}
+
+func symlinkChmod(path string, mode os.FileMode) error {
+	// linux does not support permissions on symlinks
+	return nil
+}
+
+func symlinkChtimes(linkPath string, atime, mtime time.Time) error {
+	return unix.Lutimes(linkPath, []unix.Timeval{
+		unix.NsecToTimeval(atime.UnixNano()),
+		unix.NsecToTimeval(mtime.UnixNano()),
+	})
+}

snapshot/restore/local_fs_output_unix.go

@@ -1,15 +1,23 @@
-// +build linux darwin
+// +build darwin
 
 package restore
 
 import (
-	"context"
+	"os"
 	"time"
 
 	"golang.org/x/sys/unix"
 )
 
-func symlinkChtimes(ctx context.Context, linkPath string, atime, mtime time.Time) error {
+func symlinkChown(path string, uid, gid int) error {
+	return unix.Lchown(path, uid, gid)
+}
+
+func symlinkChmod(path string, mode os.FileMode) error {
+	return unix.Fchmodat(unix.AT_FDCWD, path, uint32(mode), unix.AT_SYMLINK_NOFOLLOW)
+}
+
+func symlinkChtimes(linkPath string, atime, mtime time.Time) error {
 	return unix.Lutimes(linkPath, []unix.Timeval{
 		unix.NsecToTimeval(atime.UnixNano()),
 		unix.NsecToTimeval(mtime.UnixNano()),

snapshot/restore/local_fs_output_windows.go

@@ -1,14 +1,22 @@
 package restore
 
 import (
-	"context"
+	"os"
 	"time"
 
 	"github.com/pkg/errors"
 	"golang.org/x/sys/windows"
 )
 
-func symlinkChtimes(ctx context.Context, linkPath string, atime, mtime time.Time) error {
+func symlinkChown(path string, uid, gid int) error {
+	return nil
+}
+
+func symlinkChmod(path string, mode os.FileMode) error {
+	return nil
+}
+
+func symlinkChtimes(linkPath string, atime, mtime time.Time) error {
 	fta := windows.NsecToFiletime(atime.UnixNano())
 	ftw := windows.NsecToFiletime(mtime.UnixNano())
 

snapshot/snapshotfs/upload.go

@@ -479,16 +479,19 @@ func (b *dirManifestBuilder) addEntry(de *snapshot.DirEntry) {
 
 	b.entries = append(b.entries, de)
 
+	if de.ModTime.After(b.summary.MaxModTime) {
+		b.summary.MaxModTime = de.ModTime
+	}
+
 	// nolint:exhaustive
 	switch de.Type {
+	case snapshot.EntryTypeSymlink:
+		b.summary.TotalSymlinkCount++
+
 	case snapshot.EntryTypeFile:
 		b.summary.TotalFileCount++
 		b.summary.TotalFileSize += de.FileSize
 
-		if de.ModTime.After(b.summary.MaxModTime) {
-			b.summary.MaxModTime = de.ModTime
-		}
-
 	case snapshot.EntryTypeDirectory:
 		if childSummary := de.DirSummary; childSummary != nil {
 			b.summary.TotalFileCount += childSummary.TotalFileCount

tests/end_to_end_test/restore_test.go

@@ -21,7 +21,13 @@ import (
 	"github.com/kopia/kopia/tests/testenv"
 )
 
-const windowsOSName = "windows"
+const (
+	windowsOSName                 = "windows"
+	defaultRestoredFilePermission = 0o600
+
+	overriddenFilePermissions = 0o651
+	overriddenDirPermissions  = 0o752
+)
 
 func TestRestoreCommand(t *testing.T) {
 	t.Parallel()
@@ -144,8 +150,8 @@ func TestSnapshotRestore(t *testing.T) {
 	f.Close()
 
 	// change file permissions to something unique we can test later
-	os.Chmod(filepath.Join(source, "single-file"), 0o651)
-	os.Chmod(filepath.Join(source, "subdir1"), 0o752)
+	os.Chmod(filepath.Join(source, "single-file"), overriddenFilePermissions)
+	os.Chmod(filepath.Join(source, "subdir1"), overriddenDirPermissions)
 
 	restoreDir := t.TempDir()
 	r1 := t.TempDir()
@@ -180,8 +186,33 @@ func TestSnapshotRestore(t *testing.T) {
 	// restore using <root-id>/subdirectory.
 	restoreByOIDSubdir := t.TempDir()
 	e.RunAndExpectSuccess(t, "snapshot", "restore", rootID+"/subdir1", restoreByOIDSubdir)
+	verifyFileMode(t, restoreByOIDSubdir, os.ModeDir|os.FileMode(overriddenDirPermissions))
 
-	verifyFileMode(t, restoreByOIDSubdir, os.ModeDir|os.FileMode(0o752))
+	restoreByOIDSubdir2 := t.TempDir()
+
+	originalDirInfo, err := os.Stat(restoreByOIDSubdir2)
+	if err != nil {
+		t.Fatalf("unable to get dir permissions: %v", err)
+	}
+
+	e.RunAndExpectSuccess(t, "snapshot", "restore", "--skip-times", "--skip-owners", "--skip-permissions", rootID+"/subdir1", restoreByOIDSubdir2)
+
+	currentDirPerm, err := os.Stat(restoreByOIDSubdir2)
+	if err != nil {
+		t.Fatalf("unable to get current dir permissions: %v", err)
+	}
+
+	if currentDirPerm.Mode() != originalDirInfo.Mode() {
+		t.Fatalf("dir mode have changed, original %v, current %v", originalDirInfo.Mode(), currentDirPerm.Mode())
+	}
+
+	// current must be always at or after original, if it's not it must have been restored.
+	if currentDirPerm.ModTime().Before(originalDirInfo.ModTime()) {
+		t.Fatalf("dir ModTime has been restore, original %v, current %v", originalDirInfo.ModTime(), currentDirPerm.ModTime())
+	}
+
+	// TODO(jkowalski): find a way to verify owners, we currently cannot even change it since the test is running as
+	// non-root.
 
 	restoreByOIDFile := t.TempDir()
 
@@ -253,6 +284,40 @@ func TestSnapshotRestore(t *testing.T) {
 	e.RunAndExpectFailure(t, "snapshot", "restore", "--no-overwrite-files", snapID, restoreDir)
 }
 
+func TestRestoreSymlinkWithoutTarget(t *testing.T) {
+	t.Parallel()
+
+	e := testenv.NewCLITest(t)
+	defer e.RunAndExpectSuccess(t, "repo", "disconnect")
+
+	e.RunAndExpectSuccess(t, "repo", "create", "filesystem", "--path", e.RepoDir)
+
+	source := t.TempDir()
+
+	lnk := filepath.Join(source, "lnk")
+
+	if err := os.Symlink(".no-such-file", lnk); err != nil {
+		t.Fatal(err)
+	}
+
+	e.RunAndExpectSuccess(t, "snapshot", "create", source)
+
+	// obtain snapshot root id and use it for restore
+	si := e.ListSnapshotsAndExpectSuccess(t, source)
+	if got, want := len(si), 1; got != want {
+		t.Fatalf("got %v sources, wanted %v", got, want)
+	}
+
+	if got, want := len(si[0].Snapshots), 1; got != want {
+		t.Fatalf("got %v snapshots, wanted %v", got, want)
+	}
+
+	snapID := si[0].Snapshots[0].SnapshotID
+
+	restoredDir := t.TempDir()
+	e.RunAndExpectSuccess(t, "snapshot", "restore", "--no-ignore-permission-errors", snapID, restoredDir)
+}
+
 func TestRestoreSnapshotOfSingleFile(t *testing.T) {
 	t.Parallel()
 
@@ -322,6 +387,11 @@ func TestRestoreSnapshotOfSingleFile(t *testing.T) {
 	// restoring using snapshot ID is unambiguous and always produces file with 0o653
 	e.RunAndExpectSuccess(t, "snapshot", "restore", snapID, filepath.Join(restoreDir, "restored-4"))
 	verifyFileMode(t, filepath.Join(restoreDir, "restored-4"), os.FileMode(0o653))
+
+	// skip permissions when restoring, which results in default defaultRestoredFilePermission
+	e.RunAndExpectSuccess(t, "snapshot", "restore", rootID, "--skip-permissions", filepath.Join(restoreDir, "restored-5"))
+
+	verifyFileMode(t, filepath.Join(restoreDir, "restored-5"), defaultRestoredFilePermission)
 }
 
 func verifyFileMode(t *testing.T, filename string, want os.FileMode) {
@@ -332,7 +402,7 @@ func verifyFileMode(t *testing.T, filename string, want os.FileMode) {
 		return
 	}
 
-	s, err := os.Stat(filename)
+	s, err := os.Lstat(filename)
 	if err != nil {
 		t.Fatal(err)
 	}