Bug 36845

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.

Review of attachment 46325 [details] [review]:

::: mission-control-plugins/Makefile.am
@@ +56,3 @@
 
+if ENABLE_AEGIS
+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 @@
+#if ENABLE_AEGIS
+#include "builtin-aegis-acl.h"
+#endif

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 ());
+#endif
+

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

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

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.

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
  credentials
* 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

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.

Created attachment 46397 [details] [review]
updated cumulative patch

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

http://cgit.collabora.co.uk/git/user/smcv/telepathy-mission-control-smcv.git/commit/?h=builtin-aegis-acls&id=d2f40c4995911d3583794297f6a76b4570453c44

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

should be

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

Other than that, I think it looks reasonable,
so review+ once that change is made.

Fixed in master for 5.7.11