Bug 101899

(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.