+++ 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].
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.
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).
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?
Created attachment 136697 [details] [review] driver: Factor out bus_driver_fill_connection_credentials
Created attachment 136698 [details] [review] containers: Include credentials of initiator in container instance info
Comment on attachment 136694 [details] [review] test/containers: Don't require type, name in GetConnectionCredentials Review of attachment 136694 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136695 [details] [review] bus driver: Omit container type, name from GetConnectionCredentials Review of attachment 136695 [details] [review]: ----------------------------------------------------------------- r+
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.
(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 on attachment 136698 [details] [review] containers: Include credentials of initiator in container instance info Review of attachment 136698 [details] [review]: ----------------------------------------------------------------- r+
Comment on attachment 136694 [details] [review] test/containers: Don't require type, name in GetConnectionCredentials Applied for 1.13.0, thanks.
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.
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.
(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?
(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 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 on attachment 136697 [details] [review] driver: Factor out bus_driver_fill_connection_credentials Review of attachment 136697 [details] [review]: ----------------------------------------------------------------- r+
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. How we collect and use information is described in our Privacy Policy.