First pass on OLPC security modifications to DBus

[Noah Kantrowitz]

On Aug 17, 2007, at 5:40 PM, Havoc Pennington wrote:

> Hi,
>
> Thanks for the patch, here are some comments:
>
>  - if we get one more of these besides selinux and olpc we will  
> probably need
>    some kind of extensible security module thing ;-)

No argument from me, but I think I'll leave this to the experts.

>
>  - isn't there some name for this feature other than "olpc"? something
>    specific to the security architecture or whatever?

It could also be called "bitfrost", but I thought "olpc" was a bit  
less cryptic.

>
>  - the config file setting should be like <fork/> where its presence
> means "true"

I would like this to take an argument (when I get to it) rather than  
hard coding the name/path/function strings for what is now Rainbow.

>    (if we allowed "false" to override later in the file, that would be
> done with a
>     <nofork/> or <noolpc/> but I think there's no need for that)
>   The point here is <olpc/> rather than <olpc>on</olpc>
>
>  - config file element needs a patch to the dbus-daemon.1.in man page
>
>  - the header is wrong on olpc.c, both the Emacs magic and the  
> license don't
>    match the rest of dbus (olpc.h also)
>
>  - since you didn't have indent-tabs-mode nil, be sure to M-x untabify
>    to be sure none leaked in

Me no speaky emacs, but I leave my editor in spaces-only mode since  
most of what I do is Python coding.

>
>  - caching the system connection from dbus_bus_get() in a static  
> variable
>    just raises questions and issues, I would not do it, just call  
> dbus_bus_get
>    every time (and remember to unref the result). This will also  
> theoretically
>    allow you to survive a system bus restart, for those who feel  
> that is
>    feasible.

J5 had mentioned that setting up the bus connection was a relatively  
slow operation, and in round two of these changes we want to be  
calling out to Rainbow on all messages (in addition to all bindings)  
to do authz on the source and destination names. If you think this  
would be okay performance-wise I'll put it back.

>
>  - bus_olpc_check_can_own() fails to set the DBusError in a few  
> places...
>    it looks like this is intentional based on how this function is  
> called, but
>    I'm not sure I understand the semantics? if there's no memory you
>    should BUS_SET_OOM presumably?
>
>  - your system bus service seems to use the path "/" which I would
> consider bad practice,
>    it essentially encodes in the API that the process offers only that
> single object and
>    API, while it would be reasonable to offer some other second API  
> from the
>    process as well - say a system service control API with methods  
> like Start()
>    and Stop(), just to speculate on a possible one. This is like
> naming a global variable
>    "the variable" instead of "security_manager" or something.

I'm still not totally sure I grok this convention. We have been using  
different interfaces to split up our API, but none of it is really  
object-oriented, so we have left them on /. What is the value in  
encoding the API into both the object and interface names?

>
>  - since bus_olpc_check_can_own connects to the system bus and  
> blocks on it,
>    you probably want to take make it an error if the config file  
> has bus type
>    "system" and also contains the <olpc/> element, or something...

I am going to add a different terminal case. If the xid of the  
connection is 0, it should bypass the new checks.

>
>  - stdint.h should not be in olpc.h, since olpc.c is all
> linux-specific anyway it's
>    fine in there, though using the dbus typedefs would be more  
> consistent

Which file are the dbus ones in?

>
>  - I would add the new config file element to one of the test config
> files so the parsing code
>    at least gets tested
>
>  - I'm not sure how to meaningfully unit test the rest of this, but it
> seems straightforward
>    so hopefully no big issues
>
> We should carefully review for memory leaks and OOM handling and so
> forth when you have a more final version of the patch, I did not step
> through the code line-by-line yet.
>
> Havoc
> _______________________________________________
> dbus mailing list
> dbus at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dbus