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

Improve ability to run under non-root user; revoke unneeded capabilities; run under 'sssd' starting F41 by default. #7193

Closed
wants to merge 42 commits into from

Conversation

alexey-tikhonov
Copy link
Member

@alexey-tikhonov alexey-tikhonov commented Feb 15, 2024

tl,dr brief overview of changes:

  • make all folders sssd:sssd owned and group accessible
  • split "sssd.conf -> config.ldb" conversion into two steps: read + write
  • get&apply sssd.conf 'user' and 'debug_level' options right after reading sssd.conf and before doing anything else (so that config.ldb is written under configured service user)
  • in case SSSD is built '--with-sssd-user=sssd' but run under 'root': add 'sssd' group to the process so it can access all sssd:sssd owned folders
  • start with zero (and locked) capabilities whenever possible; otherwise drop all capabilities as soon as possible

For more lengthy and more "user facing" text see commit message of:
sssd.service: run under SSSD_USER by default
-- this will be a basis for a release note.

There are a number of known issues remains to address:

Above stuff should be addressed before sssd-2.10 release, but this work has been brewing for too long already.
I think it's in a good enough shape for a review.

(plus see additional comments below)

@alexey-tikhonov
Copy link
Member Author

An overview of relevant build/configuration options/combinations.

(I) Built --with-sssd-user=root
No configuration options available, everything is root:root owned and runs under 'root' but without any capabilities.
As per internal logs:

  • monitor:
[sssd] [main] (0x3f7c0): Started under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
[sssd] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
  • sssd_be:
[be[files]] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
  • monitor activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
  • socket activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
[ifp] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)

(II) Built --with-sssd-user=sssd
There are two configuration options available: (primary) *.service::User= and (legacy, deprecated) sssd.conf::user

(1) sssd.service::User=sssd -- this is a default and a recommended configuration (starting f41/c10s)
In this case sssd.conf::user is ignored (warnings are given if used).
Everything is sssd:sssd owned and runs under 'sssd' without capabilities:

  • monitor:
[sssd] [main] (0x3f7c0): Started under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
[sssd] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)   (nothing)
# cat /proc/63337/status | grep Cap
CapInh:	0000000000000000
CapPrm:	0000000000000000
CapEff:	0000000000000000
CapBnd:	00000000000000c3   <-- bounding set is there to allow privileged child processes to gain those caps
                               monitor/sssd_be/responders itself can't get it to effective set
CapAmb:	0000000000000000
  • sssd_be:
[be[files]] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh:	0000000000000000
CapPrm:	0000000000000000
CapEff:	0000000000000000
CapBnd:	00000000000000c3
CapAmb:	0000000000000000
NoNewPrivs:     0
  • monitor activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh:	0000000000000000
CapPrm:	0000000000000000
CapEff:	0000000000000000
CapBnd:	00000000000000c3
CapAmb:	0000000000000000
NoNewPrivs:     1
  • socket activated responder:
[ifp] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh:	0000000000000000
CapPrm:	0000000000000000
CapEff:	0000000000000000
CapBnd:	0000000000000000
CapAmb:	0000000000000000

(2) sssd.service::User=root

(2.1) sssd.conf::user not set

  • monitor -- CAP_SET*ID are required at startup (to respect potential sssd.conf::user option) but dropped after reading config:
[sssd] [main] (0x3f7c0): Started under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
                  CAP_SETGID: effective = *1*, permitted = *1*, inheritable = *1*, bounding = *1*
                  CAP_SETUID: effective = *1*, permitted = *1*, inheritable = *1*, bounding = *1*
[sssd] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
  • sssd_be:
[be[files]] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
  • monitor activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
nh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
NoNewPrivs:     1
  • socket activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 0000000000000000
CapAmb: 0000000000000000
NoNewPrivs:     0

(2.2) sssd.conf::user=sssd
Syslog warning: sssd[73920]: 'sssd.conf::user' option is deprecated. Run under 'sssd' initially instead.

  • monitor:
[sssd] [main] (0x3f7c0): Started under uid=0 (euid=0) : gid=0 (egid=0) with SECBIT_KEEP_CAPS = 0 and following capabilities:
                  CAP_SETGID: effective = *1*, permitted = *1*, inheritable = *1*, bounding = *1*
                  CAP_SETUID: effective = *1*, permitted = *1*, inheritable = *1*, bounding = *1*
