Bug 39611

Review of attachment 49643 [details] [review]:

Fair enough, but...

::: bus/config-parser.c
@@ +1057,3 @@
                e->d.policy.type = POLICY_CONSOLE;
+
+               _dbus_warn ("Obsolete at_console policy used in message bus configuration file.\n");

In practice nobody's going to see this: dbus-daemon normally closes its stderr. You want _dbus_system_log(), I think (that writes to syslog).

Created attachment 49683 [details] [review]
the patch, take 2

ok, this one uses syslog for this message.

We can get the information with grep on source code...not really convinced this is going to make people write patches.  But maybe link to this bug?  You can use https://bugs.freedesktop.org/39611 which is shorter.

I think in practice what will happen is people will file bugs on dbus here or in distribution trackers.

(In reply to comment #3)
> I think in practice what will happen is people will file bugs on dbus here or
> in distribution trackers.

Yeah, that seems likely, unfortunately; better to just fix the services directly.

Perhaps we could get a check added to lintian and rpmlint?

(In reply to comment #3)
> We can get the information with grep on source code...not really convinced this
> is going to make people write patches.  But maybe link to this bug?  You can
> use https://bugs.freedesktop.org/39611 which is shorter.
> 
> I think in practice what will happen is people will file bugs on dbus here or
> in distribution trackers.

I have done similar warnings a couple of times in the past, for example in order to get people use Avahi's native API instead of the bonjour API. I got a couple of complains (4 or so), but in the end a lot of developers came to us interested to get more information, and I think they wouldn't have come so quickly if we hadn't spit out those warnings. So I think it worked, and I think we could repeat it here.

(In reply to comment #4)
> (In reply to comment #3)
> > I think in practice what will happen is people will file bugs on dbus here or
> > in distribution trackers.
> 
> Yeah, that seems likely, unfortunately; better to just fix the services
> directly.

Well, I think we don't scale that well ;-)

> Perhaps we could get a check added to lintian and rpmlint?

It reaches the wrong people with lintian/rpmlint. You want the developers, and people pushing the developers. Since the developers have to test their stuff, you want those warnings on their screens. And you want the users to file bugs. Hence runtime warnings are the best idea I think.

Created attachment 50556 [details] [review]
the patch, take 3

Another iteration, this time with the bugzilla URL.

Review of attachment 50556 [details] [review]:

I don't want to bikeshed this too much, but OK, I accept your argument about adding a bug link.

Just one other thing - should we perhaps just do this once?  Something like Linux kernel style:

#define WARN_ONCE(condition, format...)	({			\
	static bool __warned;					\
	int __ret_warn_once = !!(condition);			\
								\
	if (unlikely(__ret_warn_once))				\
		if (WARN(!__warned, format)) 			\
			__warned = true;			\
	unlikely(__ret_warn_once);				\
})

(In reply to comment #8)
> Review of attachment 50556 [details] [review]:

> Just one other thing - should we perhaps just do this once?  Something like
> Linux kernel style:
> 
> #define WARN_ONCE(condition, format...)    ({            \
>     static bool __warned;                    \
>     int __ret_warn_once = !!(condition);            \
>                                 \
>     if (unlikely(__ret_warn_once))                \
>         if (WARN(!__warned, format))             \
>             __warned = true;            \
>     unlikely(__ret_warn_once);                \
> })

Hmm, I don't think this would really work nor is really necessary, since this is a per-policy file thing (i.e. i don't think we should hide that app a has a file with at_console if app b has one too), and we only warn when we reload the policy (i.e. only once at boot, and once on upgrades where dbus is reloaded).

