Skip to content
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

rage-mount: update dependencies fuse_mt, time and zip #324

Merged
merged 4 commits into from
Sep 3, 2022

Conversation

folliehiyuki
Copy link
Contributor

I picked the first commit of fuse_mt that switched from using fuse to fuser (fuse_mt hasn't had any new releases since 0.5.1). That way rage-mount can be built against libfuse3. time and zip dependencies update is a side effect due to TimeSpec being obsolete.

WARNING: I've never written any Rust code before, so please bear with my ignorance in the PR.

rage/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Owner

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run this locally and confirmed it compiles. However, the update results in a UX regression: while I can use rage-mount from current main to mount files fine, when I try to use this branch I get this confusing non-error that causes the mount to fail:

[INFO  rage_mount] Mounting as FUSE filesystem
[INFO  fuser::session] Mounting tmp/
[WARN  fuser::session] Given auto_unmount without allow_root or allow_other; adding allow_other, with userspace permission handling
fusermount: option allow_other only allowed if 'user_allow_other' is set in /etc/fuse.conf
Error: Success (os error 0)
[ Did rage not do what you expected? Could an error be more useful? ]
[ Tell us: https://str4d.xyz/rage/report                            ]

rage/Cargo.toml Show resolved Hide resolved
rage/Cargo.toml Show resolved Hide resolved
@str4d
Copy link
Owner

str4d commented Aug 21, 2022

rage-mount is passing the auto_unmount option:

let fuse_args: Vec<&OsStr> = vec![OsStr::new("-o"), OsStr::new("ro,auto_unmount")];
let fs = open().map(|fs| fuse_mt::FuseMT::new(fs, 1))?;
info!("{}", fl!("info-mounting-as-fuse"));
fuse_mt::mount(fs, &mountpoint, &fuse_args)?;

So my guess is that this is related to the migration from the fuse crate to the fuser crate.

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

Force-pushed to fix formatting errors.

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #324 (5e212a7) into main (26f99e2) will increase coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
+ Coverage   37.65%   37.77%   +0.11%     
==========================================
  Files          34       34              
  Lines        2998     3002       +4     
==========================================
+ Hits         1129     1134       +5     
+ Misses       1869     1868       -1     
Impacted Files Coverage Δ
rage/src/bin/rage-mount/main.rs 0.00% <0.00%> (ø)
rage/src/bin/rage-mount/tar.rs 0.00% <0.00%> (ø)
age/src/format.rs 50.54% <0.00%> (-1.68%) ⬇️
age/src/protocol.rs 61.53% <0.00%> (-0.89%) ⬇️
age/src/ssh.rs 23.65% <0.00%> (-0.82%) ⬇️
age-core/src/format.rs 61.19% <0.00%> (-0.75%) ⬇️
age-core/src/plugin.rs 30.72% <0.00%> (-0.61%) ⬇️
age/src/primitives/stream.rs 62.38% <0.00%> (+0.18%) ⬆️
age/src/primitives/armor.rs 41.66% <0.00%> (+0.81%) ⬆️
age/src/ssh/identity.rs 52.77% <0.00%> (+0.89%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

Looks like the UX regression is due to cberner/fuser#122, which I don't entirely understand because I can't find a canonical reference that specifies the requirement of either allow_other or allow_root.

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

Aha, this is upstream issue libfuse/libfuse#586. Perhaps this worked for libfuse2 but doesn't for libfuse3. I think the fix is to add allow_root, so that we widen access just enough to support auto-unmounting (which I want to ensure that the rage-mount process getting aborted doesn't leave a dangling filesystem) while not making the mount globally accessible.

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

Oh, except that fuser implements allow_root itself, by enabling allow_other and then restricting to owner+root. So we can't use auto_unmount without requiring rage-mount users to add user_allow_other to their /etc/fuse.conf, which is a system-wide configuration 😢

@folliehiyuki
Copy link
Contributor Author

Thanks for all the comments.

I'm pretty sure rage-mount worked for me the last time I updated the PR. Maybe it was just me not using auto_unmount.

@folliehiyuki
Copy link
Contributor Author

I'd say we keep note in the README about the behavior and reference it to the upstream issue. It's unlikely to get fixed soon.

I don't know how fuser handles stuff, but fusermount command (from fuse3 package) allows users to override mount options at runtime. Maybe we can do that with rage-mount?

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

Thanks for all the comments.

I'm pretty sure rage-mount worked for me the last time I updated the PR. Maybe it was just me not using auto_unmount.

auto_unmount is hard-coded into rage-mount, so you would have used it unless you modified the code. Do you have an /etc/fuse.conf file with user_allow_other (or any other settings) in it?

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

I'd say we keep note in the README about the behavior and reference it to the upstream issue. It's unlikely to get fixed soon.

I think what I want to do is add allow_root to keep the required access minimal, try and detect if the mount fails for this reason, and in that case warn the user and retry the mount without auto_unmount,allow_root. That way the user's action has a much greater chance of succeeding, with the downside that if the user kills the rage-mount process uncleanly, they may need to do some manual cleanup (and they also have a way to avoid this issue if they are willing to enable user_allow_other).

I don't know how fuser handles stuff, but fusermount command (from fuse3 package) allows users to override mount options at runtime. Maybe we can do that with rage-mount?

I'm not inclined to provide mount option configurations, in particular because there are options (specifically ro) that we need to enforce, but also because I do not see a use case for this kind of configuration. If someone can provide one, we can consider it separately.

@str4d
Copy link
Owner

str4d commented Aug 21, 2022

I think what I want to do is add allow_root to keep the required access minimal, try and detect if the mount fails for this reason, and in that case warn the user and retry the mount without auto_unmount,allow_root.

Ugh, this is made more annoying by the fact that the Filesystem impl is consumed by mount, and the relevant config check happens inside the external fusermount binary, so we'd need to either read and decrypt the age file twice (which means caching any identities or password), or figure out where the fuse.conf file is (its location differs between fuse2 and fuse3) and parse it ourselves.

Instead, I'm going to attempt to detect the error and provide a more useful error message.

@folliehiyuki
Copy link
Contributor Author

I just rebuilt rage again on this branch. rage-mount errors out now.

Sorry for the misinformation.

@str4d
Copy link
Owner

str4d commented Aug 22, 2022

Once cberner/fuser#218 is merged and released, the "Error: Success" message will be replaced by "Error: Unknown Error" which is an improvement, but still not great. The blocker on handling this error well is libfuse itself exposing sufficient error information: libfuse/libfuse#693

This also pins `fuser 0.11.1` which fixes an error message for libfuse2.
We now cleanly unmount the filesystem ourselves on Ctrl-C, instead of
relying on FUSE to do this for us. This removes the need for the ambient
`MountOptions::AllowOther` capability that `AutoUnmount` requires, while
ensuring that `rage-mount` correctly exits if the mountpoint is
unmounted externally (e.g. via `umount`).
@str4d
Copy link
Owner

str4d commented Sep 3, 2022

I figured out how to retain the current behaviour: by using spawn_mount instead of mount, and then triggering a channel from both a Ctrl-C handler and FilesystemMT::destroy, we can always cleanly unmount the filesystem on Ctrl+C without auto_unmount, while also reacting correctly to external unmounts.

I've tested this locally and it appears to work fine for me. @folliehiyuki can you confirm this works for you?

Copy link
Owner

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@str4d str4d added this to the rage 0.9.0 milestone Sep 3, 2022
@str4d
Copy link
Owner

str4d commented Sep 3, 2022

Actually, I'm going to go ahead and merge this, to unblock other PRs. If you encounter any problems, please open new issues. And thank you for the PR!

@str4d str4d merged commit 34340bb into str4d:main Sep 3, 2022
@folliehiyuki folliehiyuki deleted the fuse3 branch September 3, 2022 17:14
@folliehiyuki
Copy link
Contributor Author

I tried it out just now. Works as expected.

Thanks for doing all the heavy lifting in this PR.

2022-09-04-001556_grim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants