Bug 101354

Summary: Minimal version of restricted, identifiable bus servers for containers (#100344)
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: medium CC: alexl, dbus, desrt, smcv
Version: git masterKeywords: patch
Hardware: All   
OS: All   
URL: https://github.com/smcv/dbus/commits/containers-minimum-101354
Whiteboard: review+
i915 platform: i915 features:
Bug Depends on: 89104, 101362, 101566, 101567, 101568, 101569, 101570, 101700, 101895, 101964    
Bug Blocks: 100344, 101898, 101899, 101902, 101903, 103457, 103458, 103470, 104610    
Attachments: [01] _dbus_asv_add_object_path: Add
bus_driver_send_ack_reply: Make available to other modules
[02] spec: Document the initial Containers1 interface
[03] driver: Add a stub implementation of the Containers1 interface
[04] test/uid-permissions: Assert that AddContainerServer is privileged
[05] test/containers: New test
[07] containers: Do some basic checking on the parameters
[08] test/containers: Exercise the new parameter checking
[09] bus/containers: Build a global data structure for container instances
[10] bus/containers: Create a DBusServer and add it to the main loop
[11] bus_context_add_incoming_connection: factor out
[12] bus/containers: Set up new connections to join the bus
[13] test/containers: Exercise a successful call to AddContainerServer
[14] bus/containers: Require connecting uid to match caller of AddContainerServer
[15] bus/containers: Each connection to a container holds a reference
[16] bus/containers: Link each container to its initiating connection
[17] bus/containers: Shut down container servers when initiator goes away
[18] bus/containers: Give each instance a list of all its connections
[19] bus/containers: Implement methods to stop containers explicitly
[20] bus/containers: Don't allow stopping other users' containers
[21] test/containers: Exercise the various ways to stop a container
[22] bus/containers: Emit ContainerInstanceRemoved signal
[23] test/containers: Assert that ContainerInstanceRemoved is emitted
[24] bus/containers: Indicate in loginfo whether connection is contained
[25] bus/driver: Treat connections from inside containers as unprivileged
[26] test/containers: Check that containers can't make new containers
[27] test/containers: Check that connections from containers are unprivileged
[28] bus/driver: Add a flag for methods that can't be invoked by containers
[29] bus/driver: Containers can't use the Verbose and Stats interfaces
[30] bus/driver: Add basic container info to GetConnectionCredentials()
[31] test/dbus-daemon: Assert absence of Containers1 credentials
[32] bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo()
test/containers: Exercise trivial and non-trivial container metadata
[34] test/containers: Check that GetContainerInstanceInfo stops working
[35] bus: Add (unused) settings for resource limits for containers
[36] bus/containers: Limit the size of metadata we will store
[37] bus/containers: Enforce max_containers limit
[38] bus/containers: Enforce max_connections_per_container
[39] containers: enforce max_containers_per_user
[40] test/containers: Exercise the resource limits
bus/driver: Allow unprivileged connections to create app-containers
[02] spec: Document the initial Containers1 interface
[33] test/containers: Exercise trivial and non-trivial container metadata
[41] Revert "test/uid-permissions: Assert that AddContainerServer is privileged"
[42] bus/driver: Allow unprivileged connections to create app-containers
[33] test/containers: Exercise trivial and non-trivial container metadata
_dbus_asv_add_object_path: Add
spec: Document the initial Containers1 interface
driver: Add a stub implementation of the Containers1 interface
test/uid-permissions: Assert that AddContainerServer is privileged
test/containers: New test
containers: Do some basic checking on the parameters
test/containers: Exercise the new parameter checking
bus/containers: Build a global data structure for container instances
bus/containers: Create a DBusServer and add it to the main loop
bus_context_add_incoming_connection: factor out
bus/containers: Set up new connections to join the bus
test/containers: Exercise a successful call to AddContainerServer
bus/containers: Require connecting uid to match caller of AddContainerServer
test-utils-glib: Factor out functions for switching uid
test-utils-glib: Add function to connect with GDBus as another uid
test/containers: Exercise connecting to the new socket as the wrong uid
bus/containers: Each connection to a container holds a reference
bus/containers: Link each container to its initiating connection
bus/containers: Shut down container servers when initiator goes away
bus/containers: Give each instance a list of all its connections
bus/containers: Implement methods to stop containers explicitly
bus/containers: Don't allow stopping other users' containers
test/containers: Exercise the various ways to stop a container
bus/containers: Emit ContainerInstanceRemoved signal
test/containers: Assert that ContainerInstanceRemoved is emitted
bus/containers: Indicate in loginfo whether connection is contained
bus/driver: Treat connections from inside containers as unprivileged
test/containers: Check that containers can't make new containers
test/containers: Check that connections from containers are unprivileged
bus/driver: Add a flag for methods that can't be invoked by containers
bus/driver: Containers can't use the Verbose and Stats interfaces
bus/driver: Add basic container info to GetConnectionCredentials()
test/dbus-daemon: Assert absence of Containers1 credentials
bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo()
t/containers: Exercise trivial and non-trivial container metadata
test/containers: Check that GetContainerInstanceInfo stops working
bus: Add (unused) settings for resource limits for containers
bus/containers: Limit the size of metadata we will store
bus/containers: Enforce max_containers limit
bus/containers: Enforce max_connections_per_container
containers: enforce max_containers_per_user
test/containers: Exercise the resource limits
Revert "test/uid-permissions: Assert that AddContainerServer is privileged"
bus/driver: Allow unprivileged connections to create app-containers
system.conf: Allow creating containers on the system bus
Add a simplified backport of g_steal_pointer()
_dbus_asv_add_object_path: Add
spec: Document the initial Containers1 interface
Add a simplified backport of g_steal_pointer()
_dbus_asv_add_object_path: Add
spec: Document the initial Containers1 interface
driver: Add a stub implementation of the Containers1 interface
test/uid-permissions: Assert that AddServer is privileged
test/containers: New test
test/uid-permissions: Assert that AddServer is privileged
test/containers: New test
bus/containers: Do some basic checking on the parameters
test/containers: Exercise the new parameter checking
bus/containers: Do some basic checking on the parameters
test/containers: Exercise the new parameter checking
bus/containers: Build a global data structure for container instances
bus/containers: Create a DBusServer and add it to the main loop
bus_context_add_incoming_connection: factor out
bus/containers: Set up new connections to join the bus
test/containers: Exercise a successful call to AddServer
bus/containers: Require connecting uid to match caller of AddServer
test-utils-glib: Factor out functions for switching uid
test-utils-glib: Add function to connect with GDBus as another uid
test/containers: Exercise connecting to the new socket as the wrong uid
bus/containers: Each connection to a container holds a reference
bus/containers: Link each container to its initiating connection
bus/containers: Shut down container servers when initiator goes away
bus/containers: Give each instance a list of all its connections
bus/containers: Implement methods to stop containers explicitly
bus/containers: Don't allow stopping other users' containers
test/containers: Exercise the various ways to stop a container
bus/containers: Emit InstanceRemoved signal
test/containers: Assert that InstanceRemoved is emitted
bus/containers: Indicate in loginfo whether connection is contained
bus/driver: Treat connections from inside containers as unprivileged
test/containers: Check that containers can't make new containers
test/containers: Check that connections from containers are unprivileged
bus/driver: Add a flag for methods that can't be invoked by containers
bus/driver: Containers can't use the Verbose and Stats interfaces
bus/driver: Add basic container info to GetConnectionCredentials()
test/dbus-daemon: Assert absence of Containers1 credentials
bus/driver: Add GetConnectionInstance(), GetInstanceInfo()
t/containers: Exercise trivial and non-trivial container metadata
test/containers: Check that GetInstanceInfo stops working
bus: Add (unused) settings for resource limits for containers
bus/containers: Limit the size of metadata we will store
bus/containers: Enforce max_containers limit
bus/containers: Enforce max_connections_per_container
containers: enforce max_containers_per_user
test/containers: Exercise the resource limits
Revert "test/uid-permissions: Assert that AddServer is privileged"
bus/driver: Allow unprivileged connections to create app-containers
system.conf: Allow creating containers on the system bus
travis-ci: Enable/disable more features in various builds
cmake: Match AC_DEFINE more precisely, respecting [] quoting
Add a simplified backport of g_steal_pointer()
_dbus_asv_add_object_path: Add
spec: Document the initial Containers1 interface
driver: Add a stub implementation of the Containers1 interface
travis-ci: Do at least one build with and one without containers
test/uid-permissions: Assert that AddServer is privileged
test/containers: New test
bus/containers: Do some basic checking on the parameters
test/containers: Exercise the new parameter checking
bus/containers: Build a global data structure for container instances
bus/containers: Create a DBusServer and add it to the main loop
bus_context_add_incoming_connection: factor out
bus/containers: Set up new connections to join the bus
test/containers: Exercise a successful call to AddServer
bus/containers: Require connecting uid to match caller of AddServer
test-utils-glib: Add failable functions to connect to a bus
test-utils-glib: Factor out functions for switching uid
test-utils-glib: Add function to connect with GDBus as another uid
test/containers: Exercise connecting to the new socket as the wrong uid
bus/containers: Each connection to a container holds a reference
bus/containers: Link each container to its initiating connection
bus/containers: Shut down container servers when initiator goes away
bus/containers: Give each instance a list of all its connections
bus/containers: Implement methods to stop containers explicitly
bus/containers: Don't allow stopping other users' containers
test/containers: Exercise the various ways to stop a container
bus/containers: Emit InstanceRemoved signal
test/containers: Assert that InstanceRemoved is emitted
bus/containers: Indicate in loginfo whether connection is contained
bus/driver: Treat connections from inside containers as unprivileged
test/containers: Check that containers can't make new containers
test/containers: Check that connections from containers are unprivileged
bus/driver: Add a flag for methods that can't be invoked by containers
bus/driver: Containers can't use the Verbose and Stats interfaces
bus/driver: Add basic container info to GetConnectionCredentials()
test/dbus-daemon: Assert absence of Containers1 credentials
bus/driver: Add GetConnectionInstance(), GetInstanceInfo()
t/containers: Exercise trivial and non-trivial container metadata
test/containers: Check that GetInstanceInfo stops working
bus: Add (unused) settings for resource limits for containers
bus/containers: Limit the size of metadata we will store
bus/containers: Enforce max_containers limit
bus/containers: Enforce max_connections_per_container
containers: enforce max_containers_per_user
test/containers: Exercise the resource limits
Revert "test/uid-permissions: Assert that AddServer is privileged"
bus/driver: Allow unprivileged connections to create app-containers
system.conf: Allow creating containers on the system bus
test/containers: Use a shorter path for XDG_RUNTIME_DIR

Description Simon McVittie 2017-06-08 16:58:21 UTC
+++ This bug was initially created as a clone of Bug #100344 +++

In app-container projects like Flatpak, Snap, Firejail and AppImage, there's a need for a confined/contained/sandboxed app to be able to reach services on the normal session and system buses. However, unrestricted access to those buses is scary (and in particular, the session bus is not considered to be a security boundary, so various services there have APIs that are arbitrary code execution).

Bug #100344 outlines a plan to improve on that situation.

Here is what I think is necessary for a "minimum viable product" version of Bug #100344:

* Utility code to copy items from a message into a buffer, and from
  that buffer into a message (it will likely have to use
  _dbus_type_writer_write_reader)
    * Automated unit test

* Implement AddContainerServer(s: container_type, s: app_id,
  a{sv}: metadata, h: socket_to_accept,
  a{sv}: arguments) -> o: instance_id

    * Stub implementation that always fails
    * Call bus_driver_check_caller_is_privileged(), fail if not
    * Check that arguments is empty
    * Allocate a new instance ID, save the metadata in a data
      structure keyed by the instance ID
    * Create a new DBusServer and add it to the main loop
    * Watch for the calling connection to disconnect, and if it does,
      disconnect the DBusServer
    * Accept connections to the new DBusServer and add them to the
      bus as normal
    * Make bus_driver_check_caller_is_privileged() fail for 
      connections from the new DBusServer
    * Make connections from the new DBusServer unable to eavesdrop
    * Make sure connections from the new DBusServer can't call Stats
      methods, etc.

* Resource limit: make each new DBusServer "cost" one connection
  in the connections-per-uid quota

* Resource limit: maximum sandboxed servers per uid. Stop
  calling bus_driver_check_caller_is_privileged() from
  AddContainerServer, but still reject calls from a sandboxed
  connection, and enforce the resource limit
    * Automated test: Call AddContainerServer, wait for success,
      connect to the new socket
    * Automated test: Allow 2 sandboxed servers per uid. Call
      AddContainerServer 3 times from different connections; assert
      that the first two succeed and the third fails. Disconnect
      first connection, and wait for second connection to be notified
      that the name-owner has been lost. Call AddContainer a fourth
      time, assert it succeeds.

* Resource limit: maximum sandboxed connections per sandboxed server
    * Automated test: exceed the limit, assert that an extra
      attempt to connect is rejected. Disconnect enough sandboxed
      connections to be back below the limit. Assert that another
      attempt to connect succeeds.

* Associate basic metadata of the DBusServer with connections
  from the sandboxed server, and return it from
  GetConnectionCredentials
    * Automated test

* Implement GetConnectionContainerInstance
    * Automated test

* Implement ContainerInstanceRemoved signal
    * Automated test: close all sandboxed connections, then cause
      the server to be removed
    * Automated test: cause the server to be removed, then close
      all sandboxed connections

* Implement an 'as' property for the keys that are allowed in the
  arguments 'a{sv}' (initially an empty array)

    * Automated test

--------

Limitations to be lifted in separate patch serieses:

* no message filtering
* the container manager has to bind() and listen() on the socket,
  most likely in the host filesystem namespace, and then bind-mount
  it into the container; in future it should be able to pass in a
  socket that has not had bind() or listen() called on it yet,
  together with some way to tell the dbus-daemon when the socket
  is ready to be accept()ed
* the container manager has to stay on the bus until it's ready for
  the DBusServer to stop listening; in future it should be able to
   pass in some other way to stop the DBusServer
* no ability to virtualize the machine ID, do we want that?
* no ability to virtualize GetId(), do we want that?
Comment 1 Simon McVittie 2017-06-23 15:49:48 UTC
After a few tries at how the data structures and the order of implementation should look, I have an implementation of this. "Only" 49 commits - I'll split them up a bit into blocking bugs.

I haven't tested it "in real life", and I should check the test coverage with gcov, but I think the branch is ready for a first round of reviews.

My next step is to put a proof-of-concept implementation in Flatpak's D-Bus proxy, so that the D-Bus proxy is still responsible for filtering (and ensuring that the process has /proc/$pid/root/.flatpak-info for the older, more race-prone detection mechanism), but dbus-daemon has the opportunity to provide identification services.

(In reply to Simon McVittie from comment #0)
>     * Watch for the calling connection to disconnect, and if it does,
>       disconnect the DBusServer

We also need a method to stop listening explicitly. I added two: one which just stops, and one which also kills all the connections.

> * Resource limit: make each new DBusServer "cost" one connection
>   in the connections-per-uid quota

I decided against this. The limits I've implemented are:
max_containers
max_container_metadata_bytes
max_containers_per_user
max_connections_per_container
Comment 2 Simon McVittie 2017-06-23 16:17:34 UTC
Created attachment 132165 [details] [review]
[01] _dbus_asv_add_object_path: Add
Comment 3 Simon McVittie 2017-06-23 16:24:09 UTC
Created attachment 132167 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules
Comment 4 Simon McVittie 2017-06-23 16:24:57 UTC
Comment on attachment 132167 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules

Moved to Bug #101567
Comment 5 Simon McVittie 2017-06-23 16:25:17 UTC
Created attachment 132169 [details] [review]
[02] spec: Document the initial Containers1 interface
Comment 6 Simon McVittie 2017-06-23 16:25:33 UTC
Created attachment 132170 [details] [review]
[03] driver: Add a stub implementation of the Containers1  interface

For now, this is considered to be a privileged operation, because the
resource-limiting isn't wired up yet. It only contains the bare minimum
of API.
Comment 7 Simon McVittie 2017-06-23 16:26:12 UTC
Created attachment 132171 [details] [review]
[04] test/uid-permissions: Assert that AddContainerServer is  privileged

---

I'm not sure whether I've actually tested this as root yet.
Comment 8 Simon McVittie 2017-06-23 16:26:30 UTC
Created attachment 132172 [details] [review]
[05] test/containers: New test

So far it only exercises SupportedArguments.
Comment 9 Simon McVittie 2017-06-23 16:26:52 UTC
Created attachment 132173 [details] [review]
[07] containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).
Comment 10 Simon McVittie 2017-06-23 16:27:09 UTC
Created attachment 132174 [details] [review]
[08] test/containers: Exercise the new parameter checking
Comment 11 Simon McVittie 2017-06-23 16:27:30 UTC
Created attachment 132175 [details] [review]
[09] bus/containers: Build a global data structure for  container instances

We still don't actually create a DBusServer for incoming connections
at this point, much less accept incoming connections.
Comment 12 Simon McVittie 2017-06-23 16:28:01 UTC
Created attachment 132176 [details] [review]
[10] bus/containers: Create a DBusServer and add it to the  main loop

This means we can accept connections on the new socket. For now, we
don't process them and they get closed.
Comment 13 Simon McVittie 2017-06-23 16:28:18 UTC
Created attachment 132177 [details] [review]
[11] bus_context_add_incoming_connection: factor out
Comment 14 Simon McVittie 2017-06-23 16:28:36 UTC
Created attachment 132178 [details] [review]
[12] bus/containers: Set up new connections to join the bus
Comment 15 Simon McVittie 2017-06-23 16:28:49 UTC
Created attachment 132179 [details] [review]
[13] test/containers: Exercise a successful call to  AddContainerServer
Comment 16 Simon McVittie 2017-06-23 16:29:10 UTC
Created attachment 132180 [details] [review]
[14] bus/containers: Require connecting uid to match caller  of AddContainerServer

If we're strict now, we can relax this later (either with a named
parameter or always); but if we're lenient now, we'll be stuck with it
forever, so be strict.
Comment 17 Simon McVittie 2017-06-23 16:29:30 UTC
Created attachment 132181 [details] [review]
[15] bus/containers: Each connection to a container holds a  reference
Comment 18 Simon McVittie 2017-06-23 16:30:09 UTC
Created attachment 132182 [details] [review]
[16] bus/containers: Link each container to its initiating  connection