(In reply to comment #9)

> 
> Hmm, I don't think this would really work nor is really necessary, since this
> is a per-policy file thing (i.e. i don't think we should hide that app a has a
> file with at_console if app b has one too), and we only warn when we reload the
> policy (i.e. only once at boot, and once on upgrades where dbus is reloaded).

Fair enough - good to commit then as is now as far as I'm concerned.

Comment on attachment 50556 [details] [review]
the patch, take 3

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

typo in commit msg: instea --> instead

If we are going to obsolete at_console, it's a good idea to mention that in dbus-daemon(1) too.

(In reply to comment #6)
> It reaches the wrong people with lintian/rpmlint. You want the developers,
> and people pushing the developers. Since the developers have to test their
> stuff, you want those warnings on their screens. And you want the users to
> file bugs. Hence runtime warnings are the best idea I think.

I still think, in practice, the result of this patch is more likely to be people opening dbus bugs, either here or in their distros. I'm not going to veto someone else applying it, but I'm not going to apply it myself either.

Developers probably won't see this anyway, because the best we can do is to make it go to dbus-daemon's output in syslog/the Journal - it isn't going to show up in their daemon's output when they're debugging it, because it isn't their daemon that reads the policy files, it's dbus-daemon.

I've written a patch for Lintian: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=762609>. When that gets applied, lintian.debian.org will list all the suspicious policies in Debian, which is a reasonably large subset of all the suspicious policies, and can give us an idea of the size of the problem.

Live Lintian report: <https://lintian.debian.org/tags/dbus-policy-at-console.html>

I'm not sure that that report is fully accurate: maybe my patch was wrong. On my laptop, the bluez, hplip and system-config-printer packages have at_console in their policies, but they are not listed in that report.

(In reply to Simon McVittie from comment #14)
> I'm not sure that that report is fully accurate

It isn't, but it will be eventually - not all packages have been processed with the new Lintian snapshot yet.

I've been looking into at_console recently (thanks for the lintian report, very useful). It appears to me that we are not really making much progress on getting people to stop using it. Any chance we could reconsider applying this patch to try to bring some attention to the issue?

(In reply to Tom Gundersen from comment #16)
> I've been looking into at_console recently (thanks for the lintian report,
> very useful). It appears to me that we are not really making much progress
> on getting people to stop using it. Any chance we could reconsider applying
> this patch to try to bring some attention to the issue?

I think the problem is that adding polkit support to a project is seen as a lot of work, and at_console is trivial to use. As long as it continues to do what developers want, they’re going to continue to use it. I think a warning is going to get people to migrate to polkit only if it also comes with a deprecation timeline for at_console.

(In reply to Tom Gundersen from comment #16)
> I've been looking into at_console recently (thanks for the lintian report,
> very useful). It appears to me that we are not really making much progress
> on getting people to stop using it. Any chance we could reconsider applying
> this patch to try to bring some attention to the issue?

I'm still concerned that the likely reaction to this patch is going to be people opening misguided bugs against dbus, either upstream or downstream.

I've also had feedback from the Lintian check that Debian developers seeing their packages flagged by that check would like to fix it, but don't know how. I don't think we have any particularly coherent documentation on why polkit is preferable to at_console, and on how to convert existing services to use polkit. Unless you know of such documents? If not, I've been meaning to write one for a while, but it's in the queue with many other dbus tasks (I work for a consultancy, so dbus is rarely my primary project).

If the message was more like

    dbus[123]: /etc/dbus-1/system.d/com.example.BadPackage.conf: at_console policy is deprecated, please convert the service to use polkit. See https://dbus.freedesktop.org/doc/at-console-deprecated.html for details.

(and if there was a suitable document behind that URL!) then I think maybe that'd be OK? That's more clearly putting the blame on whichever package owns that config file, which can be queried with whatever is this distro's equivalent of dpkg -S.

(In reply to Simon McVittie from comment #18)
> (In reply to Tom Gundersen from comment #16)
> > I've been looking into at_console recently (thanks for the lintian report,
> > very useful). It appears to me that we are not really making much progress
> > on getting people to stop using it. Any chance we could reconsider applying
> > this patch to try to bring some attention to the issue?
> 
> I'm still concerned that the likely reaction to this patch is going to be
> people opening misguided bugs against dbus, either upstream or downstream.
> 
> I've also had feedback from the Lintian check that Debian developers seeing
> their packages flagged by that check would like to fix it, but don't know
> how. I don't think we have any particularly coherent documentation on why
> polkit is preferable to at_console, and on how to convert existing services
> to use polkit. Unless you know of such documents? If not, I've been meaning
> to write one for a while, but it's in the queue with many other dbus tasks
> (I work for a consultancy, so dbus is rarely my primary project).
> 
> If the message was more like
> 
>     dbus[123]: /etc/dbus-1/system.d/com.example.BadPackage.conf: at_console
> policy is deprecated, please convert the service to use polkit. See
> https://dbus.freedesktop.org/doc/at-console-deprecated.html for details.
> 
> (and if there was a suitable document behind that URL!) then I think maybe
> that'd be OK? That's more clearly putting the blame on whichever package
> owns that config file, which can be queried with whatever is this distro's
> equivalent of dpkg -S.

I entirely agree, although I think the warning would be more effective if it said ‘at_console policy is deprecated and will be removed in D-Bus x.y’, because people love to ignore deprecation warnings for as long as they can get away with.

(In reply to Philip Withnall from comment #19)
> I think the warning would be more effective if it
> said ‘at_console policy is deprecated and will be removed in D-Bus x.y’,
> because people love to ignore deprecation warnings for as long as they can
> get away with.

To be able to do this, we need to be absolutely clear about why at_console is sufficiently problematic to justify an incompatible change (its deletion). "People should really use PK" doesn't seem like enough justification.

You know that there are good reasons, and I know that there are good reasons, but if those reasons aren't written down somewhere obvious then we are never going to convince the authors of something like BlueZ that they are good.

The reasons that I am aware of:

* polkit policies are defined in terms of high-level actions that are
  intended to make sense in a domain-specific way: for example, udisks2
  has distinct actions for "mount a system partition", "mount a partition
  of a removable disk on the same seat as the user" and "mount a partition
  of a removable disk on a different seat". D-Bus policy XML is entirely
  in terms of the mechanics of the message-passing, so it cannot make
  these domain-specific distinctions.

* Editing the D-Bus policy XML language is error-prone, because any
  configuration file can affect any service and the semantics are not
  obvious. The policy language that sysadmins wishing to vary policy are
  encouraged to edit should be something with fewer sharp edges, like
  the current JavaScript and former .pkla languages in polkit.

* It is technically very problematic for dbus-daemon to call out to D-Bus
  services to implement at_console, which is a problem for projects that
  reimplement the logind parts of libsystemd in terms of D-Bus calls to
  an alternative like ConsoleKit2, for example
  <https://github.com/dimkr/LoginKit>: a synchronous call would lead to
  immediate deadlock. polkit is fully asynchronous, and making it call
  out to D-Bus services is much more straightforward. libsystemd only
  manages to avoid this problem by using files in /run as its
  read-only IPC API for the parts of logind that dbus-daemon
  interacts with.

(Yes, by mentioning that last one I am deliberately trying to preempt people who might otherwise claim this is some sort of pro-systemd conspiracy. In fact it's somewhat the opposite - systemd-logind works fine with at_console, but loginkit wouldn't.)

Are there others?

Perhaps another argument against at_console, is that (at least in my view) its semantics is really not very intuitive. It gives the impression of being a dynamic property, but it isn't.

A priori, I would have thought that at_console would be true for any message being sent from a process in a session that is currently active on a seat. As we all know, that is not at all the case: the at_console property is pinned at connection time, and it is user-wide, rather than per session. This could easily cause connections to be considered at_console, even if the associated user is not. Or even more confusingly, you could have two connections in the same session at the same time, where one is considered at_console and the other isn't.

This isn't really a technical problem, as much as a messaging/documentation one, and I consider the main issue the last one Simon mentioned: having to do IPC in order to do IPC is just asking for trouble.

Note the dbus-broker project is discussing ways of working around the latent need for at_console support here: https://github.com/bus1/dbus-broker/issues/56

-- 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/52.