IFP: allow running under non-root user #6862

alexey-tikhonov · 2023-08-02T17:48:48Z

No description provided.

alexey-tikhonov · 2023-08-07T19:29:37Z

UPDATE: this comment is outdated.

Some notes:

so far I didn't find a way to allow multiple users to own a well-known-name on a system bus -

sssd/src/responder/ifp/org.freedesktop.sssd.infopipe.conf

Line 11 in 91d32fe

<policy user="root">

- so it's either 'root' or 'sssd', users will need to change it manually. I think it makes sense to keep it in sync with default value of 'sssd.conf::user' option (i.e. currently 'root').
Makefile.am hardcodes ifp_exec_cmd = $(sssdlibexecdir)/sssd_ifp --uid 0 --gid 0 --dbus-activated that is later used in org.freedesktop.sssd.infopipe.service.in. So dbus-activated sssd-ifp still runs under root even with this PR. I think the best way forward here is to get rid of --uid/--gid and switch user right after fork() (before exec()).
imo, handling of "Request to own name refused by policy" in sssd-ifp is wrong (it assumes "sustem bus is not available" while it should fail completely)

alexey-tikhonov · 2023-08-10T17:28:26Z

Rebased on top of #6868

justin-stephenson · 2023-08-11T15:57:43Z

Some notes:

* so far I didn't find a way to allow multiple users to own a well-known-name on a system bus  - https://github.com/SSSD/sssd/blob/91d32fee16e37e46b7fc43d66f579ba088c45af3/src/responder/ifp/org.freedesktop.sssd.infopipe.conf#L11
   - so it's either 'root' or 'sssd', users will need to change it manually. I think it makes sense to keep it in sync with default value of 'sssd.conf::user' option (i.e. currently 'root').

I see this on my system installing this PR, is it expected?

  <!-- Define who can own (provide) the SSSD IFP service -->
  <policy user="root"> <allow own="org.freedesktop.sssd.infopipe"/> </policy>
  <policy user="sssd"> <allow own="org.freedesktop.sssd.infopipe"/> </policy>

alexey-tikhonov · 2023-08-11T16:06:01Z

Some notes:

* so far I didn't find a way to allow multiple users to own a well-known-name on a system bus  - https://github.com/SSSD/sssd/blob/91d32fee16e37e46b7fc43d66f579ba088c45af3/src/responder/ifp/org.freedesktop.sssd.infopipe.conf#L11
   - so it's either 'root' or 'sssd', users will need to change it manually. I think it makes sense to keep it in sync with default value of 'sssd.conf::user' option (i.e. currently 'root').

Ah, this comment is outdated...

I see this on my system installing this PR, is it expected?

  <!-- Define who can own (provide) the SSSD IFP service -->
  <policy user="root"> <allow own="org.freedesktop.sssd.infopipe"/> </policy>
  <policy user="sssd"> <allow own="org.freedesktop.sssd.infopipe"/> </policy>

Well, this is expected in the sense it is my intention.
Another question is if it's a proper way.

The rationale is:

by default most socket activated responders (with unclear exception of nss) are run under 'sssd' => to be consistent it makes sense to configure socket activated sssd_ifp to run under 'sssd' as well => policy should allow 'sssd' user to own bus name;
but default value of 'user' sssd.conf option is still 'root' => monitor by default will run 'ifp' under root => policy should allow 'root' user to own bus name.

For this reason I allowed both to ease pain of configuration.
It would be better to default to 'sssd' user everywhere if SSSD is built --with-sssd-user=sssd, but code isn't ready yet.
If/once it's ready, we will leave a single policy entry for $SSSD_USER.

Makefile.am

justin-stephenson · 2023-08-11T20:10:20Z

I see the following error in testing the infopipe with dbus activation sssctl domain-list but maybe I missed something. I am installing locally built from PR checkout 'make rpms'

Aug 11 16:04:30 fedora.test.local sssd_ifp[531105]: Starting up                                               
Aug 11 16:04:30 fedora.test.local systemd[1]: sssd-ifp.service: Main process exited, code=exited, status=3/NOTIMPLEMENTED
Aug 11 16:04:30 fedora.test.local systemd[1]: sssd-ifp.service: Failed with result 'exit-code'.                                                                                                                                               
Aug 11 16:04:30 fedora.test.local systemd[1]: Failed to start sssd-ifp.service - SSSD IFP Service responder. 



