sftp: Use mode 0700 for repository directories

The SFTP backend created repository directories with pkg/sftp's Mkdir
and MkdirAll, which take no mode argument, so the directories inherited
the SFTP server's umask instead of the 0700 used for local
repositories. Set the mode of each directory the backend creates.

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"
@@ -67,3 +70,61 @@ func BenchmarkBackendSFTP(t *testing.B) {
 
 	newTestSuite(t).RunBenchmarks(t)
 }
+
+func TestCreateSetsDirPermissions(t *testing.T) {
+	if sftpServer == "" {
+		t.Skip("sftp server binary not found")
+	}
+
+	cfg := sftp.Config{
+		Path:        filepath.Join(rtest.TempDir(t), "repo"),
+		Command:     fmt.Sprintf("%q -e", sftpServer),
+		Connections: 5,
+	}
+
+	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 := sftp.Config{
+		Path:        filepath.Join(rtest.TempDir(t), "repo"),
+		Command:     fmt.Sprintf("%q -e", sftpServer),
+		Connections: 5,
+	}
+
+	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.RemoveAll(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())
+}