Bug 101899 - Containers (#100344): new header field for container instance
Summary: Containers (#100344): new header field for container instance
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/tree/con...
Whiteboard: review+
Keywords: patch
Depends on: 101354
Blocks: 100344
  Show dependency treegraph
 
Reported: 2017-07-24 14:55 UTC by Simon McVittie
Modified: 2018-02-20 17:37 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
spec: Add a method call to request a new CONTAINER_INSTANCE header field (4.32 KB, patch)
2018-01-15 18:25 UTC, Simon McVittie
Details | Splinter Review
bus_transaction_send: Take sender and destination connections (5.67 KB, patch)
2018-01-15 18:25 UTC, Simon McVittie
Details | Splinter Review
DBusMessage: Add a header field for the container instance (6.48 KB, patch)
2018-01-15 18:25 UTC, Simon McVittie
Details | Splinter Review
containers: Add a method to ask to be sent the connection instance header (6.05 KB, patch)
2018-01-15 18:25 UTC, Simon McVittie
Details | Splinter Review
test/containers: Test the new header field (6.77 KB, patch)
2018-01-15 18:27 UTC, Simon McVittie
Details | Splinter Review
spec: Add a method call to request a new CONTAINER_INSTANCE header field (4.82 KB, patch)
2018-02-06 22:54 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-07-24 14:55:35 UTC
+++ 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).
Comment 1 Philip Withnall 2017-08-01 11:35:18 UTC
(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?
Comment 2 Simon McVittie 2017-10-20 18:00:16 UTC
(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.
Comment 3 Simon McVittie 2018-01-15 18:25:01 UTC
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.
Comment 4 Simon McVittie 2018-01-15 18:25:21 UTC
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).
Comment 5 Simon McVittie 2018-01-15 18:25:39 UTC
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).
Comment 6 Simon McVittie 2018-01-15 18:25:54 UTC
Created attachment 136740 [details] [review]
containers: Add a method to ask to be sent the connection  instance header
Comment 7 Simon McVittie 2018-01-15 18:27:38 UTC
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 8 Philip Withnall 2018-01-16 16:57:19 UTC
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 9 Philip Withnall 2018-01-16 17:01:22 UTC
Comment on attachment 136738 [details] [review]
bus_transaction_send: Take sender and destination  connections

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

r+
Comment 10 Philip Withnall 2018-01-16 17:14:08 UTC
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 11 Philip Withnall 2018-01-16 17:25:27 UTC
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 12 Philip Withnall 2018-01-16 17:31:22 UTC
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.
Comment 13 Simon McVittie 2018-02-06 22:50:11 UTC
(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.
Comment 14 Simon McVittie 2018-02-06 22:54:37 UTC
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.
Comment 15 Simon McVittie 2018-02-06 23:00:21 UTC
(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 16 Philip Withnall 2018-02-16 14:22:24 UTC
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>
Comment 17 Philip Withnall 2018-02-16 14:26:59 UTC
(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.
Comment 18 Simon McVittie 2018-02-20 17:00:17 UTC
(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
Comment 19 Simon McVittie 2018-02-20 17:37:28 UTC
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.