|
Description
Simon McVittie
2017-06-08 16:58:21 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 Created attachment 132165 [details] [review] [01] _dbus_asv_add_object_path: Add Created attachment 132167 [details] [review] bus_driver_send_ack_reply: Make available to other modules Comment on attachment 132167 [details] [review] bus_driver_send_ack_reply: Make available to other modules Moved to Bug #101567 Created attachment 132169 [details] [review] [02] spec: Document the initial Containers1 interface 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. 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. Created attachment 132172 [details] [review] [05] test/containers: New test So far it only exercises SupportedArguments. 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). Created attachment 132174 [details] [review] [08] test/containers: Exercise the new parameter checking 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. 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. Created attachment 132177 [details] [review] [11] bus_context_add_incoming_connection: factor out Created attachment 132178 [details] [review] [12] bus/containers: Set up new connections to join the bus Created attachment 132179 [details] [review] [13] test/containers: Exercise a successful call to AddContainerServer 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. Created attachment 132181 [details] [review] [15] bus/containers: Each connection to a container holds a reference 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. 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. Created attachment 132184 [details] [review] [18] bus/containers: Give each instance a list of all its connections 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". 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. Created attachment 132187 [details] [review] [21] test/containers: Exercise the various ways to stop a container Created attachment 132188 [details] [review] [22] bus/containers: Emit ContainerInstanceRemoved signal Created attachment 132189 [details] [review] [23] test/containers: Assert that ContainerInstanceRemoved is emitted Created attachment 132190 [details] [review] [24] bus/containers: Indicate in loginfo whether connection is contained 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. 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. 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 :-) 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. 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. Created attachment 132196 [details] [review] [30] bus/driver: Add basic container info to GetConnectionCredentials() Created attachment 132197 [details] [review] [31] test/dbus-daemon: Assert absence of Containers1 credentials These connections are not to a container server. Created attachment 132198 [details] [review] [32] bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo() --- I am not particularly happy with the length of these names. Created attachment 132199 [details] [review] test/containers: Exercise trivial and non-trivial container metadata Created attachment 132200 [details] [review] [34] test/containers: Check that GetContainerInstanceInfo stops working After the container instance is removed, the method should not work. Created attachment 132201 [details] [review] [35] bus: Add (unused) settings for resource limits for containers Created attachment 132202 [details] [review] [36] bus/containers: Limit the size of metadata we will store Created attachment 132203 [details] [review] [37] bus/containers: Enforce max_containers limit Created attachment 132204 [details] [review] [38] bus/containers: Enforce max_connections_per_container Created attachment 132205 [details] [review] [39] containers: enforce max_containers_per_user Created attachment 132206 [details] [review] [40] test/containers: Exercise the resource limits 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. Also available at: https://github.com/smcv/dbus containers-minimum-101354 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. (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. Created attachment 132367 [details] [review] [02] spec: Document the initial Containers1 interface --- Document the new keys for o.fd.DBus.GetConnectionCredentials() too 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). 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. 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. 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. Created attachment 132373 [details] [review] [33] test/containers: Exercise trivial and non-trivial container metadata --- Fix failure to build without --enable-containers 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. (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. (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. (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. (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. (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. 🙈 (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? Created attachment 132508 [details] [review] _dbus_asv_add_object_path: Add Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132509 [details] [review] spec: Document the initial Containers1 interface Signed-off-by: Simon McVittie <smcv@collabora.com> 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> Created attachment 132511 [details] [review] test/uid-permissions: Assert that AddContainerServer is privileged Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132512 [details] [review] test/containers: New test So far it only exercises SupportedArguments. Signed-off-by: Simon McVittie <smcv@collabora.com> 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> Created attachment 132514 [details] [review] test/containers: Exercise the new parameter checking Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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. Created attachment 132517 [details] [review] bus_context_add_incoming_connection: factor out Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132518 [details] [review] bus/containers: Set up new connections to join the bus Signed-off-by: Simon McVittie <smcv@collabora.com> 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.) 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> Created attachment 132521 [details] [review] test-utils-glib: Factor out functions for switching uid Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132524 [details] [review] bus/containers: Each connection to a container holds a reference Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132527 [details] [review] bus/containers: Give each instance a list of all its connections Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132528 [details] [review] bus/containers: Implement methods to stop containers explicitly Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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. Created attachment 132534 [details] [review] bus/containers: Emit ContainerInstanceRemoved signal Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132535 [details] [review] test/containers: Assert that ContainerInstanceRemoved is emitted Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132536 [details] [review] bus/containers: Indicate in loginfo whether connection is contained Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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() Created attachment 132539 [details] [review] test/containers: Check that connections from containers are unprivileged Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132542 [details] [review] bus/driver: Add basic container info to GetConnectionCredentials() Signed-off-by: Simon McVittie <smcv@collabora.com> 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> Created attachment 132544 [details] [review] bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo() Signed-off-by: Simon McVittie <smcv@collabora.com> 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() 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> 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> Created attachment 132548 [details] [review] bus/containers: Limit the size of metadata we will store Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132549 [details] [review] bus/containers: Enforce max_containers limit Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132550 [details] [review] bus/containers: Enforce max_connections_per_container Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132551 [details] [review] containers: enforce max_containers_per_user Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132552 [details] [review] test/containers: Exercise the resource limits Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132555 [details] [review] system.conf: Allow creating containers on the system bus Signed-off-by: Simon McVittie <smcv@collabora.com> 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 on attachment 132508 [details] [review] _dbus_asv_add_object_path: Add Review of attachment 132508 [details] [review]: ----------------------------------------------------------------- ++ 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 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 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); (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. (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. (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 on attachment 132512 [details] [review] test/containers: New test Review of attachment 132512 [details] [review]: ----------------------------------------------------------------- ++ 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 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. 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 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? (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. (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. (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-. (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. (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"). (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 (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. (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 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> 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: 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. 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> 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> 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. (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) 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. 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. 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 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. 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 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> 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. 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> 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. 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. 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 Created attachment 132891 [details] [review] bus_context_add_incoming_connection: factor out Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132892 [details] [review] bus/containers: Set up new connections to join the bus Signed-off-by: Simon McVittie <smcv@collabora.com> 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. 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> Created attachment 132895 [details] [review] test-utils-glib: Factor out functions for switching uid Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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. 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 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> 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> Created attachment 132901 [details] [review] bus/containers: Give each instance a list of all its connections Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132902 [details] [review] bus/containers: Implement methods to stop containers explicitly Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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() 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. Created attachment 132906 [details] [review] test/containers: Assert that InstanceRemoved is emitted Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132907 [details] [review] bus/containers: Indicate in loginfo whether connection is contained Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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. Created attachment 132910 [details] [review] test/containers: Check that connections from containers are unprivileged Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132913 [details] [review] bus/driver: Add basic container info to GetConnectionCredentials() Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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. 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). 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> 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> Created attachment 132919 [details] [review] bus/containers: Limit the size of metadata we will store Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132920 [details] [review] bus/containers: Enforce max_containers limit Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132921 [details] [review] bus/containers: Enforce max_connections_per_container Signed-off-by: Simon McVittie <smcv@collabora.com> Created attachment 132922 [details] [review] containers: enforce max_containers_per_user Signed-off-by: Simon McVittie <smcv@collabora.com> --- Use _dbus_clear_hash_table Created attachment 132923 [details] [review] test/containers: Exercise the resource limits Signed-off-by: Simon McVittie <smcv@collabora.com> 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> 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> Created attachment 132926 [details] [review] system.conf: Allow creating containers on the system bus Signed-off-by: Simon McVittie <smcv@collabora.com> 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. 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);
^
(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. 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> 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> 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 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: Created attachment 133007 [details] [review] spec: Document the initial Containers1 interface Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: Created attachment 133010 [details] [review] test/uid-permissions: Assert that AddServer is privileged Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133011 [details] [review] test/containers: New test So far it only exercises SupportedArguments. Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: Created attachment 133013 [details] [review] test/containers: Exercise the new parameter checking Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: Created attachment 133016 [details] [review] bus_context_add_incoming_connection: factor out Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133017 [details] [review] bus/containers: Set up new connections to join the bus Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133018 [details] [review] test/containers: Exercise a successful call to AddServer Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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> 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. 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: 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: Created attachment 133024 [details] [review] bus/containers: Each connection to a container holds a reference Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: 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: Created attachment 133028 [details] [review] bus/containers: Implement methods to stop containers explicitly Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: Created attachment 133030 [details] [review] test/containers: Exercise the various ways to stop a container Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133031 [details] [review] bus/containers: Emit InstanceRemoved signal Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133032 [details] [review] test/containers: Assert that InstanceRemoved is emitted Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133033 [details] [review] bus/containers: Indicate in loginfo whether connection is contained Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: Created attachment 133036 [details] [review] test/containers: Check that connections from containers are unprivileged Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: Created attachment 133039 [details] [review] bus/driver: Add basic container info to GetConnectionCredentials() Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: Created attachment 133041 [details] [review] bus/driver: Add GetConnectionInstance(), GetInstanceInfo() Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133042 [details] [review] t/containers: Exercise trivial and non-trivial container metadata Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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: 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: Created attachment 133045 [details] [review] bus/containers: Limit the size of metadata we will store Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133046 [details] [review] bus/containers: Enforce max_containers limit Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133047 [details] [review] bus/containers: Enforce max_connections_per_container Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: Created attachment 133048 [details] [review] containers: enforce max_containers_per_user Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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) 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: 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: Created attachment 133052 [details] [review] system.conf: Allow creating containers on the system bus Signed-off-by: Simon McVittie <smcv@collabora.com> Bug: 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 on attachment 133003 [details] [review] travis-ci: Enable/disable more features in various builds Review of attachment 133003 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133005 [details] [review] Add a simplified backport of g_steal_pointer() Review of attachment 133005 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133006 [details] [review] _dbus_asv_add_object_path: Add Review of attachment 133006 [details] [review]: ----------------------------------------------------------------- r+, still 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 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 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 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 on attachment 133011 [details] [review] test/containers: New test Review of attachment 133011 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133012 [details] [review] bus/containers: Do some basic checking on the parameters Review of attachment 133012 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133013 [details] [review] test/containers: Exercise the new parameter checking Review of attachment 133013 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133014 [details] [review] bus/containers: Build a global data structure for container instances Review of attachment 133014 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133016 [details] [review] bus_context_add_incoming_connection: factor out Review of attachment 133016 [details] [review]: ----------------------------------------------------------------- r+ 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 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 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 on attachment 133020 [details] [review] test-utils-glib: Add failable functions to connect to a bus Review of attachment 133020 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133021 [details] [review] test-utils-glib: Factor out functions for switching uid Review of attachment 133021 [details] [review]: ----------------------------------------------------------------- r+ 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 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 on attachment 133024 [details] [review] bus/containers: Each connection to a container holds a reference Review of attachment 133024 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133026 [details] [review] bus/containers: Shut down container servers when initiator goes away Review of attachment 133026 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133028 [details] [review] bus/containers: Implement methods to stop containers explicitly Review of attachment 133028 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133029 [details] [review] bus/containers: Don't allow stopping other users' containers Review of attachment 133029 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133030 [details] [review] test/containers: Exercise the various ways to stop a container Review of attachment 133030 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133031 [details] [review] bus/containers: Emit InstanceRemoved signal Review of attachment 133031 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133033 [details] [review] bus/containers: Indicate in loginfo whether connection is contained Review of attachment 133033 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133034 [details] [review] bus/driver: Treat connections from inside containers as unprivileged Review of attachment 133034 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133036 [details] [review] test/containers: Check that connections from containers are unprivileged Review of attachment 133036 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133038 [details] [review] bus/driver: Containers can't use the Verbose and Stats interfaces Review of attachment 133038 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133039 [details] [review] bus/driver: Add basic container info to GetConnectionCredentials() Review of attachment 133039 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133040 [details] [review] test/dbus-daemon: Assert absence of Containers1 credentials Review of attachment 133040 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133041 [details] [review] bus/driver: Add GetConnectionInstance(), GetInstanceInfo() Review of attachment 133041 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133043 [details] [review] test/containers: Check that GetInstanceInfo stops working Review of attachment 133043 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133045 [details] [review] bus/containers: Limit the size of metadata we will store Review of attachment 133045 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133046 [details] [review] bus/containers: Enforce max_containers limit Review of attachment 133046 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133047 [details] [review] bus/containers: Enforce max_connections_per_container Review of attachment 133047 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133048 [details] [review] containers: enforce max_containers_per_user Review of attachment 133048 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 133050 [details] [review] Revert "test/uid-permissions: Assert that AddServer is privileged" Review of attachment 133050 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133051 [details] [review] bus/driver: Allow unprivileged connections to create app-containers Review of attachment 133051 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 133052 [details] [review] system.conf: Allow creating containers on the system bus Review of attachment 133052 [details] [review]: ----------------------------------------------------------------- r+ 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. 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. (In reply to Philip Withnall from comment #234) > Comment on attachment 133003 [details] [review] > travis-ci: Enable/disable more features in various builds > > r+ Rebased with very minor conflicts, pushed to master for 1.13.0. (In reply to Philip Withnall from comment #235) > Comment on attachment 133004 [details] [review] > cmake: Match AC_DEFINE more precisely, respecting [] quoting > > I have not reviewed this regex in detail, because life is too short. If it > works, ship it. I'll call that "Acked-by". Rebased, pushed to master for 1.13.0. (In reply to Philip Withnall from comment #236) > Comment on attachment 133005 [details] [review] > Add a simplified backport of g_steal_pointer() > > r+ Rebased with very minor conflicts, pushed to master for 1.13.0. (In reply to Philip Withnall from comment #237) > Comment on attachment 133006 [details] [review] > _dbus_asv_add_object_path: Add > > r+, still Pushed to master for 1.13.0. (In reply to Philip Withnall from comment #238) > Comment on attachment 133007 [details] [review] > spec: Document the initial Containers1 interface > > Nitpick: ‘applied to them by the message bus’ to clarify that bit? Fixed locally > > + If the given object path represents an active container instance, > > Does that include container instances which have had StopListening called on > them (but which still have some open connections)? Yes it does. I've tried to clarify this. > The term ‘active’ here is > ill-defined. The same ‘active’ term is used in a few other places in this > patch and is ill-defined there too. Fixed locally. My intention was that a container instance object path is active (I used the word "valid" in the new version of the commit) if either it owns at least one connection, or it could produce connections in future - which right now means it is listening and accept()ing, but after Bug #101898 will also include container instances whose server is not ready to accept() yet. (In reply to Philip Withnall from comment #239) > Comment on attachment 133008 [details] [review] > driver: Add a stub implementation of the Containers1 interface > > Also needs to be in README.cmake and cmake/config.h, I think. Good catch, fixed locally. (In reply to Philip Withnall from comment #241) > Comment on attachment 133010 [details] [review] > test/uid-permissions: Assert that AddServer is privileged > > Question: would it be possible to factor out the test code for sending a > message and storing the reply? It gets repeated a lot. I think I'd rather do this as an orthogonal thing. (In reply to Philip Withnall from comment #246) > Comment on attachment 133015 [details] [review] > bus/containers: Create a DBusServer and add it to the main loop > > Nitpick: An internal convenience function for this init-const-then-reinit-rw > approach for DBusString would be a bit clearer (and more reusable). Bug #89104 would be better still. I can't say I'm particularly keen on adding a middle ground where a function is failable, but alters the DBusString even if it fails... that seems fairly horrible. I think I'd rather do this as an orthogonal thing. > Paranoia: potentially add: > _dbus_assert (self->refcount < INTMAX); Fixed locally. > (Same for the other _ref() functions which have been added in this patch > set.) I don't see any others, except bus_container_ref() which already had this. (In reply to Philip Withnall from comment #248) > Comment on attachment 133017 [details] [review] > bus/containers: Set up new connections to join the bus > > r+ > > Do we want to log a warning on the OOM failure case? Otherwise it > essentially gets silently ignored. Good catch. It turns out we don't log a warning in the OOM failure case (or the failed-to-set-up-LSM case) for unconfined connections either... we probably should, but that's an orthogonal bug. I've opened Bug #103592. (In reply to Philip Withnall from comment #250) > Comment on attachment 133019 [details] [review] > bus/containers: Require connecting uid to match caller of AddServer > > r+ > > > + * TODO: For the system bus we might want [...] > > Is this TODO comment worth a follow-up bug report? I’m not sure it is. I think it probably is worth it. Bug #103457. (In reply to Philip Withnall from comment #251) > Comment on attachment 133020 [details] [review] > test-utils-glib: Add failable functions to connect to a bus > > r+ Staged to push for 1.13.0 (will be pushed if tests pass) (In reply to Philip Withnall from comment #252) > Comment on attachment 133021 [details] [review] > test-utils-glib: Factor out functions for switching uid > > r+ Staged to push for 1.13.0 (In reply to Philip Withnall from comment #253) > Comment on attachment 133022 [details] [review] > test-utils-glib: Add function to connect with GDBus as another uid > > r+ Staged to push for 1.13.0 (In reply to Philip Withnall from comment #256) > Comment on attachment 133025 [details] [review] > Nitpick: In honour of the music driving these reviews, this variable should > be changed to `DBusConnection *kreator;` Yeah, I'll consider it[1]. [1] After due consideration: No. (In reply to Philip Withnall from comment #263) > Comment on attachment 133032 [details] [review] > test/containers: Assert that InstanceRemoved is emitted > > I don’t think f->containers_removed is ever freed. Good catch, fixed locally. (In reply to Philip Withnall from comment #268) > Comment on attachment 133037 [details] [review] > > This documentation comment is a near-duplicate of the one above. It would be > good to clarify somewhere what the difference is between the two flags. You're right, that comment made no sense. Fixed locally. (In reply to Philip Withnall from comment #275) > Comment on attachment 133044 [details] [review] > bus: Add (unused) settings for resource limits for containers > > Nitpick: Missing space before `*/`. It's aligned with 80% of the members of BusLimits, just not the one immediately above (which is too long). I re-indented that one by an extra tab-stop to match other long names further up (I'm not going to bother making that available as a separate commit). (In reply to Philip Withnall from comment #280) > Comment on attachment 133049 [details] [review] > test/containers: Exercise the resource limits > > extra is never used (apart from being free). Drop it. Fixed locally. > Nitpick: s/Crushed/Crushd/ Nice to see someone else likes Radiohead. Fixed locally. (In reply to Philip Withnall from comment #284) > Comment on attachment 133053 [details] [review] > test/containers: Use a shorter path for XDG_RUNTIME_DIR > > Nitpick: `error` is leaked, though this is an aborting path. I changed this to be non-aborting and use the special TAP token "Bail out!" instead, then not leak error. I also took the opportunity to pull it earlier in the branch. Comment on attachment 133003 [details] [review] travis-ci: Enable/disable more features in various builds Review of attachment 133003 [details] [review]: ----------------------------------------------------------------- this patch seems to be already applied or superseeded in git master - can anyone confirm ? Comment on attachment 133005 [details] [review] Add a simplified backport of g_steal_pointer() Review of attachment 133005 [details] [review]: ----------------------------------------------------------------- This patch seems to be applied or superseeded Comment on attachment 133015 [details] [review] bus/containers: Create a DBusServer and add it to the main loop Review of attachment 133015 [details] [review]: ----------------------------------------------------------------- Running 'git apply 101354' on recent git master results into: Wende an: bus/containers: Create a DBusServer and add it to the main loop error: SHA-1 Information is missing or invalid (bus/Makefile.am). (In reply to Ralf Habacker from comment #291) > this patch seems to be already applied or superseeded in git master - can > anyone confirm ? Yes, some of these patches have already been applied, as noted in the most recent 10 comments. The rest have already been reviewed and I am in the process of fixing Philip's comments on them. There's no need to review anything here until I've finished doing that. When I've fixed those review comments, I'm not intending to reattach the entire branch, as Philip requested in Comment #285. I'll push a revised version to Github, with git-rebase "squash!" commit messages for commits that are intended to modify a previous commit. If you are about to review things, please see the previous 290 comments (!) and Bug #100344 for context. This feature is Unix-specific and rather subtle, so if you specialize in Windows stuff, you're probably not the ideal reviewer for it. Please do not push anything from this bug. I'll push the changes when they are reviewed, tested and ready. I've updated https://github.com/smcv/dbus/commits/containers-minimum-101354 with most of the changes Philip asked for. The changes are in "squash!" commits which are going to be squashed into prior commits, but I have not done that yet, because separated commits are the form that Philip said he'd prefer to review. I think I'll also squash "test/containers: Use a shorter path for XDG_RUNTIME_DIR" into the commit before it. Reminder: please do not push anything from this bug. I'll do that when it has been reviewed and tested. I am probably not going to push the rest of the changes from here until I've been able to run them against a "live" Flatpak installation that has the corresponding changes. (In reply to Simon McVittie from comment #287) > (In reply to Philip Withnall from comment #238) > > Question: would it be possible to factor out the test code for sending a > > message and storing the reply? It gets repeated a lot. > > I think I'd rather do this as an orthogonal thing. Bug #103600. > (In reply to Philip Withnall from comment #246) > > Nitpick: An internal convenience function for this init-const-then-reinit-rw > > approach for DBusString would be a bit clearer (and more reusable). > > Bug #89104 would be better still. I can't say I'm particularly keen on > adding a middle ground where a function is failable, but alters the > DBusString even if it fails... that seems fairly horrible. > > I think I'd rather do this as an orthogonal thing. I started looking into this and accidentally found Bug #103597. (In reply to Philip Withnall from comment #241) > test/uid-permissions: Assert that AddServer is privileged > > Question: would it be possible to factor out the test code for sending a > message and storing the reply? It gets repeated a lot. I've done that on Bug #103600. However, to reduce the critical path for this branch, I am not going to rebase it onto Bug #103600, because there is only one use of the pattern in this branch, and it's in code whose addition is reverted later. (In reply to Simon McVittie from comment #296) > (In reply to Simon McVittie from comment #287) > > (In reply to Philip Withnall from comment #246) > > > Nitpick: An internal convenience function for this init-const-then-reinit-rw > > > approach for DBusString would be a bit clearer (and more reusable). > > > > Bug #89104 would be better still. I can't say I'm particularly keen on > > adding a middle ground where a function is failable, but alters the > > DBusString even if it fails... that seems fairly horrible. I've rebased this branch on Bug #89104. Still at: https://github.com/smcv/dbus/commits/containers-minimum-101354 Changes to use _DBUS_STRING_INIT_INVALID: https://github.com/smcv/dbus/commit/0fc58ba7e6c3493a179357565eb667a6b1c9b407 https://github.com/smcv/dbus/commit/0c3363d00eeabab857a6647cb6d8cee967daaeac https://github.com/smcv/dbus/commit/64b4e32be7e58a97547d4056f3c1d6158182e1db (fixes a memory leak) Unrelated build fixes: https://github.com/smcv/dbus/commit/9a030a4d0a33f735a2f9c7324f4bf812d021f80e (In reply to Simon McVittie from comment #297) > (In reply to Philip Withnall from comment #241) > > test/uid-permissions: Assert that AddServer is privileged > > > > Question: would it be possible to factor out the test code for sending a > > message and storing the reply? It gets repeated a lot. > > I've done that on Bug #103600. However, to reduce the critical path for this > branch, I am not going to rebase it onto Bug #103600, because there is only > one use of the pattern in this branch, and it's in code whose addition is > reverted later. Good plan. (In reply to Simon McVittie from comment #298) > (In reply to Simon McVittie from comment #296) > > (In reply to Simon McVittie from comment #287) > > > (In reply to Philip Withnall from comment #246) > > > > Nitpick: An internal convenience function for this init-const-then-reinit-rw > > > > approach for DBusString would be a bit clearer (and more reusable). > > > > > > Bug #89104 would be better still. I can't say I'm particularly keen on > > > adding a middle ground where a function is failable, but alters the > > > DBusString even if it fails... that seems fairly horrible. > > I've rebased this branch on Bug #89104. Still at: > https://github.com/smcv/dbus/commits/containers-minimum-101354 > > Changes to use _DBUS_STRING_INIT_INVALID: > > https://github.com/smcv/dbus/commit/0fc58ba7e6c3493a179357565eb667a6b1c9b407 > https://github.com/smcv/dbus/commit/0c3363d00eeabab857a6647cb6d8cee967daaeac > https://github.com/smcv/dbus/commit/64b4e32be7e58a97547d4056f3c1d6158182e1db > (fixes a memory leak) > > Unrelated build fixes: > > https://github.com/smcv/dbus/commit/9a030a4d0a33f735a2f9c7324f4bf812d021f80e OK. I’ll comment on the individual commits there. Hopefully that works out OK as a workflow for both of us; please say so if not. I’ve finished reviewing the `squash!` commits on https://github.com/smcv/dbus/commits/containers-minimum-101354 and have left comments on all of them: either an r+, or a nitpick. I don’t think every comment I left was trivial. Ship it? :-) I think the ‘leave review comments on the `squash!` commits’ workflow has worked well for me, since it gives the minimal delta to review. Let me know whether it works for you, or whether there’s too much accounting overhead in tracking all the `squash!` patches and their relationship to how you want the branch to look. For clarity, here are the outstanding comments. Marking as review- because one of them isn’t a nitpick: • https://github.com/smcv/dbus/commit/a60808e7a11bc80650e28f40b5a0783d89290c3f#comments • https://github.com/smcv/dbus/commit/c09fd8b799c1c919ea15d3f35ee8803ee8d83d26#comments • https://github.com/smcv/dbus/commit/64b4e32be7e58a97547d4056f3c1d6158182e1db#comments • https://github.com/smcv/dbus/commit/f7f9450641165db5de50375d3a2b071084d3e469#comments Remaining fixes to be approved: * https://github.com/smcv/dbus/commit/df837beb2fecb8caaaf6a80669d94d1fd72967a6 * https://github.com/smcv/dbus/commit/a6f5870b81ab6e5edf87b8138c7a2eca13c29881 now fixed by https://github.com/smcv/dbus/commit/9e0f7d8cc5228649b6982d2739861c896a95848b both of which are minor issues. (In reply to Simon McVittie from comment #302) > Remaining fixes to be approved: > > * > https://github.com/smcv/dbus/commit/df837beb2fecb8caaaf6a80669d94d1fd72967a6 > > * > https://github.com/smcv/dbus/commit/a6f5870b81ab6e5edf87b8138c7a2eca13c29881 > now fixed by > https://github.com/smcv/dbus/commit/9e0f7d8cc5228649b6982d2739861c896a95848b > > both of which are minor issues. All approved. Thanks! Merged \o/ I made the merge artificially a non-fast-forward, so that we can easily revert this feature if we need to. Summary of things not done, other than of course "all the other bugs that block Bug #100344": (In reply to Allison Lortie (desrt) from comment #116) > The "Containers1" style naming thing is sort of distasteful. If we ever > have a second version of this interface (unlikely) then we can perfectly > well call that "Containers2" anyway. Acknowledged but not changed. I've kept the Containers1 name anyway, because I think it's time we started taking our own advice where API naming is concerned. (But if other maintainers feel particularly strongly about this, it can be changed.) > The fully-namespaced keys inside of the GetConnectionCredentials return > value are silly and wasteful. Acknowledged but not changed, same reasoning. > I was sad to notice that the fd-passing style of creation is gone. Bug #101898 > The same-uid check is a hack and it shows off some of the problems with the > create-a-path-in-a-public-directory approach. Bug #103457 > The main missing item here in terms of usability is the addition of the > header field that we discussed for marking messages from sandboxed apps. Bug #101899, which is one of the next things on my to-do list here > I recall that we discussed that [the sandbox ID] should be a uint64 for > efficiency/simplicity reasons ... > Why did you go with the object path approach instead of ints for identifying > containers? Because we keep telling D-Bus API authors that they should use object paths instead of inventing their own opaque token scheme with strings or integers, and it seemed like it's time to take our own advice. But I'm generating the object paths from a uint64 anyway, so we can easily switch if other maintainers feel strongly about this. > Any thoughts on sandboxed apps adding match rules? Bug #101902 (which is quite a large one) |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.