Skip to content

core: initialize groups list before checking SupplementaryGroups= of …#4533

Merged
poettering merged 1 commit intosystemd:masterfrom
endocode:djalal/fix-initgroups-v1
Nov 2, 2016
Merged

core: initialize groups list before checking SupplementaryGroups= of …#4533
poettering merged 1 commit intosystemd:masterfrom
endocode:djalal/fix-initgroups-v1

Conversation

@tixxdz
Copy link
Member

@tixxdz tixxdz commented Nov 1, 2016

…a unit

Always initialize the supplementary groups of caller before checking the
unit SupplementaryGroups= option.

Fixes #4531

@tixxdz
core: initialize groups list before checking SupplementaryGroups= of …
efb1ba6 
…a unit

Always initialize the supplementary groups of caller before checking the
unit SupplementaryGroups= option.

Fixes systemd#4531
@tixxdz tixxdz mentioned this pull request Nov 1, 2016
Closed
@evverx
Copy link
Contributor

evverx commented Nov 2, 2016

@tixxdz , thanks! This fixes #4531.

But I wonder how should interact DynamicUser=, SupplementaryGroups= and RootDirectory ? :-)

# /etc/systemd/system/b.service
[Service]
DynamicUser=yes
SupplementaryGroups=1
ExecStart=/busybox id
SupplementaryGroups=2
SupplementaryGroups=
SupplementaryGroups=3
RootDirectory=/chroot

systemd[570]: b.service: Failed at step NAMESPACE spawning /busybox: No such file or directory

