mkosi icon indicating copy to clipboard operation
mkosi copied to clipboard

Implement support for generated files ownership management.

Open qdeslandes opened this issue 2 years ago • 7 comments

Implement support for generated file ownership management:

  • Add a new --map-permissions option to request mkosi to change ownership of generated files and directories to either SUDO_UID or PKEXEC_UIC (depending on which one is available).
  • Bind mount using X-mount.idmap option to remap root owner to the owner of the directory. All the files subsequently created by mkosi (as root) will be owned but the owner for the mounted directory.
  • systemd-nspawn is invoked with new rootidmap bind option (https://github.com/systemd/systemd/pull/24467).

Mount's X-mount.idmap feature is not yet merged (https://github.com/util-linux/util-linux/pull/1661). If X-mount.idmap is not supported, mount will silently ignore it and mount the directory without it. This means --map-permissions could fail silently. What would be the best way to handle this? Check for mount's version to explicitly fail when --map-permission is called?

qdeslandes avatar Aug 26 '22 23:08 qdeslandes

I've only had a quick look for now, but some thoughts:

  • Maybe I missed it, but I don't see a reasoning for making the mount executable configurable. Could you maybe explain that a bit? Since only the executable would be switched, it would still need to adhere to the CLI of regular mount, no?
  • I'm not opposed to chowning the more persistent directories - this is something that people have asked for in the past -, but I'd rather not stuff the user running mkosi into the args. Stuff like int(os.getenv("SUDO_UID") or os.getenv("PKEXEC_UID") or '0') could just be a function or possibly include the chown call into some chown_to_running_user.
  • The idmapping option for nspawn should check whether the nspawn version is recent enough, because most users will probably not run the most recent systemd releases.

behrmann avatar Aug 27 '22 08:08 behrmann

I've only had a quick look for now, but some thoughts:

Maybe I missed it, but I don't see a reasoning for making the mount executable configurable. Could you maybe explain that a bit? Since only the executable would be switched, it would still need to adhere to the CLI of regular mount, no?

Making mount executable configurable was useful to me during development, I thought I might as well include it in the PR. nspawn executable is configurable already (MKOSI_NSPAWN_EXECUTABLE), I assume the benefits are identical.

I'm not opposed to chowning the more persistent directories - this is something that people have asked for in the past -, but I'd rather not stuff the user running mkosi into the args. Stuff like int(os.getenv("SUDO_UID") or os.getenv("PKEXEC_UID") or '0') could just be a function or possibly include the chown call into some chown_to_running_user.

What you suggest would be to only be aware of the running user when chown is actually called, instead of storing it within MkosiArgs, is that what you mean?

The idmapping option for nspawn should check whether the nspawn version is recent enough, because most users will probably not run the most recent systemd releases.

I agree, although rootidmap is not yet in any nspawn release, so I don't know yet which version number to check for. Ideally, we should check for mount version too, as not all versions support X-mount.idmap.

qdeslandes avatar Aug 30 '22 19:08 qdeslandes

Making mount executable configurable was useful to me during development, I thought I might as well include it in the PR. nspawn executable is configurable already (MKOSI_NSPAWN_EXECUTABLE), I assume the benefits are identical.

Ah, that was to have an escape hatch when working on nspawn itself. Yeah, sure. I'm ±0 on this change.

What you suggest would be to only be aware of the running user when chown is actually called, instead of storing it within MkosiArgs, is that what you mean?

Yes, mkosi is either run by root or those environment variables are in the env during the whole of mkosi's run.

I agree, although rootidmap is not yet in any nspawn release, so I don't know yet which version number to check for. Ideally, we should check for mount version too, as not all versions support X-mount.idmap.

I see, then this being merged is contingent on that being merged first and ending in a release.

behrmann avatar Aug 31 '22 09:08 behrmann

What you suggest would be to only be aware of the running user when chown is actually called, instead of storing it within MkosiArgs, is that what you mean?

Yes, mkosi is either run by root or those environment variables are in the env during the whole of mkosi's run.

Sounds good to me,

I agree, although rootidmap is not yet in any nspawn release, so I don't know yet which version number to check for. Ideally, we should check for mount version too, as not all versions support X-mount.idmap.

I see, then this being merged is contingent on that being merged first and ending in a release.

This PR is composed of 4 commits, the 3rd one requires https://github.com/systemd/systemd/pull/24467 and the 4th one requires https://github.com/util-linux/util-linux/pull/1661. The 1st and 2nd one can be merged.

qdeslandes avatar Aug 31 '22 13:08 qdeslandes

This pull request introduces 1 alert when merging 1a85b959c4707e3bd941de643200499e70deb537 into b7cfe85a262ceeacbf77772e00050abd39a4bbad - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Aug 31 '22 13:08 lgtm-com[bot]

Rebased on main branch, fixed the merge conflict and added a check in Use rootidmap with nspawn's bind mounts to ensure we use systemd-nspawn 252 or later.

Last commit (Use idmapping for package cache bind mount) still needs to wait for X-mount.idmap support to be merged.

qdeslandes avatar Sep 14 '22 13:09 qdeslandes

Also, the MOUNT_EXECUTABLE stuff can be dropped, #1190 allows you to use --extra-search-paths instead to override the mount executable.

DaanDeMeyer avatar Sep 21 '22 08:09 DaanDeMeyer

instead of the mount binary stuff I think we should people towards using ExtraSearchPaths= instead. Or maybe add mkosi.path/ or so as special dir people can link files or dirs into that shall be preferred over the system ones.

poettering avatar Sep 22 '22 08:09 poettering