interfaces/desktop-launch: support confined snaps launching other snaps #8699

AlanGriffiths · 2020-05-19T15:49:06Z

Add a new interface desktop-launch that allows shells to read .desktop files from /var/lib/snapd/desktop/applications/ and call io.snapcraft.PrivilegedDesktopLauncher.OpenDesktopEntry.

Add io.snapcraft.PrivilegedDesktopLauncher.OpenDesktopEntry support to userd that implements support for this on Classic systems.

The result is that a confined desktop shell can identify other snaps and launch them with (for example) the WAYLAND_DESKTOP environment variable needed for the client to connect to the correct desktop.

Follows on from:

Discussion at forum.snapcraft.io; and,
interfaces/app-launch: support confined snaps launching other snaps #7490 (rebased and updated)

> * tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml

…apd/strutil/shlex

…escribe the shell to toolkits.

…ving a zombie process

…All" in CI

interface for PrivilegedDesktopLauncher This reinforces that the API is not in the same security domain as those exported on /io/snapcraft/Launcher.

…out mocked stat calls

…ktop files fails

… sections

jhenstridge · 2021-03-25T01:27:38Z

Once the changes from https://github.com/MirServer/snapd/pull/6 are merged into this PR, I think it would probably be ready for another review. I've tried to close off all the threads that appear to have been addressed, but the number of comments on the PR has become a bit unmanageable.

More fixes based on review feedback

…anonical/snapd#8699

1968: Update the snapcraft launch object path r=wmww a=AlanGriffiths To reflect recent changes to canonical/snapd#8699 Co-authored-by: Alan Griffiths <alan@octopull.co.uk>

jhenstridge

Looks good!

pedronis

thanks, did an almost final pass

pedronis · 2021-04-14T15:46:52Z

usersession/userd/privileged_desktop_launcher.go

+	// Iterate through the potential subdirectories formed by the first i elements of the desktop file ID.
+	// Maybe this is overkill: At the time of writing, the only use is in desktopFileIDToFilename() and there
+	// we're only checking dirs.SnapDesktopFilesDir (not all entries in $XDG_DATA_DIRS) and we know that snapd
+	// does not create subdirectories in that location.


this comment doesn't sound valid anymore as the new approach seems to be to find the file as anything else would, and then double check that is from a place that is supported

pedronis · 2021-04-14T15:48:03Z

usersession/userd/privileged_desktop_launcher.go

+		// /var/lib/snapd/desktop/applications and these desktop files are written by snapd and
+		// considered safe for userd to process. If other directories are added in the future,
+		// verifyDesktopFileLocation() and parseExecCommand() may need to be updated.
+		return fmt.Errorf("internal error: only launching snap applications from %s is supported", dirs.SnapDesktopFilesDir)


given the new approach to search this is not an internal error anymore, the prefix should be stripped

pedronis · 2021-04-14T15:52:28Z

interfaces/builtin/desktop_launch.go

+
+package builtin
+
+const desktopLaunchSummary = `allows snaps to identify and launch other snaps`


this should probably be now:

allows to identify and launch desktop applications in(or from) other snaps

?

pedronis · 2021-04-14T15:52:55Z

interfaces/builtin/desktop_launch.go

+
+const desktopLaunchSummary = `allows snaps to identify and launch other snaps`
+
+const desktopLaunchBaseDeclarationPlugs = `


the base declaration looks right, it follow the same pattern as snapd-control of a superprivileged interface

pedronis · 2021-04-14T15:55:46Z

interfaces/builtin/desktop_launch_test.go

+	err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot)
+	c.Assert(err, IsNil)
+	c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.other.app"})
+	c.Assert(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, `Can identify and launch other snaps.`)


we probably want to check as well at least for:

member=OpenDesktopEntry peer=(label=unconfined),

jhenstridge · 2021-04-16T05:14:46Z

interfaces/builtin/desktop_launch.go

@@ -48,7 +48,7 @@ dbus (send)
    bus=session
    path=/io/snapcraft/PrivilegedDesktopLauncher
    interface=io.snapcraft.PrivilegedDesktopLauncher
-    member=OpenDesktopEntry
+    peer=(label=unconfined),member=OpenDesktopEntry


This looks wrong. The comma separates AppArmor rules, so this breaks a rule in half.

Rather than changing the AppArmor snippet, @pedronis was suggesting that an extra assertion be added to the test verifying that the snippet contained this part of the rule.

anonymouse64

there's a conflict here, but +1 from me to merge this as-is, I left some comments about followups but this is looking very good thank you for the iteration on this

anonymouse64 · 2021-05-20T23:28:49Z

usersession/userd/privileged_desktop_launcher.go

+	conn *dbus.Conn
+}
+
+// Name returns the name of the interface this object implements


nitpick, but we typically like to end doc-comments with ".", but this should be done in a followup

anonymouse64 · 2021-05-20T23:31:51Z

usersession/userd/privileged_desktop_launcher.go

+// desktopFileIDToFilename determines the path associated with a desktop file ID.
+func desktopFileIDToFilename(desktopFileID string) (string, error) {
+	if !isValidDesktopFileID(desktopFileID) {
+		return "", fmt.Errorf("cannot find desktop file for %q", desktopFileID)


should this error message indicate that the desktop file ID is invalid insofar as it doesn't match the expected regex? something like

Suggested change

return "", fmt.Errorf("cannot find desktop file for %q", desktopFileID)

return "", fmt.Errorf("cannot find desktop file for %q: desktop file ID is malformed", desktopFileID)

also fine for followup if we want it

anonymouse64 · 2021-05-20T23:39:17Z

usersession/userd/privileged_desktop_launcher.go

+
+	args := make([]string, 0, len(origArgs))
+	for _, arg := range origArgs {
+		// We want to keep literal '%' (expressed as '%%') but filter our exec variables


Suggested change

// We want to keep literal '%' (expressed as '%%') but filter our exec variables

// We want to keep literal '%' (expressed as '%%') but filter out exec variables

anonymouse64 · 2021-05-20T23:41:29Z

usersession/userd/privileged_desktop_launcher_internal_test.go

+		{"/snap/bin/foo -f %U %%bar", []string{"/snap/bin/foo", "-f", "%bar"}},
+		{"/snap/bin/foo -f %U %i %%bar", []string{"/snap/bin/foo", "-f", "--icon", "/snap/chromium/1193/chromium.png", "%bar"}},
+		{"/snap/bin/foo -f %U %%bar %i", []string{"/snap/bin/foo", "-f", "%bar", "--icon", "/snap/chromium/1193/chromium.png"}},
+		{"/snap/bin/foo -f %%bar %U %i", []string{"/snap/bin/foo", "-f", "%bar", "--icon", "/snap/chromium/1193/chromium.png"}},


this is okay to do as a followup

anonymouse64 · 2021-05-20T23:42:57Z

usersession/userd/privileged_desktop_launcher_test.go

+	})
+}
+
+func (s *privilegedDesktopLauncherSuite) TestDesktopFileLookup(c *C) {


thanks for adding this test

anonymouse64 · 2021-05-20T23:44:20Z

usersession/userd/privileged_desktop_launcher_internal_test.go

+	c.Assert(err, ErrorMatches, `desktop file ".*" has an unsupported 'Exec' value: ""`)
+}
+
+func (s *privilegedDesktopLauncherInternalSuite) TestReadExecCOmmandFromDesktopFileMultipleDesktopEntrySections(c *C) {


thanks for this test too

anonymouse64 · 2021-05-20T23:46:06Z

tests/main/interfaces-desktop-launch/task.yaml

+    The desktop-launch interface allows a snap to launch other snaps via
+    the desktop files they provide to the host system.
+
+systems: [-ubuntu-core-*]


could you add a comment that userd does not yet work properly enough on ubuntu core to be able to run this test there (fine for followup)?

anonymouse64 · 2021-05-20T23:47:25Z

tests/main/interfaces-desktop-launch/task.yaml

+    snap connections test-launcher | MATCH "desktop-launch +test-launcher:desktop-launch +:desktop-launch +manual"
+
+    echo "The launcher snap can launch other snaps via userd"
+    tests.session -u test exec test-launcher \


does this need to be exec? I assume this is somehow related to user sessions?

also should we be checking the output of this command with MATCH, since we are using --print-reply?

pedronis · 2021-05-28T16:25:48Z

@AlanGriffiths @jhenstridge the last run of tests got stuck, anyway it seems something changed in master before the last master merge and now the spread test is failing and need adjusting/fixing, this probably needs again a master merge and fixes, sorry

anonymouse64

I've commented with the spread test changes necessary

tests/main/interfaces-desktop-launch/task.yaml

Co-authored-by: Ian Johnson <person.uwsome@gmail.com>

AlanGriffiths added 30 commits May 18, 2020 17:11

"shell-support" interface

0461b76

Spike OpenDesktopEntry method

a954fea

Add OpenDesktopEntryEnv to permit setting environment variables

8a60e15

Switch to Desktop File IDs

6f8343d

Extract desktopFileIdToFilename()

677a1bd

Extract readExecCommandFromDesktopFile()

2258b75

Clearer use of whitespace

5987a0a

More robust logic in readExecCommandFromDesktopFile()

2bc5922

Document the processing of the exec command

71d2123

> Missing high-level test for interface 'shell-support'. Please add to:

b7de328

> * tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml

Handle shell quoting in the exec command

abfc59a

Drop the OpenDesktopEntry() method

c48272c

Better handling of exec variables

67d2f61

deny-auto-connection: true

e8dec70

Use the MockConnectedPlug and MockConnectedSlot helpers

b769b09

Use free functions

23e7a89

Rename shell-support => app-launch

a11e9b4

Report error if desktop file not found

2671228

Search all the paths that can be formed by the desktop ID

16f03b0

We don't need github.com/google/shlex, we have github.com/snapcore/sn…

00cc0f8

…apd/strutil/shlex

Don't use error to indicate whether a desktop file is found

f1aeab9

Update comments referring to desktop-entry-spec-latest.html

196ad63

Don't ignore errors from os.Stat()

09f9474

Restrict the environment variables that may be set to those used to d…

fca6562

…escribe the shell to toolkits.

Comment to explain the code

848a73e

Use dirs.SnapDesktopFilesDir, not a hard coded path

cca474e

First cut at some internal tests

a7f75ee

Test parsing of Exec command

5dd7228

Use the shell to launch the app to avoid becoming a parent and/or lea…

2567e93

…ving a zombie process

Fix "usersession/userd/launcher.go:154:13: undefined: strings.Replace…

a3c1ed2

…All" in CI

jhenstridge added 5 commits March 24, 2021 15:23

usersession/userd: don't reuse the object path of the existing launcher

3d48386

interface for PrivilegedDesktopLauncher This reinforces that the API is not in the same security domain as those exported on /io/snapcraft/Launcher.

usersession/userd: add a direct test for DesktopFileIDToFilename with…

14009e0

…out mocked stat calls

usersession/userd: add test demonstrating that launching non-snap des…

b144983

…ktop files fails

usersession/userd: more fixups based on review comments

a8c979c

usersession/userd: reject desktop files with multiple [Desktop Entry]…

47d6a3e

… sections

Merge pull request #6 from jhenstridge/desktop-launch-iface

7be605e

More fixes based on review feedback

AlanGriffiths added a commit to canonical/mir that referenced this pull request Mar 25, 2021

Update the snapcraft launch object path to reflect recent changes to c…

e26edd2

…anonical/snapd#8699

AlanGriffiths mentioned this pull request Mar 25, 2021

Update the snapcraft launch object path canonical/mir#1968

Merged

bors bot added a commit to canonical/mir that referenced this pull request Mar 25, 2021

Merge #1968

516bd0b

1968: Update the snapcraft launch object path r=wmww a=AlanGriffiths To reflect recent changes to canonical/snapd#8699 Co-authored-by: Alan Griffiths <alan@octopull.co.uk>

jhenstridge approved these changes Mar 26, 2021

View reviewed changes

anonymouse64 self-requested a review March 29, 2021 19:41

pedronis self-requested a review April 1, 2021 08:06

pedronis reviewed Apr 14, 2021

View reviewed changes

Address latest review feedback

2284c54

jhenstridge reviewed Apr 16, 2021

View reviewed changes

AlanGriffiths added 2 commits April 16, 2021 09:26

Fix accidental paste.

4213409

Update tests to match

57ef4e2

jhenstridge added the 🎂 label May 20, 2021

anonymouse64 approved these changes May 20, 2021

View reviewed changes

Merge branch 'master' into open-desktop-entry

76a3681

pedronis closed this May 28, 2021

pedronis reopened this May 28, 2021

anonymouse64 added this to the 2.52 milestone May 28, 2021

Merge remote-tracking branch 'origin/master' into open-desktop-entry

ebb891c

anonymouse64 reviewed May 28, 2021

View reviewed changes

tests/main/interfaces-desktop-launch/task.yaml Outdated Show resolved Hide resolved

tests/main/interfaces-desktop-launch/task.yaml Outdated Show resolved Hide resolved

AlanGriffiths and others added 2 commits May 28, 2021 23:46

Update tests/main/interfaces-desktop-launch/task.yaml

76284d4

Co-authored-by: Ian Johnson <person.uwsome@gmail.com>

Update tests/main/interfaces-desktop-launch/task.yaml

13c4aea

Co-authored-by: Ian Johnson <person.uwsome@gmail.com>

pedronis merged commit 856a839 into canonical:master May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces/desktop-launch: support confined snaps launching other snaps #8699

interfaces/desktop-launch: support confined snaps launching other snaps #8699

AlanGriffiths commented May 19, 2020 •

edited by pedronis

Loading

jhenstridge commented Mar 25, 2021

jhenstridge left a comment

pedronis left a comment

pedronis Apr 14, 2021

pedronis Apr 14, 2021

pedronis Apr 14, 2021

pedronis Apr 14, 2021 •

edited

Loading

pedronis Apr 14, 2021 •

edited

Loading

jhenstridge Apr 16, 2021

anonymouse64 left a comment

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

anonymouse64 May 20, 2021

pedronis commented May 28, 2021

anonymouse64 left a comment


		package builtin

		const desktopLaunchSummary = `allows snaps to identify and launch other snaps`


		const desktopLaunchSummary = `allows snaps to identify and launch other snaps`

		const desktopLaunchBaseDeclarationPlugs = `

	return "", fmt.Errorf("cannot find desktop file for %q", desktopFileID)
	return "", fmt.Errorf("cannot find desktop file for %q: desktop file ID is malformed", desktopFileID)

	// We want to keep literal '%' (expressed as '%%') but filter our exec variables
	// We want to keep literal '%' (expressed as '%%') but filter out exec variables

interfaces/desktop-launch: support confined snaps launching other snaps #8699

interfaces/desktop-launch: support confined snaps launching other snaps #8699

Conversation

AlanGriffiths commented May 19, 2020 • edited by pedronis Loading

jhenstridge commented Mar 25, 2021

jhenstridge left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

pedronis Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented May 28, 2021

anonymouse64 left a comment

Choose a reason for hiding this comment

AlanGriffiths commented May 19, 2020 •

edited by pedronis

Loading

pedronis Apr 14, 2021 •

edited

Loading

pedronis Apr 14, 2021 •

edited

Loading