[patch] dbus system activation

Havoc Pennington hp at redhat.com
Wed Jun 13 12:20:12 PDT 2007


Cool! Thank you for working on this. Since I need my new anonymous stuff 
and you have done this, I think we should start planning on a 1.2.0 ASAP 
with those two features and whatever else is in there so far.

Richard Hughes wrote:
> helper. This service name "org.me.test" is then searched for in
> the .server files in /usr/share/dbus/services. A typical service file
> would look like:

I would suggest that we have a different directory for this, like 
datadir/system-services. I don't think it will make sense most of the 
time to launch the same stuff on both system and session, and when it 
does, the app can explicitly place the .service file in both places.
Am I wrong here?

> • We clear all environment variables except for DBUS_VERBOSE which is
> used for debugging

You could also conditionalize this with DBUS_ENABLE_VERBOSE 
(double-check what that #define is called)

> • The standard_session_servicedirs command is used in the system file,
> which we need to decide what do do with.

Add standard_system_servicedirs I would say.

> • The system "org.me.test" scripts are not integrated with make check,
> and are just dumped in test.

There are a number of activation-related tests already; you should be 
able to basically copy all those tests, but run them on a bus configured 
to use the activation helper. Then we can see that with the activation 
helper the same test-segfault, etc. things to activate all work as intended.

I would like to see some pretty solid make check tests before putting 
this in, since it is security sensitive and the current tests won't 
exercise the fairly complex codepath involved.

You might have to add a "don't actually change user" flag to the 
activation helper or something for use in make check.

Other stuff:

I think I may have tried nuking the term "activation" from public API 
and docs (perhaps with a couple oversights), so we may want to also 
rename the activation-helper to something else.

Don't worry about the term activation inside the code, just in config 
files and APIs.

The helper should probably have a short man page, it doesn't have to say 
a whole lot, just explain what it is and say "don't worry about it" - 
though I guess even Debian may not require man pages for libexec 
binaries? If there's a libexec loophole we can punt on the man page.

There is quite a bit of code in the helper; I wonder if it would be nuts 
for the helper itself to fork into a 
parse-the-config-file-and-service-files half and a 
retain-privileges-and-exec half? The extra complexity may be as much of 
a security problem as the large amount of code it's going to run right 
now, though.

Some comments inline -

> +dbus_daemon_activation_helper_SOURCES=		\
> +	$(BUS_SOURCES)				\
> +	activation-helper.c					

Here you are linking all of the source files for the bus daemon into the 
activation helper, which seems unlikely to be what you meant.

There may be some refactoring required to get the activation code to be 
more modular so it doesn't rely on the bus daemon core.

> +dbus_daemon_activation_helper_LDADD=			\
> +	$(EFENCE)					\
> +	$(DBUS_BUS_LIBS)				\
> +	$(top_builddir)/dbus/libdbus-convenience.la

The libdbus-convenience here probably makes the activation helper huge. 
In principle we could use "ld --gc-sections" but I have never set dan to 
figure it out.

We can probably punt this for now, but expect someone to be all "why the 
hell is this setuid binary so big?"

> +	## This is SETUID! <insert suitable warning here...>
> +	chown root:$(DBUS_USER) $(DESTDIR)$(libexecdir)/dbus-daemon-activation-helper;
> +	chmod 4750 $(DESTDIR)$(libexecdir)/dbus-daemon-activation-helper;

Presumably this will fail if someone is installing when they are not root?

It should probably just warn if the commands fail, or something like 
that. Or maybe just check whether we're root. Not sure if other packages 
have some standard practice.

> +  /* user is not _required_ unless we are using system activation */
> +  if (!bus_desktop_file_get_string (desktop_file,
> +                                    DBUS_SERVICE_SECTION,
> +                                    DBUS_SERVICE_USER,
> +                                    &user, NULL))
> +    user = NULL;
> +
> +  /* group is not _required_ unless we are using system activation */
> +  if (!bus_desktop_file_get_string (desktop_file,
> +                                    DBUS_SERVICE_SECTION,
> +                                    DBUS_SERVICE_USER,
> +                                    &group, NULL))
> +    group = NULL;
> +

If this can fail due to out-of-memory it probably has to be handled 
differently from failure due to the user/group not existing in the file.

> +  /* system bus uses helper */
> +  bus_type = bus_context_get_type (activation->context);
> +  if (bus_type != NULL && strcmp (bus_type, "system") == 0)

Rather than doing this, I would just add a new config file directive 
like <activation_helper/> or something of that nature. Or to avoid the 
term activation, maybe <service_launch_helper/>

Or maybe just make the executable name go in the config file? Could be 
gratuitous flexibility, but I can imagine <service_launch_helper 
path="/libexec/foo"/> might be handy both in debugging and in setting up 
the special bus daemon for make check.

> +  /* convert command into arguments */
> +  if (!_dbus_shell_parse_argv (_dbus_string_get_data (&command), &argc, &argv, error))

string_get_const_data is slightly better here.

> Index: bus/bus.h
> +#define DBUS_SERVICE_SECTION  "D-BUS Service"
> +#define DBUS_SERVICE_NAME     "Name"
> +#define DBUS_SERVICE_EXEC     "Exec"
> +#define DBUS_SERVICE_USER     "User"
> +#define DBUS_SERVICE_GROUP    "Group"
> +

This is an odd header to put this in I think, I would expect some kind 
of ".service file parser"/"activation" header.

> --- /dev/null	2007-06-13 10:10:28.245607928 +0100
> +++ bus/activation-helper.c	2007-06-12 23:11:24.000000000 +0100
> @@ -0,0 +1,548 @@
> +/* -*- mode: C; c-file-style: "gnu" -*- */
> +/* main.c  main() for message bus

Cut-and-paste glitch here.

> +static BusDesktopFile *
> +_scan_service_dir_for_desktop_file (const char *s_dir,
> +            const char *name,
> +            DBusError *error)

It looks like some code in this function might be cut-and-paste from 
existing service dir scanning? I prefer cut-and-paste to weird 
abstractions that don't make sense, but if there is some clean 
abstraction that would be good.

An optimization might be to first check for a .service file named after 
the bus name you're looking for; the .service files aren't required to 
be named that way, but people have been doing it, and it would speed 
this up a lot if people do it.

Perhaps we should even require it for the system directory - it wouldn't 
be compatible to require it for the session directory, but for this new 
stuff we could. i.e. don't scan at all, just load the file by name.

> +/* only saves DBUS_VERBOSE */
> +static dbus_bool_t
> +_clear_environment (void)

As I mentioned, could stick #ifdef DBUS_VERBOSE_MODE on this (or 
DBUS_ENABLE_VERBOSE or whatever it's called)

Also, there is a DBUS_BUS_STARTER iirc that is supposed to be set and 
probably should be set here.

> +static dbus_bool_t
> +_check_permissions (void)

This is the function you probably have to skip for make check purposes.

Also, in here you're using the DBUS_USER #define; I think since you're 
parsing the config file anyway, you should use the user set in the 
config file. In fact using DBUS_USER anywhere in the C code is probably 
a bug, since it is configurable. It's fine to use it in the Makefile.

> +  /* get the name of the service */
> +  dbus_error_init (&error);
> +  if (!bus_desktop_file_get_string (desktop_file,
> +                                    DBUS_SERVICE_SECTION,
> +                                    DBUS_SERVICE_NAME,
> +                                    &name,
> +                                    &error))
> +    {
> +      _dbus_verbose ("dbus-daemon-activation-helper: could not get name!\n");

Remember _dbus_verbose isn't normally enabled in production... it would 
be better to try to get an error visible to the app developer somehow, 
perhaps dbus-daemon could stick stderror from the activation helper in 
an activation error? In any case _dbus_warn is probably slightly better.

I'd also say something like "file %s does not have Name key" which would 
be more explanatory.

> +      goto finish;
> +    }
> +
> +  /* verify that the name is the same as the service_name (should be...) */
> +  if (strcmp (service_name, name) != 0)
> +    {
> +      _dbus_verbose ("dbus-daemon-activation-helper: name does not match!\n");

Again here I'd go for _dbus_warn("name %s does not match expected %s")

> +        _dbus_verbose ("dbus-daemon-activation-helper service.to.activate\n");

Here you probably don't want _dbus_verbose either - just fprintf(stderr 
is fine I think, look at what tools/* does

> --- /dev/null	2007-06-13 10:10:28.245607928 +0100
> +++ test/dbus-test-server.py	2007-06-12 16:00:43.000000000 +0100
> @@ -0,0 +1,55 @@
> +#!/usr/bin/python

Unfortunately make check can't have a circular dep on dbus-python ;-)


More information about the dbus mailing list