-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mount: Add default-permissions flag to set FUSE option #2017
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2017 +/- ##
==========================================
- Coverage 50.78% 46.57% -4.21%
==========================================
Files 176 176
Lines 14165 14174 +9
==========================================
- Hits 7194 6602 -592
- Misses 5920 6579 +659
+ Partials 1051 993 -58
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for your contribution! I think it'd be good to invert the behavior as you suggested: When --allow-other
is enabled, by default enforce permissions and allow to disable this with --no-default-permissions
on explicit request. Better safe than sorry, even if this means a breaking change. In my experience, there won't be so many users affected, as using --allow-other
is a rather obscure use case.
This enforces the Unix permissions of the snapshot files within the mounted filesystem, which will only allow users to access snapshot files if they had access to the file outside of the snapshot.
This option restores the previous behavior of `mount` by disabling the "DefaultPermissions" FUSE option. This allows any user that can access the mountpoint to read any file from the snapshot. Normal FUSE rules apply, so `allow-root` or `allow-other` can be used to allow users besides the mounting user to access these files.
1549d0f
to
d4ff5b6
Compare
@fd0 Okay, I inverted the option and rebased! One thing I wanted to touch on is this change doesn't just affect the Personally, this seems like such a corner case that I doubt it would affect many (if any) cases in the wild, but I figured I'd mention it |
This commit changes the logic slightly: checking the permissions in the fuse mount when nobody else besides the current user can access the fuse mount does not sense. The current user has access to the repo files in addition to the password, so they can access all data regardless of what the fuse mount does. Enabling `--allow-root` allows the root user to access the files in the fuse mount, for this user no permission checks will be done anyway. The code now enables `DefaultPermissions` automatically when `--allow-other` is set, it can be disabled with `--no-default-permissions` to restore the old behavior.
I've added a commit which changes the logic slightly: checking the permissions in the fuse mount when nobody else besides the current user can access the fuse mount does not make much sense. The current user has access to the repo files in addition to the password, so they can access all data regardless of what the fuse mount does. Enabling The code now enables That's the best compromise in my opinion, and it only changes the behavior when |
What is the purpose of this change? What does it change?
This PR adds the new flag
--default-permissions
for themount
command, which enables the "DefaultPermissions" FUSE option. This option enforces the Unix permissions of the mounted filesystem. When this flag is disabled, any user can access any file from the restic FUSE mount if the--allow-other
flag is set; when enabled, a user can only access a file from a snapshot if they could access it originally.As a quick demo of this feature, I wrote up a quick gist: https://gist.github.com/kylewlacy/99ec3859955a25dbbb94a56ce1b4de42 (note that it requires root, or at least the ability to run as two different users).
Was the change discussed in an issue or in the forum before?
No (I figured it was easier to discuss this feature by providing this small PR first, then iterating on it if needed)
Checklist
changelog/unreleased/
that describes the changes for our users (template here)gofmt
on the code in all commitsN.B. manpage changes need to be regenerated still (I tried regenerating them but there were other changes too, so I figured it would be easier to leave out of this PR for now). I can regenerate the manpages if needed though
I also didn't write any tests for this PR. First, there were no tests for the other FUSE-specific options so I wasn't sure it was warranted, and second, this test would be hard to write since it would involve running at least as a root user and a non-root user. I'd be happy to try my hand at writing some tests if anyone provided some suggestions/ideas for testing this kind of feature
Other notes
I'd be more than happy to discuss and iterate on the design on this feature. Personally, finding out this caveat with
restic mount --allow-other
was definitely a surprise to me, so maybe it would make sense to invert this? As in, maybe this PR should be changed so "DefaultPermissions" is set by default, with an optional flag to disable it (that would be a breaking change though)