570   01:20:58 openat(3</>, "chroot", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 4</chroot>
570   01:20:58 fstat(4</chroot>, {st_dev=makedev(8, 1), st_ino=10201, st_mode=S_IFDIR|0755, st_nlink=2, st_uid=0, st_gid=0, st_blksize=1024, st_blocks=2, st_size=1024, st_atime=2016/11/02-01:03:20, st_mti
570   01:20:58 close(3</>)              = 0
570   01:20:58 openat(4</chroot>, "tmp", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
570   01:20:58 close(4</chroot>)        = 0

[Service]
DynamicUser=yes
SupplementaryGroups=1
ExecStart=/bin/sh -x -c id
SupplementaryGroups=2
SupplementaryGroups=
SupplementaryGroups=3

Started b.service.
b.service: Executing: /bin/sh -x -c id
+ id
b.service: User lookup succeeded: uid=62326 gid=62326
uid=62326 gid=62326 groups=62326
b.service: Child 974 belongs to b.service
b.service: Main process exited, code=exited, status=0/SUCCESS
b.service: Changed running -> dead
b.service: Collecting.

@tixxdz
Copy link
Member Author

tixxdz commented Nov 2, 2016

@evverx I see, actually I didn't look that much into the interaction with DynamicUser= , so SupplementaryGroups= and RootDirectory works perfectly but if you add DynamicUser= it may fail. Ok will take a look at it later today. I would say we have to ignore SupplementaryGroups= when DynamicUser= is set.
Also I explicitly renamed functions that deal with User=, Group= and SupplementaryGroups= as get_fixed_user() , get_fixed_group() and get_fixed_supplementary_groups() so it's clear what we are doing...

@poettering @keszybz a comment ? thanks

@evverx
Copy link
Contributor

evverx commented Nov 2, 2016

I would say we have to ignore SupplementaryGroups= when DynamicUser= is set.

Well,

    DynamicUser=
     ...
    If a statically allocated user or group of the configured name already exists, it is used and no dynamic user/group is allocated.
    ...

But

...
DynamicUser=yes
User=hola
SupplementaryGroups=1

doesn't work. I mean we get

systemd[1]: a.service: User lookup succeeded: uid=1001 gid=1001
...
sh[184]: uid=1001(hola) gid=1001(hola) groups=1001(hola)
...

Also I explicitly renamed functions that deal with User=, Group= and SupplementaryGroups= as get_fixed_user() , get_fixed_group() and get_fixed_supplementary_groups() so it's clear what we are doing

Sure. But I'm not sure users read the code :-)

@tixxdz
Copy link
Member Author

tixxdz commented Nov 2, 2016

@evverx hmm ok!

Sure. But I'm not sure users read the code :-)

Hehe! Ok so the thing if we use supplementary groups and support them then coupled with DynamicUser= , services may share stuff, and we don't have control on the list of groups...

IMHO If you manually set SupplementaryGroups=1 DynamicUser=yes and User=hola it should only keeps the original list of supplementary groups of user hola which also relates to DynamicUser=yes and that's it, ignoring in the way the unit option SupplementaryGroups=1 cause it clashes. The later is dynamic where the first one is static and does not really relate to that user. What do you think ? in that case we should just improve the doc.

I will test later if DynamicUser= also honors the supplementary group list if the specified user if found.

Thanks

@poettering
Copy link
Member

poettering commented Nov 2, 2016

@tixxdz as documented SupplementaryGroups= should only extend, never override the supplemenary groups listed in /etc/groups.

And I have the suspicion that we should use the same codepaths for this regardless if DynamicUsers= is set or not. After all, DynamicUsers= means "use a dynamic user if there is no static user for this". It does not mean "use a dynamic user, and fail if there is a static user"...

@poettering
Copy link
Member

poettering commented Nov 2, 2016

the patch looks OK to me, merging. We still need to fix the example @evverx described. @evverx, can you file a separate issue about it, we can track independently?

@poettering poettering merged commit bbeea27 into systemd:master Nov 2, 2016
@poettering poettering added the pid1 label Nov 2, 2016
@tixxdz
Copy link
Member Author

tixxdz commented Nov 2, 2016

@poettering ok, @evverx just assign to me since I'm still in the context. Thanks!

@evverx evverx mentioned this pull request Nov 2, 2016
Closed
@evverx
Copy link
Contributor

evverx commented Nov 3, 2016 •
edited
Loading

@poettering , I'm not sure about DynamicUser=yes and RootDirectory=. Should I file another issue? Or is this an expected behaviour?

# /etc/systemd/system/b.service
[Service]
DynamicUser=yes
SupplementaryGroups=1
ExecStart=/busybox id
SupplementaryGroups=2
SupplementaryGroups=
SupplementaryGroups=3
RootDirectory=/chroot

systemd[570]: b.service: Failed at step NAMESPACE spawning /busybox: No such file or directory

570   01:20:58 openat(3</>, "chroot", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 4</chroot>
570   01:20:58 fstat(4</chroot>, {st_dev=makedev(8, 1), st_ino=10201, st_mode=S_IFDIR|0755, st_nlink=2, st_uid=0, st_gid=0, st_blksize=1024, st_blocks=2, st_size=1024, st_atime=2016/11/02-01:03:20, st_mti
570   01:20:58 close(3</>)              = 0
570   01:20:58 openat(4</chroot>, "tmp", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
570   01:20:58 close(4</chroot>)        = 0

@tixxdz
Copy link
Member Author

tixxdz commented Nov 3, 2016

@evverx for DynamicUser=yes and RootDirectory I'm not sure my first guess would be maybe it's related to the runtime directory ?

@tixxdz
Copy link
Member Author

tixxdz commented Nov 3, 2016 •
edited
Loading

@evverx to complete on this, it may also fail on RootDirectory and PrivateTmp , not only DynamicUser= , so RootDirectory fails maybe on some or all options that create a mount namespace...

@poettering
Copy link
Member

poettering commented Nov 3, 2016

@evverx, @tixxdz hmm, i am not entirely sure what you are trying to say, what exactly goes wrong with RootDirectory? i figure filing a new bug would be best if there's something you have the suspicion needs fixing.

@evverx
Copy link
Contributor

evverx commented Nov 4, 2016

so RootDirectory fails maybe on some or all options that create a mount namespace...

@tixxdz, oh, indeed

what exactly goes wrong with RootDirectory?

@poettering Failed at step NAMESPACE :-)

And this breaks the TestPathsStat-test:

=== RUN   TestPathsStat
--- FAIL: TestPathsStat (7.72s)
    rkt_tests.go:324: didn't receive expected output "/sys/firmware: mode: d---------": image: using image from file /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/stage1-src.aci
        image: using image from file /home/vagrant/rkt/build-rkt-1.18.0+git/tmp/functional/test-tmp/rkt-inspect-stat-procfs.aci
        stage0: Preparing stage1
        stage0: Writing image manifest
        stage0: Loading image sha512-f0927d9c538b3f7aa1ed55f51233b03d1b0828cd024a98224fd7a92395b1e023
        stage0: Writing image manifest
...
        [FAILED] Failed to start Application=rkt-inspect Image=coreos.com/rkt-inspect.
        See 'systemctl status rkt-inspect.service' for details.
                 Stopping rkt-inspect Reaper...
        [  OK  ] Reached target rkt apps target.
        [  OK  ] Reached target Exit the container.
        [  OK  ] Stopped rkt-inspect Reaper.
        [  OK  ] Reached target Halt.
                 Stopping Pod shutdown...
        Sending SIGTERM to remaining processes...
        Sending SIGKILL to remaining processes...
        Container rkt-8ed7bcce-7e92-458c-a3c6-854bcd64f973 failed with error code 226.
    rkt_tests.go:142: rkt terminated with unexpected status 226, expected 0

cc @lucab

How to reproduce:

-bash-4.3# systemctl cat hola --no-pager
# /etc/systemd/system/hola.service
[Service]
ExecStart=/busybox id
RootDirectory=/chroot
ProtectKernelTunables=yes

-bash-4.3# systemctl start hola

-bash-4.3# journalctl -u hola --no-pager
...
Nov 03 08:58:00 systemd-testsuite systemd[1]: hola.service: Passing 0 fds to service
Nov 03 08:58:00 systemd-testsuite systemd[1]: hola.service: About to execute: /busybox id
Nov 03 08:58:00 systemd-testsuite systemd[1]: hola.service: Forked /busybox as 195
Nov 03 08:58:00 systemd-testsuite systemd[1]: hola.service: Changed dead -> running
Nov 03 08:58:00 systemd-testsuite systemd[1]: hola.service: Job hola.service/start finished, result=done
Nov 03 08:58:00 systemd-testsuite systemd[1]: Started hola.service.
Nov 03 08:58:01 systemd-testsuite systemd[1]: hola.service: Failed to send unit change signal for hola.service: Connection reset by peer
Nov 03 08:58:01 systemd-testsuite systemd[1]: hola.service: Child 195 belongs to hola.service
Nov 03 08:58:01 systemd-testsuite systemd[1]: hola.service: Main process exited, code=exited, status=226/NAMESPACE
Nov 03 08:58:01 systemd-testsuite systemd[1]: hola.service: Changed running -> failed

@evverx
Copy link
Contributor

evverx commented Nov 4, 2016

Hm, actually, the TestPathsStat prepares the /chroot. And the strace-output scares me (note: we try to openat some junk from the memory: "\320J\205\302\374\177", "0\\\205\302\374\177" and so on:

279   close(3</chroot/proc>)            = 0
279   close(4</chroot/proc/sys>)        = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "\1", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "s@\376\321\314U", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</> <unfinished ...>
279   <... close resumed> )             = 0
279   getcwd( <unfinished ...>
279   <... getcwd resumed> "/", 4096)   = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "\320J\205\302\374\177", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "0\\\205\302\374\177", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "pJ\205\302\374\177", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "0\\\205\302\374\177", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "\2", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   getcwd("/", 4096)                 = 2
279   open("/", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3</>
279   openat(3</>, "\30", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = -1 ENOENT (No such file or directory)
279   close(3</>)                       = 0
279   socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0) = 3<UNIX:[10508]>
279   getsockopt(3<UNIX:[10508]>, SOL_SOCKET, SO_SNDBUF, [212992], [4]) = 0
279   setsockopt(3<UNIX:[10508]>, SOL_SOCKET, SO_SNDBUFFORCE, [8388608], 4) = 0
279   setsockopt(3<UNIX:[10508]>, SOL_SOCKET, SO_SNDTIMEO, "\n\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0", 16 <unfinished ...>
279   <... setsockopt resumed> )        = 0
279   connect(3<UNIX:[10508]>, {sa_family=AF_UNIX, sun_path="/run/systemd/journal/socket"}, 29) = 0
279   sendmsg(3<UNIX:[10508]>, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="PRIORITY=3\nSYSLOG_FACILITY=3\nCODE_FILE=src/core/execute.c\nCODE_LINE=2915\nCODE_FUNCTION=exec_spawn\nERRNO=2\nSYSLOG_IDENTIFIER=systemd\n", iov_len=132}, {iov_base="MESSAGE_ID=641257651c1b4ec9a8624d7a40a9e1e7", iov_len=43}, {iov_base="\n", iov_len=1}, {iov_base="UNIT=hola.service", iov_len=17}, {iov_base="\n", iov_len=1}, {iov_base="MESSAGE=hola.service: Failed at step NAMESPACE spawning /busybox: No such file or directory", iov_len=91}, {iov_base="\n", iov_len=1}, {iov_base="EXECUTABLE=/busybox", iov_len=19}, {iov_base="\n", iov_len=1}], msg_iovlen=9, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 306
279   exit_group(226)                   = ?
279   +++ exited with 226 +++

This was referenced Nov 4, 2016
Merged
Closed
Closed
Closed
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

pid1

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tixxdz @evverx @poettering