We will need this to be able to shut down the container when its
creator vanishes.
Comment 19 Simon McVittie 2017-06-23 16:30:43 UTC
Created attachment 132183 [details] [review]
[17] bus/containers: Shut down container servers when  initiator goes away

We will eventually want to have other ways to signal that a
container server should stop listening, so that the container manager
doesn't have to stay on D-Bus (fd-passing the read end of a pipe
whose write end will be closed by the container manager has been
suggested as easier to deal with for Flatpak/Bubblewrap), but for
now we're doing the simplest possible thing.
Comment 20 Simon McVittie 2017-06-23 16:31:01 UTC
Created attachment 132184 [details] [review]
[18] bus/containers: Give each instance a list of all its  connections
Comment 21 Simon McVittie 2017-06-23 16:31:48 UTC
Created attachment 132185 [details] [review]
[19] bus/containers: Implement methods to stop containers  explicitly

---

We don't really *need* these, but they follow the rule of thumb "if something can be gc'd by falling off the bus, then it should also be possible to remove it early".
Comment 22 Simon McVittie 2017-06-23 16:32:16 UTC
Created attachment 132186 [details] [review]
[20] bus/containers: Don't allow stopping other users'  containers

On the system bus, that would be a denial of service, assuming we
relax the access-control from METHOD_FLAG_PRIVILEGED to a new
METHOD_FLAG_NOT_CONTAINERS later.
Comment 23 Simon McVittie 2017-06-23 16:32:28 UTC
Created attachment 132187 [details] [review]
[21] test/containers: Exercise the various ways to stop a  container
Comment 24 Simon McVittie 2017-06-23 16:32:44 UTC
Created attachment 132188 [details] [review]
[22] bus/containers: Emit ContainerInstanceRemoved signal
Comment 25 Simon McVittie 2017-06-23 16:32:59 UTC
Created attachment 132189 [details] [review]
[23] test/containers: Assert that ContainerInstanceRemoved  is emitted
Comment 26 Simon McVittie 2017-06-23 16:33:13 UTC
Created attachment 132190 [details] [review]
[24] bus/containers: Indicate in loginfo whether connection  is contained
Comment 27 Simon McVittie 2017-06-23 16:33:37 UTC
Created attachment 132191 [details] [review]
[25] bus/driver: Treat connections from inside containers as  unprivileged

Even if the uid matches, a contained app shouldn't count as the owner
of the bus.
Comment 28 Simon McVittie 2017-06-23 16:35:17 UTC
Created attachment 132192 [details] [review]
[26] test/containers: Check that containers can't make new  containers

We should prevent containers from trying to putting a container in our
container so we can contain while we contain; the implementation doesn't
actually have any concept of nesting or layering, so that would potentially
be privilege escalation.

At the moment, this is just prevented by METHOD_FLAG_PRIVILEGED. If we
remove that flag when we've introduced better resource limits, then
we can specifically restrict this method to not be called by
containers instead: this test will make sure we do.
Comment 29 Simon McVittie 2017-06-23 16:36:45 UTC
Created attachment 132193 [details] [review]
[27] test/containers: Check that connections from containers  are unprivileged

---

I hope the difference between

                                       g_variant_new ("(a{ss})", NULL),

and

  dbus_message_iter_init_append (m, &args_iter);

  /* Append an empty a{ss} (string => string dictionary). */
  if (!dbus_message_iter_open_container (&args_iter, DBUS_TYPE_ARRAY,
        "{ss}", &arr_iter) ||
      !dbus_message_iter_close_container (&args_iter, &arr_iter))
    g_error ("OOM");

illustrates why I'm using GVariant for this test :-)
Comment 30 Simon McVittie 2017-06-23 16:37:05 UTC
Created attachment 132194 [details] [review]
[28] bus/driver: Add a flag for methods that can't be  invoked by containers

We can consider relaxing AddContainerServer() from PRIVILEGED to
NOT_CONTAINERS when we've put resource limits in place.
Comment 31 Simon McVittie 2017-06-23 16:38:01 UTC
Created attachment 132195 [details] [review]
[29] bus/driver: Containers can't use the Verbose and Stats  interfaces

These are debugging interfaces, which are essentially read-only.
By default, Verbose is not available on the system bus at all and
Stats is only available to uid 0, but both are available on the
session bus, and they can be allowed for other uids by configuring
the system bus.

---

We document how to enable Stats on the system bus, so I didn't want to just make it METHOD_FLAG_PRIVILEGED.
Comment 32 Simon McVittie 2017-06-23 16:38:15 UTC
Created attachment 132196 [details] [review]
[30] bus/driver: Add basic container info to  GetConnectionCredentials()
Comment 33 Simon McVittie 2017-06-23 16:38:32 UTC
Created attachment 132197 [details] [review]
[31] test/dbus-daemon: Assert absence of Containers1  credentials

These connections are not to a container server.
Comment 34 Simon McVittie 2017-06-23 16:39:00 UTC
Created attachment 132198 [details] [review]
[32] bus/driver: Add GetConnectionContainerInstance(),  GetContainerInstanceInfo()

---

I am not particularly happy with the length of these names.
Comment 35 Simon McVittie 2017-06-23 16:39:31 UTC
Created attachment 132199 [details] [review]
test/containers: Exercise trivial and non-trivial  container metadata
Comment 36 Simon McVittie 2017-06-23 16:39:49 UTC
Created attachment 132200 [details] [review]
[34] test/containers: Check that GetContainerInstanceInfo  stops working

After the container instance is removed, the method should not work.
Comment 37 Simon McVittie 2017-06-23 16:40:08 UTC
Created attachment 132201 [details] [review]
[35] bus: Add (unused) settings for resource limits for  containers
Comment 38 Simon McVittie 2017-06-23 16:40:23 UTC
Created attachment 132202 [details] [review]
[36] bus/containers: Limit the size of metadata we will  store
Comment 39 Simon McVittie 2017-06-23 16:40:39 UTC
Created attachment 132203 [details] [review]
[37] bus/containers: Enforce max_containers limit
Comment 40 Simon McVittie 2017-06-23 16:40:53 UTC
Created attachment 132204 [details] [review]
[38] bus/containers: Enforce max_connections_per_container
Comment 41 Simon McVittie 2017-06-23 16:41:07 UTC
Created attachment 132205 [details] [review]
[39] containers: enforce max_containers_per_user
Comment 42 Simon McVittie 2017-06-23 16:41:23 UTC
Created attachment 132206 [details] [review]
[40] test/containers: Exercise the resource limits
Comment 43 Simon McVittie 2017-06-23 16:41:44 UTC
Created attachment 132207 [details] [review]
bus/driver: Allow unprivileged connections to create  app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.
Comment 44 Simon McVittie 2017-06-23 16:43:23 UTC
Also available at: https://github.com/smcv/dbus containers-minimum-101354
Comment 45 Simon McVittie 2017-06-23 16:44:50 UTC
Comment on attachment 132196 [details] [review]
[30] bus/driver: Add basic container info to  GetConnectionCredentials()

Review of attachment 132196 [details] [review]:
-----------------------------------------------------------------

This should also be described in the commit that documents this stuff in the spec, of course.

I'm using the fully qualified (long) name to set a good example for others.
Comment 46 Simon McVittie 2017-06-30 11:03:00 UTC
(In reply to Simon McVittie from comment #7)
> test/uid-permissions: Assert that AddContainerServer is  privileged

This needs to be reverted before applying Attachment #132207 [details], or not applied at all.
Comment 47 Simon McVittie 2017-06-30 11:52:24 UTC
Created attachment 132367 [details] [review]
[02] spec: Document the initial Containers1 interface

---
Document the new keys for o.fd.DBus.GetConnectionCredentials() too
Comment 48 Simon McVittie 2017-06-30 11:54:25 UTC
Created attachment 132368 [details] [review]
[33] test/containers: Exercise trivial and non-trivial  container metadata

---

Don't rely on GLib having G_DBUS_ERROR_UNKNOWN_INTERFACE. That error is new in 2.42, but we build against 2.40 on travis-ci. Match the D-Bus error name instead, like we already did for the new NOT_CONTAINER error from this branch (which won't be in GLib for a while).
Comment 49 Simon McVittie 2017-06-30 11:56:29 UTC
Created attachment 132369 [details] [review]
[41] Revert "test/uid-permissions: Assert that  AddContainerServer is privileged"

I'm about to make that not be true.

This reverts Attachment #132171 [details].

---

When pushing the branch I might rebase both these two out of existence, but it seems valuable to have while developing, so I can assert that the method is correctly restricted until the point where I deliberately de-restrict it.
Comment 50 Simon McVittie 2017-06-30 11:57:04 UTC
Created attachment 132370 [details] [review]
[42] bus/driver: Allow unprivileged connections to create  app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.
Comment 51 Simon McVittie 2017-06-30 12:02:08 UTC
Open issues on which I would appreciate comment:

* The names are all too long. Can we make them shorter without ambiguity?

* We represent container instances as (type, name, metadata) tuples,
  where the type (Flatpak or Snap or whatever) defines the other two.
  Do we also need to include the uid of the creator of the container
  in the tuple? On the system bus, the type is only as trustworthy as the
  creator of the container, and the name is only as trustworthy as the type.
Comment 52 Simon McVittie 2017-06-30 13:37:45 UTC
Created attachment 132373 [details] [review]
[33] test/containers: Exercise trivial and non-trivial  container metadata

---
Fix failure to build without --enable-containers
Comment 53 Simon McVittie 2017-06-30 19:02:02 UTC
Comment on attachment 132176 [details] [review]
[10] bus/containers: Create a DBusServer and add it to the  main loop

Review of attachment 132176 [details] [review]:
-----------------------------------------------------------------

