[patch] dbus system activation

[Richard Hughes]

On Wed, 2007-06-13 at 15:20 -0400, Havoc Pennington wrote:
> Cool! Thank you for working on this.

No problem.

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

Sounds good.

> > 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?

No, totally agree.

> > • 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)

Done.

> > • 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.

Agreed, and done.

> > • 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.

Sure, I'll give these a go when I've got the core code in place. All the
time I'm refactoring there doesn't seem much need to add new tests. 

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

Sure. I'll have a crack myself really soon.

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

Flag? Can't I just use a #define?

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

dbus-daemon-launch-helper?

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

Sure, done.

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

Yes, I think that's best. I don't think libexec stuff needs a manpage as
it's not designed to be run by the user.

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

Ick, forking the helper seems very complicated to me. I'll make the
service file scanning stuff ridiculously small and hopefully that will
be easier to audit.

> > +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.

Indeed. Make it build and post the patch. :-) I've tried reducing the
file count, and it will need some pretty major re-factoring to reduce
this substantially.

config-parser.c in particular has some deep dependencies, I think this
needs to be split into two files, one to extract the information from a
single .conf file, and one to process and merge them. The former can be
used in the launch-helper, on the assumption that we say that you can't
put the location of the setuid helper in an included conf file. Or maybe
there's a better way to do this (#ifndef SETUID_HELPER?).

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

Sure. Lots.

> > +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.

1.7M unstripped, 732K stripped. Ick. A setuid binary of that size is
going to make a lot of people very nervous.

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

Good plan.

> > +	## 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?

Totally. Crash and burn. See below.

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

Check if we are root _and_ warn I guess. I've done something like that.

> > +  /* 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.

Agree, done.

> > +  /* 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/>

Done.

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

Yes, this is a very good idea, done.

> > +  /* 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.

Done.

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

Sure, I've pushed this to desktop-file.h for now, but we need to do some
refactoring to split the config-parser up a little.

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

Sure. Newbie mistake.

> > +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.

Sure, this bit is straight from davidz's patch, I've cut it right down,
see a few lines below...

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

Is this a hard requirement for the service files? If so, this should be
documented somewhere. If this is the case, it makes looking for the
other parameters *much* easier. This is what I've done in this current
patch.

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

Yes, this is what I've done. It's quicker, and way less code.

> > +/* 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)

Done.

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

Set, or just saved and restored from before clearenv?

> > +static dbus_bool_t
> > +_check_permissions (void)
> 
> This is the function you probably have to skip for make check purposes.

Done.

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

Done.

> > +  /* 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?

I wasn't keen on doing this, as it means effectively screenscraping an
output and feeding it back through the bus to the user. This is what HAL
does when external tools want to propagate an error, and I can do this
if you wish.

> In any case _dbus_warn is probably slightly better.

Done.

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

Done.

> > +      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")

Done.

> > +        _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

Done.

> > --- /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 ;-)

Heh, you know what I mean. Second patch attached. Thanks for the
comprehensive review.

Richard,

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-system-activation-05.patch
Type: text/x-patch
Size: 55350 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/dbus/attachments/20070614/010602d3/attachment-0001.bin