[ifp] [server_setup] (0x0080): Failed setting process group: Operation not permitted[1]. We might leak processes in case of failure
(2023-08-11 15:33:40): [ifp] [server_setup] (0x3f7c0): Starting with debug level = 0x00f0
(2023-08-11 15:33:40): [ifp] [sss_iface_connect_address] (0x0020): check_file failed for [/var/lib/sss/pipes/private/sbus-dp_ipa.test].
   *  (2023-08-11 15:33:40): [ifp] [perform_checks] (0x1000): File must be owned by uid [387].
   *  (2023-08-11 15:33:40): [ifp] [sss_iface_connect_address] (0x0020): check_file failed for [/var/lib/sss/pipes/private/sbus-dp_ipa.test].


[root@fedora ~]# ll /var/lib/sss/pipes/private/
total 4
srw-------. 1 root root  0 Aug 11 15:21 pam
lrwxrwxrwx. 1 root root 50 Aug 11 15:21 sbus-dp_ipa.test -> /var/lib/sss/pipes/private/sbus-dp_ipa.test.412091
srw-------. 1 root root  0 Aug 11 15:21 sbus-dp_ipa.test.412091
srw-------. 1 root root  0 Feb 22 16:04 sbus-dp_ipa.test.568887
srw-------. 1 root root  0 Feb 22 16:04 sbus-dp_ldap.test.568886
srw-------. 1 root root  0 Aug 11 15:21 sbus-monitor


[root@fedora ~]# id sssd
uid=387(sssd) gid=386(sssd) groups=386(sssd)

alexey-tikhonov · 2023-08-11T20:39:28Z

(2023-08-11 15:33:40): [ifp] [sss_iface_connect_address] (0x0020): check_file failed for [/var/lib/sss/pipes/private/sbus-dp_ipa.test].

(2023-08-11 15:33:40): [ifp] [perform_checks] (0x1000): File must be owned by uid [387].

That's because you mix 'sssd_be' running under 'root' and socket activated 'ifp' running under 'sssd'.
To make it work currently one needs to run everything either under 'root' or under 'sssd' (i.e. to add 'user=sssd' in sssd.conf)

We could overcome this via chown sssd:sssd pipe in sssd_be, but I'm not sure we should.
I think a better goal is to fix things to be able to have everything configured to run under 'sssd' by default.

alexey-tikhonov · 2023-08-14T15:14:51Z

(rebased)

justin-stephenson

Changes LGTM, Ack.

sumit-bose · 2023-08-24T11:09:17Z

src/sysv/systemd/sssd-ifp.service.in

@@ -9,6 +9,9 @@ Environment=DEBUG_LOGGER=--logger=files
 EnvironmentFile=-@environment_file@
 Type=dbus
 BusName=org.freedesktop.sssd.infopipe
-ExecStart=@ifp_exec_cmd@ ${DEBUG_LOGGER}
+ExecStartPre=-/bin/chown @SSSD_USER@:@SSSD_USER@ @logpath@/sssd_ifp.log
+ExecStart=@libexecdir@/sssd/sssd_ifp ${DEBUG_LOGGER} --dbus-activated
 CapabilityBoundingSet= @additional_caps@ CAP_IPC_LOCK CAP_CHOWN CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SETGID CAP_SETUID


Hi,

in my testing it looks like the CapabilityBoundingSet is only needed for the chown in ExecStartPre. What do you think about adding + to ExecStartPre and remove CapabilityBoundingSet so that chown runs with full privileges but sssd_ifp has no extra rights?

bye,
Sumit

Right, there are a lot of excessive caps listed.
I'm working on clearing this stuff in #6873, 53c49f5 in particular.