::: bus/containers.c
@@ +87,5 @@
> +  _dbus_string_init_const (&self->address_template, "");
> +
> +  if (_dbus_getuid () == 0)
> +    {
> +      _dbus_string_init_const (&dir, DBUS_RUNSTATEDIR);

The system bus case (here) is wrong: we should be using ${runstatedir}/dbus/containers, as set up by the tmpfiles snippet. Trying to listen on /run/dbus-XXXXXX is clearly not going to work after we have dropped privileges to messagebus, but listening on /run/dbus/containers/dbus-XXXXXX can.

I should also adapt the regression test to be runnable as root, partly so we can test this code path and partly so we can assert that connecting to someone else's container's socket is rejected.
Comment 54 Philip Withnall 2017-07-06 10:37:25 UTC
(In reply to Simon McVittie from comment #51)
> Open issues on which I would appreciate comment:
> 
> * The names are all too long. Can we make them shorter without ambiguity?

Skimming through the list of patches, I can’t see any names which look too long. Could you be more specific about which names? (Give me patch numbers, since I haven’t looked at any of the patches in detail yet, since you haven’t asked for review. And I’m scared of a 33-change patchset.)

> * We represent container instances as (type, name, metadata) tuples,
>   where the type (Flatpak or Snap or whatever) defines the other two.
>   Do we also need to include the uid of the creator of the container
>   in the tuple? On the system bus, the type is only as trustworthy as the
>   creator of the container, and the name is only as trustworthy as the type.

What do you mean by ‘trustworthy’ here? As in, if we think a particular UID is ‘untrustworthy’, does that mean they could create a snap container but label it as flatpak? Could you point me to the APIs which are relevant here (like, the API which creates these tuples)?

My initial feeling (based purely on your question and not reading any code) is that hard-coding a representation of the UID is a bit limiting, and we might end up in the GetConnectionUnixUser vs GetConnectionCredentials case again. Might be best to advertise an a{sv} of credentials from the outset.
Comment 55 Simon McVittie 2017-07-06 11:31:01 UTC
(In reply to Philip Withnall from comment #54)
> Skimming through the list of patches, I can’t see any names which look too
> long. Could you be more specific about which names?

Attachment #132367 [details] (the additions to the spec) is probably the best overview.

I think the root cause of the long names is the use of the term "container instance" for the thing created by AddContainerServer(). Perhaps we should use "container" as a shorthand in APIs, but "container instance" in text documentation?

> What do you mean by ‘trustworthy’ here? As in, if we think a particular UID
> is ‘untrustworthy’, does that mean they could create a snap container but
> label it as flatpak?

Yes. Some random user (uid 1000, say) could create a private DBusServer and label it as (org.flatpak, org.gnome.Weather), but actually use it for a Snap container that is running gnome-maps, or for no container at all. System services that accept requests from that server (let's say Geoclue) would believe the request came from (uid 1000, container technology Flatpak, app org.gnome.Weather) when in fact it is from (uid 1000, container technology Snap, app ID gnome-maps) or from (uid 1000, not a container, app ID n/a).

> My initial feeling (based purely on your question and not reading any code)
> is that hard-coding a representation of the UID is a bit limiting, and we
> might end up in the GetConnectionUnixUser vs GetConnectionCredentials case
> again. Might be best to advertise an a{sv} of credentials from the outset.

Hmm, perhaps. Now that we have DBusVariant technology, we can store whatever details we want to, even after the initiating connection has closed.
Comment 56 Simon McVittie 2017-07-06 18:15:57 UTC
(In reply to Philip Withnall from comment #54)
> And I’m scared of a 33-change patchset.

Actually it's a 42-change patchset, which became 45 when I added test coverage for trying to connect to someone else's container server (currently only on my github repo, and needed a bit of refactoring of test utilities), and is about to become 46 with coverage for trying to stop someone else's container.
Comment 57 Simon McVittie 2017-07-06 18:17:05 UTC
(In reply to Simon McVittie from comment #56)
> and is about to become 46

Or not, I might just add the new test coverage to an existing commit in an attempt to avoid potential reviewers running away.
Comment 58 Philip Withnall 2017-07-06 18:32:39 UTC
(In reply to Simon McVittie from comment #57)
> (In reply to Simon McVittie from comment #56)
> > and is about to become 46
> 
> Or not, I might just add the new test coverage to an existing commit in an
> attempt to avoid potential reviewers running away.

🙈
Comment 59 Simon McVittie 2017-07-06 18:49:55 UTC
(In reply to Simon McVittie from comment #53)
> The system bus case (here) is wrong: we should be using
> ${runstatedir}/dbus/containers, as set up by the tmpfiles snippet. Trying to
> listen on /run/dbus-XXXXXX is clearly not going to work after we have
> dropped privileges to messagebus, but listening on
> /run/dbus/containers/dbus-XXXXXX can.

Fixed on my github branch. I'm not going to reattach all 45 patches until I've done a bit more testing tomorrow (including rechecking the code coverage, and retrying it against <https://github.com/flatpak/flatpak/pull/890>) but you can have a look at it on github if you want a preview.

If you would prefer to review it in fewer/larger patches, that can be arranged, but I assumed you'd prefer them somewhat bite-sized. There's a limit to what I can do with ~ 4k lines of diff (1394 of implementation, 450 of spec, 1533 of tests, plus misc changes to glue it into existing code).

I think what's here is the minimum that I would be prepared to merge as a unit - once we have started implementing Containers, we need to advance at least this far, otherwise we'll have something useless or insecure.

Limitations to be lifted in separate patch serieses:

* No message filtering, name-ownership filtering, etc. For now this
  feature just provides identity, not authorization.

* For now it's dbus-daemon that bind()s and listen()s on the socket.
  In future it should be able to pass in a socket that has not had
  bind() or listen() called on it yet, together with some way to tell
  the dbus-daemon when the socket is ready to be accept()ed. But that's
  a bit of a fd-passing can of worms because we need to make sure the
  caller can't DoS the bus by lying about the socket's state and causing
  the dbus-daemon to busy-loop.

* The caller (container manager) has to stay on the bus until it's ready
  for the DBusServer to stop listening; in future it should be able to
  pass in some other way to stop the DBusServer, like the read end
  of a pipe whose write end will close later, and allow the container
  manager to go away without affecting those servers.

* No ability to virtualize the machine ID, do we want that?

* No ability to virtualize GetId(), do we want that?
Comment 60 Simon McVittie 2017-07-07 18:52:50 UTC
Created attachment 132508 [details] [review]
_dbus_asv_add_object_path: Add

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 61 Simon McVittie 2017-07-07 18:53:05 UTC
Created attachment 132509 [details] [review]
spec: Document the initial Containers1 interface

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 62 Simon McVittie 2017-07-07 18:53:16 UTC
Created attachment 132510 [details] [review]
driver: Add a stub implementation of the Containers1 interface

For now, this is considered to be a privileged operation, because the
resource-limiting isn't wired up yet. It only contains the bare minimum
of API.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 63 Simon McVittie 2017-07-07 18:53:41 UTC
Created attachment 132511 [details] [review]
test/uid-permissions: Assert that AddContainerServer is privileged

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 64 Simon McVittie 2017-07-07 18:53:56 UTC
Created attachment 132512 [details] [review]
test/containers: New test

So far it only exercises SupportedArguments.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 65 Simon McVittie 2017-07-07 18:54:08 UTC
Created attachment 132513 [details] [review]
containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 66 Simon McVittie 2017-07-07 18:54:28 UTC
Created attachment 132514 [details] [review]
test/containers: Exercise the new parameter checking

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 67 Simon McVittie 2017-07-07 18:54:39 UTC
Created attachment 132515 [details] [review]
bus/containers: Build a global data structure for container instances

We still don't actually create a DBusServer for incoming connections
at this point, much less accept incoming connections.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 68 Simon McVittie 2017-07-07 18:55:34 UTC
Created attachment 132516 [details] [review]
bus/containers: Create a DBusServer and add it to the main loop

This means we can accept connections on the new socket. For now, we
don't process them and they get closed.

For the system bus (or root's session bus, where the difference is
harmless but makes automated testing easier), rely on system-wide
infrastructure to create /run/dbus/containers. At the moment this
is only done for systemd systems via tmpfiles.d; I'd accept a tested
patch for our Red Hat, Slackware and Cygwin init scripts to do the
same, but I suspect they might actually be unused and unmaintained
(see Bug #101706).

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
Create sockets for system bus (or root) in /run/dbus/containers,
not /run. Rely on the init script or OS infrastructure to create
that directory and let messagebus write there; if it doesn't,
listening will just fail.

Create other sockets in $XDG_RUNTIME_DIR/dbus-1/containers, not
$XDG_RUNTIME_DIR. Defer setting that up until just before we listen,
so we can report any errors encountered while creating those
directories.

Tear down the listening sockets while the dbus-daemon is shutting down.
Comment 69 Simon McVittie 2017-07-07 18:55:56 UTC
Created attachment 132517 [details] [review]
bus_context_add_incoming_connection: factor out

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 70 Simon McVittie 2017-07-07 18:56:06 UTC
Created attachment 132518 [details] [review]
bus/containers: Set up new connections to join the bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 71 Simon McVittie 2017-07-07 18:56:17 UTC
Created attachment 132519 [details] [review]
test/containers: Exercise a successful call to AddContainerServer

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
Don't fail if we are running uninstalled as root and the directory
doesn't exist yet.

Factor out add_container_server() so we can use it in subsequent tests.

Assert that the socket is cleaned up properly when the dbus-daemon is
SIGTERM'd. (This was wrong in the initial implementation.)
Comment 72 Simon McVittie 2017-07-07 18:56:28 UTC
Created attachment 132520 [details] [review]
bus/containers: Require connecting uid to match caller of AddContainerServer

If we're strict now, we can relax this later (either with a named
parameter or always); but if we're lenient now, we'll be stuck with it
forever, so be strict.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 73 Simon McVittie 2017-07-07 18:56:39 UTC
Created attachment 132521 [details] [review]
test-utils-glib: Factor out functions for switching uid

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 74 Simon McVittie 2017-07-07 18:56:52 UTC
Created attachment 132522 [details] [review]
test-utils-glib: Add function to connect with GDBus as another uid

This will be used in a test for connecting to container servers
as the wrong uid.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 75 Simon McVittie 2017-07-07 18:57:10 UTC
Created attachment 132523 [details] [review]
test/containers: Exercise connecting to the new socket as the wrong uid

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 76 Simon McVittie 2017-07-07 18:57:28 UTC
Created attachment 132524 [details] [review]
bus/containers: Each connection to a container holds a reference

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 77 Simon McVittie 2017-07-07 18:57:39 UTC
Created attachment 132525 [details] [review]
bus/containers: Link each container to its initiating connection

We will need this to be able to shut down the container when its
creator vanishes.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 78 Simon McVittie 2017-07-07 18:57:48 UTC
Created attachment 132526 [details] [review]
bus/containers: Shut down container servers when initiator goes away

We will eventually want to have other ways to signal that a
container server should stop listening, so that the container manager
doesn't have to stay on D-Bus (fd-passing the read end of a pipe
whose write end will be closed by the container manager has been
suggested as easier to deal with for Flatpak/Bubblewrap), but for
now we're doing the simplest possible thing.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 79 Simon McVittie 2017-07-07 18:57:57 UTC
Created attachment 132527 [details] [review]
bus/containers: Give each instance a list of all its connections

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 80 Simon McVittie 2017-07-07 18:58:06 UTC
Created attachment 132528 [details] [review]
bus/containers: Implement methods to stop containers explicitly

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 81 Simon McVittie 2017-07-07 18:58:57 UTC
Created attachment 132531 [details] [review]
bus/containers: Don't allow stopping other users' containers

On the system bus, that would be a denial of service, assuming we
relax the access-control from METHOD_FLAG_PRIVILEGED to a new
METHOD_FLAG_NOT_CONTAINERS later.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 82 Simon McVittie 2017-07-07 18:59:16 UTC
Created attachment 132532 [details] [review]
test/containers: Exercise the various ways to stop a container

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
Use add_container_server(). Assert that another uid cannot disconnect us.
Comment 83 Simon McVittie 2017-07-07 18:59:37 UTC
Created attachment 132534 [details] [review]
bus/containers: Emit ContainerInstanceRemoved signal

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 84 Simon McVittie 2017-07-07 18:59:53 UTC
Created attachment 132535 [details] [review]
test/containers: Assert that ContainerInstanceRemoved is emitted

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 85 Simon McVittie 2017-07-07 19:00:02 UTC
Created attachment 132536 [details] [review]
bus/containers: Indicate in loginfo whether connection is contained

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 86 Simon McVittie 2017-07-07 19:00:12 UTC
Created attachment 132537 [details] [review]
bus/driver: Treat connections from inside containers as unprivileged

Even if the uid matches, a contained app shouldn't count as the owner
of the bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 87 Simon McVittie 2017-07-07 19:00:25 UTC
Created attachment 132538 [details] [review]
test/containers: Check that containers can't make new containers

We should prevent containers from trying to putting a container in our
container so we can contain while we contain; the implementation doesn't
actually have any concept of nesting or layering, so that would potentially
be privilege escalation.

At the moment, this is just prevented by METHOD_FLAG_PRIVILEGED. If we
remove that flag when we've introduced better resource limits, then
we can specifically restrict this method to not be called by
containers instead: this test will make sure we do.

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
Use refactored add_container_server()
Comment 88 Simon McVittie 2017-07-07 19:00:37 UTC
Created attachment 132539 [details] [review]
test/containers: Check that connections from containers are unprivileged

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 89 Simon McVittie 2017-07-07 19:00:48 UTC
Created attachment 132540 [details] [review]
bus/driver: Add a flag for methods that can't be invoked by containers

We can consider relaxing AddContainerServer() from PRIVILEGED to
NOT_CONTAINERS when we've put resource limits in place.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 90 Simon McVittie 2017-07-07 19:01:02 UTC
Created attachment 132541 [details] [review]
bus/driver: Containers can't use the Verbose and Stats interfaces

These are debugging interfaces, which are essentially read-only.
By default, Verbose is not available on the system bus at all and
Stats is only available to uid 0, but both are available on the
session bus, and they can be allowed for other uids by configuring
the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 91 Simon McVittie 2017-07-07 19:01:14 UTC
Created attachment 132542 [details] [review]
bus/driver: Add basic container info to GetConnectionCredentials()

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 92 Simon McVittie 2017-07-07 19:01:25 UTC
Created attachment 132543 [details] [review]
test/dbus-daemon: Assert absence of Containers1 credentials

These connections are not to a container server.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 93 Simon McVittie 2017-07-07 19:01:37 UTC
Created attachment 132544 [details] [review]
bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo()

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 94 Simon McVittie 2017-07-07 19:01:54 UTC
Created attachment 132545 [details] [review]
t/containers: Exercise trivial and non-trivial container metadata

Signed-off-by: Simon McVittie <smcv@collabora.com>
---
Use factored-out add_container_server()
Comment 95 Simon McVittie 2017-07-07 19:02:06 UTC
Created attachment 132546 [details] [review]
test/containers: Check that GetContainerInstanceInfo stops working

After the container instance is removed, the method should not work.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 96 Simon McVittie 2017-07-07 19:02:17 UTC
Created attachment 132547 [details] [review]
bus: Add (unused) settings for resource limits for containers

These will be enforced in subsequent commits.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 97 Simon McVittie 2017-07-07 19:02:27 UTC
Created attachment 132548 [details] [review]
bus/containers: Limit the size of metadata we will store

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 98 Simon McVittie 2017-07-07 19:02:37 UTC
Created attachment 132549 [details] [review]
bus/containers: Enforce max_containers limit

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 99 Simon McVittie 2017-07-07 19:02:49 UTC
Created attachment 132550 [details] [review]
bus/containers: Enforce max_connections_per_container

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 100 Simon McVittie 2017-07-07 19:02:59 UTC
Created attachment 132551 [details] [review]
containers: enforce max_containers_per_user

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 101 Simon McVittie 2017-07-07 19:03:17 UTC
Created attachment 132552 [details] [review]
test/containers: Exercise the resource limits

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 102 Simon McVittie 2017-07-07 19:03:31 UTC
Created attachment 132553 [details] [review]
Revert "test/uid-permissions: Assert that AddContainerServer is privileged"

I'm about to make that not be true.

This reverts commit 80c7e13cb500c7ffea1b6adff6664c3e36c496f3.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 103 Simon McVittie 2017-07-07 19:03:43 UTC
Created attachment 132554 [details] [review]
bus/driver: Allow unprivileged connections to create app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 104 Simon McVittie 2017-07-07 19:04:01 UTC
Created attachment 132555 [details] [review]
system.conf: Allow creating containers on the system bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 105 Simon McVittie 2017-07-07 19:07:40 UTC
Comment on attachment 132545 [details] [review]
t/containers: Exercise trivial and non-trivial container metadata

This somehow picked up the wrong commit message during a rebase. It should say:

"""
t/containers: Exercise trivial and non-trivial container metadata
"""

Fixed locally and on github, I'm not going to reattach the whole pile.
Comment 106 Philip Withnall 2017-07-11 09:03:03 UTC
Comment on attachment 132508 [details] [review]
_dbus_asv_add_object_path: Add

Review of attachment 132508 [details] [review]:
-----------------------------------------------------------------

++
Comment 107 Philip Withnall 2017-07-11 10:07:31 UTC
Comment on attachment 132509 [details] [review]
spec: Document the initial Containers1 interface

Review of attachment 132509 [details] [review]:
-----------------------------------------------------------------

I reserve the right to come back to this patch and make further comments after I’ve read the subsequent patches and got some experience with the APIs.

Looks good so far. Various nitpicks.

::: doc/dbus-specification.xml
@@ +6447,5 @@
> +                  The contained app name for the container instance,
> +                  as passed to <literal>AddContainerServer</literal>. Omitted
> +                  from the dictionary if this connection is not via a
> +                  container's server. For example, a typical value for
> +                  a Flatpak app would be <literal>org.gnome.Weather</literal>.

s/app/application/? Probably best to be consistent in use of terminology about apps/applications/bundles/flatpaks.

@@ +6713,5 @@
> +                  <entry>
> +                    Reversed domain name identifying a container manager or
> +                    container technology, such as
> +                    <literal>org.flatpak</literal> or
> +                    <literal>io.snapcraft</literal>

Nitpick: Other argument descriptions in this table end in a full stop. Probably best for them to all do so, for consistency.

@@ +6794,5 @@
> +          <firstterm>container instance</firstterm>, identified by an object
> +          path. This is true even if the specified container type and name
> +          are the same as for a pre-existing container instance, because the
> +          new container instance might represent a different version of the
> +          confined application with different characteristics and restrictions.

So a single object path can refer to multiple containers (different versions of the same app)?

@@ +6802,5 @@
> +
> +        <para>
> +          Metadata from the first three arguments is stored by the message
> +          bus, but not interpreted; software that interacts with the
> +          container manager can use this metadata. The message bus may limit

Should we point out that this data is not to be trusted then? Or should only be trusted as much as clients trust the container manager?

@@ +6803,5 @@
> +        <para>
> +          Metadata from the first three arguments is stored by the message
> +          bus, but not interpreted; software that interacts with the
> +          container manager can use this metadata. The message bus may limit
> +          the size of the metadata that it is willing to store.

What happens if the bus limits the size? Presumably it rejects the AddContainerServer call, rather than truncating the metadata?

@@ +6814,5 @@
> +          containing <literal>.</literal> are reserved by this specification
> +          for this purpose. It can also contain implementation-specific
> +          arguments for a particular message bus implementation, which
> +          should start with a reversed domain name in the same way as
> +          interface names.

Do we want to put in rules about capitalisation after the reverse-DNS name? e.g. org.flatpak.MyNewKey vs org.flatpak.my-new-key vs org.flatpak.mynewkey.

@@ +6857,5 @@
> +        <title><literal>org.freedesktop.DBus.Containers1.StopContainerInstance</literal></title>
> +        <para>
> +          As a method:
> +          <programlisting>
> +            StopContainerInstance (in OBJECT_PATH container_instance)

Suggestion: to make the method names shorter, you could drop ‘Container’, since it’s already in the interface name. So, org.freedesktop.DBus.Containers1.StopInstance.

@@ +6875,5 @@
> +                  <entry>0</entry>
> +                  <entry>OBJECT_PATH</entry>
> +                  <entry>
> +                    The opaque object path that was returned from the
> +                    AddContainerServer method, identifying a container instance

Nitpick: Same nitpick about full stops.

Also, nitpick: Add <literal> around AddContainerServer.

@@ +6892,5 @@
> +          <literal>org.freedesktop.DBus.Error.NotContainer</literal> error
> +          is returned. If the given container instance exists but
> +          the caller of this method is not allowed to stop it (for example
> +          because the caller is in an app-container or because their user
> +          ID does not match the user ID of the creator of the app-container),

Nitpick: ‘app-container’ is a new term. Is this what you want to standardise on for app/container/flatpak/bundle/bonghits?

@@ +6921,5 @@
> +                  <entry>0</entry>
> +                  <entry>OBJECT_PATH</entry>
> +                  <entry>
> +                    The opaque object path that was returned from the
> +                    AddContainerServer method, identifying a container instance

Nitpick: Add <literal> around AddContainerServer.

@@ +6935,5 @@
> +          container instance, but do not disconnect any existing connections.
> +          If the given object path does not represent an active container
> +          instance, the
> +          <literal>org.freedesktop.DBus.Error.NotContainer</literal> error
> +          is returned. If the given container instance exists but

Does that mean NotContainer is returned if I were to call StopContainerListening twice in a row on the same container instance?

@@ +6971,5 @@
> +                  <entry>0</entry>
> +                  <entry>STRING</entry>
> +                  <entry>Unique or well-known bus name of the connection to
> +                    query, such as <literal>:12.34</literal> or
> +                    <literal>com.example.tea</literal></entry>

Nitpick: Same nitpick about full stops.

@@ +6978,5 @@
> +                  <entry>1</entry>
> +                  <entry>OBJECT_PATH</entry>
> +                  <entry>
> +                    The opaque object path that was returned from the
> +                    AddContainerServer method, identifying a container instance

Nitpick: <literal> around AddContainerServer.

@@ +6997,5 @@
> +                  <entry>STRING</entry>
> +                  <entry>
> +                    Some unique identifier for an application or container,
> +                    whose meaning is defined by the maintainers of the
> +                    container type

Do we want to require that it’s non-empty?

@@ +7052,5 @@
> +                  <entry>0</entry>
> +                  <entry>OBJECT_PATH</entry>
> +                  <entry>
> +                    The opaque object path that was returned from the
> +                    AddContainerServer method, identifying a container instance

Nitpick: <literal>, full stops.
Comment 108 Philip Withnall 2017-07-11 10:14:39 UTC
Comment on attachment 132510 [details] [review]
driver: Add a stub implementation of the Containers1 interface

Review of attachment 132510 [details] [review]:
-----------------------------------------------------------------

A couple of questions/minor nitpicks.

::: bus/containers.c
@@ +51,5 @@
> +                                           &arr_iter) &&
> +         dbus_message_iter_close_container (var_iter, &arr_iter);
> +}
> +
> +#endif /* HAVE_UNIX_FD_PASSING */

s/HAVE_UNIX_FD_PASSING/DBUS_ENABLE_CONTAINERS/

::: bus/containers.h
@@ +30,5 @@
> +                                                        DBusMessage    *message,
> +                                                        DBusError      *error);
> +
> +dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context,
> +                                                       DBusMessageIter *var_iter);

Nitpick: These arguments aren’t lined up like the others are.

::: configure.ac
@@ +1876,5 @@
>  
> +AC_ARG_ENABLE([containers],
> +  [AS_HELP_STRING([--enable-containers],
> +    [enable restricted servers for app containers])],
> +  [], [enable_containers=no])

Shouldn’t the action-if-given argument be set to [enable_containers=$enableval]? The autoconf documentation doesn’t say it has a default action.
Comment 109 Philip Withnall 2017-07-11 10:21:25 UTC
Comment on attachment 132511 [details] [review]
test/uid-permissions: Assert that AddContainerServer is privileged

Review of attachment 132511 [details] [review]:
-----------------------------------------------------------------

++

::: test/uid-permissions.c
@@ +233,5 @@
> +      pc == NULL)
> +    g_error ("OOM");
> +
> +  dbus_message_unref (m);
> +  m = NULL;

+1 for adding a static inline convenience dbus_clear_message(DBusMessage **to_die);
Comment 110 Simon McVittie 2017-07-11 13:36:38 UTC
(In reply to Philip Withnall from comment #107)
> So a single object path can refer to multiple containers (different versions
> of the same app)?

No, I must have been unclear. It's the other way round. Each AddContainerServer() call yields a new container instance identified by a unique object path, so you could easily have two container instances both of which represent the pair (org.flatpak, org.gnome.Weather) (with either the same or different versions).

> Should we point out that this data is not to be trusted then? Or should only
> be trusted as much as clients trust the container manager?

Yes, and this is why I was talking about maybe recording the initiator's uid: on the system bus, if uid 1000 said container /.../c23 is (org.flatpak, org.gnome.Weather), then uid 1001 can only trust that statement to the extent that it trusts uid 1000. (However, as implemented, connections to container servers can't make new container servers, so it's trusted as much as *unconfined* uid 1000 is trusted.)

> What happens if the bus limits the size? Presumably it rejects the
> AddContainerServer call, rather than truncating the metadata?

Yes.

> Do we want to put in rules about capitalisation after the reverse-DNS name?
> e.g. org.flatpak.MyNewKey vs org.flatpak.my-new-key vs org.flatpak.mynewkey.

More like com.github.Bus1.TheirNewKey or org.gnome.GDBus.OtherNewKey rather than Flatpak. I don't think we particularly need to constrain syntax.

> Suggestion: to make the method names shorter, you could drop ‘Container’,
> since it’s already in the interface name. So,
> org.freedesktop.DBus.Containers1.StopInstance.

Yeah, maybe. I've been leaning towards names that are somewhat friendly to deficient client implementations that don't always specify interfaces or make it hard to have the same method name in more than one interface, but perhaps it's time to say enough is enough.

> Nitpick: ‘app-container’ is a new term. Is this what you want to standardise
> on for app/container/flatpak/bundle/bonghits?

I'm not sure. It's a term I've been trying out for that purpose, at least...

> Does that mean NotContainer is returned if I were to call
> StopContainerListening twice in a row on the same container instance?

Good question. As implemented, StopContainerInstance() kills the whole instance (so the second call raises NotContainer), but StopContainerListening() is usually idempotent (it shuts down the listening socket, but pre-established client connections remain up, and the container instance itself remains up until it is not listening and has no connections).

> > +                  <entry>STRING</entry>
> > +                  <entry>
> > +                    Some unique identifier for an application or container,
> > +                    whose meaning is defined by the maintainers of the
> > +                    container type
> 
> Do we want to require that it’s non-empty?

If a container manager has the empty string as a valid container name I don't see why we should forbid it. Also, if a container manager doesn't really have a concept of naming its containers for some reason, I'd assumed it would use the empty string.
Comment 111 Simon McVittie 2017-07-11 13:40:36 UTC
(In reply to Philip Withnall from comment #108)
> s/HAVE_UNIX_FD_PASSING/DBUS_ENABLE_CONTAINERS/

Good point. For context, that's left over from when I thought this was going to be #ifdef HAVE_UNIX_FD_PASSING, because the socket was always fd-passed in from the caller (but there was no separate configure option).

> > +AC_ARG_ENABLE([containers],
> > +  [AS_HELP_STRING([--enable-containers],
> > +    [enable restricted servers for app containers])],
> > +  [], [enable_containers=no])
> 
> Shouldn’t the action-if-given argument be set to
> [enable_containers=$enableval]? The autoconf documentation doesn’t say it
> has a default action.

enableval and enable_containers are already set to the same thing in the IF-GIVEN case. It is documented, although not amazingly clearly:

 -- Macro: AC_ARG_ENABLE (FEATURE, [...])
     [...]
     The option's argument is available to the shell commands
     ACTION-IF-GIVEN in the shell variable 'enableval', which is
     actually just the value of the shell variable named
     'enable_FEATURE', with any non-alphanumeric characters in FEATURE
     changed into '_'.  You may use that variable instead, if you wish.
Comment 112 Philip Withnall 2017-07-11 23:25:48 UTC
(In reply to Simon McVittie from comment #110)
> (In reply to Philip Withnall from comment #107)
> > So a single object path can refer to multiple containers (different versions
> > of the same app)?
> 
> No, I must have been unclear. It's the other way round. Each
> AddContainerServer() call yields a new container instance identified by a
> unique object path, so you could easily have two container instances both of
> which represent the pair (org.flatpak, org.gnome.Weather) (with either the
> same or different versions).

Maybe some rewording would clarify this. How about s/This is true/A unique object path is returned/?

> > Should we point out that this data is not to be trusted then? Or should only
> > be trusted as much as clients trust the container manager?
> 
> Yes, and this is why I was talking about maybe recording the initiator's
> uid: on the system bus, if uid 1000 said container /.../c23 is (org.flatpak,
> org.gnome.Weather), then uid 1001 can only trust that statement to the
> extent that it trusts uid 1000. (However, as implemented, connections to
> container servers can't make new container servers, so it's trusted as much
> as *unconfined* uid 1000 is trusted.)

ack.

> > What happens if the bus limits the size? Presumably it rejects the
> > AddContainerServer call, rather than truncating the metadata?
> 
> Yes.

Probably best to mention this as well as the error code it will return.

> > Do we want to put in rules about capitalisation after the reverse-DNS name?
> > e.g. org.flatpak.MyNewKey vs org.flatpak.my-new-key vs org.flatpak.mynewkey.
> 
> More like com.github.Bus1.TheirNewKey or org.gnome.GDBus.OtherNewKey rather
> than Flatpak. I don't think we particularly need to constrain syntax.

We don’t need to constrain syntax, but it would probably be useful to put an example or two in the spec to guide people towards some consistent names.

> > Suggestion: to make the method names shorter, you could drop ‘Container’,
> > since it’s already in the interface name. So,
> > org.freedesktop.DBus.Containers1.StopInstance.
> 
> Yeah, maybe. I've been leaning towards names that are somewhat friendly to
> deficient client implementations that don't always specify interfaces or
> make it hard to have the same method name in more than one interface, but
> perhaps it's time to say enough is enough.

Enough is enough.

> > Nitpick: ‘app-container’ is a new term. Is this what you want to standardise
> > on for app/container/flatpak/bundle/bonghits?
> 
> I'm not sure. It's a term I've been trying out for that purpose, at least...

I think ‘container’ would be fine; unless you think it will get confused with some other use of the term? Containers aren’t necessarily to be used for ‘apps’.

> > Does that mean NotContainer is returned if I were to call
> > StopContainerListening twice in a row on the same container instance?
> 
> Good question. As implemented, StopContainerInstance() kills the whole
> instance (so the second call raises NotContainer), but
> StopContainerListening() is usually idempotent (it shuts down the listening
> socket, but pre-established client connections remain up, and the container
> instance itself remains up until it is not listening and has no connections).

⇒ Clarify it in the spec.

> > > +                  <entry>STRING</entry>
> > > +                  <entry>
> > > +                    Some unique identifier for an application or container,
> > > +                    whose meaning is defined by the maintainers of the
> > > +                    container type
> > 
> > Do we want to require that it’s non-empty?
> 
> If a container manager has the empty string as a valid container name I
> don't see why we should forbid it. Also, if a container manager doesn't
> really have a concept of naming its containers for some reason, I'd assumed
> it would use the empty string.

Sure. Better make sure there are no assumptions of non-emptiness in the implementation then. (It’s the kind of assumption I might accidentally put into code I write.)
Comment 113 Philip Withnall 2017-07-11 23:29:19 UTC
Comment on attachment 132512 [details] [review]
test/containers: New test

Review of attachment 132512 [details] [review]:
-----------------------------------------------------------------

++
Comment 114 Philip Withnall 2017-07-11 23:35:40 UTC
Comment on attachment 132513 [details] [review]
containers: Do some basic checking on the parameters

Review of attachment 132513 [details] [review]:
-----------------------------------------------------------------

++, with one fix suggested for the spec as a result.

::: bus/containers.c
@@ +52,5 @@
> +
> +  _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING);
> +  dbus_message_iter_get_basic (&iter, &type);
> +
> +  if (!dbus_validate_interface (type, NULL))

The spec says this is a reverse DNS name, but doesn’t actually say that it has to meet the rules of D-Bus interface names. Probably best to mention that in the spec. (I agree that validating the container type as an interface name is reasonable.)

@@ +72,5 @@
> +  if (!dbus_message_iter_next (&iter))
> +    _dbus_assert_not_reached ("Message type was already checked");
> +
> +  _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY);
> +  /* TODO: Copy the metadata */

(I assume this is handled in a subsequent commit.)
Comment 115 Philip Withnall 2017-07-11 23:43:01 UTC
Comment on attachment 132514 [details] [review]
test/containers: Exercise the new parameter checking

Review of attachment 132514 [details] [review]:
-----------------------------------------------------------------

++ with a few maintainability nitpicks. I think the g_steal_pointer() comment is something which I’m going to notice throughout this patch set (I’ve been spoilt by Colin living in a glorious C future in ostree.git), but I’ll try to just mention it this once. Feel free to remedy this comment everywhere or nowhere.

::: test/containers.c
@@ +111,5 @@
>  }
>  
>  static void
> +test_unsupported_parameter (Fixture *f,
> +                            gconstpointer context)

Nitpick: It would be good if each test function had a brief comment above explaining what the test does, at a high level. For example:

/* Test that unsupported named parameters are rejected in AddContainerServer() calls. */
static void
test_unsupported_parameter (…)

@@ +133,5 @@
> +  g_variant_dict_insert (&named_argument_builder,
> +                         "ThisArgumentIsntImplemented",
> +                         "b", FALSE);
> +
> +  /* Floating reference, call_..._sync takes ownership */

Could you just use g_steal_pointer() to make that obvious in the code, instead of needing a comment?

I realise that D-Bus depends on GLib 2.40, and g_steal_pointer() was introduced in 2.44. It would mean a conditional backport of the function, but you might consider that worthwhile. (If going down that route, I’d also consider backporting g_autoptr() [libglnx has a copy if needed], although I’m less sure that it’s portable to all the systems D-Bus is meant to support. I haven’t checked.)

In any case, it would be good to set `parameters = NULL` after the g_dbus_proxy_call_sync() call, to make things a bit more obvious.
Comment 116 Allison Lortie (desrt) 2017-07-12 09:30:37 UTC
Thanks for the rapid work on implementing this Simon.  This already looks pretty usable.

Philip is already handling the code review aspects nicely, so here are some higher level notes:

The "Containers1" style naming thing is sort of distasteful.  If we ever have a second version of this interface (unlikely) then we can perfectly well call that "Containers2" anyway.

The fully-namespaced keys inside of the GetConnectionCredentials return value are silly and wasteful.  It also clashes with the appearance of the other values in this dict.  This data can't be arbitrarily populated by other people, so unnamespaced values are (and continue to be) fine here.

I was sad to notice that the fd-passing style of creation is gone.  I guess you plan to address this via a{sv} fields in the future?  If not, it would be nice to have a slightly more specific method name than AddContainerServer, perhaps mentioning "path" or so.

The same-uid check is a hack and it shows off some of the problems with the create-a-path-in-a-public-directory approach.

The main missing item here in terms of usability is the addition of the header field that we discussed for marking messages from sandboxed apps.  I recall that we discussed that this should be a uint64 for efficiency/simplicity reasons.  The uint64 found at the end of your "opaque object path" is the obvious candidate.  As a reminder, this will allow services to quickly discover if an incoming call (a) is sandboxed (for the cases where you want to deny all sandboxed calls); and (b) if so, if the credentials have already been looked up.  I plan to add some nice API for this on GDBusMethodInvocation.  I even consider disabling all methods by default for container users, unless they are explicitly annotated as being safe (but this has some potential compatibility issues).

Why did you go with the object path approach instead of ints for identifying containers?  Do you imagine a future interface for that object path on the dbus daemon?

Any thoughts on sandboxed apps adding match rules?  It seems like all of the "policy" stuff we were dreaming up during the hackfest is gone, and that's probably for the better, but this is one little corner that still needs to be solved.
Comment 117 Philip Withnall 2017-07-12 11:23:49 UTC
Comment on attachment 132515 [details] [review]
bus/containers: Build a global data structure for container instances

Review of attachment 132515 [details] [review]:
-----------------------------------------------------------------

A few small fixes/improvements to make.

::: bus/containers.c
@@ +84,5 @@
> +
> +BusContainers *
> +bus_containers_ref (BusContainers *self)
> +{
> +  _dbus_assert (self->refcount > 0);

Paranoia: also add _dbus_assert (self->refcount < INT_MAX);

@@ +93,5 @@
> +
> +void
> +bus_containers_unref (BusContainers *self)
> +{
> +  _dbus_assert (self != NULL);

Paranoia: also add _dbus_assert (self->refcount > 0);

@@ +175,5 @@
> +
> +  /* We assume PRIu64 exists on all Unix platforms: it's ISO C99, and the
> +   * only non-C99 platform we support is MSVC on Windows. */
> +  if (!_dbus_string_append_printf (&path,
> +                                   "/org/freedesktop/DBus/Containers/c%" PRIu64,

As per the API design guidelines (https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning), if we’re going to version the interfaces for this, the object paths should also be versioned, otherwise they can’t support multiple interface versions.

This object path is only used as an opaque identifier (right?), but it’s probably best to be consistent with other object paths which do implement interfaces and hence should be versioned.

@@ +228,4 @@
>  
>    _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING);
>    dbus_message_iter_get_basic (&iter, &type);
> +  instance->type = _dbus_strdup (type);

Check for OOM here?

@@ +244,4 @@
>  
>    _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING);
>    dbus_message_iter_get_basic (&iter, &name);
> +  instance->name = _dbus_strdup (name);

Check for OOM here?
Comment 118 Simon McVittie 2017-07-12 14:44:57 UTC
(I am not really here, more detailed responses will follow when I'm back to working on this.)

(In reply to Philip Withnall from comment #115)
> (If going down that route, I’d also
> consider backporting g_autoptr() [libglnx has a copy if needed], although
> I’m less sure that it’s portable to all the systems D-Bus is meant to
> support. I haven’t checked.)

I'm pretty sure I can't do this. g_autoptr is specifically for gcc and clang; dbus is meant to support MSVC (although only for Windows, so not directly relevant to this particular API) and any random proprietary C compiler that implements ISO C.
Comment 119 Simon McVittie 2017-07-12 15:03:03 UTC
(In reply to Allison Lortie (desrt) from comment #116)
> The fully-namespaced keys inside of the GetConnectionCredentials return
> value are silly and wasteful.  It also clashes with the appearance of the
> other values in this dict.  This data can't be arbitrarily populated by
> other people, so unnamespaced values are (and continue to be) fine here.

Reimplementors of dbus-daemon (bus1, gdbus-daemon) are the other people who can populate this dict with namespaced keys.

> I was sad to notice that the fd-passing style of creation is gone.  I guess
> you plan to address this via a{sv} fields in the future?

Yes, that's part of what the named parameters a{sv} is for.

This branch is, as I said in Comment #0 and again in Comment #59, the bare minimum that could be merged - in particular the dbus-daemon is responsible for doing the bind() and listen() in this version, because that is the simplest thing to do, and avoids having to block this entire topic on how we detect that a caller has given us an unsuitable fd without the dbus-daemon going into a busy-loop when accept() fails (which I think is what it would do at the moment).

More details at <https://bugs.freedesktop.org/show_bug.cgi?id=100344#c13>, and elsewhere on that bug.

> The same-uid check is a hack and it shows off some of the problems with the
> create-a-path-in-a-public-directory approach.

It is, and I anticipate that we'll want to either drop this restriction or introduce an {AllowAnyUser: <true>} named parameter in future. But as the commit message says, if we are initially conservative, we have the option to drop the check in future without breaking existing code, whereas if we are initially liberal, we're stuck with it forever.

> The main missing item here in terms of usability is the addition of the
> header field that we discussed for marking messages from sandboxed apps.

Again, minimal implementation. Given the number of patches already present here, I didn't want to extend the patch series with non-essential features until we had the basics down.

> I recall that we discussed that this should be a uint64 for
> efficiency/simplicity reasons.

I think we went backwards and forwards on this. Philip pointed out in <https://bugs.freedesktop.org/show_bug.cgi?id=100344#c8> that GDBus' API for arg0 matching is very char*-specific, and providing an opaque identifier for a thing is what we keep saying object paths are for, so I went for object paths.

> I even consider disabling all methods by
> default for container users, unless they are explicitly annotated as being
> safe (but this has some potential compatibility issues).

The reason I didn't do this is that it lengthens the minimal patch series, because to be able to make the feature useful (and do unit tests) we would have to add at least a basic form of message filtering.

I anticipate that message filtering will have to be one of the next steps after this implementation, at which point we can define the meaning of an empty list of "allow this" rules to be allowing nothing.

> Any thoughts on sandboxed apps adding match rules?  It seems like all of the
> "policy" stuff we were dreaming up during the hackfest is gone, and that's
> probably for the better, but this is one little corner that still needs to
> be solved.

The policy stuff is needed, but it is not part of the minimal viable product. We could merge this without it and add it as a named parameter later, and the feature would already be usable as a way to provide reliable identification of connections made by (Flatpak's dbus-proxy on behalf of) containers.
Comment 120 Philip Withnall 2017-07-12 15:03:37 UTC
(In reply to Simon McVittie from comment #118)
> (I am not really here, more detailed responses will follow when I'm back to
> working on this.)

👻

> (In reply to Philip Withnall from comment #115)
> > (If going down that route, I’d also
> > consider backporting g_autoptr() [libglnx has a copy if needed], although
> > I’m less sure that it’s portable to all the systems D-Bus is meant to
> > support. I haven’t checked.)
> 
> I'm pretty sure I can't do this. g_autoptr is specifically for gcc and
> clang; dbus is meant to support MSVC (although only for Windows, so not
> directly relevant to this particular API) and any random proprietary C
> compiler that implements ISO C.

Fair. Thought it was worth asking. g_steal_pointer() should still be feasible though, and I think it would be a code clarity win. However, I appreciate you’ve already written the branch without it, so I’m going to leave it as a nitpick rather than a review-.
Comment 121 Simon McVittie 2017-07-21 12:48:27 UTC
(In reply to Allison Lortie (desrt) from comment #116)
> Philip is already handling the code review aspects nicely, so here are some
> higher level notes

Let's take this to the parent bug, Bug #100344.
Comment 122 Simon McVittie 2017-07-21 17:01:12 UTC
(In reply to Philip Withnall from comment #107)
> s/app/application/? Probably best to be consistent in use of terminology
> about apps/applications/bundles/flatpaks.

Fixed locally. I said "application".

> > +                    <literal>io.snapcraft</literal>
> 
> Nitpick: Other argument descriptions in this table end in a full stop.
> Probably best for them to all do so, for consistency.

Fixed locally.

> So a single object path can refer to multiple containers (different versions
> of the same app)?

I tried to clarify this.

> Should we point out that this data is not to be trusted then? Or should only
> be trusted as much as clients trust the container manager?

Not done yet. See <https://bugs.freedesktop.org/show_bug.cgi?id=100344#c15>. I added a TODO comment for now.

> What happens if the bus limits the size? Presumably it rejects the
> AddContainerServer call, rather than truncating the metadata?

Yes. Clarified locally.

> Do we want to put in rules about capitalisation after the reverse-DNS name?
> e.g. org.flatpak.MyNewKey vs org.flatpak.my-new-key vs org.flatpak.mynewkey.

I added an example, org.gtk.GDBus.SomeArgument (although I don't think GDBus is going to implement this interface any time soon).

> > +            StopContainerInstance (in OBJECT_PATH container_instance)
> 
> Suggestion: to make the method names shorter, you could drop ‘Container’,
> since it’s already in the interface name. So,
> org.freedesktop.DBus.Containers1.StopInstance.

Good idea; done locally.

> Nitpick: Same nitpick about full stops.
> 
> Also, nitpick: Add <literal> around AddContainerServer.

Both fixed in assorted places.

> Nitpick: ‘app-container’ is a new term. Is this what you want to standardise
> on for app/container/flatpak/bundle/bonghits?

Changed to "container instance".

> Does that mean NotContainer is returned if I were to call
> StopContainerListening twice in a row on the same container instance?

Usually no, as said previously. Clarified.

> Do we want to require that it’s non-empty?

I don't think so. Clarified why the empty string might be present (if the container type has no concept of uniquely naming "apps").
Comment 123 Simon McVittie 2017-07-21 17:09:58 UTC
(In reply to Philip Withnall from comment #108)
> s/HAVE_UNIX_FD_PASSING/DBUS_ENABLE_CONTAINERS/

Fixed (was also fixed later in the branch anyway)

> Nitpick: These arguments aren’t lined up like the others are.

Realigned, within reason

> Shouldn’t the action-if-given argument be set to
> [enable_containers=$enableval]? The autoconf documentation doesn’t say it
> has a default action.

No action taken as previously discussed
Comment 124 Simon McVittie 2017-07-21 20:01:37 UTC
(In reply to Philip Withnall from comment #109)
> +1 for adding a static inline convenience dbus_clear_message(DBusMessage
> **to_die);

In progress (the branch needs a lot of rebasing and fixing to introduce this consistently). Also for connections, servers, pending calls, arrays of address entries, and the new local objects in bus/ introduced by Containers1.
Comment 125 Simon McVittie 2017-07-24 19:02:14 UTC
(In reply to Allison Lortie (desrt) from comment #116)
> I was sad to notice that the fd-passing style of creation is gone.  I guess
> you plan to address this via a{sv} fields in the future?

Yes, this feature request is Bug #101898.

> The main missing item here in terms of usability is the addition of the
> header field that we discussed for marking messages from sandboxed apps.

Bug #101899

> Any thoughts on sandboxed apps adding match rules?

Bug #101902
Comment 126 Simon McVittie 2017-07-24 19:03:06 UTC
Created attachment 132870 [details] [review]
Add a simplified backport of g_steal_pointer()

This will be used in tests later in the branch.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 127 Simon McVittie 2017-07-24 19:03:10 UTC
Created attachment 132871 [details] [review]
_dbus_asv_add_object_path: Add

Signed-off-by: Simon McVittie <smcv@collabora.com>
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Bug:
Comment 128 Simon McVittie 2017-07-24 19:03:15 UTC
Created attachment 132872 [details] [review]
spec: Document the initial Containers1 interface

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Consistently say application, not app.

Rename AddContainerServer() etc. to AddServer() etc.

Consistently say application, not app.

Consistently end argument descriptions with a full stop.

Try to clarify that each call to AddServer creates a new
unique object path.

Add missing documentation for InstanceRemoved signal.

Clarify that a second call to StopInstance() will fail, but
StopListening() is idempotent (except in the trivial case of no
connections left alive, in which case it's equivalent to StopInstance()).

Say that container types (container technology names) are syntactically
interface names.
Comment 129 Simon McVittie 2017-07-24 19:03:49 UTC
Created attachment 132873 [details] [review]
Add a simplified backport of g_steal_pointer()

This will be used in tests later in the branch.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 130 Simon McVittie 2017-07-24 19:04:04 UTC
Created attachment 132874 [details] [review]
_dbus_asv_add_object_path: Add

Signed-off-by: Simon McVittie <smcv@collabora.com>
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Comment 131 Simon McVittie 2017-07-24 19:04:20 UTC
Created attachment 132875 [details] [review]
spec: Document the initial Containers1 interface

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Consistently say application, not app.

Rename AddContainerServer() etc. to AddServer() etc.

Consistently say application, not app.

Consistently end argument descriptions with a full stop.

Try to clarify that each call to AddServer creates a new
unique object path.

Add missing documentation for InstanceRemoved signal.

Clarify that a second call to StopInstance() will fail, but
StopListening() is idempotent (except in the trivial case of no
connections left alive, in which case it's equivalent to StopInstance()).

Say that container types (container technology names) are syntactically
interface names.
Comment 132 Simon McVittie 2017-07-24 19:05:13 UTC
(In reply to Philip Withnall from comment #117)
> bus/containers: Build a global data structure for container instances
...
> A few small fixes/improvements to make.

(TODO: I have not done these yet)
Comment 133 Simon McVittie 2017-07-24 19:05:20 UTC
Created attachment 132876 [details] [review]
driver: Add a stub implementation of the Containers1 interface

For now, this is considered to be a privileged operation, because the
resource-limiting isn't wired up yet. It only contains the bare minimum
of API.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Correct comment on #endif: DBUS_ENABLE_CONTAINERS, not
HAVE_UNIX_FD_PASSING.

Tediously line up arguments in header.
Comment 134 Simon McVittie 2017-07-24 19:05:40 UTC
Created attachment 132877 [details] [review]
test/uid-permissions: Assert that AddServer is privileged

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use dbus_clear_foo. Don't leak DBusPendingCall.
Comment 135 Simon McVittie 2017-07-24 19:05:49 UTC
Created attachment 132878 [details] [review]
test/containers: New test

So far it only exercises SupportedArguments.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Add a brief doc-comment for the test
Comment 136 Simon McVittie 2017-07-24 19:07:30 UTC
Created attachment 132879 [details] [review]
test/uid-permissions: Assert that AddServer is privileged

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use dbus_clear_foo. Don't leak DBusPendingCall.
Add a brief doc-comment for the test.
Comment 137 Simon McVittie 2017-07-24 19:07:44 UTC
Created attachment 132880 [details] [review]
test/containers: New test

So far it only exercises SupportedArguments.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Add a brief doc-comment for the test
Comment 138 Simon McVittie 2017-07-24 19:08:10 UTC
Created attachment 132881 [details] [review]
bus/containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 139 Simon McVittie 2017-07-24 19:08:22 UTC
Created attachment 132882 [details] [review]
test/containers: Exercise the new parameter checking

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer. Add brief doc-comments.
Comment 140 Simon McVittie 2017-07-24 19:56:23 UTC
Created attachment 132885 [details] [review]
bus/containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 141 Simon McVittie 2017-07-24 19:56:35 UTC
Created attachment 132886 [details] [review]
test/containers: Exercise the new parameter checking

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer. Add brief doc-comments.
Comment 142 Simon McVittie 2017-07-24 19:57:34 UTC
Created attachment 132887 [details] [review]
bus/containers: Build a global data structure for container instances

We still don't actually create a DBusServer for incoming connections
at this point, much less accept incoming connections.

Signed-off-by: Simon McVittie <smcv@collabora.com>

https://bugs.freedesktop.org/show_bug.cgi?id=101354

---

Improve refcount checks. Check for OOM from _dbus_strdup(), twice.
Include versioning in object path (for now, while we discuss
whether any of this should be versioned).
Resolves Comment #117, I think.
Comment 143 Simon McVittie 2017-07-24 19:57:54 UTC
Created attachment 132888 [details] [review]
bus/containers: Create a DBusServer and add it to the main loop

This means we can accept connections on the new socket. For now, we
don't process them and they get closed.

For the system bus (or root's session bus, where the difference is
harmless but makes automated testing easier), rely on system-wide
infrastructure to create /run/dbus/containers. At the moment this
is only done for systemd systems via tmpfiles.d; I'd accept a tested
patch for our Red Hat, Slackware and Cygwin init scripts to do the
same, but I suspect they might actually be unused and unmaintained
(see Bug #101706).

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Make more use of dbus_clear_whatever
Comment 144 Simon McVittie 2017-07-24 19:58:23 UTC
Created attachment 132891 [details] [review]
bus_context_add_incoming_connection: factor out

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 145 Simon McVittie 2017-07-24 19:59:25 UTC
Created attachment 132892 [details] [review]
bus/containers: Set up new connections to join the bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 146 Simon McVittie 2017-07-24 19:59:46 UTC
Created attachment 132893 [details] [review]
test/containers: Exercise a successful call to AddServer

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer(). Add a doc-comment.
Comment 147 Simon McVittie 2017-07-24 20:00:06 UTC
Created attachment 132894 [details] [review]
bus/containers: Require connecting uid to match caller of AddServer

If we're strict now, we can relax this later (either with a named
parameter or always); but if we're lenient now, we'll be stuck with it
forever, so be strict.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 148 Simon McVittie 2017-07-24 20:00:20 UTC
Created attachment 132895 [details] [review]
test-utils-glib: Factor out functions for switching uid

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 149 Simon McVittie 2017-07-24 20:00:30 UTC
Created attachment 132896 [details] [review]
test-utils-glib: Add function to connect with GDBus as another uid

This will be used in a test for connecting to container servers
as the wrong uid.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 150 Simon McVittie 2017-07-24 20:00:49 UTC
Created attachment 132897 [details] [review]
test/containers: Exercise connecting to the new socket as the wrong uid

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer. Add a doc-comment.
Comment 151 Simon McVittie 2017-07-24 20:01:15 UTC
Created attachment 132898 [details] [review]
bus/containers: Each connection to a container holds a reference

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Roll back a use of bus_clear_containers(), because we need to
distinguish between NULL and non-NULL anyway
Comment 152 Simon McVittie 2017-07-24 20:01:30 UTC
Created attachment 132899 [details] [review]
bus/containers: Link each container to its initiating connection

We will need this to be able to shut down the container when its
creator vanishes.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 153 Simon McVittie 2017-07-24 20:01:43 UTC
Created attachment 132900 [details] [review]
bus/containers: Shut down container servers when initiator goes away

We will eventually want to have other ways to signal that a
container server should stop listening, so that the container manager
doesn't have to stay on D-Bus (fd-passing the read end of a pipe
whose write end will be closed by the container manager has been
suggested as easier to deal with for Flatpak/Bubblewrap), but for
now we're doing the simplest possible thing.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 154 Simon McVittie 2017-07-24 20:01:54 UTC
Created attachment 132901 [details] [review]
bus/containers: Give each instance a list of all its connections

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 155 Simon McVittie 2017-07-24 20:02:10 UTC
Created attachment 132902 [details] [review]
bus/containers: Implement methods to stop containers explicitly

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 156 Simon McVittie 2017-07-24 20:02:59 UTC
Created attachment 132903 [details] [review]
bus/containers: Don't allow stopping other users' containers

On the system bus, that would be a denial of service, assuming we
relax the access-control from METHOD_FLAG_PRIVILEGED to a new
METHOD_FLAG_NOT_CONTAINERS later.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 157 Simon McVittie 2017-07-24 20:03:15 UTC
Created attachment 132904 [details] [review]
test/containers: Exercise the various ways to stop a container

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer()
Comment 158 Simon McVittie 2017-07-24 20:03:51 UTC
Created attachment 132905 [details] [review]
bus/containers: Emit InstanceRemoved signal

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use dbus_clear_message. Don't invent an equivalent for BusTransaction
(for now) because it has no conventional free-function, only
cancel_and_free and execute_and_free, which would lead to odd semantics.
Comment 159 Simon McVittie 2017-07-24 20:09:15 UTC
Created attachment 132906 [details] [review]
test/containers: Assert that InstanceRemoved is emitted

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 160 Simon McVittie 2017-07-24 20:09:34 UTC
Created attachment 132907 [details] [review]
bus/containers: Indicate in loginfo whether connection is contained

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 161 Simon McVittie 2017-07-24 20:09:46 UTC
Created attachment 132908 [details] [review]
bus/driver: Treat connections from inside containers as unprivileged

Even if the uid matches, a contained app shouldn't count as the owner
of the bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 162 Simon McVittie 2017-07-24 20:09:59 UTC
Created attachment 132909 [details] [review]
test/containers: Check that containers can't make new containers

We should prevent containers from trying to put a container in our
container so we can sandbox while we sandbox. The implementation doesn't
actually have any concept of nesting or layering, so that would potentially
be privilege escalation.

At the moment, this is just prevented by METHOD_FLAG_PRIVILEGED. When we
remove that flag (after we've introduced better resource limits), we can
specifically restrict this method to not be called by containers
instead. This test will make sure we do.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use g_steal_pointer(). Add a doc-comment.
Comment 163 Simon McVittie 2017-07-24 20:10:12 UTC
Created attachment 132910 [details] [review]
test/containers: Check that connections from containers are unprivileged

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 164 Simon McVittie 2017-07-24 20:10:26 UTC
Created attachment 132911 [details] [review]
bus/driver: Add a flag for methods that can't be invoked by containers

We can relax AddServer() from PRIVILEGED to NOT_CONTAINERS when we've
put resource limits in place, although for now it must remain
PRIVILEGED because it uses up resources.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 165 Simon McVittie 2017-07-24 20:10:40 UTC
Created attachment 132912 [details] [review]
bus/driver: Containers can't use the Verbose and Stats interfaces

These are debugging interfaces, which are essentially read-only.
By default, Verbose is not available on the system bus at all and
Stats is only available to uid 0, but both are available on the
session bus, and they can be allowed for other uids by configuring
the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 166 Simon McVittie 2017-07-24 20:10:56 UTC
Created attachment 132913 [details] [review]
bus/driver: Add basic container info to GetConnectionCredentials()

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 167 Simon McVittie 2017-07-24 20:11:10 UTC
Created attachment 132914 [details] [review]
test/dbus-daemon: Assert absence of Containers1 credentials

These connections are not to a container server.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 168 Simon McVittie 2017-07-24 20:11:25 UTC
Created attachment 132915 [details] [review]
bus/driver: Add GetConnectionInstance(), GetInstanceInfo()

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use dbus_clear_message().

bus_containers_handle_get_connection_instance() isn't lined up with
the other methods in the header because it's too long for that to be
convenient.
Comment 169 Simon McVittie 2017-07-24 20:11:40 UTC
Created attachment 132916 [details] [review]
t/containers: Exercise trivial and non-trivial container metadata

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Check that the empty string is an acceptable app name.

Use g_steal_pointer().

Add doc-comments.

Rename test_invalid_metadata to test_invalid_metadata_getters - we
are testing invalid calls to the getters, not syntactically invalid
metadata (there is no such thing, beyond normal D-Bus constraints).
Comment 170 Simon McVittie 2017-07-24 20:11:54 UTC
Created attachment 132917 [details] [review]
test/containers: Check that GetInstanceInfo stops working

After the container instance is removed, the method should not work.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 171 Simon McVittie 2017-07-24 20:12:08 UTC
Created attachment 132918 [details] [review]
bus: Add (unused) settings for resource limits for containers

These will be enforced in subsequent commits.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 172 Simon McVittie 2017-07-24 20:12:21 UTC
Created attachment 132919 [details] [review]
bus/containers: Limit the size of metadata we will store

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 173 Simon McVittie 2017-07-24 20:12:44 UTC
Created attachment 132920 [details] [review]
bus/containers: Enforce max_containers limit

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 174 Simon McVittie 2017-07-24 20:13:00 UTC
Created attachment 132921 [details] [review]
bus/containers: Enforce max_connections_per_container

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 175 Simon McVittie 2017-07-24 20:13:14 UTC
Created attachment 132922 [details] [review]
containers: enforce max_containers_per_user

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Use _dbus_clear_hash_table
Comment 176 Simon McVittie 2017-07-24 20:13:29 UTC
Created attachment 132923 [details] [review]
test/containers: Exercise the resource limits

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 177 Simon McVittie 2017-07-24 20:13:41 UTC
Created attachment 132924 [details] [review]
Revert "test/uid-permissions: Assert that AddServer is privileged"

I'm about to make that not be true.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 178 Simon McVittie 2017-07-24 20:13:53 UTC
Created attachment 132925 [details] [review]
bus/driver: Allow unprivileged connections to create app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 179 Simon McVittie 2017-07-24 20:14:12 UTC
Created attachment 132926 [details] [review]
system.conf: Allow creating containers on the system bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 180 Simon McVittie 2017-07-24 20:15:47 UTC
This is ready for another review round when people are ready. I'm going to have very limited time for this feature for the next few weeks, so no rush.
Comment 181 Ralf Habacker 2017-07-24 21:04:07 UTC
Just applied the patches from this bug to dbus master branch and got on compiling with autotools: 

In file included from ../../dbus/bus/bus.c:32:0:
../../dbus/bus/containers.h: In function ‘bus_clear_containers’:
../../dbus/bus/containers.h:68:3: error: implicit declaration of function ‘_dbus_clear_pointer_impl’ [-Werror=implicit-function-declaration]
   _dbus_clear_pointer_impl (BusContainers, containers_p, bus_containers_unref);
   ^
../../dbus/bus/containers.h:68:3: error: nested extern declaration of ‘_dbus_clear_pointer_impl’ [-Werror=nested-externs]
../../dbus/bus/containers.h:68:29: error: expected expression before ‘BusContainers’
   _dbus_clear_pointer_impl (BusContainers, containers_p, bus_containers_unref);
                             ^
Comment 182 Simon McVittie 2017-07-25 00:30:38 UTC
(In reply to Ralf Habacker from comment #181)
> Just applied the patches from this bug to dbus master branch and got on
> compiling with autotools: 
> 
> In file included from ../../dbus/bus/bus.c:32:0:
> ../../dbus/bus/containers.h: In function ‘bus_clear_containers’:
> ../../dbus/bus/containers.h:68:3: error: implicit declaration of function
> ‘_dbus_clear_pointer_impl’ [-Werror=implicit-function-declaration]

That's new functionality in Bug #101895, which this one lists as a dependency.
Comment 183 Simon McVittie 2017-07-26 20:45:51 UTC
Created attachment 133003 [details] [review]
travis-ci: Enable/disable more features in various builds

In the debug build, enable features that are off by default. In the
reduced build, explicitly disable features, some of which are
on by default. In the legacy build, check that we can compile the
default feature-set without inotify, dnotify, systemd, etc.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 184 Simon McVittie 2017-07-26 20:45:56 UTC
Created attachment 133004 [details] [review]
cmake: Match AC_DEFINE more precisely, respecting [] quoting

The regular expression previously used here to select the second
comma-delimited argument won't work when we introduce an argument
containing a comma, which I need to do now. We can address this by
recognising Autoconf's quoting mechanism (which uses square
brackets).

This is not 100% right (it doesn't understand nested square brackets),
but it's good enough in practice.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 185 Simon McVittie 2017-07-26 20:46:01 UTC
Created attachment 133005 [details] [review]
Add a simplified backport of g_steal_pointer()

This will be used in tests later in the branch.

Sadly we can't use GLIB_VERSION_2_44 unless we are willing to have a
hard dependency on GLib 2.44, which would force us to do all our
Travis-CI builds in Docker containers rather than in ye olde base
system, and that adds 50% to the time taken to do builds.

Signed-off-by: Simon McVittie <smcv@collabora.com>

---

Avoid GLIB_VERSION_2_44
Comment 186 Simon McVittie 2017-07-26 20:46:06 UTC
Created attachment 133006 [details] [review]
_dbus_asv_add_object_path: Add

Signed-off-by: Simon McVittie <smcv@collabora.com>
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Bug:
Comment 187 Simon McVittie 2017-07-26 20:46:11 UTC
Created attachment 133007 [details] [review]
spec: Document the initial Containers1 interface

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 188 Simon McVittie 2017-07-26 20:46:16 UTC
Created attachment 133008 [details] [review]
driver: Add a stub implementation of the Containers1 interface

For now, this is considered to be a privileged operation, because the
resource-limiting isn't wired up yet. It only contains the bare minimum
of API.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 189 Simon McVittie 2017-07-26 20:46:20 UTC
Created attachment 133009 [details] [review]
travis-ci: Do at least one build with and one without containers

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 190 Simon McVittie 2017-07-26 20:46:25 UTC
Created attachment 133010 [details] [review]
test/uid-permissions: Assert that AddServer is privileged

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 191 Simon McVittie 2017-07-26 20:46:30 UTC
Created attachment 133011 [details] [review]
test/containers: New test

So far it only exercises SupportedArguments.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 192 Simon McVittie 2017-07-26 20:46:35 UTC
Created attachment 133012 [details] [review]
bus/containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 193 Simon McVittie 2017-07-26 20:46:41 UTC
Created attachment 133013 [details] [review]
test/containers: Exercise the new parameter checking

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 194 Simon McVittie 2017-07-26 20:46:46 UTC
Created attachment 133014 [details] [review]
bus/containers: Build a global data structure for container instances

We still don't actually create a DBusServer for incoming connections
at this point, much less accept incoming connections.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 195 Simon McVittie 2017-07-26 20:46:52 UTC
Created attachment 133015 [details] [review]
bus/containers: Create a DBusServer and add it to the main loop

This means we can accept connections on the new socket. For now, we
don't process them and they get closed.

For the system bus (or root's session bus, where the difference is
harmless but makes automated testing easier), rely on system-wide
infrastructure to create /run/dbus/containers. At the moment this
is only done for systemd systems via tmpfiles.d; I'd accept a tested
patch for our Red Hat, Slackware and Cygwin init scripts to do the
same, but I suspect they might actually be unused and unmaintained
(see Bug #101706).

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 196 Simon McVittie 2017-07-26 20:46:57 UTC
Created attachment 133016 [details] [review]
bus_context_add_incoming_connection: factor out

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 197 Simon McVittie 2017-07-26 20:47:02 UTC
Created attachment 133017 [details] [review]
bus/containers: Set up new connections to join the bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 198 Simon McVittie 2017-07-26 20:47:07 UTC
Created attachment 133018 [details] [review]
test/containers: Exercise a successful call to AddServer

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 199 Simon McVittie 2017-07-26 20:47:12 UTC
Created attachment 133019 [details] [review]
bus/containers: Require connecting uid to match caller of AddServer

If we're strict now, we can relax this later (either with a named
parameter or always); but if we're lenient now, we'll be stuck with it
forever, so be strict.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 200 Simon McVittie 2017-07-26 20:47:17 UTC
Created attachment 133020 [details] [review]
test-utils-glib: Add failable functions to connect to a bus

Instead of calling g_test_skip() internally, raise a distinctive error
and let the caller handle it.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 201 Simon McVittie 2017-07-26 20:47:22 UTC
Created attachment 133021 [details] [review]
test-utils-glib: Factor out functions for switching uid

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354

---

Adjusted to apply on top of "try_" versions of the functions that
connect to the bus. Otherwise, some tests are wrongly marked as skipped
when in fact only part of the test was skipped.
Comment 202 Simon McVittie 2017-07-26 20:47:27 UTC
Created attachment 133022 [details] [review]
test-utils-glib: Add function to connect with GDBus as another uid

This will be used in a test for connecting to container servers
as the wrong uid.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 203 Simon McVittie 2017-07-26 20:47:32 UTC
Created attachment 133023 [details] [review]
test/containers: Exercise connecting to the new socket as the wrong uid

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 204 Simon McVittie 2017-07-26 20:47:37 UTC
Created attachment 133024 [details] [review]
bus/containers: Each connection to a container holds a reference

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 205 Simon McVittie 2017-07-26 20:47:42 UTC
Created attachment 133025 [details] [review]
bus/containers: Link each container to its initiating connection

We will need this to be able to shut down the container when its
creator vanishes.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 206 Simon McVittie 2017-07-26 20:47:47 UTC
Created attachment 133026 [details] [review]
bus/containers: Shut down container servers when initiator goes away

We will eventually want to have other ways to signal that a
container server should stop listening, so that the container manager
doesn't have to stay on D-Bus (fd-passing the read end of a pipe
whose write end will be closed by the container manager has been
suggested as easier to deal with for Flatpak/Bubblewrap), but for
now we're doing the simplest possible thing.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 207 Simon McVittie 2017-07-26 20:47:52 UTC
Created attachment 133027 [details] [review]
bus/containers: Give each instance a list of all its connections

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 208 Simon McVittie 2017-07-26 20:47:57 UTC
Created attachment 133028 [details] [review]
bus/containers: Implement methods to stop containers explicitly

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 209 Simon McVittie 2017-07-26 20:48:02 UTC
Created attachment 133029 [details] [review]
bus/containers: Don't allow stopping other users' containers

On the system bus, that would be a denial of service, assuming we
relax the access-control from METHOD_FLAG_PRIVILEGED to a new
METHOD_FLAG_NOT_CONTAINERS later.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 210 Simon McVittie 2017-07-26 20:48:08 UTC
Created attachment 133030 [details] [review]
test/containers: Exercise the various ways to stop a container

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 211 Simon McVittie 2017-07-26 20:48:13 UTC
Created attachment 133031 [details] [review]
bus/containers: Emit InstanceRemoved signal

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 212 Simon McVittie 2017-07-26 20:48:18 UTC
Created attachment 133032 [details] [review]
test/containers: Assert that InstanceRemoved is emitted

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 213 Simon McVittie 2017-07-26 20:48:23 UTC
Created attachment 133033 [details] [review]
bus/containers: Indicate in loginfo whether connection is contained

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 214 Simon McVittie 2017-07-26 20:48:28 UTC
Created attachment 133034 [details] [review]
bus/driver: Treat connections from inside containers as unprivileged

Even if the uid matches, a contained app shouldn't count as the owner
of the bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 215 Simon McVittie 2017-07-26 20:48:33 UTC
Created attachment 133035 [details] [review]
test/containers: Check that containers can't make new containers

We should prevent containers from trying to put a container in our
container so we can sandbox while we sandbox. The implementation doesn't
actually have any concept of nesting or layering, so that would potentially
be privilege escalation.

At the moment, this is just prevented by METHOD_FLAG_PRIVILEGED. When we
remove that flag (after we've introduced better resource limits), we can
specifically restrict this method to not be called by containers
instead. This test will make sure we do.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 216 Simon McVittie 2017-07-26 20:48:38 UTC
Created attachment 133036 [details] [review]
test/containers: Check that connections from containers are unprivileged

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 217 Simon McVittie 2017-07-26 20:48:43 UTC
Created attachment 133037 [details] [review]
bus/driver: Add a flag for methods that can't be invoked by containers

We can relax AddServer() from PRIVILEGED to NOT_CONTAINERS when we've
put resource limits in place, although for now it must remain
PRIVILEGED because it uses up resources.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 218 Simon McVittie 2017-07-26 20:48:48 UTC
Created attachment 133038 [details] [review]
bus/driver: Containers can't use the Verbose and Stats interfaces

These are debugging interfaces, which are essentially read-only.
By default, Verbose is not available on the system bus at all and
Stats is only available to uid 0, but both are available on the
session bus, and they can be allowed for other uids by configuring
the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 219 Simon McVittie 2017-07-26 20:48:54 UTC
Created attachment 133039 [details] [review]
bus/driver: Add basic container info to GetConnectionCredentials()

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 220 Simon McVittie 2017-07-26 20:49:00 UTC
Created attachment 133040 [details] [review]
test/dbus-daemon: Assert absence of Containers1 credentials

These connections are not to a container server.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 221 Simon McVittie 2017-07-26 20:49:05 UTC
Created attachment 133041 [details] [review]
bus/driver: Add GetConnectionInstance(), GetInstanceInfo()

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 222 Simon McVittie 2017-07-26 20:49:11 UTC
Created attachment 133042 [details] [review]
t/containers: Exercise trivial and non-trivial container metadata

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 223 Simon McVittie 2017-07-26 20:49:17 UTC
Created attachment 133043 [details] [review]
test/containers: Check that GetInstanceInfo stops working

After the container instance is removed, the method should not work.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 224 Simon McVittie 2017-07-26 20:49:23 UTC
Created attachment 133044 [details] [review]
bus: Add (unused) settings for resource limits for containers

These will be enforced in subsequent commits.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 225 Simon McVittie 2017-07-26 20:49:28 UTC
Created attachment 133045 [details] [review]
bus/containers: Limit the size of metadata we will store

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 226 Simon McVittie 2017-07-26 20:49:34 UTC
Created attachment 133046 [details] [review]
bus/containers: Enforce max_containers limit

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 227 Simon McVittie 2017-07-26 20:49:40 UTC
Created attachment 133047 [details] [review]
bus/containers: Enforce max_connections_per_container

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 228 Simon McVittie 2017-07-26 20:49:45 UTC
Created attachment 133048 [details] [review]
containers: enforce max_containers_per_user

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 229 Simon McVittie 2017-07-26 20:49:51 UTC
Created attachment 133049 [details] [review]
test/containers: Exercise the resource limits

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354

---

Tolerate G_IO_ERROR_FAILED (from old GLib) as well as
G_IO_ERROR_CONNECTION_CLOSED aka G_IO_ERROR_BROKEN_PIPE (from new GLib)
Comment 230 Simon McVittie 2017-07-26 20:49:57 UTC
Created attachment 133050 [details] [review]
Revert "test/uid-permissions: Assert that AddServer is privileged"

I'm about to make that not be true.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 231 Simon McVittie 2017-07-26 20:50:04 UTC
Created attachment 133051 [details] [review]
bus/driver: Allow unprivileged connections to create app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 232 Simon McVittie 2017-07-26 20:50:11 UTC
Created attachment 133052 [details] [review]
system.conf: Allow creating containers on the system bus

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug:
Comment 233 Simon McVittie 2017-07-26 20:50:17 UTC
Created attachment 133053 [details] [review]
test/containers: Use a shorter path for XDG_RUNTIME_DIR

Travis-CI uses a long enough path to the $builddir that appending what
we do for the container pushes us over the length limit for AF_UNIX.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Comment 234 Philip Withnall 2017-07-31 13:38:19 UTC
Comment on attachment 133003 [details] [review]
travis-ci: Enable/disable more features in various builds

Review of attachment 133003 [details] [review]:
-----------------------------------------------------------------

r+
Comment 235 Philip Withnall 2017-07-31 13:40:39 UTC
Comment on attachment 133004 [details] [review]
cmake: Match AC_DEFINE more precisely, respecting [] quoting

Review of attachment 133004 [details] [review]:
-----------------------------------------------------------------

I have not reviewed this regex in detail, because life is too short. If it works, ship it.
Comment 236 Philip Withnall 2017-07-31 13:41:37 UTC
Comment on attachment 133005 [details] [review]
Add a simplified backport of g_steal_pointer()

Review of attachment 133005 [details] [review]:
-----------------------------------------------------------------

r+
Comment 237 Philip Withnall 2017-07-31 13:42:15 UTC
Comment on attachment 133006 [details] [review]
_dbus_asv_add_object_path: Add

Review of attachment 133006 [details] [review]:
-----------------------------------------------------------------

r+, still
Comment 238 Philip Withnall 2017-07-31 14:01:55 UTC
Comment on attachment 133007 [details] [review]
spec: Document the initial Containers1 interface

Review of attachment 133007 [details] [review]:
-----------------------------------------------------------------

r+, with one nitpick and one minor issue. I reserve the right to make more comments on the API design as I work through the rest of the patches.

::: doc/dbus-specification.xml
@@ +6850,5 @@
> +          <firstterm>confined</firstterm>. The message bus may prevent
> +          confined connections from calling certain security-sensitive methods,
> +          and may apply lower limits to these connections. However, container
> +          managers and services must not rely on confined connections having
> +          any particular filtering applied to them.

Nitpick: ‘applied to them by the message bus’ to clarify that bit?

@@ +7141,5 @@
> +          </informaltable>
> +        </para>
> +
> +        <para>
> +          If the given object path represents an active container instance,

Does that include container instances which have had StopListening called on them (but which still have some open connections)? The term ‘active’ here is ill-defined. The same ‘active’ term is used in a few other places in this patch and is ill-defined there too.
Comment 239 Philip Withnall 2017-07-31 14:06:40 UTC
Comment on attachment 133008 [details] [review]
driver: Add a stub implementation of the Containers1 interface

Review of attachment 133008 [details] [review]:
-----------------------------------------------------------------

The mandatory CMake nitpicks, but otherwise r+.

::: cmake/CMakeLists.txt
@@ +129,4 @@
>  option (DBUS_DISABLE_ASSERT "Disable assertion checking" OFF)
>  
>  option (DBUS_ENABLE_STATS "enable bus daemon usage statistics" OFF)
> +option (DBUS_ENABLE_CONTAINERS "enable restricted servers for app-containers" OFF)

Also needs to be in README.cmake and cmake/config.h, I think.
Comment 240 Philip Withnall 2017-07-31 14:07:01 UTC
Comment on attachment 133009 [details] [review]
travis-ci: Do at least one build with and one without containers

Review of attachment 133009 [details] [review]:
-----------------------------------------------------------------

r+
Comment 241 Philip Withnall 2017-07-31 14:12:34 UTC
Comment on attachment 133010 [details] [review]
test/uid-permissions: Assert that AddServer is privileged

Review of attachment 133010 [details] [review]:
-----------------------------------------------------------------

r+ to this patch, but a question about general refactoring improvements to the test code (which should be addressed in a separate patch, if at all).

::: test/uid-permissions.c
@@ +232,5 @@
> +
> +  if (m == NULL)
> +    g_error ("OOM");
> +
> +  if (!dbus_connection_send_with_reply (f->conn, m, &pc,

Question: would it be possible to factor out the test code for sending a message and storing the reply? It gets repeated a lot. Something like:

test_send_message_and_store_reply (g_steal_pointer (&m), f->conn, f->ctx, &pc, DBUS_TIMEOUT_USE_DEFAULT);

it would expand to the code between:

if (m == NULL)
  g_error ("OOM");

and test_main_context_iterate().
Comment 242 Philip Withnall 2017-07-31 14:17:39 UTC
Comment on attachment 133011 [details] [review]
test/containers: New test

Review of attachment 133011 [details] [review]:
-----------------------------------------------------------------

r+
Comment 243 Philip Withnall 2017-07-31 14:19:14 UTC
Comment on attachment 133012 [details] [review]
bus/containers: Do some basic checking on the parameters

Review of attachment 133012 [details] [review]:
-----------------------------------------------------------------

r+
Comment 244 Philip Withnall 2017-07-31 14:20:28 UTC
Comment on attachment 133013 [details] [review]
test/containers: Exercise the new parameter checking

Review of attachment 133013 [details] [review]:
-----------------------------------------------------------------

r+
Comment 245 Philip Withnall 2017-07-31 14:29:07 UTC
Comment on attachment 133014 [details] [review]
bus/containers: Build a global data structure for container instances

Review of attachment 133014 [details] [review]:
-----------------------------------------------------------------

r+
Comment 246 Philip Withnall 2017-07-31 14:49:06 UTC
Comment on attachment 133015 [details] [review]
bus/containers: Create a DBusServer and add it to the main loop

Review of attachment 133015 [details] [review]:
-----------------------------------------------------------------

r+ with two nitpicks.

::: bus/containers.c
@@ +85,5 @@
> +  _dbus_string_init_const (&self->address_template, "");
> +
> +  /* Make it mutable */
> +  if (!_dbus_string_init (&self->address_template))
> +    goto oom;

Nitpick: An internal convenience function for this init-const-then-reinit-rw approach for DBusString would be a bit clearer (and more reusable).

@@ +149,5 @@
>  
> +static BusContainerInstance *
> +bus_container_instance_ref (BusContainerInstance *self)
> +{
> +  _dbus_assert (self->refcount > 0);

Paranoia: potentially add:
_dbus_assert (self->refcount < INTMAX);

(Same for the other _ref() functions which have been added in this patch set.)
Comment 247 Philip Withnall 2017-07-31 14:50:11 UTC
Comment on attachment 133016 [details] [review]
bus_context_add_incoming_connection: factor out

Review of attachment 133016 [details] [review]:
-----------------------------------------------------------------

r+
Comment 248 Philip Withnall 2017-07-31 14:51:47 UTC
Comment on attachment 133017 [details] [review]
bus/containers: Set up new connections to join the bus

Review of attachment 133017 [details] [review]:
-----------------------------------------------------------------

r+

::: bus/containers.c
@@ +290,5 @@
>  {
> +  BusContainerInstance *instance = data;
> +
> +  if (!bus_context_add_incoming_connection (instance->context, new_connection))
> +    return;

Do we want to log a warning on the OOM failure case? Otherwise it essentially gets silently ignored.
Comment 249 Philip Withnall 2017-07-31 15:15:17 UTC
Comment on attachment 133018 [details] [review]
test/containers: Exercise a successful call to AddServer

Review of attachment 133018 [details] [review]:
-----------------------------------------------------------------

r-, type safety bug which doesn’t affect correctness.

::: test/containers.c
@@ +154,5 @@
> +   * system infrastructure to create; so it's OK for AddServer to fail
> +   * when uninstalled, although not OK if it fails as an installed-test. */
> +  if (f->error != NULL &&
> +      _dbus_getuid () == 0 &&
> +      _dbus_getenv ("DBUS_TEST_UNINSTALLED") != 0)

_dbus_getenv() returns a const char*, not an int.
Comment 250 Philip Withnall 2017-07-31 15:38:15 UTC
Comment on attachment 133019 [details] [review]
bus/containers: Require connecting uid to match caller of AddServer

Review of attachment 133019 [details] [review]:
-----------------------------------------------------------------

r+

::: bus/containers.c
@@ +318,5 @@
> +   *
> +   * TODO: For the system bus we might want a way to opt-in to allowing
> +   * other uids, in which case we would refrain from overwriting the
> +   * BusContext's unix_user_function; but that isn't part of the
> +   * lowest-common-denominator implementation. */

Is this TODO comment worth a follow-up bug report? I’m not sure it is.
Comment 251 Philip Withnall 2017-07-31 15:40:59 UTC
Comment on attachment 133020 [details] [review]
test-utils-glib: Add failable functions to connect to a bus

Review of attachment 133020 [details] [review]:
-----------------------------------------------------------------

r+
Comment 252 Philip Withnall 2017-07-31 15:43:21 UTC
Comment on attachment 133021 [details] [review]
test-utils-glib: Factor out functions for switching uid

Review of attachment 133021 [details] [review]:
-----------------------------------------------------------------

r+
Comment 253 Philip Withnall 2017-07-31 15:44:38 UTC
Comment on attachment 133022 [details] [review]
test-utils-glib: Add function to connect with GDBus as another uid

Review of attachment 133022 [details] [review]:
-----------------------------------------------------------------

r+
Comment 254 Philip Withnall 2017-07-31 15:45:36 UTC
Comment on attachment 133023 [details] [review]
test/containers: Exercise connecting to the new socket as the wrong uid

Review of attachment 133023 [details] [review]:
-----------------------------------------------------------------

r+
Comment 255 Philip Withnall 2017-07-31 15:49:12 UTC
Comment on attachment 133024 [details] [review]
bus/containers: Each connection to a container holds a reference

Review of attachment 133024 [details] [review]:
-----------------------------------------------------------------

r+
Comment 256 Philip Withnall 2017-07-31 15:54:13 UTC
Comment on attachment 133025 [details] [review]
bus/containers: Link each container to its initiating connection

Review of attachment 133025 [details] [review]:
-----------------------------------------------------------------

r+

::: bus/containers.c
@@ +54,4 @@
>    BusContext *context;
>    BusContainers *containers;
>    DBusServer *server;
> +  DBusConnection *creator;

Nitpick: In honour of the music driving these reviews, this variable should be changed to `DBusConnection *kreator;`
Comment 257 Philip Withnall 2017-07-31 16:00:16 UTC
Comment on attachment 133026 [details] [review]
bus/containers: Shut down container servers when initiator goes away

Review of attachment 133026 [details] [review]:
-----------------------------------------------------------------

r+
Comment 258 Philip Withnall 2017-07-31 16:06:47 UTC
Comment on attachment 133027 [details] [review]
bus/containers: Give each instance a list of all its connections

Review of attachment 133027 [details] [review]:
-----------------------------------------------------------------

r+
Comment 259 Philip Withnall 2017-08-01 07:13:22 UTC
Comment on attachment 133028 [details] [review]
bus/containers: Implement methods to stop containers explicitly

Review of attachment 133028 [details] [review]:
-----------------------------------------------------------------

r+
Comment 260 Philip Withnall 2017-08-01 07:14:51 UTC
Comment on attachment 133029 [details] [review]
bus/containers: Don't allow stopping other users' containers

Review of attachment 133029 [details] [review]:
-----------------------------------------------------------------

r+
Comment 261 Philip Withnall 2017-08-01 07:35:32 UTC
Comment on attachment 133030 [details] [review]
test/containers: Exercise the various ways to stop a container

Review of attachment 133030 [details] [review]:
-----------------------------------------------------------------

r+
Comment 262 Philip Withnall 2017-08-01 09:37:00 UTC
Comment on attachment 133031 [details] [review]
bus/containers: Emit InstanceRemoved signal

Review of attachment 133031 [details] [review]:
-----------------------------------------------------------------

r+
Comment 263 Philip Withnall 2017-08-01 09:41:10 UTC
Comment on attachment 133032 [details] [review]
test/containers: Assert that InstanceRemoved is emitted

Review of attachment 133032 [details] [review]:
-----------------------------------------------------------------

r-, memory leak.

::: test/containers.c
@@ +150,5 @@
> +      (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION |
> +       G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT),
> +      NULL, NULL, &f->error);
> +  g_assert_no_error (f->error);
> +  f->containers_removed = g_hash_table_new_full (g_str_hash, g_str_equal,

I don’t think f->containers_removed is ever freed.
Comment 264 Philip Withnall 2017-08-01 09:42:19 UTC
Comment on attachment 133033 [details] [review]
bus/containers: Indicate in loginfo whether connection is contained

Review of attachment 133033 [details] [review]:
-----------------------------------------------------------------

r+
Comment 265 Philip Withnall 2017-08-01 09:45:21 UTC
Comment on attachment 133034 [details] [review]
bus/driver: Treat connections from inside containers as unprivileged

Review of attachment 133034 [details] [review]:
-----------------------------------------------------------------

r+
Comment 266 Philip Withnall 2017-08-01 09:47:14 UTC
Comment on attachment 133035 [details] [review]
test/containers: Check that containers can't make new containers

Review of attachment 133035 [details] [review]:
-----------------------------------------------------------------

r+ while you r+
Comment 267 Philip Withnall 2017-08-01 09:47:50 UTC
Comment on attachment 133036 [details] [review]
test/containers: Check that connections from containers are unprivileged

Review of attachment 133036 [details] [review]:
-----------------------------------------------------------------

r+
Comment 268 Philip Withnall 2017-08-01 09:50:00 UTC
Comment on attachment 133037 [details] [review]
bus/driver: Add a flag for methods that can't be invoked by containers

Review of attachment 133037 [details] [review]:
-----------------------------------------------------------------

r-, comment problems.

::: bus/driver.c
@@ +2403,5 @@
>    METHOD_FLAG_PRIVILEGED = (1 << 1),
>  
> +  /* If set, callers must be privileged. On Unix, the uid of the connection
> +   * must either be the uid of this process, or 0 (root). On Windows,
> +   * the SID of the connection must be the SID of this process. */

This documentation comment is a near-duplicate of the one above. It would be good to clarify somewhere what the difference is between the two flags.
Comment 269 Philip Withnall 2017-08-01 09:50:49 UTC
Comment on attachment 133038 [details] [review]
bus/driver: Containers can't use the Verbose and Stats interfaces

Review of attachment 133038 [details] [review]:
-----------------------------------------------------------------

r+
Comment 270 Philip Withnall 2017-08-01 09:52:19 UTC
Comment on attachment 133039 [details] [review]
bus/driver: Add basic container info to GetConnectionCredentials()

Review of attachment 133039 [details] [review]:
-----------------------------------------------------------------

r+
Comment 271 Philip Withnall 2017-08-01 09:52:49 UTC
Comment on attachment 133040 [details] [review]
test/dbus-daemon: Assert absence of Containers1 credentials

Review of attachment 133040 [details] [review]:
-----------------------------------------------------------------

r+
Comment 272 Philip Withnall 2017-08-01 09:58:35 UTC
Comment on attachment 133041 [details] [review]
bus/driver: Add GetConnectionInstance(), GetInstanceInfo()

Review of attachment 133041 [details] [review]:
-----------------------------------------------------------------

r+
Comment 273 Philip Withnall 2017-08-01 10:05:19 UTC
Comment on attachment 133042 [details] [review]
t/containers: Exercise trivial and non-trivial container metadata

Review of attachment 133042 [details] [review]:
-----------------------------------------------------------------

r+

::: test/containers.c
@@ +472,5 @@
> +  g_variant_dict_insert (&dict, "IsCrepuscular", "b", TRUE);
> +  g_variant_dict_insert (&dict, "NChildren", "u", 2);
> +
> +  parameters = g_variant_new ("(ss@a{sv}a{sv})",
> +                              "org.example.Springwatch",

I worry about your daytime TV habits.
Comment 274 Philip Withnall 2017-08-01 10:06:03 UTC
Comment on attachment 133043 [details] [review]
test/containers: Check that GetInstanceInfo stops working

Review of attachment 133043 [details] [review]:
-----------------------------------------------------------------

r+
Comment 275 Philip Withnall 2017-08-01 10:39:39 UTC
Comment on attachment 133044 [details] [review]
bus: Add (unused) settings for resource limits for containers

Review of attachment 133044 [details] [review]:
-----------------------------------------------------------------

r+ with one nitpick.

::: bus/bus.h
@@ +66,4 @@
>    int max_match_rules_per_connection; /**< Max number of match rules for a single connection */
>    int max_replies_per_connection;     /**< Max number of replies that can be pending for each connection */
>    int reply_timeout;                  /**< How long to wait before timing out a reply */
> +  int max_containers;               /**< Max number of restricted servers for app-containers*/

Nitpick: Missing space before `*/`.
Comment 276 Philip Withnall 2017-08-01 10:41:56 UTC
Comment on attachment 133045 [details] [review]
bus/containers: Limit the size of metadata we will store

Review of attachment 133045 [details] [review]:
-----------------------------------------------------------------

r+
Comment 277 Philip Withnall 2017-08-01 10:42:31 UTC
Comment on attachment 133046 [details] [review]
bus/containers: Enforce max_containers limit

Review of attachment 133046 [details] [review]:
-----------------------------------------------------------------

r+
Comment 278 Philip Withnall 2017-08-01 10:43:03 UTC
Comment on attachment 133047 [details] [review]
bus/containers: Enforce max_connections_per_container

Review of attachment 133047 [details] [review]:
-----------------------------------------------------------------

r+
Comment 279 Philip Withnall 2017-08-01 10:46:15 UTC
Comment on attachment 133048 [details] [review]
containers: enforce max_containers_per_user

Review of attachment 133048 [details] [review]:
-----------------------------------------------------------------

r+
Comment 280 Philip Withnall 2017-08-01 11:00:08 UTC
Comment on attachment 133049 [details] [review]
test/containers: Exercise the resource limits

Review of attachment 133049 [details] [review]:
-----------------------------------------------------------------

r-, minor comments.

::: test/containers.c
@@ +1149,5 @@
> +{
> +#ifdef HAVE_CONTAINERS_TEST
> +  GVariant *parameters;
> +  GVariant *tuple;
> +  gchar *extra = NULL;

extra is never used (apart from being free). Drop it.

@@ +1487,5 @@
> +
> +  /* Floating reference, call_..._sync takes ownership */
> +  parameters = g_variant_new ("(ss@a{sv}a{sv})",
> +                              "com.wasteheadquarters",
> +                              "Packt Like Sardines in a Crushed Tin Box",

Nitpick: s/Crushed/Crushd/
Comment 281 Philip Withnall 2017-08-01 11:00:27 UTC
Comment on attachment 133050 [details] [review]
Revert "test/uid-permissions: Assert that AddServer is privileged"

Review of attachment 133050 [details] [review]:
-----------------------------------------------------------------

r+
Comment 282 Philip Withnall 2017-08-01 11:01:50 UTC
Comment on attachment 133051 [details] [review]
bus/driver: Allow unprivileged connections to create app-containers

Review of attachment 133051 [details] [review]:
-----------------------------------------------------------------

r+
Comment 283 Philip Withnall 2017-08-01 11:02:23 UTC
Comment on attachment 133052 [details] [review]
system.conf: Allow creating containers on the system bus

Review of attachment 133052 [details] [review]:
-----------------------------------------------------------------

r+
Comment 284 Philip Withnall 2017-08-01 11:04:24 UTC
Comment on attachment 133053 [details] [review]
test/containers: Use a shorter path for XDG_RUNTIME_DIR

Review of attachment 133053 [details] [review]:
-----------------------------------------------------------------

r+ with a nitpick.

::: test/containers.c
@@ +1552,5 @@
> +
> +  runtime_dir = g_dir_make_tmp ("dbus-test-containers.XXXXXX", &error);
> +
> +  if (runtime_dir == NULL)
> +    g_error ("%s", error->message);

Nitpick: `error` is leaked, though this is an aborting path.
Comment 285 Philip Withnall 2017-08-01 11:06:44 UTC
review- overall, but that’s for a few minor comments and some nitpicks. Overall, I have very few review comments for this patch set. Yay!

I’d rather you didn’t upload the entire patch set to Bugzilla again, since it’s hard to track the review comments between iterations of a patch. Either push directly with fixes in place, or only upload the changed patches (ignore rebase changes unless they’re non-trivial). Thanks.
Comment 286 Simon McVittie 2017-11-06 15:46:43 UTC
(In reply to Philip Withnall from comment #234)
> Comment on attachment 133003 [details] [review]
> travis-ci: Enable/disable more features in various builds
>
> r+

Rebased with very minor conflicts, pushed to master for 1.13.0.

(In reply to Philip Withnall from comment #235)
> Comment on attachment 133004 [details] [review]
> cmake: Match AC_DEFINE more precisely, respecting [] quoting
> 
> I have not reviewed this regex in detail, because life is too short. If it
> works, ship it.

I'll call that "Acked-by". Rebased, pushed to master for 1.13.0.

(In reply to Philip Withnall from comment #236)
> Comment on attachment 133005 [details] [review]
> Add a simplified backport of g_steal_pointer()
> 
> r+

Rebased with very minor conflicts, pushed to master for 1.13.0.

(In reply to Philip Withnall from comment #237)
> Comment on attachment 133006 [details] [review]
> _dbus_asv_add_object_path: Add
> 
> r+, still

Pushed to master for 1.13.0.
Comment 287 Simon McVittie 2017-11-06 15:47:56 UTC
(In reply to Philip Withnall from comment #238)
> Comment on attachment 133007 [details] [review]
> spec: Document the initial Containers1 interface
> 
> Nitpick: ‘applied to them by the message bus’ to clarify that bit?

Fixed locally

> > +          If the given object path represents an active container instance,
> 
> Does that include container instances which have had StopListening called on
> them (but which still have some open connections)?

Yes it does. I've tried to clarify this.

> The term ‘active’ here is
> ill-defined. The same ‘active’ term is used in a few other places in this
> patch and is ill-defined there too.

Fixed locally. My intention was that a container instance object path is active (I used the word "valid" in the new version of the commit) if either it owns at least one connection, or it could produce connections in future - which right now means it is listening and accept()ing, but after Bug #101898 will also include container instances whose server is not ready to accept() yet.

(In reply to Philip Withnall from comment #239)
> Comment on attachment 133008 [details] [review]
> driver: Add a stub implementation of the Containers1 interface
> 
> Also needs to be in README.cmake and cmake/config.h, I think.

Good catch, fixed locally.

(In reply to Philip Withnall from comment #241)
> Comment on attachment 133010 [details] [review]
> test/uid-permissions: Assert that AddServer is privileged
> 
> Question: would it be possible to factor out the test code for sending a
> message and storing the reply? It gets repeated a lot.

I think I'd rather do this as an orthogonal thing.

(In reply to Philip Withnall from comment #246)
> Comment on attachment 133015 [details] [review]
> bus/containers: Create a DBusServer and add it to the main loop
> 
> Nitpick: An internal convenience function for this init-const-then-reinit-rw
> approach for DBusString would be a bit clearer (and more reusable).

Bug #89104 would be better still. I can't say I'm particularly keen on adding a middle ground where a function is failable, but alters the DBusString even if it fails... that seems fairly horrible.

I think I'd rather do this as an orthogonal thing.

> Paranoia: potentially add:
> _dbus_assert (self->refcount < INTMAX);

Fixed locally.

> (Same for the other _ref() functions which have been added in this patch
> set.)

I don't see any others, except bus_container_ref() which already had this.
Comment 288 Simon McVittie 2017-11-06 16:30:23 UTC
(In reply to Philip Withnall from comment #248)
> Comment on attachment 133017 [details] [review]
> bus/containers: Set up new connections to join the bus
> 
> r+
> 
> Do we want to log a warning on the OOM failure case? Otherwise it
> essentially gets silently ignored.

Good catch. It turns out we don't log a warning in the OOM failure case (or the failed-to-set-up-LSM case) for unconfined connections either... we probably should, but that's an orthogonal bug. I've opened Bug #103592.
Comment 289 Simon McVittie 2017-11-06 16:47:07 UTC
(In reply to Philip Withnall from comment #250)
> Comment on attachment 133019 [details] [review]
> bus/containers: Require connecting uid to match caller of AddServer
> 
> r+
> 
> > +   * TODO: For the system bus we might want [...]
> 
> Is this TODO comment worth a follow-up bug report? I’m not sure it is.

I think it probably is worth it. Bug #103457.

(In reply to Philip Withnall from comment #251)
> Comment on attachment 133020 [details] [review]
> test-utils-glib: Add failable functions to connect to a bus
> 
> r+

Staged to push for 1.13.0 (will be pushed if tests pass)

(In reply to Philip Withnall from comment #252)
> Comment on attachment 133021 [details] [review]
> test-utils-glib: Factor out functions for switching uid
> 
> r+

Staged to push for 1.13.0

(In reply to Philip Withnall from comment #253)
> Comment on attachment 133022 [details] [review]
> test-utils-glib: Add function to connect with GDBus as another uid
> 
> r+

Staged to push for 1.13.0

(In reply to Philip Withnall from comment #256)
> Comment on attachment 133025 [details] [review]
> Nitpick: In honour of the music driving these reviews, this variable should
> be changed to `DBusConnection *kreator;`

Yeah, I'll consider it[1].

[1] After due consideration: No.
Comment 290 Simon McVittie 2017-11-06 17:02:42 UTC
(In reply to Philip Withnall from comment #263)
> Comment on attachment 133032 [details] [review]
> test/containers: Assert that InstanceRemoved is emitted
> 
> I don’t think f->containers_removed is ever freed.

Good catch, fixed locally.

(In reply to Philip Withnall from comment #268)
> Comment on attachment 133037 [details] [review]
> 
> This documentation comment is a near-duplicate of the one above. It would be
> good to clarify somewhere what the difference is between the two flags.

You're right, that comment made no sense. Fixed locally.

(In reply to Philip Withnall from comment #275)
> Comment on attachment 133044 [details] [review]
> bus: Add (unused) settings for resource limits for containers
> 
> Nitpick: Missing space before `*/`.

It's aligned with 80% of the members of BusLimits, just not the one immediately above (which is too long). I re-indented that one by an extra tab-stop to match other long names further up (I'm not going to bother making that available as a separate commit).

(In reply to Philip Withnall from comment #280)
> Comment on attachment 133049 [details] [review]
> test/containers: Exercise the resource limits
> 
> extra is never used (apart from being free). Drop it.

Fixed locally.

> Nitpick: s/Crushed/Crushd/

Nice to see someone else likes Radiohead. Fixed locally.

(In reply to Philip Withnall from comment #284)
> Comment on attachment 133053 [details] [review]
> test/containers: Use a shorter path for XDG_RUNTIME_DIR
> 
> Nitpick: `error` is leaked, though this is an aborting path.

I changed this to be non-aborting and use the special TAP token "Bail out!" instead, then not leak error. I also took the opportunity to pull it earlier in the branch.
Comment 291 Ralf Habacker 2017-11-06 17:26:26 UTC
Comment on attachment 133003 [details] [review]
travis-ci: Enable/disable more features in various builds

Review of attachment 133003 [details] [review]:
-----------------------------------------------------------------

this patch seems to be already applied or superseeded in git master - can anyone confirm ?
Comment 292 Ralf Habacker 2017-11-06 17:27:40 UTC
Comment on attachment 133005 [details] [review]
Add a simplified backport of g_steal_pointer()

Review of attachment 133005 [details] [review]:
-----------------------------------------------------------------

This patch seems to be applied or superseeded
Comment 293 Ralf Habacker 2017-11-06 17:29:28 UTC
Comment on attachment 133015 [details] [review]
bus/containers: Create a DBusServer and add it to the main loop

Review of attachment 133015 [details] [review]:
-----------------------------------------------------------------

Running 'git apply 101354' on recent git master results into:

Wende an: bus/containers: Create a DBusServer and add it to the main loop
error: SHA-1 Information is missing or invalid (bus/Makefile.am).
Comment 294 Simon McVittie 2017-11-06 17:56:05 UTC
(In reply to Ralf Habacker from comment #291)
> this patch seems to be already applied or superseeded in git master - can
> anyone confirm ?

Yes, some of these patches have already been applied, as noted in the most recent 10 comments. The rest have already been reviewed and I am in the process of fixing Philip's comments on them. There's no need to review anything here until I've finished doing that.

When I've fixed those review comments, I'm not intending to reattach the entire branch, as Philip requested in Comment #285. I'll push a revised version to Github, with git-rebase "squash!" commit messages for commits that are intended to modify a previous commit.

If you are about to review things, please see the previous 290 comments (!) and Bug #100344 for context. This feature is Unix-specific and rather subtle, so if you specialize in Windows stuff, you're probably not the ideal reviewer for it.

Please do not push anything from this bug. I'll push the changes when they are reviewed, tested and ready.
Comment 295 Simon McVittie 2017-11-06 20:15:30 UTC
I've updated https://github.com/smcv/dbus/commits/containers-minimum-101354 with most of the changes Philip asked for.

The changes are in "squash!" commits which are going to be squashed into prior commits, but I have not done that yet, because separated commits are the form that Philip said he'd prefer to review. I think I'll also squash "test/containers: Use a shorter path for XDG_RUNTIME_DIR" into the commit before it.

Reminder: please do not push anything from this bug. I'll do that when it has been reviewed and tested.

I am probably not going to push the rest of the changes from here until I've been able to run them against a "live" Flatpak installation that has the corresponding changes.
Comment 296 Simon McVittie 2017-11-06 20:20:08 UTC
(In reply to Simon McVittie from comment #287)
> (In reply to Philip Withnall from comment #238)
> > Question: would it be possible to factor out the test code for sending a
> > message and storing the reply? It gets repeated a lot.
> 
> I think I'd rather do this as an orthogonal thing.

Bug #103600.

> (In reply to Philip Withnall from comment #246)
> > Nitpick: An internal convenience function for this init-const-then-reinit-rw
> > approach for DBusString would be a bit clearer (and more reusable).
> 
> Bug #89104 would be better still. I can't say I'm particularly keen on
> adding a middle ground where a function is failable, but alters the
> DBusString even if it fails... that seems fairly horrible.
> 
> I think I'd rather do this as an orthogonal thing.

I started looking into this and accidentally found Bug #103597.
Comment 297 Simon McVittie 2017-11-21 16:42:29 UTC
(In reply to Philip Withnall from comment #241)
> test/uid-permissions: Assert that AddServer is privileged
>
> Question: would it be possible to factor out the test code for sending a
> message and storing the reply? It gets repeated a lot.

I've done that on Bug #103600. However, to reduce the critical path for this branch, I am not going to rebase it onto Bug #103600, because there is only one use of the pattern in this branch, and it's in code whose addition is reverted later.
Comment 298 Simon McVittie 2017-11-21 18:00:09 UTC
(In reply to Simon McVittie from comment #296)
> (In reply to Simon McVittie from comment #287)
> > (In reply to Philip Withnall from comment #246)
> > > Nitpick: An internal convenience function for this init-const-then-reinit-rw
> > > approach for DBusString would be a bit clearer (and more reusable).
> > 
> > Bug #89104 would be better still. I can't say I'm particularly keen on
> > adding a middle ground where a function is failable, but alters the
> > DBusString even if it fails... that seems fairly horrible.

I've rebased this branch on Bug #89104. Still at: https://github.com/smcv/dbus/commits/containers-minimum-101354

Changes to use _DBUS_STRING_INIT_INVALID:

https://github.com/smcv/dbus/commit/0fc58ba7e6c3493a179357565eb667a6b1c9b407
https://github.com/smcv/dbus/commit/0c3363d00eeabab857a6647cb6d8cee967daaeac
https://github.com/smcv/dbus/commit/64b4e32be7e58a97547d4056f3c1d6158182e1db (fixes a memory leak)

Unrelated build fixes:

https://github.com/smcv/dbus/commit/9a030a4d0a33f735a2f9c7324f4bf812d021f80e
Comment 299 Philip Withnall 2017-12-05 13:17:07 UTC
(In reply to Simon McVittie from comment #297)
> (In reply to Philip Withnall from comment #241)
> > test/uid-permissions: Assert that AddServer is privileged
> >
> > Question: would it be possible to factor out the test code for sending a
> > message and storing the reply? It gets repeated a lot.
> 
> I've done that on Bug #103600. However, to reduce the critical path for this
> branch, I am not going to rebase it onto Bug #103600, because there is only
> one use of the pattern in this branch, and it's in code whose addition is
> reverted later.

Good plan.

(In reply to Simon McVittie from comment #298)
> (In reply to Simon McVittie from comment #296)
> > (In reply to Simon McVittie from comment #287)
> > > (In reply to Philip Withnall from comment #246)
> > > > Nitpick: An internal convenience function for this init-const-then-reinit-rw
> > > > approach for DBusString would be a bit clearer (and more reusable).
> > > 
> > > Bug #89104 would be better still. I can't say I'm particularly keen on
> > > adding a middle ground where a function is failable, but alters the
> > > DBusString even if it fails... that seems fairly horrible.
> 
> I've rebased this branch on Bug #89104. Still at:
> https://github.com/smcv/dbus/commits/containers-minimum-101354
> 
> Changes to use _DBUS_STRING_INIT_INVALID:
> 
> https://github.com/smcv/dbus/commit/0fc58ba7e6c3493a179357565eb667a6b1c9b407
> https://github.com/smcv/dbus/commit/0c3363d00eeabab857a6647cb6d8cee967daaeac
> https://github.com/smcv/dbus/commit/64b4e32be7e58a97547d4056f3c1d6158182e1db
> (fixes a memory leak)
> 
> Unrelated build fixes:
> 
> https://github.com/smcv/dbus/commit/9a030a4d0a33f735a2f9c7324f4bf812d021f80e

OK. I’ll comment on the individual commits there. Hopefully that works out OK as a workflow for both of us; please say so if not.
Comment 300 Philip Withnall 2017-12-05 13:51:56 UTC
I’ve finished reviewing the `squash!` commits on https://github.com/smcv/dbus/commits/containers-minimum-101354 and have left comments on all of them: either an r+, or a nitpick. I don’t think every comment I left was trivial.

Ship it? :-)

I think the ‘leave review comments on the `squash!` commits’ workflow has worked well for me, since it gives the minimal delta to review. Let me know whether it works for you, or whether there’s too much accounting overhead in tracking all the `squash!` patches and their relationship to how you want the branch to look.
Comment 303 Philip Withnall 2017-12-11 18:42:06 UTC
(In reply to Simon McVittie from comment #302)
> Remaining fixes to be approved:
> 
> *
> https://github.com/smcv/dbus/commit/df837beb2fecb8caaaf6a80669d94d1fd72967a6
> 
> *
> https://github.com/smcv/dbus/commit/a6f5870b81ab6e5edf87b8138c7a2eca13c29881
> now fixed by
> https://github.com/smcv/dbus/commit/9e0f7d8cc5228649b6982d2739861c896a95848b
> 
> both of which are minor issues.

All approved. Thanks!
Comment 304 Simon McVittie 2017-12-12 17:51:11 UTC
Merged \o/

I made the merge artificially a non-fast-forward, so that we can easily revert this feature if we need to.

Summary of things not done, other than of course "all the other bugs that block Bug #100344":

(In reply to Allison Lortie (desrt) from comment #116)
> The "Containers1" style naming thing is sort of distasteful.  If we ever
> have a second version of this interface (unlikely) then we can perfectly
> well call that "Containers2" anyway.

Acknowledged but not changed. I've kept the Containers1 name anyway, because I think it's time we started taking our own advice where API naming is concerned.

(But if other maintainers feel particularly strongly about this, it can be changed.)

> The fully-namespaced keys inside of the GetConnectionCredentials return
> value are silly and wasteful.

Acknowledged but not changed, same reasoning.

> I was sad to notice that the fd-passing style of creation is gone.

Bug #101898

> The same-uid check is a hack and it shows off some of the problems with the
> create-a-path-in-a-public-directory approach.

Bug #103457

> The main missing item here in terms of usability is the addition of the
> header field that we discussed for marking messages from sandboxed apps.

Bug #101899, which is one of the next things on my to-do list here

> I recall that we discussed that [the sandbox ID] should be a uint64 for
> efficiency/simplicity reasons
...
> Why did you go with the object path approach instead of ints for identifying
> containers?

Because we keep telling D-Bus API authors that they should use object paths instead of inventing their own opaque token scheme with strings or integers, and it seemed like it's time to take our own advice. But I'm generating the object paths from a uint64 anyway, so we can easily switch if other maintainers feel strongly about this.

> Any thoughts on sandboxed apps adding match rules?

Bug #101902 (which is quite a large one)

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.