Enable admin to decide group access to repository files

[DanielG]

This fixes #2351, see also #2351 (comment)

Since the directory mode doesn't allow the execute bit (search permission)
to be set regardless of umask the files don't need to have empty group
permissions to disallow group access. By default the missing group execute
access on the containing directory will block any read/write attempt on the
files.

The upside of this is that the sysadmin can choose to change the directory
permissions to allow group execute access since all repo directories are
created at init time and not touched again after that. This will then allow
the group access to the files.

Checklist

• I have read the Contribution Guidelines
• I have enabled maintainer edits for this PR
• I have added tests for all changes in this PR
• I have added documentation for the changes (in the manual)
• There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
• I have run gofmt on the code in all commits
• All commit messages are formatted in the same style as the other commits in the repo
• I'm done, this Pull Request is ready for review

[greatroar]

This new mode applies also to the SFTP backend, which does not (cannot, at least not portably) honor the local umask.

[DanielG]

Indeed the sftp backend handles this by chmod'ing instead:

internal/backend/sftp/sftp.go:277:

	// pkg/sftp doesn't allow creating with a mode.
	// Chmod while the file is still empty.
	if err == nil {
		err = f.Chmod(backend.Modes.File)
	}

[beewoolie]

This makes me wonder a bit. If I understand the change correctly, the init of the repo requires an extra step, after creation, to change the permissions in the repo directories. If that's true, I'd say it is likely to be more evident to users if an option is added to the init subfunction that allows local and sftp to backends to manage permissions in a way that permits multiple users explicitly. It might need to have a group name, for example, so that it will, at a minimum, make the directories sticky for this group.

Making this an option for the repo means that it can be queried as part of the configuration. Inspection of the setup further enables validation of the expected configuration.

That said, this is something that blocks an expected use case for restic, multiple contributing users to the backup data store with a local store.

[DanielG]

On Sat, Nov 13, 2021 at 12:55:07PM -0800, Marc Singer wrote: If I understand the change correctly, the init of the repo requires an extra step, after creation, to change the permissions in the repo directories.

Yeah you got that right. The user/admin has to do a chmod call after repo init, but only once ever after that it all just works.

If that's true, I'd say it is likely to be more evident to users if an option is added to the init subfunction that allows local and sftp to backends to manage permissions in a way that permits multiple users explicitly.

Just to be clear since you bring up sftp again: this PR makes the shared repo use-case work for both local and sftp repos. Maybe I wasn't quite clear on that point above. The only problem with the sftp code is that the sftp server side can't currently restict the modes further than what we set in restic because we set the modes we want instead of taking the modes the file was created with by the server side into account. Do keep in mind however that the sftp umask was being ignored already and this PR doesn't represent a change in behaviour on that end. So if an sftp server was trying to restrict the file permissions that way it was already broken :)

It might need to have a group name, for example, so that it will, at a minimum, make the directories sticky for this group.

Right, or (default) ACLs could be used. But restic just doesn't need to be involved with any of that. Since we're alreay in a situation where a user has to create a system group to make this work I don't really see why we would need a fancy option in the init command to do what a chmod call can do.

Making this an option for the repo means that it can be queried as part of the configuration. Inspection of the setup further enables validation of the expected configuration.

Sure, but honestly that sounds like waaaaaay more work than literally a one character change. I don't think merging this as-is would proclude anyone from implementing what you suggest though. As things stand tough this is a major pain in my neck as I have to maintain a restic fork just to make backups work for systems in my domain :] The only real problem with my approach is a hypothetical repo where someone accidentally changed the directory mode to world/group readable without wanting files to be exposed. I find that scenario sufficiently unlikey not to worry about, especially since the repo files are encrypted to boot.

…

--Daniel

[beewoolie]

I'd certainly be happy to have this wee change applied. The lack of a willing member of the team to review the change is, well, disapointing.

My point with the options is that it daylights what could be a hidden detail. When it comes to permissions, IMHO, I'd rather give a little extra boost to make the effect apparent. It adds some noise to be sure.

And, on the other hand, we seldom run these systems with shared groups (e.g.) such that the likely impact of this change is harmless.

[MichaelEischer]

The change look like a big hack, that will cause problems if restic ever has to create a new directory. I'd rather propose a different approach:

Use the current permissions of the config file to let the local backend (and probably sftp too) detect whether the repository should be accessible only to the current user or also to the group. A file permission of 0400 would mean user-only and 0440 would allow group access. To switch between both variants, just run chmod on the repository.

[DanielG]

@MichaelEischer Good idea. I've implemented it.

[beewoolie]

Interesting strategy. Makes me wonder how it will be documented. IMHO, it seems discrete to use the file mode of a configuration file created by restic to determine file modes for files created within the repo.

[MichaelEischer]

Besides the comments I've added, the PR also should add a short notice to the documentation (probably to doc/030_preparing_a_new_repo.rst) that a repository can be made user/group accessible depending on the config file permissions and how to switch.

[MichaelEischer]

Interesting strategy. Makes me wonder how it will be documented. IMHO, it seems discrete to use the file mode of a configuration file created by restic to determine file modes for files created within the repo.

The config file is part of the repository and not some external configuration file. So, I don't really understand what should be a problem here?

[DanielG]

Changes:

• Address review comments
• Rewrite changelog
• Add docs

[DanielG]

@MichaelEischer ping, I'd appreciate another review.

Changes:

• Improve wording in docs

[DanielG]

Ok, so it looks like this can't work as-is yet because the config file is created not as part of the backend Create function but rather by the repository Init/init functions.

[MichaelEischer]

Ok, so it looks like this can't work as-is yet because the config file is created not as part of the backend Create function but rather by the repository Init/init functions.

My suggestion would to ignore the stat error in Open if the config file does not exist (yet).

[DanielG]

Oh yeah, another good idea. Implemented.

[MichaelEischer]

I've rebased to PR to fix merge conflicts with the current master. Besides the few comments on the documentation entry, the code seems fine.

[DanielG]

I've reworked the docs a bit and (hopefully) properly resolved the conflict with cd78335 by undoing the merging of code from Open/Create into open. @MichaelEischer I think your conflict resolution would have failed to work in the Create code path as the config file wouldn't exist there yet so stat would fail. EDIT: nvm. I forgot we handle the error case in DeriveModesFromFileInfo now.

[MichaelEischer]

LGTM. Thanks!

I've rebased the PR to fix the merge conflict in doc/030_preparing_a_new_repo.rst .