[sssd] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
  • sssd_be:
[be[files]] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
  • monitor activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
apInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 00000000000000c3
CapAmb: 0000000000000000
  • socket activated responder:
[nss] [server_loop] (0x3f7c0): Entering main loop under uid=981 (euid=981) : gid=976 (egid=976) with SECBIT_KEEP_CAPS = 0 and following capabilities:
   (nothing)
CapInh: 0000000000000000
CapPrm: 0000000000000000
CapEff: 0000000000000000
CapBnd: 0000000000000000
CapAmb: 0000000000000000

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Feb 15, 2024

Now about testing of all those different combinations.

CI of this PR itself covers (I) (everything besides f41 and centos-9) and (II.1) (f-41 and centos-9)

#7195 covers (II.2.1) - almost all tests on f-41 and centos-9 and (II.2.2) - core subset of system tests on f-41 and centos-9 (see c4e8094). Other platforms can be ignored in this PR as it doesn't bring anything new.
#7195 is only a draft with tmp tweaks to get a test run for those combinations. I don't know if we want to keep testing those combinations systematically. On the one hand, I'd like to declare "sssd.conf::user" option deprecated (already in sssd-2.10) and remove it fast (ideally already in f-41 and for sure in c10s). On the other hand, this is what c9s will keep using (if we ever rebase it on sssd-2.10+). And most probably we should keep testing (II.2.1), but this will require a re-work of c4e8094 and related patches to manage sssd.service::User instead of sssd.conf::user

@alexey-tikhonov
Copy link
Member Author

A note that there is currently some discrepancy between centos-9 configuration in this PR and a real/projected centos-9 config. This PR configures centos-9 with User=sssd while "real" C9S will keep using "User=root" (even once/if rebased on sssd-2.10) This is intentional to have a better CI coverage, at least until there is C10S compose available.

@alexey-tikhonov alexey-tikhonov force-pushed the nonprivileged branch 3 times, most recently from e7fb9b6 to 297a5ce Compare February 20, 2024 12:34
@alexey-tikhonov
Copy link
Member Author

CI of this PR itself covers (I) (everything besides f41 and centos-9) and (II.1) (f-41 and centos-9)

#7195 covers (II.2.1) - almost all tests on f-41 and centos-9 and (II.2.2) - core subset of system tests on f-41 and centos-9

#7195 helped to discover a couple of missing bits:

  • group's access to pubconf/keytab folders
  • read access for 'other' to pidfile (otherwise SSSD can't read leftover pidfile after service user change, might be not a significant problem in the wild, but breaks CI)

Now all CI tests are green in both PRs.

@alexey-tikhonov
Copy link
Member Author

Rebased and added a patch to SSSDDomain::write() to 'chown()' sssd.conf to service user.

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Feb 26, 2024

Rebased.

@justin-stephenson
Copy link
Contributor

Some PRCI jobs use configure sssd with this action, does with-sssd-user= need to be added to fedconfig here?

@alexey-tikhonov
Copy link
Member Author

Some PRCI jobs use configure sssd with this action, does with-sssd-user= need to be added to fedconfig here?

Added (and rebased).

Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, thank you.

src/conf_macros.m4 Outdated Show resolved Hide resolved
src/monitor/monitor.c Outdated Show resolved Hide resolved
This will also allow to use "SecureBits=noroot" in sssd.service
in sssd.service to avoid processes gaining all capabilities
from bounding set during execv() with uid=0/gid=0 (so that, for
example, 'sssd_be' runs without capabilities even if "User=root")
so that SSSD built --with-sssd-user=sssd but run under 'root' can get
to sssd.conf without capabilities (using "SupplementaryGroups=sssd")

sssd.conf still needs to be chown'ed to 'root:root' manually in this
case.
For a real device this is handled by udev rule that makes device
readable by SSSD. This rule doesn't work with mocked device.
@alexey-tikhonov
Copy link
Member Author

(rebase)

Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you very much for your persistence and patience on this task, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7193

  • master
    • cb4dbea - SPEC/SYSTEMD: try harder making sure logs ownership matches service user
    • e37a8c7 - MAN: 'monitor' exit codes description
    • 1287778 - MONITOR: free 'tmp_ctx' in case of failure too
    • d45b85b - SSSDConfig: chown() sssd.conf to SSSD service user
    • 869ee96 - SPEC: build Fedora >= 41 package with sssd user support
    • 07f0013 - TESTS: system: skip 'passkey' tests if SSSD runs under non-root
    • 9eed387 - SPEC: make conf folder g+rx
    • 84c3034 - SYSTEMD: set "SecureBits=noroot noroot-locked"
    • e2c26e8 - SPEC: replace SUID bit with more fine-grained capabilities
    • 29b1e47 - UTILS: make pidfile readable by everyone
    • 6ca4e47 - CONF: store pid file in /run/sssd
    • 583ea7f - SYSTEMD: remove "PIDFile="
    • 9fbaf6d - SSS_INI: only check file ownership from 'sssd'
    • b88d56a - SYSTEMD: KCM capabilities
    • aa7cddf - SPEC: make sure config files are accesible
    • 4c42ca7 - SPEC: make sure cache files are accessible
    • 2a59991 - sssd.service: run under SSSD_USER by default
    • 0e2ed44 - KRB5_/LDAP_CHILD: print capabilities at startup
    • 1ea6965 - MONITOR: startup logic was changed
    • 304fe75 - SYSTEMD: responders do not need any capabilities
    • 5bd5202 - SYSTEMD: remove unused CAP_KILL
    • 0d686b5 - TOOLS: remove the upgrade-cache command
    • d7042fe - DEBUG: a couple of message changes
    • a05b025 - SSS_INI: remove 'const' specifier from getter
    • fd23a94 - MONITOR: move nscd check code to a function
    • 34f7c2e - MONITOR: move keyring setup code to a function
    • e306d93 - MONITOR: remove unused mt_ctx::conf_path
    • 87b77a0 - MONITOR: no need to read domain list twice
    • b1cbf5f - CONFDB: always delete old ldb-file
    • cff8e1f - CONFDB: split confdb_setup() into 2 steps
    • 8d1b3ef - SSS_INI: const correctness
    • 4a44cca - Get rid of --genconf and --genconf-section monitor options.
    • f4ad8c2 - UTILS: add capabilities management helpers
    • 60853c6 - NSS: don't fchown() mem-cache files
    • 52fa441 - Make all SSSD processes a member of sssd supplementary group.
    • 521f88e - SPEC: make '%{pipepath}/private' sssd:sssd owned
    • 47da0b6 - SPEC: make most folders group accessible
    • 102c30a - MONITOR: get rid of unsed FLAGS_GEN_CONF definition
    • d79e0e7 - MNITOR: cosmetics
    • 419120f - MONITOR: replace fprintf() with ERROR()
    • 40cea81 - MONITOR: remove 'opt_netlinkoff' removal notice
    • 40e5309 - MONITOR: remove useless trailing ''

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Apr 8, 2024

Hi @sumit-bose,

maybe something for the TODO list:

Mar 12 11:32:37 client.test sssd[1182]: (2024-03-12 11:32:37): [be[ipa.test]] [sss_krb5_touch_config] (0x0020): Unable to change mtime of "/etc/krb5.conf" [13]: Permission denied
Mar 12 11:32:37 client.test sssd[1182]: (2024-03-12 11:32:37): [be[ipa.test]] [sss_write_krb5_conf_snippet] (0x0020): Unable to change last modification time of krb5.conf. Created mappings may not be loaded.

/etc/krb5.conf is touched to indicate to long running processes to reload the Kerberos configuration to at least refresh [domain_realms] and most probably any other iteratable sections.

This "touch" is done in two functions:
(1) sss_write_krb5_conf_snippet()
(2) sss_write_domain_mappings()

What is the reason for (1)? It looks like sss_write_krb5_conf_snippet() writes static, domain independent configs.

Most probably this "touch" can be moved to krb5_child and/or ldap_child.

I tried to follow code logic to figure out when functions (1) and (2) are executed.
It seems both are triggered by a long chain initiated by DPM_DOMAINS_HANDLER:: ipa/ad_subdomains_handler_send() or ipa/ad_subdomains_init()
It sounds plausible that ldap_child should be used in this chain (to get TGT) but so far I wasn't able to identify where this happens :-/

@sumit-bose
Copy link
Contributor

Hi @sumit-bose,

maybe something for the TODO list:

Mar 12 11:32:37 client.test sssd[1182]: (2024-03-12 11:32:37): [be[ipa.test]] [sss_krb5_touch_config] (0x0020): Unable to change mtime of "/etc/krb5.conf" [13]: Permission denied
Mar 12 11:32:37 client.test sssd[1182]: (2024-03-12 11:32:37): [be[ipa.test]] [sss_write_krb5_conf_snippet] (0x0020): Unable to change last modification time of krb5.conf. Created mappings may not be loaded.

/etc/krb5.conf is touched to indicate to long running processes to reload the Kerberos configuration to at least refresh [domain_realms] and most probably any other iteratable sections.

This "touch" is done in two functions: (1) sss_write_krb5_conf_snippet() (2) sss_write_domain_mappings()

What is the reason for (1)? It looks like sss_write_krb5_conf_snippet() writes static, domain independent configs.

Hi,

yes, currently it is not expected that the settings written by sss_write_krb5_conf_snippet() will change during the runtime of SSSD. But at least for the first time the touch would be needed if not sss_write_domain_mappings() would be called immediately afterwards. Maybe both functions can be called from a single function which does the touch at the end.

Most probably this "touch" can be moved to krb5_child and/or ldap_child.

I tried to follow code logic to figure out when functions (1) and (2) are executed. It seems both are triggered by a long chain initiated by DPM_DOMAINS_HANDLER:: ipa/ad_subdomains_handler_send() or ipa/ad_subdomains_init() It sounds plausible that ldap_child should be used in this chain (to get TGT) but so far I wasn't able to identify where this happens :-/

Yes, ti happens during startup and when updating the information about subdomains. I would not couple this with getting the TGT because during ipa/ad_subdomains_handler_send() you already have a TGT.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Apr 9, 2024

This "touch" is done in two functions: (1) sss_write_krb5_conf_snippet() (2) sss_write_domain_mappings()
What is the reason for (1)? It looks like sss_write_krb5_conf_snippet() writes static, domain independent configs.

yes, currently it is not expected that the settings written by sss_write_krb5_conf_snippet() will change during the runtime of SSSD. But at least for the first time the touch would be needed if not sss_write_domain_mappings() would be called immediately afterwards. Maybe both functions can be called from a single function which does the touch at the end.

Practically that's already (almost) the case: it's either ad_subdom_reinit() or ipa_subdom_reinit() that calls both (1) and (2)

Most probably this "touch" can be moved to krb5_child and/or ldap_child.

I tried to follow code logic to figure out when functions (1) and (2) are executed. It seems both are triggered by a long chain initiated by DPM_DOMAINS_HANDLER:: ipa/ad_subdomains_handler_send() or ipa/ad_subdomains_init() It sounds plausible that ldap_child should be used in this chain (to get TGT) but so far I wasn't able to identify where this happens :-/

Yes, ti happens during startup and when updating the information about subdomains. I would not couple this with getting the TGT because during ipa/ad_subdomains_handler_send() you already have a TGT.

@sumit-bose, what calls krb5_child or ldap_child in this chain then?

@sumit-bose
Copy link
Contributor

I tried to follow code logic to figure out when functions (1) and (2) are executed. It seems both are triggered by a long chain initiated by DPM_DOMAINS_HANDLER:: ipa/ad_subdomains_handler_send() or ipa/ad_subdomains_init() It sounds plausible that ldap_child should be used in this chain (to get TGT) but so far I wasn't able to identify where this happens :-/

Yes, ti happens during startup and when updating the information about subdomains. I would not couple this with getting the TGT because during ipa/ad_subdomains_handler_send() you already have a TGT.

@sumit-bose, what calls krb5_child or ldap_child in this chain then?

Hi,

nothing, this has to be added if one of those helpers should do the touch.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Yes, ti happens during startup and when updating the information about subdomains. I would not couple this with getting the TGT because during ipa/ad_subdomains_handler_send() you already have a TGT.

@sumit-bose, what calls krb5_child or ldap_child in this chain then?

nothing, this has to be added if one of those helpers should do the touch.

So a new (sync) command like we did to iterate keytab?
I wonder if it's really critical. Perhaps this shouldn't be a blocker for sssd-2.10...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. non-privileged Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants