Bug 106339

Created attachment 139254 [details] [review]
bus: Mark service as requiring nss-user-lookup.target

Comment on attachment 139254 [details] [review]
bus: Mark service as requiring nss-user-lookup.target

Review of attachment 139254 [details] [review]:
-----------------------------------------------------------------

::: bus/dbus.service.in
@@ +2,4 @@
>  Description=D-Bus System Message Bus
>  Documentation=man:dbus-daemon(1)
>  Requires=dbus.socket
> +After=nss-user-lookup.target

After= seems fine.

@@ +2,5 @@
>  Description=D-Bus System Message Bus
>  Documentation=man:dbus-daemon(1)
>  Requires=dbus.socket
> +After=nss-user-lookup.target
> +Requires=nss-user-lookup.target

I'm not so sure about Requires=.

How recent is that target? Can we assume that it exists on every systemd-using OS? (New dependencies are OK in 1.13.x but not in 1.12.x.)

systemd.special(7) says

"""
All services for which the availability of the full user/group database is essential should be ordered after this target, but not pull it in.
"""

which I think means "use After, don't use Requires"?

Comment on attachment 139254 [details] [review]
bus: Mark service as requiring nss-user-lookup.target

Review of attachment 139254 [details] [review]:
-----------------------------------------------------------------

Nitpick: s/synchronization points/synchronization point/ in the commit message.

(In reply to Simon McVittie from comment #2)
> systemd.special(7) says
> 
> """
> All services for which the availability of the full user/group database is
> essential should be ordered after this target, but not pull it in.
> """
> 
> which I think means "use After, don't use Requires"?

I’d concur with that interpretation.

Looking at the output of `ack nss-user-lookup` in my /lib/systemd/, it seems that other unit files just do After= (and in the case of accounts-daemon.service, also Wants=), but never Requires=.

> which I think means "use After, don't use Requires"?

Yeah, I think you're right, but at the same time, I feel like I'm not fully understanding the intent in not pulling it in. I guess the idea is that we try to limp on if we don't reach nss-user-lookup.target?

---
/me sees Philip's new comment when trying to post the above
---

OK, `Wants=` makes sense to me. Will switch to that then.

Created attachment 139256 [details] [review]
bus: Mark service as requiring nss-user-lookup.target

Switched to Wants= instead of Requires= and fixed commit message nit!

Created attachment 139257 [details] [review]
bus: Mark service as wanting nss-user-lookup.target

And finally, reword commit message title (s/requiring/wanting/) and add sentence about `Wants=`.

(In reply to Philip Withnall from comment #4)
> Looking at the output of `ack nss-user-lookup` in my /lib/systemd/, it seems
> that other unit files just do After= (and in the case of
> accounts-daemon.service, also Wants=), but never Requires=.

If I'm understanding the meaning of "pulls in" correctly, then I think the intended use is to just use After= and not Wants=; that's what systemd-logind and systemd-user-sessions do. So perhaps accounts-daemon.service is doing this wrong? My understanding is that "pulls in" is a colloquialism for the Requires/Wants dependency relationship (Wants is just a Requires that is allowed to be left unsatisfied).

I suspect that non-trivial NSS user providers like sssd are meant to have Wants= and Before= on it, as is done in <https://sources.debian.org/src/sssd/1.16.1-1/src/sysv/systemd/sssd.service.in/>? If so, then it would look a lot like network-online.target:

* sssd.service enabled:
  - the default target Wants sssd.service, so it gets included
    in the startup transaction
  - sssd.service Wants nss-user-lookup.target, so it gets included
    in the startup transaction too
  - sssd.service is Before nss-user-lookup.target
  - dbus.service, systemd-logind.service etc. are After nss-user-lookup.target
  - resulting sequence:
    + ... other units ...
    + sssd.service starts
    + dbus.service, systemd-logind.service etc. start
    + ... other units ...

* sssd.service not enabled:
  - nothing Wants or Requires sssd.service, so it does not get included
    in the startup transaction
  - nothing Wants or Requires nss-user-lookup.target either
  - the fact that dbus.service, systemd-logind.service etc. are After
    nss-user-lookup.target is irrelevant because it is not part of the
    transaction
  - no sequence is imposed on dbus.service, systemd-logind.service etc.
    (because they are assumed to only need trivial file-based nss)
  - boot sequence is not over-constrained

Lennart, perhaps you could shed some light on this?

(In reply to Simon McVittie from comment #8)
> If so, then it would look a lot like network-online.target

Actually, network-online.target doesn't work like that. (But the rest of what I said stands.)

My conjecture is that nss-user-lookup.target is a *passive target*, like network.target and network-pre.target as documented in <https://www.freedesktop.org/wiki/Software/systemd/NetworkTarget/>, but unlike network-online.target.

Yepp, Simon, your interpretation is right. 

That said, this matters only if dbus-daemon's policy files actually contains references to users defined in sssd. And that's something that I'd really question: systemd requires that system users and groups are resolvable during earliest boot already, and to my understanding "sssd" is not for system users is it?

If sssd is for system users, then it needs to be written in a style that it can run during earliest boot already, i.e. before sysinit.target essentially (i.e. as early as systemd-sysusers.service), but if it does run that early then ordering against nss-user-lookup.target is unnecessary, as sysinit.target is effectively ordered long before that...

So what is it now? Is sssd intended for system users? If so, then sssd should be ordered before sysinit.target, hence you don't need any dep on dbus and this issue can be closed. If it's not intended for system users, but only for regular "human" users (which was how I understood it), then the question is: is it expected that dbus policy lists non-system users? (I don't think it should, PK is almost always the better option when configuring access for "human" users) If D-Bus policy is supposed to support non-system users, then yes, adding the dep is right. But if that's not the case, then no, the dep shouldn't be there.

The way I understood sssd is that it is for "human" users only really. And the way I understood D-Bus policy is not supposed to list non-system users. And for both reasons individually I don't think the requested dep should be added.

I mean, people can always ignore how D-Bus and sssd are supposed to be used, and for example place system users in sssd and configure human user access in dbus policy. And that's entirely fine, but the question is whether dbus upstream needs to care for that and carry the deps for it, or whether it wouldn't be better to leave this to people doing this changes locally, and let them figure this out. Not all such adjustments have to be carried upstream.

Hope that makes sense.

(In reply to Lennart Poettering from comment #10)
> So what is it now? Is sssd intended for system users?

A valid question. It sounds as though Jonathan is using it for system users, as a replacement for nss-altfiles? If so, then yes it will need to be made suitable for use before sysinit.target.

I'm not sure that I understand why defining the system users in a file wasn't suitable? Wasn't the point of using nss-altfiles that the uids in /usr/lib/passwd always need to be in sync with the uids that own files in the OSTree-deployed tree?

> That said, this matters only if dbus-daemon's policy files actually contains
> references to users defined in sssd. And that's something that I'd really
> question

dbus-daemon's policy files can contain group-based rules like <policy group="netdev"> (which are somewhat discouraged in favour of polkit, but exist in the wild anyway), and in a sssd/etc. environment we might need to ask those services to be able to find out who is in those groups.

They can also contain user-based rules like <policy user="smcv">, or even user-defined rules like <policy user="staff">; that isn't something I would recommend in production, but it does work.

> is it expected that dbus policy lists non-system users? (I
> don't think it should, PK is almost always the better option when
> configuring access for "human" users)

polkit is absolutely a better option, but dbus-daemon has always supported arbitrary user and group names, so arbitrarily forbidding them now would be an incompatible change.

(Also, I don't know how we'd forbid them or warn abou them, even if we wanted to - how can we tell which users and groups are system users, and which are human user accounts?)

(In reply to Simon McVittie from comment #11)
> They can also contain user-based rules like <policy user="smcv">, or even
> user-defined rules like <policy user="staff">

Er, that should say: user-defined local rules like <policy user="smcv"> or <policy group="staff">.

(Access to org.freedesktop.DBus.Debug.Stats on the system bus is one place where rules like that are helpful; we can't use polkit from dbus-daemon because it would be a circular dependency, and accessing org.freedesktop.DBus.Debug.Stats is enough of an information leak that it should be denied for non-root users by default, but a small enough information leak that we don't necessarily need to hard-code it to be root-only.)

BTW, I prepped this now:

https://github.com/systemd/systemd/pull/8884

Which is supposed to document this from upstream's PoV

> (Also, I don't know how we'd forbid them or warn abou them, even if we wanted to - how can we tell which users and groups are system users, and which are human user accounts?)

That's a very valid question. In systemd we are now answering this according to this document:

https://github.com/systemd/systemd/blob/master/doc/UIDS-GIDS.md

i.e. we compile in the numeric UID boundary between system users and regular users, and it's usually 999. You can query it with pkg-config, too if you like. D-Bus could import that value too, but not sure you really want too, as systemd's stance on this is not the most traditional approach, which defines the boundary in /etc/login.defs and makes this user configurable, which I am very sure is a pretty broken concept: the boundary should really be decided by distros, and not admins… My understanding is that D-Bus would rather not follow systemd's strict approach on these things, but I'll you figure that out...

Thanks Simon and Lennart for the analyses.

> So what is it now? Is sssd intended for system users?

In this endeavour, yes. The idea here is to replace nss-altfiles by sssd because of conflicts with the recent authselect Fedora change[1][2]. On Atomic Host, system users are shoved into /usr/lib (at compose time) and "human" users in /etc. (To clarify, this is but one suggested approach at the OSTree level[3] -- in practice, I'm not sure what other approaches have been used). The long-term plan is to try to leverage systemd-sysusers instead[4]. This is more meant as a quick fix until then, though it's turning out more involved than it may be worth.

> If so, then sssd should be ordered before sysinit.target, hence you don't need any dep on dbus and this issue can be closed.

OK, that makes sense. The issue is that this never really worked on Atomic Host I think because /usr/lib/passwd isn't actually accessible until ostree-prepare-root.service figures out the deployment to put at /sysroot and we switch root. So today, booting up an Atomic Host will have messages like these:

Apr 29 22:00:23 localhost systemd-udevd[327]: Specified group 'tty' unknown
Apr 29 22:00:23 localhost systemd-udevd[327]: Specified group 'kmem' unknown
Apr 29 22:00:23 localhost systemd-udevd[327]: Specified group 'input' unknown
...
Apr 29 22:00:31 localhost systemd-tmpfiles[549]: [/usr/lib/tmpfiles.d/systemd.conf:11] Unknown group 'utmp'.
Apr 29 22:00:31 localhost systemd-tmpfiles[549]: [/usr/lib/tmpfiles.d/systemd.conf:19] Unknown user 'systemd-network'.
Apr 29 22:00:31 localhost systemd-tmpfiles[549]: [/usr/lib/tmpfiles.d/systemd.conf:20] Unknown user 'systemd-network'.

In that respect, switching from nss-altfiles to sssd isn't much worse.

> I'm not sure that I understand why defining the system users in a file wasn't suitable? Wasn't the point of using nss-altfiles that the uids in /usr/lib/passwd always need to be in sync with the uids that own files in the OSTree-deployed tree?

That is still the case (i.e. we're still defining uids in /usr/lib/passwd), we're just trying to use sssd for it instead of nss-altfiles. Both provide this "alternate store" functionality.

> They can also contain user-based rules like <policy user="smcv">, or even user-defined rules like <policy user="staff">; that isn't something I would recommend in production, but it does work.

Gotcha. So if it's a supported (albeit not recommended) path, then we do want to make sure dbus.service is started after `nss-user-lookup.target` is reached, right? If that's the case, I can change this patch to just drop the `Wants=` bit. Otherwise, I'm satisfied with the discussions here to just do this differently somewhere downstream.

[1] https://fedoraproject.org/wiki/Changes/Authselect
[2] https://github.com/pbrezina/authselect/issues/48
[3] http://ostree.readthedocs.io/en/latest/manual/adapting-existing/#usrlibpasswd
[4] https://github.com/projectatomic/rpm-ostree/issues/49

https://github.com/systemd/systemd/pull/8884 has been merged.

Just be aware that startup-ordering also affects shutdown-ordering. So the more stuff dbus-daemon depends on, the earlier it gets shut down. This means more and more services are precluded from using D-Bus, because dbus is not ready when they are.

I really think it is unfortunate to disallow D-Bus from NSS-modules. If we make this ordering explicit, then we really make D-Bus from NSS a no-go. Not just because it dead-locks during startup, but also because it is ordered wrongly during shutdown. Failing NSS during shutdown just because D-Bus is already down seems counter-intuitive.

I would rather vote against extending the use of NSS in D-Bus. Yes, this is all already possible, but it has been a pain in the past. I would rather like to discourage everyone from declaring policies based on users/groups, and instead use polkit or their own alternative.

(In reply to David Herrmann from comment #17)
> I would rather vote against extending the use of NSS in D-Bus. Yes, this is
> all already possible, but it has been a pain in the past. I would rather
> like to discourage everyone from declaring policies based on users/groups,
> and instead use polkit or their own alternative.

I would like to discourage that too, but sorry, you are about 15 years too late to prevent the meaning of dbus-daemon policies from depending on NSS lookups. (My own involvement in D-Bus also started after this had become part of the API.)

There is at least one use-case for policies based on uid or gid that cannot use polkit:

(In reply to Simon McVittie from comment #12)
> (Access to org.freedesktop.DBus.Debug.Stats on the system bus is one place
> where rules like that are helpful; we can't use polkit from dbus-daemon
> because it would be a circular dependency, and accessing
> org.freedesktop.DBus.Debug.Stats is enough of an information leak that it
> should be denied for non-root users by default, but a small enough
> information leak that we don't necessarily need to hard-code it to be
> root-only.)

One possibility would be to have a two-phase startup for dbus-daemon --system:

* During early boot: start, but only accept connections from uid 0, and
  assume that any user- or group-based policy is inapplicable (except
  for user="root", which is applicable to connections with uid 0).
  This needs to be able to resolve two uids, namely root and messagebus
  (or dbus, or _dbus, or whatever your OS calls the uid of the
  dbus-daemon --system process), plus messagebus's primary gid.

* Later: when told that NSS is fully operational, reload policy.

However, this has some flaws:

* Extra complexity (more bugs)

* It's still going to deadlock if dbus-daemon blocks on NSS calls that are
  implemented in terms of D-Bus, unless the policy/configuration loading
  is offloaded into a separate process that serializes it and sends it
  to the dbus-daemon to deserialize (more complexity, more bugs)

* We still need to learn messagebus's uid and gid before we can operate
  D-Bus, unless dbus-daemon --system runs as root, which doesn't seem like
  the best idea given its size and attack surface; and we need to learn
  them without deadlocking, which means we have to be able to avoid
  calling into arbitrary NSS modules that (in a world where NSS is
  allowed to use D-Bus) might themselves use D-Bus

I think the only conservative route here is to say that NSS modules cannot use D-Bus.

> Just be aware that startup-ordering also affects shutdown-ordering.
> So the more stuff dbus-daemon depends on, the earlier it gets shut down.

However, this is a persuasive argument for not making the dependency on NSS fully explicit: we want to start after NSS services start, but we don't necessarily want to stop before NSS services stop.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/209.