Bug 101354 - Minimal version of restricted, identifiable bus servers for containers (#100344)
Summary: Minimal version of restricted, identifiable bus servers for containers (#100344)
Status: ASSIGNED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review-
Keywords: patch
Depends on: 101964 101362 101566 101567 101568 101569 101570 101700 101895
Blocks: 100344 101898 101899 101902 101903
  Show dependency treegraph
 
Reported: 2017-06-08 16:58 UTC by Simon McVittie
Modified: 2017-08-01 11:06 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[01] _dbus_asv_add_object_path: Add (2.67 KB, patch)
2017-06-23 16:17 UTC, Simon McVittie
Details | Splinter Review
bus_driver_send_ack_reply: Make available to other modules (3.71 KB, patch)
2017-06-23 16:24 UTC, Simon McVittie
Details | Splinter Review
[02] spec: Document the initial Containers1 interface (18.26 KB, patch)
2017-06-23 16:25 UTC, Simon McVittie
Details | Splinter Review
[03] driver: Add a stub implementation of the Containers1 interface (9.11 KB, patch)
2017-06-23 16:25 UTC, Simon McVittie
Details | Splinter Review
[04] test/uid-permissions: Assert that AddContainerServer is privileged (3.44 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
[05] test/containers: New test (5.53 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
[07] containers: Do some basic checking on the parameters (3.69 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
[08] test/containers: Exercise the new parameter checking (4.00 KB, patch)
2017-06-23 16:27 UTC, Simon McVittie
Details | Splinter Review
[09] bus/containers: Build a global data structure for container instances (12.31 KB, patch)
2017-06-23 16:27 UTC, Simon McVittie
Details | Splinter Review
[10] bus/containers: Create a DBusServer and add it to the main loop (14.15 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
[11] bus_context_add_incoming_connection: factor out (2.31 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
[12] bus/containers: Set up new connections to join the bus (862 bytes, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
[13] test/containers: Exercise a successful call to AddContainerServer (5.94 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
[14] bus/containers: Require connecting uid to match caller of AddContainerServer (3.43 KB, patch)
2017-06-23 16:29 UTC, Simon McVittie
Details | Splinter Review
[15] bus/containers: Each connection to a container holds a reference (2.46 KB, patch)
2017-06-23 16:29 UTC, Simon McVittie
Details | Splinter Review
[16] bus/containers: Link each container to its initiating connection (7.94 KB, patch)
2017-06-23 16:30 UTC, Simon McVittie
Details | Splinter Review
[17] bus/containers: Shut down container servers when initiator goes away (3.39 KB, patch)
2017-06-23 16:30 UTC, Simon McVittie
Details | Splinter Review
[18] bus/containers: Give each instance a list of all its connections (3.46 KB, patch)
2017-06-23 16:31 UTC, Simon McVittie
Details | Splinter Review
[19] bus/containers: Implement methods to stop containers explicitly (6.43 KB, patch)
2017-06-23 16:31 UTC, Simon McVittie
Details | Splinter Review
[20] bus/containers: Don't allow stopping other users' containers (2.66 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
[21] test/containers: Exercise the various ways to stop a container (12.67 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
[22] bus/containers: Emit ContainerInstanceRemoved signal (4.97 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
[23] test/containers: Assert that ContainerInstanceRemoved is emitted (7.63 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
[24] bus/containers: Indicate in loginfo whether connection is contained (3.58 KB, patch)
2017-06-23 16:33 UTC, Simon McVittie
Details | Splinter Review
[25] bus/driver: Treat connections from inside containers as unprivileged (2.54 KB, patch)
2017-06-23 16:33 UTC, Simon McVittie
Details | Splinter Review
[26] test/containers: Check that containers can't make new containers (4.47 KB, patch)
2017-06-23 16:35 UTC, Simon McVittie
Details | Splinter Review
[27] test/containers: Check that connections from containers are unprivileged (1.46 KB, patch)
2017-06-23 16:36 UTC, Simon McVittie
Details | Splinter Review
[28] bus/driver: Add a flag for methods that can't be invoked by containers (2.70 KB, patch)
2017-06-23 16:37 UTC, Simon McVittie
Details | Splinter Review
[29] bus/driver: Containers can't use the Verbose and Stats interfaces (1.80 KB, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
[30] bus/driver: Add basic container info to GetConnectionCredentials() (1.56 KB, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
[31] test/dbus-daemon: Assert absence of Containers1 credentials (833 bytes, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
[32] bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo() (6.73 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise trivial and non-trivial container metadata (12.54 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
[34] test/containers: Check that GetContainerInstanceInfo stops working (3.42 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
[35] bus: Add (unused) settings for resource limits for containers (8.40 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
[36] bus/containers: Limit the size of metadata we will store (2.46 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
[37] bus/containers: Enforce max_containers limit (1.48 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
[38] bus/containers: Enforce max_connections_per_container (1.43 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
[39] containers: enforce max_containers_per_user (6.67 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review
[40] test/containers: Exercise the resource limits (16.80 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Allow unprivileged connections to create app-containers (1.33 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review
[02] spec: Document the initial Containers1 interface (20.05 KB, patch)
2017-06-30 11:52 UTC, Simon McVittie
Details | Splinter Review
[33] test/containers: Exercise trivial and non-trivial container metadata (12.82 KB, patch)
2017-06-30 11:54 UTC, Simon McVittie
Details | Splinter Review
[41] Revert "test/uid-permissions: Assert that AddContainerServer is privileged" (3.55 KB, patch)
2017-06-30 11:56 UTC, Simon McVittie
Details | Splinter Review
[42] bus/driver: Allow unprivileged connections to create app-containers (1.33 KB, patch)
2017-06-30 11:57 UTC, Simon McVittie
Details | Splinter Review
[33] test/containers: Exercise trivial and non-trivial container metadata (12.78 KB, patch)
2017-06-30 13:37 UTC, Simon McVittie
Details | Splinter Review
_dbus_asv_add_object_path: Add (2.66 KB, patch)
2017-07-07 18:52 UTC, Simon McVittie
Details | Splinter Review
spec: Document the initial Containers1 interface (20.04 KB, patch)
2017-07-07 18:53 UTC, Simon McVittie
Details | Splinter Review
driver: Add a stub implementation of the Containers1 interface (9.10 KB, patch)
2017-07-07 18:53 UTC, Simon McVittie
Details | Splinter Review
test/uid-permissions: Assert that AddContainerServer is privileged (3.43 KB, patch)
2017-07-07 18:53 UTC, Simon McVittie
Details | Splinter Review
test/containers: New test (5.52 KB, patch)
2017-07-07 18:53 UTC, Simon McVittie
Details | Splinter Review
containers: Do some basic checking on the parameters (3.69 KB, patch)
2017-07-07 18:54 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the new parameter checking (3.99 KB, patch)
2017-07-07 18:54 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Build a global data structure for container instances (12.30 KB, patch)
2017-07-07 18:54 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Create a DBusServer and add it to the main loop (18.67 KB, patch)
2017-07-07 18:55 UTC, Simon McVittie
Details | Splinter Review
bus_context_add_incoming_connection: factor out (2.30 KB, patch)
2017-07-07 18:55 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Set up new connections to join the bus (855 bytes, patch)
2017-07-07 18:56 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise a successful call to AddContainerServer (8.35 KB, patch)
2017-07-07 18:56 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Require connecting uid to match caller of AddContainerServer (3.43 KB, patch)
2017-07-07 18:56 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Factor out functions for switching uid (3.97 KB, patch)
2017-07-07 18:56 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Add function to connect with GDBus as another uid (2.19 KB, patch)
2017-07-07 18:56 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise connecting to the new socket as the wrong uid (2.41 KB, patch)
2017-07-07 18:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Each connection to a container holds a reference (2.41 KB, patch)
2017-07-07 18:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Link each container to its initiating connection (7.93 KB, patch)
2017-07-07 18:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Shut down container servers when initiator goes away (3.39 KB, patch)
2017-07-07 18:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Give each instance a list of all its connections (3.46 KB, patch)
2017-07-07 18:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Implement methods to stop containers explicitly (6.46 KB, patch)
2017-07-07 18:58 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Don't allow stopping other users' containers (2.65 KB, patch)
2017-07-07 18:58 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the various ways to stop a container (14.66 KB, patch)
2017-07-07 18:59 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Emit ContainerInstanceRemoved signal (4.96 KB, patch)
2017-07-07 18:59 UTC, Simon McVittie
Details | Splinter Review
test/containers: Assert that ContainerInstanceRemoved is emitted (7.95 KB, patch)
2017-07-07 18:59 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Indicate in loginfo whether connection is contained (3.57 KB, patch)
2017-07-07 19:00 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Treat connections from inside containers as unprivileged (2.54 KB, patch)
2017-07-07 19:00 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that containers can't make new containers (3.75 KB, patch)
2017-07-07 19:00 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that connections from containers are unprivileged (1.55 KB, patch)
2017-07-07 19:00 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add a flag for methods that can't be invoked by containers (2.69 KB, patch)
2017-07-07 19:00 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Containers can't use the Verbose and Stats interfaces (1.79 KB, patch)
2017-07-07 19:01 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add basic container info to GetConnectionCredentials() (1.56 KB, patch)
2017-07-07 19:01 UTC, Simon McVittie
Details | Splinter Review
test/dbus-daemon: Assert absence of Containers1 credentials (824 bytes, patch)
2017-07-07 19:01 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo() (6.76 KB, patch)
2017-07-07 19:01 UTC, Simon McVittie
Details | Splinter Review
t/containers: Exercise trivial and non-trivial container metadata (12.23 KB, patch)
2017-07-07 19:01 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that GetContainerInstanceInfo stops working (3.47 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
bus: Add (unused) settings for resource limits for containers (8.39 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Limit the size of metadata we will store (2.46 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_containers limit (1.47 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_connections_per_container (1.42 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
containers: enforce max_containers_per_user (6.66 KB, patch)
2017-07-07 19:02 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the resource limits (16.25 KB, patch)
2017-07-07 19:03 UTC, Simon McVittie
Details | Splinter Review
Revert "test/uid-permissions: Assert that AddContainerServer is privileged" (3.54 KB, patch)
2017-07-07 19:03 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Allow unprivileged connections to create app-containers (1.32 KB, patch)
2017-07-07 19:03 UTC, Simon McVittie
Details | Splinter Review
system.conf: Allow creating containers on the system bus (957 bytes, patch)
2017-07-07 19:04 UTC, Simon McVittie
Details | Splinter Review
Add a simplified backport of g_steal_pointer() (1.60 KB, patch)
2017-07-24 19:03 UTC, Simon McVittie
Details | Splinter Review
_dbus_asv_add_object_path: Add (2.76 KB, patch)
2017-07-24 19:03 UTC, Simon McVittie
Details | Splinter Review
spec: Document the initial Containers1 interface (24.08 KB, patch)
2017-07-24 19:03 UTC, Simon McVittie
Details | Splinter Review
Add a simplified backport of g_steal_pointer() (1.60 KB, patch)
2017-07-24 19:03 UTC, Simon McVittie
Details | Splinter Review
_dbus_asv_add_object_path: Add (2.76 KB, patch)
2017-07-24 19:04 UTC, Simon McVittie
Details | Splinter Review
spec: Document the initial Containers1 interface (24.08 KB, patch)
2017-07-24 19:04 UTC, Simon McVittie
Details | Splinter Review
driver: Add a stub implementation of the Containers1 interface (9.22 KB, patch)
2017-07-24 19:05 UTC, Simon McVittie
Details | Splinter Review
test/uid-permissions: Assert that AddServer is privileged (3.53 KB, patch)
2017-07-24 19:05 UTC, Simon McVittie
Details | Splinter Review
test/containers: New test (5.70 KB, patch)
2017-07-24 19:05 UTC, Simon McVittie
Details | Splinter Review
test/uid-permissions: Assert that AddServer is privileged (3.70 KB, patch)
2017-07-24 19:07 UTC, Simon McVittie
Details | Splinter Review
test/containers: New test (5.70 KB, patch)
2017-07-24 19:07 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Do some basic checking on the parameters (3.70 KB, patch)
2017-07-24 19:08 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the new parameter checking (4.38 KB, patch)
2017-07-24 19:08 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Do some basic checking on the parameters (3.70 KB, patch)
2017-07-24 19:56 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the new parameter checking (4.38 KB, patch)
2017-07-24 19:56 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Build a global data structure for container instances (13.09 KB, patch)
2017-07-24 19:57 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Create a DBusServer and add it to the main loop (18.12 KB, patch)
2017-07-24 19:57 UTC, Simon McVittie
Details | Splinter Review
bus_context_add_incoming_connection: factor out (2.35 KB, patch)
2017-07-24 19:58 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Set up new connections to join the bus (908 bytes, patch)
2017-07-24 19:59 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise a successful call to AddServer (8.36 KB, patch)
2017-07-24 19:59 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Require connecting uid to match caller of AddServer (3.46 KB, patch)
2017-07-24 20:00 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Factor out functions for switching uid (4.02 KB, patch)
2017-07-24 20:00 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Add function to connect with GDBus as another uid (2.24 KB, patch)
2017-07-24 20:00 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise connecting to the new socket as the wrong uid (2.62 KB, patch)
2017-07-24 20:00 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Each connection to a container holds a reference (2.64 KB, patch)
2017-07-24 20:01 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Link each container to its initiating connection (7.98 KB, patch)
2017-07-24 20:01 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Shut down container servers when initiator goes away (3.49 KB, patch)
2017-07-24 20:01 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Give each instance a list of all its connections (3.51 KB, patch)
2017-07-24 20:01 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Implement methods to stop containers explicitly (6.48 KB, patch)
2017-07-24 20:02 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Don't allow stopping other users' containers (2.67 KB, patch)
2017-07-24 20:02 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the various ways to stop a container (14.54 KB, patch)
2017-07-24 20:03 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Emit InstanceRemoved signal (5.13 KB, patch)
2017-07-24 20:03 UTC, Simon McVittie
Details | Splinter Review
test/containers: Assert that InstanceRemoved is emitted (7.84 KB, patch)
2017-07-24 20:09 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Indicate in loginfo whether connection is contained (3.66 KB, patch)
2017-07-24 20:09 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Treat connections from inside containers as unprivileged (2.59 KB, patch)
2017-07-24 20:09 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that containers can't make new containers (3.99 KB, patch)
2017-07-24 20:09 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that connections from containers are unprivileged (1.61 KB, patch)
2017-07-24 20:10 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add a flag for methods that can't be invoked by containers (2.79 KB, patch)
2017-07-24 20:10 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Containers can't use the Verbose and Stats interfaces (1.85 KB, patch)
2017-07-24 20:10 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add basic container info to GetConnectionCredentials() (1.61 KB, patch)
2017-07-24 20:10 UTC, Simon McVittie
Details | Splinter Review
test/dbus-daemon: Assert absence of Containers1 credentials (877 bytes, patch)
2017-07-24 20:11 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add GetConnectionInstance(), GetInstanceInfo() (6.82 KB, patch)
2017-07-24 20:11 UTC, Simon McVittie
Details | Splinter Review
t/containers: Exercise trivial and non-trivial container metadata (13.12 KB, patch)
2017-07-24 20:11 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that GetInstanceInfo stops working (3.41 KB, patch)
2017-07-24 20:11 UTC, Simon McVittie
Details | Splinter Review
bus: Add (unused) settings for resource limits for containers (8.44 KB, patch)
2017-07-24 20:12 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Limit the size of metadata we will store (2.49 KB, patch)
2017-07-24 20:12 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_containers limit (1.51 KB, patch)
2017-07-24 20:12 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_connections_per_container (1.47 KB, patch)
2017-07-24 20:13 UTC, Simon McVittie
Details | Splinter Review
containers: enforce max_containers_per_user (6.66 KB, patch)
2017-07-24 20:13 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the resource limits (16.27 KB, patch)
2017-07-24 20:13 UTC, Simon McVittie
Details | Splinter Review
Revert "test/uid-permissions: Assert that AddServer is privileged" (3.54 KB, patch)
2017-07-24 20:13 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Allow unprivileged connections to create app-containers (1.31 KB, patch)
2017-07-24 20:13 UTC, Simon McVittie
Details | Splinter Review
system.conf: Allow creating containers on the system bus (1010 bytes, patch)
2017-07-24 20:14 UTC, Simon McVittie
Details | Splinter Review
travis-ci: Enable/disable more features in various builds (2.04 KB, patch)
2017-07-26 20:45 UTC, Simon McVittie
Details | Splinter Review
cmake: Match AC_DEFINE more precisely, respecting [] quoting (2.09 KB, patch)
2017-07-26 20:45 UTC, Simon McVittie
Details | Splinter Review
Add a simplified backport of g_steal_pointer() (2.00 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
_dbus_asv_add_object_path: Add (2.76 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
spec: Document the initial Containers1 interface (23.48 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
driver: Add a stub implementation of the Containers1 interface (9.10 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
travis-ci: Do at least one build with and one without containers (1.25 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
test/uid-permissions: Assert that AddServer is privileged (3.67 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
test/containers: New test (5.66 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Do some basic checking on the parameters (3.71 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the new parameter checking (4.33 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Build a global data structure for container instances (12.93 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Create a DBusServer and add it to the main loop (18.08 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
bus_context_add_incoming_connection: factor out (2.36 KB, patch)
2017-07-26 20:46 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Set up new connections to join the bus (912 bytes, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise a successful call to AddServer (8.31 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Require connecting uid to match caller of AddServer (3.46 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Add failable functions to connect to a bus (5.95 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Factor out functions for switching uid (4.71 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
test-utils-glib: Add function to connect with GDBus as another uid (2.04 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise connecting to the new socket as the wrong uid (2.64 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Each connection to a container holds a reference (2.54 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Link each container to its initiating connection (7.98 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Shut down container servers when initiator goes away (3.49 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Give each instance a list of all its connections (3.51 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Implement methods to stop containers explicitly (6.48 KB, patch)
2017-07-26 20:47 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Don't allow stopping other users' containers (2.67 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the various ways to stop a container (14.74 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Emit InstanceRemoved signal (4.93 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
test/containers: Assert that InstanceRemoved is emitted (7.84 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Indicate in loginfo whether connection is contained (3.67 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Treat connections from inside containers as unprivileged (2.59 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that containers can't make new containers (3.95 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that connections from containers are unprivileged (1.61 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add a flag for methods that can't be invoked by containers (2.79 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Containers can't use the Verbose and Stats interfaces (1.85 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add basic container info to GetConnectionCredentials() (1.61 KB, patch)
2017-07-26 20:48 UTC, Simon McVittie
Details | Splinter Review
test/dbus-daemon: Assert absence of Containers1 credentials (881 bytes, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add GetConnectionInstance(), GetInstanceInfo() (6.64 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
t/containers: Exercise trivial and non-trivial container metadata (12.83 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that GetInstanceInfo stops working (3.41 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus: Add (unused) settings for resource limits for containers (8.45 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Limit the size of metadata we will store (2.49 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_containers limit (1.52 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_connections_per_container (1.48 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
containers: enforce max_containers_per_user (6.64 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the resource limits (17.05 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
Revert "test/uid-permissions: Assert that AddServer is privileged" (3.55 KB, patch)
2017-07-26 20:49 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Allow unprivileged connections to create app-containers (1.32 KB, patch)
2017-07-26 20:50 UTC, Simon McVittie
Details | Splinter Review
system.conf: Allow creating containers on the system bus (1014 bytes, patch)
2017-07-26 20:50 UTC, Simon McVittie
Details | Splinter Review
test/containers: Use a shorter path for XDG_RUNTIME_DIR (1.79 KB, patch)
2017-07-26 20:50 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.