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()) +}