homed uidmapping (just for the directory backend) #21136

poettering · 2021-10-26T17:11:06Z

This is the first part of #21135, to make things easier to digest for reviewers.

This contains uidmapping for the directory backend only. the fscrypt + luks + cifs changes are left out and will be submitted in a separate PR later on.

bluca · 2021-10-26T17:22:15Z

../src/home/homework-mount.c:123:30: error: unused variable 'a' [-Werror,-Wunused-variable]
        _cleanup_free_ char *a = NULL;
                             ^

poettering · 2021-10-26T20:25:51Z

force pushed a new version, with the CI issue hopefully fixed

bluca · 2021-10-26T22:35:46Z

test-process-util in CentOS is failing, but doesn't seem related as the changes are contained in src/home - @mrc0mmand seen this before?

22:13:53 #1  0x00007f32ccef1db5 in abort () from /lib64/libc.so.6
22:13:53 No symbol table info available.
22:13:53 #2  0x00007f32cd406a85 in log_assert_failed (
22:13:53     text=text@entry=0x408e00 "limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH", file=file@entry=0x407b63 "src/test/test-process-util.c", 
22:13:53     line=line@entry=855, 
22:13:53     func=func@entry=0x408f90 <__PRETTY_FUNCTION__.10378> "test_get_process_ppid") at ../src/basic/log.c:866
22:13:53 No locals.
22:13:53 #3  0x0000000000406aa4 in test_get_process_ppid ()
22:13:53     at ../src/test/test-process-util.c:855
22:13:53         limit = 3416
22:13:53         r = <optimized out>
22:13:53         __PRETTY_FUNCTION__ = "test_get_process_ppid"
22:13:53         __func__ = "test_get_process_ppid"
22:13:53 #4  0x0000000000406e53 in main (argc=1, argv=0x7ffefe2da4a8)
22:13:53     at ../src/test/test-process-util.c:925
``

mrc0mmand · 2021-10-27T07:38:28Z

test-process-util in CentOS is failing, but doesn't seem related as the changes are contained in src/home - @mrc0mmand seen this before?

22:13:53 #1  0x00007f32ccef1db5 in abort () from /lib64/libc.so.6
22:13:53 No symbol table info available.
22:13:53 #2  0x00007f32cd406a85 in log_assert_failed (
22:13:53     text=text@entry=0x408e00 "limit >= INT_MAX || get_process_ppid(limit+1, NULL) == -ESRCH", file=file@entry=0x407b63 "src/test/test-process-util.c", 
22:13:53     line=line@entry=855, 
22:13:53     func=func@entry=0x408f90 <__PRETTY_FUNCTION__.10378> "test_get_process_ppid") at ../src/basic/log.c:866
22:13:53 No locals.
22:13:53 #3  0x0000000000406aa4 in test_get_process_ppid ()
22:13:53     at ../src/test/test-process-util.c:855
22:13:53         limit = 3416
22:13:53         r = <optimized out>
22:13:53         __PRETTY_FUNCTION__ = "test_get_process_ppid"
22:13:53         __func__ = "test_get_process_ppid"
22:13:53 #4  0x0000000000406e53 in main (argc=1, argv=0x7ffefe2da4a8)
22:13:53     at ../src/test/test-process-util.c:925
``

Yep, I've seen the a couple of times and it's definitely unrelated.

src/home/homework-mount.c

bluca · 2021-10-27T13:45:20Z

src/home/homework-mount.c