With regards to CAP_DAC_OVERRIDE it's a bit tricky because if log file is missing at startup (i.e. nothing to chown()) then if sssd_ifp is run under 'root' it needs CAP_DAC_OVERRIDE to create log file in /var/log/sssd
Perhaps this can be worked around by addition of 'touch' to ExecStartPre
Moreover, if User=sssd then no CapabilityBoundingSet is needed at all (and it's presence is confusing).

I hope to improve all this in #6873

Btw, merely removing CapabilityBoundingSet means "unlimited capabilities". So to run 'sssd_ifp' under root but without caps it's required to assign empty set, IIUC.

Another case where a responder needs CAP_DAC_OVERRIDE:

sssd.conf::user=sssd, so sbus server sockets are sssd:sssd owned:

srw-------. 1 sssd sssd 0 Aug 25 11:53 sbus-dp_files.234537 srw-------. 1 sssd sssd 0 Aug 25 11:53 sbus-monitor

but a socket activated responder is run under root for some reason (for example, for sssd_nss this is the only option currently)

Then a responder needs CAP_DAC_OVERRIDE to connect to monitor and DP (this is a reverse of a problem where sssd_be is run under root but a responder under sssd).
Currently this works because sssd-nss.service doesn't define any CapabilityBoundingSet at all, so no limits.

Our options are:

make socket activated sssd_nss to be able to run under sssd and then say we don't support mix of root/sssd at runtime

keep CAP_DAC_OVERRIDE

maybe something else.

@sumit-bose, what would you say?

Actually:

sssd/src/responder/nss/nsssrv.c

Line 617 in 15a2213

* Adding the NSS process to the SSSD supplementary group avoids

so I think another option could be to make sbus server sockets g+rw.

What I can't understand - what was the source of

type=AVC msg=audit(1543524888.125:1495): avc: denied { fsetid } for pid=7706 comm="sssd_nss" capability=4 scontext=system_u:system_r:sssd_t:s0 tcontext=system_u:system_r:sssd_t:s0 tclass=capability

mentioned in the commit message of 61e4ba5

First of all, IIRC, by that time all SSSD components run under root without any capabilities restrictions.
Another thing is that I don't understand how writing to non-executable mem-cache file would trigger such AVC (fsetid).
Moreover, tclass=capability - not a file...

And right, I missed '+' before ExecStartPre.
In other service files it's solved with deprecated 'PermissionsStartOnly':

systemd/sssd-ssh.service.in:19:PermissionsStartOnly=true systemd/sssd-autofs.service.in:19:PermissionsStartOnly=true systemd/sssd-sudo.service.in:19:PermissionsStartOnly=true systemd/sssd-pam.service.in:19:PermissionsStartOnly=true systemd/sssd-pac.service.in:19:PermissionsStartOnly=true

-- it defaults to 'false'.
So currently ExecStartPre for 'ifp.service' is executed with "User=..." and doesn't make sense.
I'll fix this in #6873

sumit-bose

Hi,

thanks, I have no further comments, patch is working as expected, ACK.

bye,
Sumit

:relnote: Infopipe responder (ifp) can now be run under non-privileged 'sssd' user if SSSD is configured and built `--with-sssd-user=sssd` option. As with other components, for 'monitor' activated 'ifp' service feature is enabled by setting `user=sssd` sssd.conf option. For dbus-socket activated 'ifp' service it's a matter of User=/Group= in 'sssd-ifp.service' (configured to 'sssd' by default).

alexey-tikhonov · 2023-08-24T13:23:05Z

(just a rebase on the top of latest 'master' to see CI status)

alexey-tikhonov · 2023-08-24T14:47:50Z

CI is green

alexey-tikhonov · 2023-08-24T14:51:05Z

Pushed PR: #6862

master
- 41427f9 - IFP: allow running under non-root user

alexey-tikhonov force-pushed the ifp-nonroot branch from 5323dc2 to b99549e Compare August 4, 2023 10:21

alexey-tikhonov added the no-backport This should go to target branch only. label Aug 4, 2023

alexey-tikhonov force-pushed the ifp-nonroot branch 3 times, most recently from 901b9ad to 27a1b8c Compare August 7, 2023 18:24

alexey-tikhonov force-pushed the ifp-nonroot branch 4 times, most recently from 9af9a2b to aaba9ba Compare August 9, 2023 19:10

andreboscatto assigned justin-stephenson and sumit-bose Aug 10, 2023

andreboscatto requested a review from sumit-bose August 10, 2023 12:41

alexey-tikhonov requested a review from justin-stephenson August 10, 2023 12:43

alexey-tikhonov force-pushed the ifp-nonroot branch from aaba9ba to b9c0482 Compare August 10, 2023 17:28

alexey-tikhonov force-pushed the ifp-nonroot branch from b9c0482 to 7183635 Compare August 10, 2023 18:02

alexey-tikhonov marked this pull request as ready for review August 10, 2023 19:57

alexey-tikhonov added the Waiting for review label Aug 10, 2023

alexey-tikhonov force-pushed the ifp-nonroot branch from 7183635 to f5aec6a Compare August 11, 2023 13:49

justin-stephenson reviewed Aug 11, 2023

View reviewed changes

Makefile.am Show resolved Hide resolved

alexey-tikhonov force-pushed the ifp-nonroot branch from f5aec6a to a563cb9 Compare August 14, 2023 15:14

justin-stephenson approved these changes Aug 14, 2023

View reviewed changes

sumit-bose reviewed Aug 24, 2023

View reviewed changes

sumit-bose approved these changes Aug 24, 2023

View reviewed changes

alexey-tikhonov force-pushed the ifp-nonroot branch from a563cb9 to 9fcc676 Compare August 24, 2023 13:22

alexey-tikhonov added Accepted Ready to push Ready to push and removed Waiting for review labels Aug 24, 2023

alexey-tikhonov added Pushed and removed Accepted Ready to push Ready to push labels Aug 24, 2023

alexey-tikhonov closed this Aug 24, 2023

alexey-tikhonov mentioned this pull request Mar 21, 2024

sssd container support #5443

Open

alexey-tikhonov added the non-privileged label Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IFP: allow running under non-root user #6862

IFP: allow running under non-root user #6862

alexey-tikhonov commented Aug 2, 2023

alexey-tikhonov commented Aug 7, 2023 •

edited

Loading

alexey-tikhonov commented Aug 10, 2023

justin-stephenson commented Aug 11, 2023

alexey-tikhonov commented Aug 11, 2023 •

edited

Loading

justin-stephenson commented Aug 11, 2023 •

edited

Loading

alexey-tikhonov commented Aug 11, 2023

alexey-tikhonov commented Aug 14, 2023

justin-stephenson left a comment

sumit-bose Aug 24, 2023

alexey-tikhonov Aug 24, 2023 •

edited

Loading

alexey-tikhonov Aug 24, 2023

alexey-tikhonov Aug 25, 2023 •

edited

Loading

alexey-tikhonov Aug 25, 2023

alexey-tikhonov Aug 25, 2023

alexey-tikhonov Aug 25, 2023 •

edited

Loading

sumit-bose left a comment •

edited

Loading

alexey-tikhonov commented Aug 24, 2023

alexey-tikhonov commented Aug 24, 2023

alexey-tikhonov commented Aug 24, 2023

IFP: allow running under non-root user #6862

IFP: allow running under non-root user #6862

Conversation

alexey-tikhonov commented Aug 2, 2023

alexey-tikhonov commented Aug 7, 2023 • edited Loading

alexey-tikhonov commented Aug 10, 2023

justin-stephenson commented Aug 11, 2023

alexey-tikhonov commented Aug 11, 2023 • edited Loading

justin-stephenson commented Aug 11, 2023 • edited Loading

alexey-tikhonov commented Aug 11, 2023

alexey-tikhonov commented Aug 14, 2023

justin-stephenson left a comment

Choose a reason for hiding this comment

sumit-bose Aug 24, 2023

Choose a reason for hiding this comment

alexey-tikhonov Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov Aug 24, 2023

Choose a reason for hiding this comment

alexey-tikhonov Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov Aug 25, 2023

Choose a reason for hiding this comment

alexey-tikhonov Aug 25, 2023

Choose a reason for hiding this comment

alexey-tikhonov Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

sumit-bose left a comment • edited Loading

Choose a reason for hiding this comment

alexey-tikhonov commented Aug 24, 2023

alexey-tikhonov commented Aug 24, 2023

alexey-tikhonov commented Aug 24, 2023

alexey-tikhonov commented Aug 7, 2023 •

edited

Loading

alexey-tikhonov commented Aug 11, 2023 •

edited

Loading

justin-stephenson commented Aug 11, 2023 •

edited

Loading

alexey-tikhonov Aug 24, 2023 •

edited

Loading

alexey-tikhonov Aug 25, 2023 •

edited

Loading

alexey-tikhonov Aug 25, 2023 •

edited

Loading

sumit-bose left a comment •

edited

Loading