-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Retrieve a not-empty Application ID for un-sandboxed clients that use cgroups #581
Conversation
16375ef
to
d9aa3e8
Compare
This is a good change IMO. However some code doesn't use XdpAppInfo's XDP_APP_INFO_KIND_HOST to determine that the client is non-sandboxed but instead just checks the app_id for NULL. Non-sandboxed clients for which the app id can be determined then get the same treatment as sandboxed clients. |
In the current implementation when would the To avoid changing the code without being sure, I post here the potential tests that perhaps could look at
xdg-desktop-portal/src/location.c Line 331 in 89d2197
xdg-desktop-portal/src/location.c Line 369 in 89d2197
xdg-desktop-portal/src/location.c Line 532 in 89d2197
xdg-desktop-portal/src/wallpaper.c Line 184 in 89d2197
Additionally there are many checks on |
Where could this not be desirable? |
I guess in case of document portal trying unsandboxed apps to use it isn't desirable although the portal will fail anyway as it won't be able to determine app file permissions which is unfortunately also true for sandboxed but non-flatpak/snap apps. |
We should pass down the appinfo here.
Here too.
Not sure it makes a difference here, but we have the app_info right there. |
22c58df
to
2fc9546
Compare
I was trying to do so, bu is there a simple way to retrieve XdgAppInfo from XdgDomain or from Problem is that I don't think I am able to retrieve the AppInfo from there. I can see where the app_id is coming from, but atm I think I lack the knowledge in order to get something else.
P.S. similarly here:
|
2fc9546
to
c0ce358
Compare
c0ce358
to
24f41d2
Compare
@@ -27,6 +27,7 @@ CLEANFILES += $(dbus_service_DATA) | |||
CLEANFILES += $(systemduserunit_DATA) | |||
EXTRA_DIST += $(service_in_files) | |||
|
|||
CFLAGS = -lsystemd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps make it a compile-time optional, enabled by default?
char *unit, **dash_splits, **at_splits, **app_id; | ||
int res, len; | ||
|
||
res = sd_pid_get_user_unit(pid, &unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: missing space before (
/* | ||
* the session might not be managed by systemd | ||
* or there could be error fetching own systemd units | ||
* or the unit might not be started by the the desktop environment (e.g. it's a script run from terminal) | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is necessary - probably better to mention these cases in the commit message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a helpful comment to me
* or there could be error fetching own systemd units | ||
* or the unit might not be started by the the desktop environment (e.g. it's a script run from terminal) | ||
*/ | ||
if (res == -ENODATA || res < 0 || !g_str_has_prefix (unit, "app-")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
g_str_has_prefix
is not NULL-safe, so !unit || !g_str_has_prefix (...)
g_strfreev(at_splits); | ||
g_strfreev (dash_splits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove these calls by declaring these variables as g_auto(GStrv) at_splits = NULL
and g_auto(GStrv) dash_splits = NULL
else { | ||
at_splits = g_strsplit (dash_splits[len-1], "@", 2); | ||
app_id = &at_splits[0]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to check if (g_str_has_suffix (unit, ".service"))
here, and let the else
condition set the app id to an empty string and return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @matthiasclasen that using XdpAppInfo
everywhere is a prerequisite for doing this.
I think if someone wants to move this forward (@GeorgesStavracas? @mwleeds?) then the way to do it would be:
- first, go through all the places that use an app ID for decisions, and make sure they get an
XdpAppInfo
instead, withkind == XDP_APP_INFO_KIND_HOST
instead ofg_str_equal (app_id, "")
as the way to tell that the caller is un-sandboxed; - then, reapply changes similar to these on top of that refactoring
@@ -27,6 +27,7 @@ CLEANFILES += $(dbus_service_DATA) | |||
CLEANFILES += $(systemduserunit_DATA) | |||
EXTRA_DIST += $(service_in_files) | |||
|
|||
CFLAGS = -lsystemd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become a check for libsystemd in the build system (Flatpak has a suitable check in its configure.ac
), keeping libsystemd optional. If compiled without libsystemd support, the answer should always be "I don't know what app this is".
if ((*str)[i] == '\\' && (*str)[i+1] == 'x') | ||
{ | ||
hex[2] = (*str)[i+2]; | ||
hex[3] = (*str)[i+3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If hex[2]
is \0
then this will be out of bounds.
char **splits = g_strsplit(*str, "\\x\\", -1); | ||
g_free (*str); | ||
*str = g_strjoinv ("", splits); | ||
g_strfreev (splits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After going to heroic lengths to do the decoding in-place with no extra allocations, this does a lot of malloc/free cycles at the end...
I think a better approach to this would be to take a const char *
argument and iterate through it, appending characters to a GString
buffer, returning a newly allocated string with g_string_free (gstr, FALSE)
.
* app[-<launcher>]-<ApplicationID>-<RANDOM>.scope | ||
* app[-<launcher>]-<ApplicationID>-autostart.service -> no longer true since systemd v248 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead of relying on the part after the app-ID to not contain any additional -
(e.g. app-gnome-com.example.Foobar-123-456.service
would break this logic), it might make more sense to assume that the <launcher>
does not contain dots (in practice it's something like gnome
, flatpak
or steam
, which doesn't), so that the logic could be:
- remove trailing
.service
,.slice
or.scope
- remove an
@
and anything after it, if present - split into dash-separated components
- if second component contains at least two dots, it's the app ID
- else if third component contains at least two dots, it's the app ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came up with a regex that seems to work for this: "^app-(?:[[:alnum:]]+\\-)?(.+?\\..+?\\..+?)(?:[@\\-][0-9]*)?(?:-autostart)?(?:\\.scope|\\.service|\\.slice)$"
But unfortunately the unit returned by systemd for gnome-software (which runs as /usr/bin/gnome-software --gapplication-service
) is app-gnome-gnome\x2dsoftware\x2dservice-9502.scope
(where 9502 is the pid in my case) so parsing that in any way is not going to get us the correct app ID of org.gnome.Software
.
char *unit, **dash_splits, **at_splits, **app_id; | ||
int res, len; | ||
|
||
res = sd_pid_get_user_unit(pid, &unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit
is leaked, at the moment.
Seems to me we need two variants of |
Looks like there is no need to change We can test the document portal with these patches (when they're ready) just to be sure, but I think it would be a bit pointless since the document portal doesn't use the fuse filesystem to expose files to unsandboxed callers, and passes them the host path directly instead. |
There may be sandboxed but not recognized as such callers though. I wonder if there could be a opt-in switch to force document portal to use fuse filesystem for those. |
As discussed in #581, we need to change how we handle xdp_app_info_get_id() returning an empty string. Currently, this only happens when the calling pid is unsandboxed (not Flatpak or Snap) so it has so far been safe to assume that xdp_app_info_get_id() will return the empty string if and only if xdp_app_info_is_host() returns TRUE. However since we are about to add support for setting the app ID in XdpAppInfo even for unsandboxed callers, we need to: (a) Only use xdp_app_info_is_host() if the intention is to give unsandboxed processes access to something, and (b) Only check if the app ID is the empty string if we're doing so to handle that case gracefully, e.g. by leaving out the app ID from a user-facing message. This commit does entail a slight change in behavior: for those portals that use the app ID in the permission store even if the app ID is the empty string, such as the wallpaper portal, the behavior before this commit is that all unsandboxed apps share the same permission since the empty string is used as the key. After this commit, only unsandboxed callers for whom an app ID could not be determined share the same permission (in other words granting a permission for one grants it for all). I don't know if storing the empty string as the app ID in the permission store is actually a good idea, but doing so is not a new change in this commit, so if we want to change that we can do it separately.
Preparatory PR for this is here: #718 |
In a sense there already is a switch in that you can create |
As discussed in #581, we need to change how we handle xdp_app_info_get_id() returning an empty string. Currently, this only happens when the calling pid is unsandboxed (not Flatpak or Snap) so it has so far been safe to assume that xdp_app_info_get_id() will return the empty string if and only if xdp_app_info_is_host() returns TRUE. However since we are about to add support for setting the app ID in XdpAppInfo even for unsandboxed callers, we need to: (a) Only use xdp_app_info_is_host() if the intention is to give unsandboxed processes access to something, and (b) Only check if the app ID is the empty string if we're doing so to handle that case gracefully, e.g. by leaving out the app ID from a user-facing message. This commit does entail a slight change in behavior: for those portals that use the app ID in the permission store even if the app ID is the empty string, such as the wallpaper portal, the behavior before this commit is that all unsandboxed apps share the same permission since the empty string is used as the key. After this commit, only unsandboxed callers for whom an app ID could not be determined share the same permission (in other words granting a permission for one grants it for all). I don't know if storing the empty string as the app ID in the permission store is actually a good idea, but doing so is not a new change in this commit, so if we want to change that we can do it separately.
The https://github.com/flatpak/xdg-desktop-portal/blob/main/document-portal/document-portal.c#L648 https://github.com/flatpak/xdg-desktop-portal/blob/main/document-portal/document-portal.c#L569 Obviously the former will always fail for non-flatpak sandbox and portal will pass the host path even if it doesn't exist in sandbox. So setting non-empty appid for document portal is pointless as you observed because it's impossible to use it anyway but I wonder if it can be made meaningful in future? The issue for that exist: #680 |
Closing this in favor of #719 which is in better shape but also not ready to be merged. |
As discussed in #581, we need to change how we handle xdp_app_info_get_id() returning an empty string. Currently, this only happens when the calling pid is unsandboxed (not Flatpak or Snap) so it has so far been safe to assume that xdp_app_info_get_id() will return the empty string if and only if xdp_app_info_is_host() returns TRUE. However since we are about to add support for setting the app ID in XdpAppInfo even for unsandboxed callers, we need to: (a) Only use xdp_app_info_is_host() if the intention is to give unsandboxed processes access to something, and (b) Only check if the app ID is the empty string if we're doing so to handle that case gracefully, e.g. by leaving out the app ID from a user-facing message. This commit does entail a slight change in behavior: for those portals that use the app ID in the permission store even if the app ID is the empty string, such as the wallpaper portal, the behavior before this commit is that all unsandboxed apps share the same permission since the empty string is used as the key. After this commit, only unsandboxed callers for whom an app ID could not be determined share the same permission (in other words granting a permission for one grants it for all). I don't know if storing the empty string as the app ID in the permission store is actually a good idea, but doing so is not a new change in this commit, so if we want to change that we can do it separately.
Following #579, I present a simple but yet effective solution to retrieve the Application ID from PID's cgroup.
The usage of cgroups to manage processes by DEs is an innovative approach pushed forward by both KDE & GNOME, and uses the application's ID to distinguish the various resources.
I thus intend to check if the cgroup of the client is compliant to the standard and, if it is, use the contained ID. Otherwise proceed with how we have always done: assigning
""
asapp_id
.You can test this patch by running my demo with an appropriate
.desktop
file or with one of the two following command:Currently missing
Documentation update. I'd write two lines in Chapter 1. Common Conventions informing that a non-empty
app_id
is passed down only if the launcher is compliant with the standard: https://systemd.io/DESKTOP_ENVIRONMENTS/