Bug 104610 - Containers (#100344): include creator's credentials in instance information
Summary: Containers (#100344): include creator's credentials in instance information
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on: 101354
Blocks: 100344 103457
  Show dependency treegraph
 
Reported: 2018-01-12 19:57 UTC by Simon McVittie
Modified: 2018-01-16 14:23 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
test/containers: Don't require type, name in GetConnectionCredentials (1.55 KB, patch)
2018-01-12 20:02 UTC, Simon McVittie
Details | Splinter Review
bus driver: Omit container type, name from GetConnectionCredentials (3.50 KB, patch)
2018-01-12 20:02 UTC, Simon McVittie
Details | Splinter Review
spec: Add initiator credentials to container instance information (6.77 KB, patch)
2018-01-12 20:04 UTC, Simon McVittie
Details | Splinter Review
driver: Factor out bus_driver_fill_connection_credentials (7.31 KB, patch)
2018-01-12 20:04 UTC, Simon McVittie
Details | Splinter Review
containers: Include credentials of initiator in container instance info (9.25 KB, patch)
2018-01-12 20:05 UTC, Simon McVittie
Details | Splinter Review
[v2] spec: Add initiator credentials to container instance information (7.18 KB, patch)
2018-01-15 12:44 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 2018-01-12 19:57:43 UTC
+++ This bug was initially created as a clone of Bug #103457 +++

> The other thing I should do here in the short term is to add the container
> instance's initiator's credentials to GetInstanceInfo() and and
> GetConnectionInstance() (which will be an API break, but that's OK, this is
> still very much at the experimental stage and has never been in a release).
> Then anyone calling one of those functions will at least have all the tools
> to make their own decisions [about how far they trust the Type, Name and
> metadata].
Comment 1 Simon McVittie 2018-01-12 20:02:40 UTC
Created attachment 136694 [details] [review]
test/containers: Don't require type, name in  GetConnectionCredentials

On the session bus, the container type and name might be
uncontroversial, but on the system bus, it's questionable how far
they can be trusted: they're supplied by the initiator of the
per-container server, so we only have their word for it.
Comment 2 Simon McVittie 2018-01-12 20:02:59 UTC
Created attachment 136695 [details] [review]
bus driver: Omit container type, name from  GetConnectionCredentials

On the session bus, the container type and name might be
uncontroversial, but on the system bus, it's questionable how far
they can be trusted: they're supplied by the initiator of the
per-container server, so we only have their word for it. While we
think about what to do about this, remove them, leaving only the
instance (which can be used to look up the rest).
Comment 3 Simon McVittie 2018-01-12 20:04:21 UTC
Created attachment 136696 [details] [review]
spec: Add initiator credentials to container instance  information

---

I have temporarily reverted the addition of the Containers1 spec, because I'm not convinced it's ready to be published in the D-Bus Specification yet (and in particular this change is an API break). I'm not sure what to do about publishing it. I might create a long-running containers-spec branch for it, and put the spec changes up for review as though that branch was already merged?
Comment 4 Simon McVittie 2018-01-12 20:04:42 UTC
Created attachment 136697 [details] [review]
driver: Factor out bus_driver_fill_connection_credentials
Comment 5 Simon McVittie 2018-01-12 20:05:12 UTC
Created attachment 136698 [details] [review]
containers: Include credentials of initiator in container  instance info
Comment 6 Philip Withnall 2018-01-15 12:16:27 UTC
Comment on attachment 136694 [details] [review]
test/containers: Don't require type, name in  GetConnectionCredentials

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

r+
Comment 7 Philip Withnall 2018-01-15 12:18:14 UTC
Comment on attachment 136695 [details] [review]
bus driver: Omit container type, name from  GetConnectionCredentials

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

r+
Comment 8 Philip Withnall 2018-01-15 12:20:27 UTC
Comment on attachment 136696 [details] [review]
spec: Add initiator credentials to container instance  information

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

::: doc/dbus-specification.xml
@@ +7282,5 @@
> +                    The credentials of the connection that called the
> +                    <literal>AddServer</literal> method, encoded in the
> +                    same way as the result of the
> +                    <link linkend="bus-messages-get-connection-credentials"
> +                      >GetConnectionCredentials</link> method.

Would it make sense to clarify that these credentials come from the dbus-daemon, so can be trusted; as comparison against the arguments below?

@@ +7379,5 @@
> +                    The credentials of the connection that called the
> +                    <literal>AddServer</literal> method, encoded in the
> +                    same way as the result of the
> +                    <link linkend="bus-messages-get-connection-credentials"
> +                      >GetConnectionCredentials</link> method.

Same here.
Comment 9 Simon McVittie 2018-01-15 12:24:57 UTC
(In reply to Philip Withnall from comment #8)
> Would it make sense to clarify that these credentials come from the
> dbus-daemon, so can be trusted; as comparison against the arguments below?

Good idea.
Comment 10 Philip Withnall 2018-01-15 12:29:14 UTC
Comment on attachment 136698 [details] [review]
containers: Include credentials of initiator in container  instance info

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

r+
Comment 11 Simon McVittie 2018-01-15 12:31:28 UTC
Comment on attachment 136694 [details] [review]
test/containers: Don't require type, name in  GetConnectionCredentials

Applied for 1.13.0, thanks.
Comment 12 Simon McVittie 2018-01-15 12:32:54 UTC
Comment on attachment 136695 [details] [review]
bus driver: Omit container type, name from  GetConnectionCredentials

Applied for 1.13.0, except for the spec change, which doesn't currently apply (I reverted the addition of the Containers1 spec until it settles down). I pushed the spec change as Reviewed-by: you to a new containers-spec branch (which starts with a revert of that revert), currently only present on my github clone.
Comment 13 Simon McVittie 2018-01-15 12:44:13 UTC
Created attachment 136725 [details] [review]
[v2] spec: Add initiator credentials to container instance information

---

Added this text twice, as suggested:

+                    Unlike the container type, name and metadata, these
+                    are supplied by the message bus, and can be trusted to
+                    the same extent as GetConnectionCredentials.
Comment 14 Simon McVittie 2018-01-15 12:46:10 UTC
(In reply to Simon McVittie from comment #4)
> Created attachment 136697 [details] [review]
> driver: Factor out bus_driver_fill_connection_credentials

I assume you're coming back to this one?
Comment 15 Philip Withnall 2018-01-15 13:00:36 UTC
(In reply to Simon McVittie from comment #14)
> (In reply to Simon McVittie from comment #4)
> > Created attachment 136697 [details] [review] [review]
> > driver: Factor out bus_driver_fill_connection_credentials
> 
> I assume you're coming back to this one?

Yeah, I stopped for lunch, and I left it to last because it looked non-trivial. Reviewing now.
Comment 16 Philip Withnall 2018-01-15 13:01:41 UTC
Comment on attachment 136725 [details] [review]
[v2] spec: Add initiator credentials to container instance information

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

r+ with a nitpick

::: doc/dbus-specification.xml
@@ +7285,5 @@
> +                    <link linkend="bus-messages-get-connection-credentials"
> +                      >GetConnectionCredentials</link> method.
> +                    Unlike the container type, name and metadata, these
> +                    are supplied by the message bus, and can be trusted to
> +                    the same extent as GetConnectionCredentials.

Nitpick: add <literal> around GetConnectionCredentials.

(Same below.)
Comment 17 Philip Withnall 2018-01-15 13:10:00 UTC
Comment on attachment 136697 [details] [review]
driver: Factor out bus_driver_fill_connection_credentials

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

r+
Comment 18 Simon McVittie 2018-01-16 14:23:47 UTC
All merged to master for 1.13.0, except for the spec change, which is currently in my containers-spec branch because I'm not sure Containers1 is ready to be in the spec.


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