From 33bea4a3f1a7a11c85d95faf3a77bb18928e2daf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ey=C3=BCp=20Can=20Akman?= Date: Wed, 20 May 2026 21:38:48 +0300 Subject: [PATCH] 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 | 10 +++++ internal/backend/sftp/sftp.go | 45 ++++++++++++++++++---- internal/backend/sftp/sftp_test.go | 61 ++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 changelog/unreleased/issue-4447 diff --git a/changelog/unreleased/issue-4447 b/changelog/unreleased/issue-4447 new file mode 100644 index 000000000..e2e59e414 --- /dev/null +++ b/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 diff --git a/internal/backend/sftp/sftp.go b/internal/backend/sftp/sftp.go index 55f92ec7b..141b71bac 100644 --- a/internal/backend/sftp/sftp.go +++ b/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 { diff --git a/internal/backend/sftp/sftp_test.go b/internal/backend/sftp/sftp_test.go index 75adc0c6b..00e938273 100644 --- a/internal/backend/sftp/sftp_test.go +++ b/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()) +}