+        return TAKE_FD(userns_fd);
+}
+
+int home_shift_uid(int dir_fd, const char *target, uid_t stored_uid, uid_t exposed_uid, int *ret_mount_fd) {


This will be useful in the near future for other components, maybe add it directly to the library (eg, basic/namespace-util.c or similar)? The only homed-specific part as far as I can see are HOME_UID_MIN and HOME_UID_MAX, which could simply be taken as parameters.
Then there could even be a unit test added, which would be quite useful to ensure the fallback/detection works nicely everywhere.

i am always concerned about putting stuff in src/basic/ + src/shared/ if it doesn't have at least two users — unless it's a totally obvious case.

I am not sure about this one. Yes, this might be something we can reuse later, but I am pretty sure it's something that will require further modifications. But if that's the case we might as well move it into generic code later on. Unless the needed semantics of the other stuff are really clear and obvious I doubt we can generalize this properly.

ok, makes sense

poettering · 2021-10-27T14:41:38Z

added the assert.

(actually a few others too)

ptal

bluca

lgtm

yuwata

One question, and one minor suggestion.

src/home/home-util.h

src/home/homework-directory.c

Let's migrate home_create_directory_or_subvolume() to also use HomeSetup for storing its runtime objects we'd like to destroy in case of failure. In the beginning this is just the root_fd, but later on we can add more. No change in behaviour, just shifting things around.

…ting record For the other backends we synthesize a "binding" section in the json record of the user that stores meta info how a user record is "bound" to the local host. It declares storage info and such. Let's do the same for the directory/subvolume backends.

…ser-home-mount before moving them to /home This does what we already do for the LUKS backend: instead of mounting the source directory directly to the final home dir, we instead bind mount it to /run/systemd/user-home-mount (where /run/ is unshared and specific to our own mount namespace), then adjust its mount flags and then bind mount it in a single atomic operation into the final destination, fully set up. This doesn't improve much on its own, but it makes things a tiny bit more correct: this way MS_NODEV/MS_NOEXEC/MS_NOSUID will already be applied when the bind mount appears in the host mount namespace, instead of being adjusted after the fact. Doing things this way also makes things work more like the LUKS backend, reducing surprises. Most importantly it's preparation for doing uidmapping for directory homes, added in a later commit.

This new helper is not used yet, but it's useful for apply UID/GID shifts so that the underlying home dir can use an arbitrary UID (for example "nobody") and we'll still make it appear as owned by the target UID. This operates roughly like this: 1. The relevant underlying UID is mapped to the target UID 2. Everything in the homed UID range except for the target UID is left unmapped (and thus will appear as "nobody") 3. Everything in the 16bit UID range outside of the homed UID range/target UID/nobody user is mapped to itself 4. Everything else is left unmapped (in particular everything outside of the 16 bit range). Why do it like this? The 2nd rule done to ensure that any files from homed's managed UID range that do not match the user's own UID will be shown as "unmapped" basically. Of course, IRL this should never happen, except if people managed to manipulate the underlying fs directly. The 3rd rule is to allow that if devs untar an OS image it more or less just works as before: 16bit UIDs outside of the homed range will be mapped onto themselves: you can untar things and tar it back up and things will just work.

poettering · 2021-10-27T15:42:28Z

made the two indicated changes. since so minor upgrading green label again

poettering added the homed homed, homectl, pam_homed label Oct 26, 2021

poettering mentioned this pull request Oct 26, 2021

homed: make use of uidmap'ped mounts for avoiding recursive chown() #21135

Merged

bluca added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Oct 26, 2021

poettering force-pushed the homed-uidmap-dir-only branch from 0632b45 to d6db75d Compare October 26, 2021 20:23

poettering removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Oct 26, 2021

poettering added the please-review label Oct 27, 2021

poettering force-pushed the homed-uidmap-dir-only branch from d6db75d to 8029f0f Compare October 27, 2021 08:12

bluca reviewed Oct 27, 2021

View reviewed changes

bluca removed the please-review label Oct 27, 2021

poettering force-pushed the homed-uidmap-dir-only branch from 8029f0f to 5edd7ea Compare October 27, 2021 14:41

bluca approved these changes Oct 27, 2021

View reviewed changes

bluca added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 27, 2021

yuwata reviewed Oct 27, 2021

View reviewed changes

src/home/home-util.h Show resolved Hide resolved

src/home/homework-directory.c Outdated Show resolved Hide resolved

yuwata added good-to-merge/with-minor-suggestions and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels Oct 27, 2021

poettering added 6 commits October 27, 2021 17:37

homed: move HOME_UID_{MIN,MAX} into a header we can reuse in homework.c

f509ed4

homework: support uidmapping in the "directory" backend

cf5115f

poettering force-pushed the homed-uidmap-dir-only branch from 5edd7ea to cf5115f Compare October 27, 2021 15:42

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Oct 27, 2021

yuwata merged commit 1462a94 into systemd:main Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

homed uidmapping (just for the directory backend) #21136

homed uidmapping (just for the directory backend) #21136

poettering commented Oct 26, 2021

bluca commented Oct 26, 2021

poettering commented Oct 26, 2021

bluca commented Oct 26, 2021

mrc0mmand commented Oct 27, 2021

bluca Oct 27, 2021

poettering Oct 27, 2021

bluca Oct 27, 2021

poettering commented Oct 27, 2021

bluca left a comment

yuwata left a comment

poettering commented Oct 27, 2021 •

edited

homed uidmapping (just for the directory backend) #21136

homed uidmapping (just for the directory backend) #21136

Conversation

poettering commented Oct 26, 2021

bluca commented Oct 26, 2021

poettering commented Oct 26, 2021

bluca commented Oct 26, 2021

mrc0mmand commented Oct 27, 2021

bluca Oct 27, 2021

Choose a reason for hiding this comment

poettering Oct 27, 2021

Choose a reason for hiding this comment

bluca Oct 27, 2021

Choose a reason for hiding this comment

poettering commented Oct 27, 2021

bluca left a comment

Choose a reason for hiding this comment

yuwata left a comment

Choose a reason for hiding this comment

poettering commented Oct 27, 2021 • edited

poettering commented Oct 27, 2021 •

edited