sftp: Use mode 0700 for repository directories#21817
Conversation
a266449 to
33bea4a
Compare
MichaelEischer
left a comment
There was a problem hiding this comment.
Thanks for the PR, I have two minor remarks. Besides that LGTM.
|
|
||
| // Remove a data subdirectory so that Save has to recreate it. | ||
| subdir := filepath.Join(cfg.Path, "data", "01") | ||
| rtest.OK(t, os.RemoveAll(subdir)) |
There was a problem hiding this comment.
The repository is empty at that point such that os.Remove(...) should also be sufficient.
| t.Skip("sftp server binary not found") | ||
| } | ||
|
|
||
| cfg := sftp.Config{ |
There was a problem hiding this comment.
Please deduplicate the sftp.Config generation in the new and existing tests.
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.
33bea4a to
62cf574
Compare
|
Done, switched to |
|
I'm not convinced that changing this default and set 0700 permissions "in stone" is a good idea. (Although I have to admit I was not aware of the 0700 default for local repos either, and don't think that to be a great choice as well ;)) Taking away the opportunity to have system-wide and user-set defaults via umask affect a number of use cases that might not be immediately obvious, but still worthwhile - such as having a dedicated user account with read-only access to all repo data be able to rsync/copy all repo data over to another host. Since restic's threat model does allow for unauthorized parties to read repository content (because the attacker cannot learn anything about snapshot contents that way), I don't think the potential gain (keeping ciphertext inaccessible via DAC) is necessarily worth any potential breakage. |
The sftp backend already enforces specific permissions on files, thus extending this to directories does not introduce new problems. Besides, there's a mechanism for group-accessible repositories for the local and sftp backend: https://restic.readthedocs.io/en/stable/030_preparing_a_new_repo.html#group-accessible-repositories |
Ah, thanks for the pointer - I was not aware of this feature/subtlety until now. After having learned these facts, I wholeheartedly support the suggested change ;) |
What does this PR change? What problem does it solve?
When a repository is initialized over SFTP, its directories were created with the SFTP server's default permissions, often 0755, instead of the 0700 used for local repositories.
The SFTP backend creates them with pkg/sftp's Mkdir and MkdirAll, which take no mode argument.
It now sets each created directory to 0700.
Was the change previously discussed in an issue or on the forum?
Closes #4447
Checklist
changelog/unreleased/that describes the changes for our users (see template).