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

mount_tomb: make use of ACL in specific locations #475

Merged
merged 1 commit into from
Oct 30, 2024
Merged

Conversation

Narrat
Copy link
Collaborator

@Narrat Narrat commented Jun 23, 2023

Namely /run/media/$USER.
The mount point itself is owned by root, therefore one needs to know the name of the mountpoint to change to the location. Other tools for mounting media like udisksctl set ACL to allow the owner to use it normally (autocompletion and such).

Fixes #461


To get the solution for the named issue going.
There were some undecided points and maybe with some changed code this can go on further.

Therefore this PR is only a draft. Request for comments

@jaromil
Copy link
Member

jaromil commented Jun 24, 2023

This is especially interesting if it allows us to remove the chown to $USER in case ACL==1 and just use setfacl.

@Narrat
Copy link
Collaborator Author

Narrat commented Jun 25, 2023

Interesting point. Theoretically it would allow that. But then it probably needs a better check for ACL support. Let's see what can be done.

@Narrat
Copy link
Collaborator Author

Narrat commented Jun 26, 2023

Urg.. not sure how to proceed, as this could easily spiral somewhat out of control and cause big changes :D At least it seems currently like that.
In general all major file systems have ACL support (ext, btrfs, zfs, tmpfs, f2fs). At least that worry is gone. And it seems to be included in the default mount options.
But doesn't hinder people to mount with noacl. And it currently seems there is no easy way to detect if it is supported in the mount location. Unless trying to parse the output of mount?
In general it could be nice to use it also for mount points like /media (as an alternative to chown). But that needs special or a changed handling as /media and /run/media/$USER/ are somewhat different. The ACL in the latter case currently just makes sure that the $USER folder can be accessed by the $USER as it isn't (and shouldn't?) covered by the chown.
The actual mountpoint can indeed be additionally handled by ACL instead of the chown.
And from that point things are simpler. Default ext4, default options. So replacing the chown and whatever should happend with the -p option could be viable. And ACL could be stored in an additional file for easy restoration and allowing the $USER customizing it.
Comments? Or something I overlooked?

@jaromil
Copy link
Member

jaromil commented Sep 21, 2023

We need more exhaustive testing for this, many corner cases can arise... Should we leave the code as-is? Perhaps ACL as an opt-in rather than auto-detect.

@Narrat
Copy link
Collaborator Author

Narrat commented Sep 22, 2023

This is definitively nothing for 2.10 :D

I lean to split the approach:

  • Inside of the tomb
  • the eventual mointpoint

For the first point we can use auto-detect or entirely switch, as there is control over the initial tomb. But could also be be opt-in. Set a flag and a ACL config file is placed beside the other tomb files in a tomb. If it's there apply that instead of the chown.
Regarding the second point.. yeah... Opt-in may be the way to go. But still not really sure how to approach it the best (although I must admit this got again lost in daily chores...)

@Narrat Narrat force-pushed the acl branch 3 times, most recently from 5fcbea1 to dac78c5 Compare August 8, 2024 17:41
@Narrat Narrat marked this pull request as ready for review August 8, 2024 17:43
@Narrat
Copy link
Collaborator Author

Narrat commented Aug 8, 2024

The scope of this PR adjusted to its original intent to match the behaviour when mounting a tomb at /run/media/$USER/.
Additional thoughts on replacing chown or support ACL-files in a tomb aren't forgotten, but should discussed separately. I got some ideas on how to proceed with those cases, but currently there are enough open construction sites :D

Namely /run/media/$USER, which was introduced as a replacement for the
classic /media. Main motiviation being, that $USER_B shouldn't get
access to or information about mounted devices from $USER_A.
The mount point itself is owned by root, therefore one needs currently
to know the name of the mountpoint to change to the location.
Other tools for mounting media like udisksctl set ACL to allow the
owner to use it normally (autocompletion and such).

Fixes dyne#461
@jaromil
Copy link
Member

jaromil commented Oct 30, 2024

excellent! will merge this now. If you have time I am very interested in your opinion on kdf in the other PR

@jaromil jaromil merged commit a6e6a9c into dyne:master Oct 30, 2024
3 checks passed
@Narrat Narrat deleted the acl branch October 30, 2024 18:48
@Narrat
Copy link
Collaborator Author

Narrat commented Oct 30, 2024

Rebasing seems to be have something which it shouldn't have :D It reintroduced partly old behaviour of tomb slam

			[[ "$subcommand" == "slam" ]] && {
				SLAM=1
				[[ $LSOF == 0 ]] && {
					unset SLAM
					_warning "lsof not installed: cannot slam tombs."
					_warning "Trying a regular close." }}

https://github.com/dyne/tomb/blob/master/tomb#L3320-L3325
is now back.

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.

mountpoint /run/media/$USER always with root ownership
2 participants