Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce XDP_APP_INFO_KIND_CONTAINERS1 which gets all information from D-Bus #1268

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

swick
Copy link
Contributor

@swick swick commented Jan 18, 2024

Get the org.fdo.DBus.Containers1 idea rolling!

flatpak: flatpak/flatpak#5656
dbus: https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/449

This falls back to the PID based lookup (flatpak/snap) if D-Bus doesn't support the new interface.

Remaining issues, everything else is supposed to work:

  • Remapping paths (xdp_app_info_remap_path, xdp_app_info_get_path_for_fd) is not supported
  • Dynamic Launcher's TryExec (xdp_app_info_get_tryexec_path) is not supported

Should be ready for review.

@swick swick changed the title Wip/containers1 Introduce XDP_APP_INFO_KIND_CONTAINERS1 which gets all information from D-Bus Jan 23, 2024
@swick swick marked this pull request as ready for review January 23, 2024 16:08
@swick swick force-pushed the wip/containers1 branch 5 times, most recently from 68dff4e to 2d9f7a9 Compare January 23, 2024 16:50
@swick
Copy link
Contributor Author

swick commented Jan 23, 2024

Found a few bugs and CI is green.

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Just a pair of comments so far, but I don't think I'm the best person to review this. It would have helped if the commits had commit messages.

data/org.freedesktop.portal.Device.xml Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
A shell of its former self remains which informs any potential user
about the removal.

The Device portal is, to the best of our knowledge, not used by any
component. It's also weird because it's not a portal expoed to clients
which caused confusion.

The service it provides is also provided by the permission store, minus
the ability to map from arbitrary PID to an app id. This PID to app id
mapping isn't something that can be done in general and is most likely
broken when the PID is not of the xdg-dbus-proxy.
One was a left-over and for the other we just removed the last users.
This will become useful once we support the dbus Containers1 interface
and a new XDP_APP_INFO kind can still refer to a flatpak.
…fo_sync

This will make it easier to follow which method of identiying an app
will be used under what conditions.
@swick
Copy link
Contributor Author

swick commented Jan 23, 2024

Made the commit messages much more useful and addressed the review.

Who is the person I should nag for a review then?

@smcv
Copy link
Collaborator

smcv commented Jan 23, 2024

Who is the person I should nag for a review then?

Please be patient, I will get to this when I can. Nagging potential reviewers is likely to be counterproductive.

@GeorgesStavracas
Copy link
Member

@swick thanks. I was going to mention @smcv , but since they're already aware of this PR, let's wait for their review.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I haven't reviewed everything in detail)

src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
Comment on lines +2448 to +2457
if (app_info->kind == XDP_APP_INFO_KIND_FLATPAK)
return xdp_app_info_ensure_pidns_flatpak (app_info, proc, error);

if (app_info->kind == XDP_APP_INFO_KIND_HOST)
return xdp_app_info_ensure_pidns_host (app_info, proc, error);

if (app_info->kind == XDP_APP_INFO_KIND_CONTAINERS1)
return xdp_app_info_ensure_pidns_containers1 (app_info, proc, error);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this deserves to be a switch().

data/org.freedesktop.portal.Device.xml Show resolved Hide resolved
src/device.c Show resolved Hide resolved
src/xdp-utils.c Show resolved Hide resolved
Comment on lines +916 to +933
if (g_variant_lookup (metadata, "DesktopFile", "&s", &desktop_file))
app_info->u.containers1.desktop_file = g_strdup (desktop_file);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned on flatpak/flatpak#5656, this isn't a standardized field. I know this because we haven't yet defined any standardized fields, or even where you would go to standardize them!

@swick swick force-pushed the wip/containers1 branch 2 times, most recently from 6c366f8 to 03d9394 Compare February 8, 2024 20:13
Instead of returning either NULL with error, NULL without error and
non-NULL without error, adhere to GLib convention and return either TRUE
with an out param set or FALSE with an error set.
The org.fdo.DBus.GetConnectionCredentials method gives us both a PID
(ProcessID) and a pidfd (ProcessFD) in one roundtrip. It fails when the
PID can't be retrieved but allows the pidfd to be -1.

The pidfd will become useful for host and Containers1 clients later.
Returns a GVariant containign all the metadata that a container engine
like flatpak has set on the socket for this app instance. This contains
the container type, app id, instance id and additional arbitrary
metadata such as the desktop file name.

If the D-Bus broker doesn't support the Containers1 interface or the app
using the portal is not running in a container, we fall back to the
other ways of identifying apps.

This specifically means it's safe to run a D-Bus broker which supports
Containers1 while flatpak doesn't. Eventually we want to remove the
flatpak and snap specific paths at which point it's a requirement to
have a Containers1 capable flatpak.
Both host apps and Containers1 apps can do pid mappings via pidfd. This
is some prep work for that.
Do some best effort pid remapping for host apps. This will only work
when the D-Bus broker supports pidfd (ProcessFD).
This new app kind is authenticated by D-Bus and x-d-p gets all the
information about the app from D-Bus which itself get the information
from the sandbox engine which set up the sandbox of the app.

Currently only flatpak and dbus (dbus-daemon) supports this mechanism
but it can be supported by any other sandbox engine, such as snap and
firejail.

This also means that an app with XDP_APP_INFO_KIND_CONTAINERS1 can still
be a flatpak or a snap app.

The goal is to make as many paths in x-d-p agnostic to the actual
sandbox engine and handle all XDP_APP_INFO_KIND_CONTAINERS1 apps the
same. Eventually we can then remove any other XDP_APP_INFO_KIND
variants.

This commit sets up a XdpAppInfo object with all the metadata x-d-p will
need. The follow up commit implements various XdpAppInfo
functionalities.
Uses some of the metadata from Containers1 directly and falls back to
the flatpak path in some cases.

Some of the fallbacks can be removed once the D-Bus broker supports ACL
and we can get rid of xdg-dbus-proxy.

The path remapping and tryexec functionalities are not implemented right
now.
@swick
Copy link
Contributor Author

swick commented Feb 8, 2024

Tried to pull out the non-Containers1 bits to #1281.

@GeorgesStavracas
Copy link
Member

This can be rebased now

@swick
Copy link
Contributor Author

swick commented May 24, 2024

Turns out a few things can't really be expressed with just some key value pairs, such as path remapping and the details of installing dynamic launchers. I'm going to use the metadata to pass a bus and object which implements a common Contaienrs1 interface. Flatpak then implements this interface in the flatpak-session-helper and x-d-p can offload the functionality there.

@swick swick marked this pull request as draft June 3, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

3 participants