Bug 36845

Summary: As well as ACLs for DBus calls, we need ACLs to filter which handlers get channels
Product: Telepathy Reporter: Vivek Dasmohapatra <vivek>
Component: mission-controlAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: 5.7   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.co.uk/git/user/smcv/telepathy-mission-control-smcv.git/log/?h=builtin-aegis-acls
Whiteboard: NB#206747, review-
i915 platform: i915 features:
Attachments: git diff -M showing first two commits
cumulative diff
updated cumulative patch

Description Vivek Dasmohapatra 2011-05-04 08:15:59 UTC
To paraphrase slightly:

“Currently, Mission Control checks a client's credentials when
 it requests a channel, and forbids clients without appropriate 
 credentials from making such requests if an ACL is interested 
 in the channel. But it does not check the credentials of clients
 to which it dispatches channels.”

We also need to move the aegis ACLs (if configured) to be builtins
rather than separate files (a requirement which emerges from the 
way aegis works)
Comment 1 Simon McVittie 2011-05-04 08:31:36 UTC
Created attachment 46325 [details] [review]
git diff -M showing first two commits

The first two commits have an unreviewable diffstat due to being delete + add (as two separate commits, no less); here's the git diff -M output, showing the rename a bit more sensibly than gitweb does.
Comment 2 Simon McVittie 2011-05-04 08:58:36 UTC
Review of attachment 46325 [details] [review]:

::: mission-control-plugins/Makefile.am
@@ +56,3 @@
+libmission_control_plugins_la_SOURCES += builtin-aegis-acl.c

This should be in plugins or src, not in m-c-p. m-c-p should only contain things we're willing to treat as stable C API (i.e. the GInterfaces for people to implement, the GInterfaces representing what they'll be given, and an absolute minimum of utility code).

I'd actually be tempted to move this back to plugins/, and just change the build system to build it as a static convenience library (noinst_LTLIBRARIES), to be linked into MC in src.

::: mission-control-plugins/builtin-aegis-acl.h
@@ +26,3 @@
+#include <sys/types.h>
+#include <sys/creds.h>
+#include <glib-object.h>

Don't include sys/types.h or sys/creds.h in this header unless you really need to. They can almost certainly move to the .c file, when you've made the changes below.

@@ +32,3 @@
+typedef struct {
+  GObject parent;
+} _AegisAcl;

C lets you point to a struct whose members you haven't defined, as long as you tell it that it's a struct. The correct GObject idiom to make use of this is as follows:

Near the top of the .h file:

    typedef struct _AegisAcl AegisAcl;

