+++ This bug was initially created as a clone of Bug #100344 +++ See Bug #100344. After the basic implementation (Bug #101354), we want an easy, cheap way for the dbus-daemon to tell services like dconf "this message came from container instance /.../c23". If dconf already knows about c23, it can short-cut to just doing the right thing; if not, it will have to call GetInstanceInfo("/.../c23") before deciding what to do with the request. Design sketch: * New method on the bus driver, o.fd.DBus.C1.RequestContainerHeaders() or o.fd.DBus.RequestHeaders(["o.fd.DBus.Containers1"]) or similar. If this method call succeeds, the dbus-daemon will add CONTAINER_INSTANCE to every unicast message sent to the connection that called the new method. * Open question: Do we need this for broadcasts too? If yes, we would have to either run the matchmaker *before* sending the message to the addressed recipient (at the moment it's after), so that we could edit the message header; or copy the message, add the header and send the copy instead of the original. * New header field, CONTAINER_INSTANCE, of type OBJECT_PATH. If present with value "/", this message did not come from a container. If present with any other value, this message came from that container instance. If not present, we make no particular statement about that. * If we need this for broadcasts too, we might have to say: The message bus may add this field to messages even if the recipient did not ask for it (as an optimization for the case where a different recipient did ask for it).
(In reply to Simon McVittie from comment #0) > * If we need this for broadcasts too, we might have to say: > The message bus may add this field to messages even if the recipient > did not ask for it (as an optimization for the case where a different > recipient did ask for it). Doesn’t that negate the point of adding RequestHeaders() in the first place, since the header is now potentially being sent to connections whose client library doesn’t understand it?
(In reply to Philip Withnall from comment #1) > Doesn’t that negate the point of adding RequestHeaders() in the first place, > since the header is now potentially being sent to connections whose client > library doesn’t understand it? I think that's fine: the point of adding RequestHeaders() in the first place was optimization, not lack of client library support. Client libraries are already required to ignore header fields they don't understand.
Created attachment 136737 [details] [review] spec: Add a method call to request a new CONTAINER_INSTANCE header field This follows the pattern established in fd.o#100317 by combining a request "please give me more information", for efficiency (most destinations not wanting the extra information will not receive it), with a mechanism for "tell me whether I can trust this new header field", for correctness/safety (a successful return indicates that all future CONTAINER_INSTANCE header fields can be trusted). --- This depends on my containers-spec branch, which I will merge to master when we're reasonably happy that the spec is stable. I'd rather not do that until I've at least done a real-life test against a patched Flatpak and xdg-desktop-portal; this is (I think) the last piece of dbus work that's needed before I can usefully do that.
Created attachment 136738 [details] [review] bus_transaction_send: Take sender and destination connections We'll need this if we want to stamp optional header fields on the message according to the preferences of the recipient(s).
Created attachment 136739 [details] [review] DBusMessage: Add a header field for the container instance In the bus daemon, don't pass through the container instance path: if there's any value here at all, we want to be able to guarantee that we sent it (in a later commit).
Created attachment 136740 [details] [review] containers: Add a method to ask to be sent the connection instance header
Created attachment 136741 [details] [review] test/containers: Test the new header field --- I'm not particularly happy with iterate_both_main_loops(), but I think the simplicity of using GDBus for 95% of this test outweighs the amount of dislike I have for it. I am not intending to rewrite the main loop handling (in this test, or in libdbus) any time soon, sorry.
Comment on attachment 136737 [details] [review] spec: Add a method call to request a new CONTAINER_INSTANCE header field Review of attachment 136737 [details] [review]: ----------------------------------------------------------------- r- ::: doc/dbus-specification.xml @@ +1797,5 @@ > + that sent this message, or the root object path > + <literal>/</literal> if the message did not come from a > + connection to a container instance. > + In message bus implementations that implement the > + <literal>Containers1.RequestHeader method</literal>, this ‘method’ shouldn’t be inside <literal>. @@ +1800,5 @@ > + In message bus implementations that implement the > + <literal>Containers1.RequestHeader method</literal>, this > + header field is controlled by the message bus: see the > + <link linkend="bus-messages-containers1-request-header" > + >Containers1.RequestHeader method</link> for details. Containers.RequestHeader should be <literal> here. @@ +1802,5 @@ > + header field is controlled by the message bus: see the > + <link linkend="bus-messages-containers1-request-header" > + >Containers1.RequestHeader method</link> for details. > + Senders must not attempt to include this header field > + in messages. Or else? Might want to briefly link/refer to the paragraph in the RequestHeader documentation about the message bus stripping mistaken/malicious headers. @@ +7478,5 @@ > + message that will be received by the connection that called > + this method. If the message bus sends a successful reply, > + the caller may assume that it will receive the > + <literal>CONTAINER_INSTANCE</literal> header field in every > + message, and that the value of that header field is authentic. Nitpick: Has ‘authentic’ been defined? Or are you happy with the user assuming what it means? Might be better to make it clear that this means the header was generated by the message bus and cannot be forged. @@ +7486,5 @@ > + <para> > + If a sender mistakenly or maliciously includes this header field > + in any message, then all message bus implementations that > + implement this method call must remove the header field before > + relaying the message. Do you want to write in the option for the dbus-daemon to kick the mistaken/malicious sender off the bus?
Comment on attachment 136738 [details] [review] bus_transaction_send: Take sender and destination connections Review of attachment 136738 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136739 [details] [review] DBusMessage: Add a header field for the container instance Review of attachment 136739 [details] [review]: ----------------------------------------------------------------- r? Looks OK apart from this one question. ::: bus/dispatch.c @@ +290,5 @@ > /* Make sure the message does not have any header fields that we > * don't understand (or validate), so that we can add header fields > * in future and clients can assume that we have checked them. */ > + if (!_dbus_message_remove_unknown_fields (message) || > + !dbus_message_set_container_instance (message, NULL)) Would it make sense to make this change in _dbus_message_remove_unknown_fields() instead of just here? It wouldn’t really make any functional difference, but might make a bit more semantic sense?
Comment on attachment 136740 [details] [review] containers: Add a method to ask to be sent the connection instance header Review of attachment 136740 [details] [review]: ----------------------------------------------------------------- r+, looks good.
Comment on attachment 136741 [details] [review] test/containers: Test the new header field Review of attachment 136741 [details] [review]: ----------------------------------------------------------------- r+, but only because you seem suitably ashamed of the iterate_both_main_loops() thing. ::: test/containers.c @@ +111,5 @@ > + * be better than sleeping, but do we have enough API to do that without > + * reinventing dbus-glib? */ > + g_usleep (G_USEC_PER_SEC / 100); > + test_main_context_iterate (ctx, FALSE); > + g_main_context_iteration (NULL, FALSE); Wowwww. Justification seems appropriate, though.
(In reply to Philip Withnall from comment #8) > spec: Add a method call to request a new CONTAINER_INSTANCE header field > > Review of attachment 136737 [details] [review]: > ----------------------------------------------------------------- > > + In message bus implementations that implement the > > + <literal>Containers1.RequestHeader method</literal>, this > > ‘method’ shouldn’t be inside <literal>. Fixed > > + <link linkend="bus-messages-containers1-request-header" > > + >Containers1.RequestHeader method</link> for details. > > Containers.RequestHeader should be <literal> here. Seriously? You're even more thorough than I thought. (Done.) > > + Senders must not attempt to include this header field > > + in messages. > > Or else? Might want to briefly link/refer to the paragraph in the > RequestHeader documentation about the message bus stripping > mistaken/malicious headers. I've said "if they do, the message bus must not relay the message without first removing it". > @@ +7478,5 @@ > > + message that will be received by the connection that called > > + this method. If the message bus sends a successful reply, > > + the caller may assume that it will receive the > > + <literal>CONTAINER_INSTANCE</literal> header field in every > > + message, and that the value of that header field is authentic. > > Nitpick: Has ‘authentic’ been defined? Or are you happy with the user > assuming what it means? Might be better to make it clear that this means the > header was generated by the message bus and cannot be forged. I used wording suspiciously similar to yours :-) > > + If a sender mistakenly or maliciously includes this header field > > + in any message, then all message bus implementations that > > + implement this method call must remove the header field before > > + relaying the message. > > Do you want to write in the option for the dbus-daemon to kick the > mistaken/malicious sender off the bus? Not really, because until recently, the spec said "must ignore" and the reference implementation interpreted that as "pass through without understanding" - I'm less comfortable with going from there to "kick the client" (definitely an incompatible change) than I am with applying filtering (arguably an incompatible change). But I've rephrased this so that it doesn't explicitly say "must relay", only "must not relay without filtering", so that an extra-strict implementation that sent back an error reply, dropped the message on the floor or kicked clients would be valid. (In reply to Philip Withnall from comment #10) > DBusMessage: Add a header field for the container instance > > Review of attachment 136739 [details] [review]: > ----------------------------------------------------------------- > > + if (!_dbus_message_remove_unknown_fields (message) || > > + !dbus_message_set_container_instance (message, NULL)) > > Would it make sense to make this change in > _dbus_message_remove_unknown_fields() instead of just here? It wouldn’t > really make any functional difference, but might make a bit more semantic > sense? I originally did, but I didn't think that made semantic sense, because it isn't an unknown field - it's a known field that the client shouldn't be supplying.
Created attachment 137201 [details] [review] spec: Add a method call to request a new CONTAINER_INSTANCE header field --- Say that the message bus must not relay forged container IDs, but do not specifically say that it must relay the filtered message, so that we don't forbid a strict implementation that interprets such messages as malicious. Fix up <literal> use, hopefully to Philip's satisfaction. As with the previous version, this depends on my containers-spec branch.
(In reply to Philip Withnall from comment #12) > Comment on attachment 136741 [details] [review] > test/containers: Test the new header field > > > + g_usleep (G_USEC_PER_SEC / 100); > > + test_main_context_iterate (ctx, FALSE); > > + g_main_context_iteration (NULL, FALSE); > > Wowwww. Justification seems appropriate, though. Now that I've turned the main loop integration glue in dbus-glib into a subtree/submodule-able separate branch: https://cgit.freedesktop.org/dbus/dbus-glib/log/?h=dbus-gmain and bolted it onto dbus-python instead of using dbus-glib directly: https://cgit.freedesktop.org/dbus/dbus-python/commit/?id=28098f2c7c23bdef9247c5dfbb8437ca3f03dac4 https://cgit.freedesktop.org/dbus/dbus-python/commit/?id=80948a2ec2ed1a2b19f1287c99bff1e5bd5b1b67 we could in principle subtree-merge this main loop into test/dbus-gmain/ and link it into test/libdbus-testutils.la. What do you think? Worth it or not? (In general I would prefer to use `git subtree` over `git submodule`, because `git subtree` makes `git archive` and trivial uses of `git clone` do the right thing. Third-party software is welcome to submodule it in though.)
Comment on attachment 137201 [details] [review] spec: Add a method call to request a new CONTAINER_INSTANCE header field Review of attachment 137201 [details] [review]: ----------------------------------------------------------------- r+, thanks for the changes. <literal>☺</literal>
(In reply to Simon McVittie from comment #15) > we could in principle subtree-merge this main loop into test/dbus-gmain/ and > link it into test/libdbus-testutils.la. What do you think? Worth it or not? Not worth it just for this one TODO comment. If there are several other places where doing this subtree-merge would be of benefit, then sure. As it stands, though, since the TODO works and is only in test code, I don’t think any changes to it are of particular priority.
(In reply to Simon McVittie from comment #3) > This depends on my containers-spec branch, which I will merge to master when > we're reasonably happy that the spec is stable. I'd rather not do that until > I've at least done a real-life test against a patched Flatpak and > xdg-desktop-portal; this is (I think) the last piece of dbus work that's > needed before I can usefully do that. Here are manual test instructions. Preconditions: * Have a dbus-daemon with this feature integrated, installed system-wide. I applied these patches (plus some unrelated patches that are queued for review on other bugs) to Debian's dbus 1.13.0-1 and installed the result. * Have a flatpak with https://github.com/flatpak/flatpak/pull/890, and build it (installing it is not required). Use an OS where unprivileged user namespaces are allowed, or combine --with-system-bubblewrap with a setuid-root bubblewrap installed system-wide. I used 0.11.3-8-g86be4134 on Debian unstable, with the setuid system bubblewrap 0.2.0-4. * Have an xdg-desktop-portal with https://github.com/flatpak/xdg-desktop-portal/pull/164, and build it (installing it is not required). Kill any xdg-desktop-portal instances that were already running. To test the fast-path using this header field, build xdg-desktop-portal with CPPFLAGS=-DRELY_ON_GLIB_IMPLEMENTATION_DETAILS for now (I'm relying on behaviour that is not strictly API, to avoid needing a patched GLib as well as everything else). I used xdg-desktop-portal 0.10-10-ga84e717. * Have https://github.com/matthiasclasen/portal-test already installed as a Flatpak app. Test instructions: * dbus-monitor --session * In the xdg-desktop-portal $builddir: G_MESSAGES_DEBUG=all gdb ./xdg-desktop-portal Among the debug spew you should see: ** (./xdg-desktop-portal:xxx): DEBUG: DBus.Containers1 supported * In the gdb: (gdb) break openat (gdb) commands 1 >bt >cont >end (gdb) run * In the dbus-monitor, you should see a call to Containers1.RequestHeader() * In the flatpak $builddir: FLATPAK_DBUSPROXY=$(pwd)/flatpak-dbus-proxy \ $(pwd)/flatpak run org.gnome.PortalTest * Click "Notify me!" * In the xdg-desktop-portal output, you should see: ** (./xdg-desktop-portal:xxx): DEBUG: Creating new app info for Flatpak app org.gnome.PortalTest (container instance /org/freedesktop/DBus/Containers1/c14) * In the gdb output, you should *not* see an openat() on "/proc/nnn/root" followed by one on ".flatpak-info" * You should see a freedesktop.org notification
Based on successful review and successful testing against a real service, I've merged the implementation of this feature. I think the spec is also reasonably solid now.
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.