[patch] dbus system activation
Havoc Pennington
hp at redhat.com
Wed Jun 13 12:20:12 PDT 2007
Hi,
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 ;-)
Havoc
More information about the dbus
mailing list