In the .c file if it's private (and here, it should be), or lower down the .h file otherwise:

    struct _AegisAcl {

In this particular case you don't necessarily need AegisAcl to appear in the header at all: all MC cares about is that it's a GObject implementing McpDBusAcl, so why not make aegis_acl_new() just return GObject* or McpDBusAcl*? Then you can move both the structs, both the typedefs, and both the sys/*.h into the implementation.

::: mission-control-plugins/dbus-acl.c
@@ +67,3 @@
+#include "builtin-aegis-acl.h"

Do this in src/plugin-loader.c instead.

Not directly relevant to this bug, but the logic for ENABLE_AEGIS is astonishing. Normally, you're meant to #define the magic symbol to 1 to enable something, or *leave it undefined* to disable - but you're defining it to 1 or 0, which violates least-astonishment for autoconf users.

@@ +148,3 @@
+  dbus_acls = g_list_prepend (dbus_acls, aegis_acl_new ());

This entire function should have been in src/, really, but never mind.

Instead of this change, apply this pseudo-patch to src/plugin-loader.c:

>     mcp_read_dir (dir);
> +
> +   /* if enabled, the Aegis pseudo-plugin is part of the binary,
> +    * so you can't just bypass it by deleting the file */
> +   #if ENABLE_AEGIS
> +   mcp_add_object (aegis_acl_new ());
> +   #endif
> +
>      g_once_init_leave (&ready, 1);

(mcp_add_object() prepends, so that will treat the Aegis plugin as highest-priority.)
Comment 3 Simon McVittie 2011-05-04 09:42:40 UTC
> +gboolean
> +mcp_dbus_channel_acl_authorised (const TpDBusDaemon *dbus,

This entire function, and also cached_acls(), should be in src/ somewhere. If you want a wrapper around authorised() in MCP, it should be totally trivial, just like mcp_dispatch_operation_policy_check().

It occurs to me that a new GInterface for McpDBusChannelAcl (whose name I dislike anyway, see below) is overkill. Why don't you just add a new method to McpDispatchOperationPolicy:

     * Check whether a Handler is eligible to handle the given channels.
     * This is called later than mcp_dispatch_operation_policy_check(),
     * when MC has a particular Handler in mind.
     * Returns: %TRUE if @handler can be considered as a handler for
     *   the channels in @dispatch_op, or %FALSE to disqualify that handler
    gboolean mcp_dispatch_operation_policy_handler_is_available (
        McpDispatchOperationPolicy *policy,
        McpDispatchOperation *dispatch_op,
        TpProxy *handler);

and make your pseudo-plugin implement that method, but not check()?

> + * The contents of the #McpDBusChannelAcl struct are not public,
> + * so to provide an implementation of the virtual methods,
> + * plugins should call mcp_dbus_channel_acl_iface_implement_*()
> + * from the interface initialization function

You don't actually need to do this dance: just show the API user the FooIface struct and let them fill it. Enlarging a GInterface's FooIface struct is considered by GObject upstream to be a compatible change, as long as you don't remove, change or re-order any existing function pointers.

> Implement the Aegis DBus Channel/Handler policy as an ACL plugin

Like the other remarkably similar pseudo-plugin, this should be in plugins/ or src/.

I don't see any reason why this is a separate object at all - please just have one Aegis pseudo-plugin object that implements both GInterfaces.

> + creds_t caller = creds_gettask (pid);

AIUI you're now meant to make a call out to some API that has been bolted onto the Aegis-patched dbus-daemon, to avoid time of check/time of use problems. Use that instead of GetConnectionUnixProcessID.

> + channels = _mcd_tp_channels_build_from_list (self->priv->channels);

This whole function is unnecessary if you have, and use, a McpDispatchOperation (which you do - it's accessible as self->priv->plugin_api here). As a bonus, not allocating @channels means you can revert the bit that adds the "dispatched" variable and "goto done".

> + DEBUG ("handler %s rejected by ACL: %s", name, error->message);

This code path leaks @error.

> Filter reinvocation of handlers through the DBus Aegis ACL as well

You don't need this commit: the handler is already handling the channels here, so this can only be a problem if you already got it wrong. There's no transfer of responsibility.


Things which were wrong with McpDBusChannelAcl, but are academic if you follow the suggestion above:

> +/* Mission Control plugin API - DBus Channel Handler/Observer ACL

Is this Handler/Observer or just Handler? As far as I can see, just Handler.

> + * SECTION:dbus-channel-acl
> + * @title: McpDBusChannelAcl
> + * @short_description: DBus ACLs for channel handlers

Nothing in this API is specific to either D-Bus or ACLs - the plugin implementing this interface is just asked for a yes/no decision, and it's up to the plugin how it implements that internally.

I'd call this thing McpHandlerPolicy (analogous to McpDispatchOperationPolicy), and describe it as:

    @short_description: disqualify certain Handlers from acting on
        certain Channels

> +#include <mission-control-plugins/mcp-signals-marshal.h>

I don't see any signals, so you can probably remove this.

> +#include "config.h"

Should always be the very first code in the file, so it can do archaic workarounds like "#define inline /* nothing */" if needed.
Comment 4 Simon McVittie 2011-05-05 10:56:51 UTC
Created attachment 46370 [details] [review]
cumulative diff

Here's a cumulative diff for my branch, with rather less diffstat. It passes tests when built normally (well, 3 tests fail in my environment, but that's not a regression - the same tests fail on master), and it compiles with Aegis enabled, but I haven't yet tested it on a device where Aegis works.
Comment 5 Simon McVittie 2011-05-06 01:52:49 UTC
I now realise that neither of our implementations of the Aegis pseudo-plugin is sufficiently complete: both make an implicit assumption that the prospective Handler is already running, and will reject it if it is not.

This happens to "usually work" if the Handler has Client.I.Requests, or if the platform-default UI is pre-started to minimize latency, but will fail otherwise.

To fix that, we'll need to do this:

* make the suitability check asynchronous (but return rapidly in the
  common case)
* if the Channel is one that should be restricted, activate the
  prospective Handler (and wait for it to start) before inspecting its
* to avoid time-of-check/time-of-use problems, each Handler with the
  magic token should disallow processes without the magic token from owning
  its well-known Client name
Comment 6 Simon McVittie 2011-05-06 03:57:39 UTC
We probably also want Claim() by an insufficiently privileged handler to fail, but I think that can go via the speculatively-general "D-Bus ACL" plugin interface.
Comment 7 Simon McVittie 2011-05-06 07:28:50 UTC
Created attachment 46397 [details] [review]
updated cumulative patch
Comment 8 Simon McVittie 2011-05-06 07:30:29 UTC
(In reply to comment #6)
> We probably also want Claim() by an insufficiently privileged handler to fail,
> but I think that can go via the speculatively-general "D-Bus ACL" plugin
> interface.

I made this go via McpDispatchOperationPolicy too - in this case the plugin gets a NULL TpClient, but will get a non-NULL unique name, which happens to be enough for Aegis to make an informed decision.

Still needs testing under Aegis, but I think that's a job for next-week man.
Comment 9 Vivek Dasmohapatra 2011-05-10 12:30:25 UTC

while ((debug_type = creds_list (caller_creds, i, &debug_value))

should be

while ((debug_type = creds_list (caller_creds, i++, &debug_value))
Comment 10 Vivek Dasmohapatra 2011-05-10 12:47:09 UTC
Other than that, I think it looks reasonable,
so review+ once that change is made.
Comment 11 Simon McVittie 2011-05-11 03:37:15 UTC
Fixed in master for 5.7.11

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.