Merge pull request #21817 from eyupcanakman/fix/sftp-dir-permissions

sftp: Use mode 0700 for repository directories

changelog/unreleased/issue-4447

@@ -0,0 +1,10 @@
+Bugfix: Use mode 0700 for repository directories created over SFTP
+
+When creating a repository over SFTP, restic created the repository directories
+with the SFTP server's default permissions, often 0755, rather than the 0700
+permissions it uses for local repositories.
+
+Restic now creates these directories with 0700 permissions.
+
+https://github.com/restic/restic/issues/4447
+https://github.com/restic/restic/pull/21817

internal/backend/sftp/sftp.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"os/exec"
 	"path"
+	"syscall"
 	"time"
 
 	"github.com/restic/restic/internal/backend"
@@ -177,20 +178,50 @@ func (r *SFTP) mkdirAllDataSubdirs(ctx context.Context, nconn uint) error {
 
 	for _, d := range r.Paths() {
 		g.Go(func() error {
-			// First try Mkdir. For most directories in Paths, this takes one
-			// round trip, not counting duplicate parent creations causes by
-			// concurrency. MkdirAll first does Stat, then recursive MkdirAll
-			// on the parent, so calls typically take three round trips.
+			// First try Mkdir, then chmod: for most directories in Paths
+			// this is two round trips. pkg/sftp has no mkdir that sets a
+			// mode. When the parent is missing, fall back to mkdirAll, which
+			// adds a Stat and recurses, taking several more round trips.
 			if err := r.c.Mkdir(d); err == nil {
-				return nil
+				return errors.Wrapf(r.c.Chmod(d, r.Modes.Dir), "Chmod %v", d)
 			}
-			return errors.Wrapf(r.c.MkdirAll(d), "MkdirAll %v", d)
+			return errors.Wrapf(r.mkdirAll(d, r.Modes.Dir), "MkdirAll %v", d)
 		})
 	}
 
 	return g.Wait()
 }
 
+// mkdirAll creates dir and any missing parent directories with the given mode.
+// (*sftp.Client).MkdirAll does not accept a mode, so directories would
+// otherwise inherit the SFTP server's umask.
+func (r *SFTP) mkdirAll(dir string, mode os.FileMode) error {
+	// If dir already exists, leave it and its mode untouched.
+	if fi, err := r.c.Stat(dir); err == nil {
+		if fi.IsDir() {
+			return nil
+		}
+		return &os.PathError{Op: "mkdir", Path: dir, Err: syscall.ENOTDIR}
+	}
+
+	// Create the parent directory first, then dir itself.
+	if parent := path.Dir(dir); parent != dir && parent != "." {
+		if err := r.mkdirAll(parent, mode); err != nil {
+			return err
+		}
+	}
+
+	if err := r.c.Mkdir(dir); err != nil {
+		// Ignore the error if another connection created dir concurrently.
+		if fi, statErr := r.c.Lstat(dir); statErr == nil && fi.IsDir() {
+			return nil
+		}
+		return err
+	}
+
+	return r.c.Chmod(dir, mode)
+}
+
 // IsNotExist returns true if the error is caused by a not existing file.
 func (r *SFTP) IsNotExist(err error) bool {
 	return errors.Is(err, os.ErrNotExist)
@@ -314,7 +345,7 @@ func (r *SFTP) Save(_ context.Context, h backend.Handle, rd backend.RewindReader
 
 	if r.IsNotExist(err) {
 		// error is caused by a missing directory, try to create it
-		mkdirErr := r.c.MkdirAll(r.Dirname(h))
+		mkdirErr := r.mkdirAll(r.Dirname(h), r.Modes.Dir)
 		if mkdirErr != nil {
 			debug.Log("error creating dir %v: %v", r.Dirname(h), mkdirErr)
 		} else {

internal/backend/sftp/sftp_test.go

@@ -1,12 +1,15 @@
 package sftp_test
 
 import (
+	"context"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/restic/restic/internal/backend"
 	"github.com/restic/restic/internal/backend/sftp"
 	"github.com/restic/restic/internal/backend/test"
 	"github.com/restic/restic/internal/errors"
@@ -27,6 +30,14 @@ func findSFTPServerBinary() string {
 
 var sftpServer = findSFTPServerBinary()
 
+func testConfig(path string) sftp.Config {
+	return sftp.Config{
+		Path:        path,
+		Command:     fmt.Sprintf("%q -e", sftpServer),
+		Connections: 5,
+	}
+}
+
 func newTestSuite(t testing.TB) *test.Suite[sftp.Config] {
 	return &test.Suite[sftp.Config]{
 		// NewConfig returns a config for a new temporary backend that will be used in tests.
@@ -34,12 +45,8 @@ func newTestSuite(t testing.TB) *test.Suite[sftp.Config] {
 			dir := rtest.TempDir(t)
 			t.Logf("create new backend at %v", dir)
 
-			cfg := &sftp.Config{
-				Path:        dir,
-				Command:     fmt.Sprintf("%q -e", sftpServer),
-				Connections: 5,
-			}
-			return cfg, nil
+			cfg := testConfig(dir)
+			return &cfg, nil
 		},
 
 		Factory: sftp.NewFactory(),
@@ -67,3 +74,53 @@ func BenchmarkBackendSFTP(t *testing.B) {
 
 	newTestSuite(t).RunBenchmarks(t)
 }
+
+func TestCreateSetsDirPermissions(t *testing.T) {
+	if sftpServer == "" {
+		t.Skip("sftp server binary not found")
+	}
+
+	cfg := testConfig(filepath.Join(rtest.TempDir(t), "repo"))
+
+	be, err := sftp.Create(context.Background(), cfg, func(string, ...interface{}) {})
+	rtest.OK(t, err)
+	defer func() { rtest.OK(t, be.Close()) }()
+
+	rtest.OK(t, filepath.WalkDir(cfg.Path, func(name string, d fs.DirEntry, err error) error {
+		if err != nil || !d.IsDir() {
+			return err
+		}
+		fi, err := d.Info()
+		if err != nil {
+			return err
+		}
+		if mode := fi.Mode().Perm(); mode != 0o700 {
+			return fmt.Errorf("directory %v has mode %o, want 700", name, mode)
+		}
+		return nil
+	}))
+}
+
+func TestSaveSetsDirPermissions(t *testing.T) {
+	if sftpServer == "" {
+		t.Skip("sftp server binary not found")
+	}
+
+	cfg := testConfig(filepath.Join(rtest.TempDir(t), "repo"))
+
+	be, err := sftp.Create(context.Background(), cfg, func(string, ...interface{}) {})
+	rtest.OK(t, err)
+	defer func() { rtest.OK(t, be.Close()) }()
+
+	// Remove a data subdirectory so that Save has to recreate it.
+	subdir := filepath.Join(cfg.Path, "data", "01")
+	rtest.OK(t, os.Remove(subdir))
+
+	data := []byte("test data")
+	h := backend.Handle{Type: backend.PackFile, Name: "0100000000000000000000000000000000000000000000000000000000000000"}
+	rtest.OK(t, be.Save(context.Background(), h, backend.NewByteReader(data, be.Hasher())))
+
+	fi, err := os.Stat(subdir)
+	rtest.OK(t, err)
+	rtest.Equals(t, os.FileMode(0o700), fi.Mode().Perm())
+}