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

wrappers: allow snaps to install icon theme icons #6767

Merged
merged 15 commits into from Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions dirs/dirs.go
Expand Up @@ -87,6 +87,7 @@ var (
SnapServicesDir string
SnapSystemdConfDir string
SnapDesktopFilesDir string
SnapDesktopIconsDir string
SnapBusPolicyDir string

SystemApparmorDir string
Expand Down Expand Up @@ -232,6 +233,7 @@ func SetRootDir(rootdir string) {
SnapMetaDir = filepath.Join(rootdir, snappyDir, "meta")
SnapBlobDir = filepath.Join(rootdir, snappyDir, "snaps")
SnapDesktopFilesDir = filepath.Join(rootdir, snappyDir, "desktop", "applications")
zyga marked this conversation as resolved.
Show resolved Hide resolved
SnapDesktopIconsDir = filepath.Join(rootdir, snappyDir, "desktop", "icons")
SnapRunDir = filepath.Join(rootdir, "/run/snapd")
SnapRunNsDir = filepath.Join(SnapRunDir, "/ns")
SnapRunLockDir = filepath.Join(SnapRunDir, "/lock")
Expand Down
14 changes: 13 additions & 1 deletion overlord/snapstate/backend/link.go
Expand Up @@ -170,6 +170,13 @@ func generateWrappers(s *snap.Info) error {
wrappers.RemoveSnapBinaries(s)
return err
}
// add the desktop icons
if err := wrappers.AddSnapIcons(s); err != nil {
jhenstridge marked this conversation as resolved.
Show resolved Hide resolved
wrappers.RemoveSnapDesktopFiles(s)
wrappers.RemoveSnapServices(s, progress.Null)
wrappers.RemoveSnapBinaries(s)
return err
}

return nil
}
Expand All @@ -190,7 +197,12 @@ func removeGeneratedWrappers(s *snap.Info, meter progress.Meter) error {
logger.Noticef("Cannot remove desktop files for %q: %v", s.InstanceName(), err3)
}

return firstErr(err1, err2, err3)
err4 := wrappers.RemoveSnapIcons(s)
if err4 != nil {
logger.Noticef("Cannot remove desktop icons for %q: %v", s.InstanceName(), err4)
}

return firstErr(err1, err2, err3, err4)
}

// UnlinkSnap makes the snap unavailable to the system removing wrappers and symlinks.
Expand Down
3 changes: 3 additions & 0 deletions tests/lib/snaps/test-snapd-icon-theme/bin/echo
@@ -0,0 +1,3 @@
#!/bin/sh

jhenstridge marked this conversation as resolved.
Show resolved Hide resolved
echo "From test-snapd-icon-theme snap"
5 changes: 5 additions & 0 deletions tests/lib/snaps/test-snapd-icon-theme/meta/gui/echo.desktop
@@ -0,0 +1,5 @@
[Desktop Entry]
Type=Application
Name=Echo
Exec=test-snapd-icon-theme.echo
Icon=snap.test-snapd-icon-theme.foo
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 6 additions & 0 deletions tests/lib/snaps/test-snapd-icon-theme/meta/snap.yaml
@@ -0,0 +1,6 @@
name: test-snapd-icon-theme
version: 1.0

apps:
echo:
command: bin/echo
37 changes: 37 additions & 0 deletions tests/main/parallel-install-snap-icons/task.yaml
@@ -0,0 +1,37 @@
summary: Parallel installed snaps have non-conflicting icons
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not related to icons but I think it would be neat if our parallel installed desktop files had a way to name the instance automatically.


restore: |
snap set system experimental.parallel-instances=null
jhenstridge marked this conversation as resolved.
Show resolved Hide resolved

execute: |
#shellcheck source=tests/lib/snaps.sh
. "$TESTSLIB"/snaps.sh

echo "Install a snap providing icons"
install_local test-snapd-icon-theme

echo "Install additional instances of the snap"
snap set system experimental.parallel-instances=true
install_local_as test-snapd-icon-theme test-snapd-icon-theme_longname
install_local_as test-snapd-icon-theme test-snapd-icon-theme_foo

echo "Each instance provides its own icons"
icondir=/var/lib/snapd/desktop/icons/hicolor/scalable/apps
[ -f "$icondir/snap.test-snapd-icon-theme.foo.svg" ]
[ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]
[ -f "$icondir/snap.test-snapd-icon-theme_foo.foo.svg" ]

echo "Each instance's desktop file references its own icon"
desktopdir=/var/lib/snapd/desktop/applications
MATCH '^Icon=snap.test-snapd-icon-theme.foo$' < "$desktopdir/test-snapd-icon-theme_echo.desktop"
MATCH '^Icon=snap.test-snapd-icon-theme_longname.foo$' < "$desktopdir/test-snapd-icon-theme+longname_echo.desktop"
MATCH '^Icon=snap.test-snapd-icon-theme_foo.foo$' < "$desktopdir/test-snapd-icon-theme+foo_echo.desktop"

echo "Removing once instance does not remove the other instances' icons"
snap remove test-snapd-icon-theme_foo
[ -f "$icondir/snap.test-snapd-icon-theme.foo.svg" ]
[ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]
[ ! -f "$icondir/snap.test-snapd-icon-theme_foo.foo.svg" ]

snap remove test-snapd-icon-theme
[ -f "$icondir/snap.test-snapd-icon-theme_longname.foo.svg" ]
29 changes: 29 additions & 0 deletions tests/main/snap-icons/task.yaml
@@ -0,0 +1,29 @@
summary: Snaps can install icon theme icons

execute: |
#shellcheck source=tests/lib/snaps.sh
. "$TESTSLIB"/snaps.sh

echo "Install a snap providing icons"
install_local test-snapd-icon-theme

echo "Icons provided by the snap are installed to a shared location"
iconfile=/var/lib/snapd/desktop/icons/hicolor/scalable/apps/snap.test-snapd-icon-theme.foo.svg
[ -f "$iconfile" ]
MATCH "icon from test-snapd-icon-theme" < "$iconfile"

echo "Desktop files can reference installed icons"
desktopfile=/var/lib/snapd/desktop/applications/test-snapd-icon-theme_echo.desktop
MATCH '^Icon=snap.test-snapd-icon-theme.foo$' < "$desktopfile"

echo "Remove the snap"
snap remove test-snapd-icon-theme

echo "The icon has been removed"
[ ! -f "$iconfile" ]

echo "The empty icon theme subdirectories have also been removed"
[ ! -d /var/lib/snapd/desktop/icons/hicolor ]

echo "But the base icons directory remains"
[ -d /var/lib/snapd/desktop/icons ]
34 changes: 34 additions & 0 deletions wrappers/desktop.go
Expand Up @@ -142,6 +142,30 @@ func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
return "", fmt.Errorf("invalid exec command: %q", cmd)
}

func rewriteIconLine(s *snap.Info, line string) (string, error) {
icon := strings.SplitN(line, "=", 2)[1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if line has no =? Then this will panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The calling code reads as:

if bytes.HasPrefix(bline, []byte("Icon=")) {
	line, err := rewriteIconLine(s, string(bline))

So there will always be an equals sign. I followed the pattern of rewriteExecLine here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is something for a follow-up, CC @mvo5 but I'd like to refactor it so that the caller splits and passes key, value arguments so that the code is both correct and obviously correct.


// If there is a path separator, assume the icon is a path name
if strings.ContainsRune(icon, filepath.Separator) {
return line, nil
}
Copy link

@jdstrand jdstrand Aug 13, 2019

Choose a reason for hiding this comment

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

This means that the desktop file can specify Icon=/path/to/anything. This seems overly broad; why wouldn't we limit this in some manner?

I just checked and it is snapcraft that is doing the munging of the Icon file, not snapd. Eg, I used Icon=/etc/passwd, installed the snap and then found it was passed all the way through to /var/lib/snapd/desktop/applications/*.desktop after install. I'll add a review-tools test to make sure it is compliant with this PR, but it seems snapd could do more here. I suspect before the variable expansion if it starts with '/' or if the normalized path is different from the specified path, then failing would be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some additional checks to RewriteIconLine that should cover this:

  1. ensure that the icon begins with ${SNAP}/
  2. ensure that filepath.Clean(icon) == icon

The second part catches escapes like ${SNAP}/../other/icon.png. It won't cover the case of symlinks within the snap that point outside. Presumably that's something the review tools already check for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And just so there's no confusion: errors in this function are logged and cause the Icon= line to be omitted from the sanitised desktop file. The change @jdstrand requested will not prevent the install or upgrade of a snap.

Choose a reason for hiding this comment

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

I've added some additional checks to RewriteIconLine that should cover this:

1. ensure that the icon begins with `${SNAP}/`

2. ensure that `filepath.Clean(icon) == icon`

The second part catches escapes like ${SNAP}/../other/icon.png. It won't cover the case of symlinks within the snap that point outside. Presumably that's something the review tools already check for?

Thanks! I've adjusted the review-tools to be inline with this PR and so if '/' is in the filename. As such, if the Icon contains '/', it must start with ${SNAP}/ (and end with .png and .svg and the path is the same as the normalized path, but that isn't relevant for what I'm describing), so the existing review-tools check for external symlinks would catch this.


// If the icon is prefixed with "snap.${SNAP_NAME}.", rewrite
// to the instance name.
snapIconPrefix := fmt.Sprintf("snap.%s.", s.SnapName())
if strings.HasPrefix(icon, snapIconPrefix) {
return fmt.Sprintf("Icon=snap.%s.%s", s.InstanceName(), icon[len(snapIconPrefix):]), nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

silly question, what happens if the app refers to the icons from code, or is that not expected? or would it in that case use them from the snap, and not the system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are (currently) no interfaces that grant a snap access to /var/lib/snapd/desktop/icons, so the naming should be irrelevant to the snap. As with the exported desktop files, this is for the benefit of the host system.

From within the snap sandbox, the app can access un-munged versions of its own icons in ${SNAP}/meta/gui/icons.

}

// If the icon has any other "snap." prefix, treat this as an error.
if strings.HasPrefix(icon, "snap.") {
return "", fmt.Errorf("invalid icon name: %q", icon)
jhenstridge marked this conversation as resolved.
Show resolved Hide resolved
}

// Allow other icons names through unchanged.
return line, nil
}

func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []byte {
var newContent bytes.Buffer
mountDir := []byte(s.MountDir())
Expand All @@ -165,6 +189,16 @@ func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []
bline = []byte(line)
}

// rewrite icon line if it references an icon theme icon
if bytes.HasPrefix(bline, []byte("Icon=")) {
line, err := rewriteIconLine(s, string(bline))
if err != nil {
logger.Debugf("ignoring icon in source desktop file %q: %s", filepath.Base(desktopFile), err)
zyga marked this conversation as resolved.
Show resolved Hide resolved
continue
}
bline = []byte(line)
}

// do variable substitution
bline = bytes.Replace(bline, []byte("${SNAP}"), mountDir, -1)

Expand Down
53 changes: 53 additions & 0 deletions wrappers/desktop_test.go
Expand Up @@ -482,6 +482,59 @@ func (s *sanitizeDesktopFileSuite) TestLangLang(c *C) {
}
}

func (s *sanitizeDesktopFileSuite) TestRewriteIconLine(c *C) {
snap, err := snap.InfoFromSnapYaml([]byte(`
name: snap
version: 1.0
`))
c.Assert(err, IsNil)

newl, err := wrappers.RewriteIconLine(snap, "Icon=${SNAP}/icon.png")
c.Check(err, IsNil)
c.Check(newl, Equals, "Icon=${SNAP}/icon.png")

newl, err = wrappers.RewriteIconLine(snap, "Icon=snap.snap.icon")
c.Check(err, IsNil)
c.Check(newl, Equals, "Icon=snap.snap.icon")
zyga marked this conversation as resolved.
Show resolved Hide resolved

newl, err = wrappers.RewriteIconLine(snap, "Icon=other-icon")
c.Check(err, IsNil)
c.Check(newl, Equals, "Icon=other-icon")

snap.InstanceKey = "bar"
newl, err = wrappers.RewriteIconLine(snap, "Icon=snap.snap.icon")
c.Check(err, IsNil)
c.Check(newl, Equals, "Icon=snap.snap_bar.icon")

_, err = wrappers.RewriteIconLine(snap, "Icon=snap.othersnap.icon")
c.Check(err, ErrorMatches, `invalid icon name: "snap.othersnap.icon"`)
}

func (s *sanitizeDesktopFileSuite) TestSanitizeParallelInstancesIconName(c *C) {
snap, err := snap.InfoFromSnapYaml([]byte(`
name: snap
version: 1.0
apps:
app:
command: cmd
`))
snap.InstanceKey = "bar"
c.Assert(err, IsNil)
desktopContent := []byte(`[Desktop Entry]
Name=foo
Icon=snap.snap.icon
Exec=snap.app
`)
df := filepath.Base(snap.Apps["app"].DesktopFile())
e := wrappers.SanitizeDesktopFile(snap, df, desktopContent)
c.Assert(string(e), Equals, fmt.Sprintf(`[Desktop Entry]
X-SnapInstanceName=snap_bar
Name=foo
Icon=snap.snap_bar.icon
Exec=env BAMF_DESKTOP_FILE_HINT=snap_bar_app.desktop %s/bin/snap_bar.app
`, dirs.SnapMountDir))
}

func (s *desktopSuite) TestAddRemoveDesktopFiles(c *C) {
var tests = []struct {
instance string
Expand Down
4 changes: 4 additions & 0 deletions wrappers/export_test.go
Expand Up @@ -33,10 +33,14 @@ var (
// desktop
SanitizeDesktopFile = sanitizeDesktopFile
RewriteExecLine = rewriteExecLine
RewriteIconLine = rewriteIconLine
IsValidDesktopFileLine = isValidDesktopFileLine

// timers
GenerateOnCalendarSchedules = generateOnCalendarSchedules

// icons
FindIconFiles = findIconFiles
)

func MockKillWait(wait time.Duration) (restore func()) {
Expand Down
125 changes: 125 additions & 0 deletions wrappers/icons.go
@@ -0,0 +1,125 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2019 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package wrappers

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"strings"

"github.com/snapcore/snapd/dirs"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/snap"
)

func findIconFiles(s *snap.Info, rootDir string) (icons []string, err error) {
jhenstridge marked this conversation as resolved.
Show resolved Hide resolved
if !osutil.IsDirectory(rootDir) {
return nil, nil
}
iconGlob := fmt.Sprintf("snap.%s.*", s.SnapName())
forbiddenDirGlob := "snap.*"
err = filepath.Walk(rootDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
rel, err := filepath.Rel(rootDir, path)
if err != nil {
return err
}
base := filepath.Base(path)
if info.IsDir() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be caught by the validation layer earlier on (at installation time)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed this locally and agreed to move this to validation layer in the next release. It's not a blocker for this PR.

// Ignore directories that could match an icon glob
if ok, err := filepath.Match(forbiddenDirGlob, base); ok || err != nil {
return filepath.SkipDir
}
} else {
if ok, err := filepath.Match(iconGlob, base); err != nil {
return err
} else if ok {
ext := filepath.Ext(base)
if ext == ".png" || ext == ".svg" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we put up a documentation page somewhere that explains that only those two extensions are supported.

CC @degville to inspect later on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also validate that only those two extensions are present in the icon directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was following the lead of the desktop file parsing code in ignoring things that don't pass validation.

As a new feature, I suppose we could be more strict in what we accept. I don't think there is anything currently stopping someone shipping a snap containing a meta/gui/icons directory, but it is unlikely anyone has done so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just document the properties of the meta/gui/icons directory that we enforce here https://snapcraft.io/docs/snap-format

icons = append(icons, rel)
}
}
}
return nil
})
return icons, err
}

func deriveIconContent(s *snap.Info, rootDir string, icons []string) (content map[string]map[string]*osutil.FileState, err error) {
jhenstridge marked this conversation as resolved.
Show resolved Hide resolved
content = make(map[string]map[string]*osutil.FileState)
snapPrefix := fmt.Sprintf("snap.%s.", s.SnapName())
instancePrefix := fmt.Sprintf("snap.%s.", s.InstanceName())

for _, iconFile := range icons {
dir := filepath.Dir(iconFile)
base := filepath.Base(iconFile)
dirContent := content[dir]
if dirContent == nil {
dirContent = make(map[string]*osutil.FileState)
content[dir] = dirContent
}
data, err := ioutil.ReadFile(filepath.Join(rootDir, iconFile))
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we check at least that icons haven't an unreasonable size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably a good idea. I wonder what we should consider unreasonable?

Looking at the icons in /usr/share/icons on my system, the largest ones seem to be Xcursor format files at about 4MB, which seem to be animated mouse cursors. For actual application icons, Handbrake seems to be an outlier with 3.5MB SVG icon (which seems to contain a large embedded base64 encoded PNG file).

Ignoring those outliers, there are a number of PNG and SVG icons between 100KB-140KB, so "reasonable" needs to include those sizes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we put a limit (8MB ?) it probably needs to be checked already in snap/validate.go so that is not a surprise only at installation.

Another option is to have Source path field in FileState, if it's set and the source is over some threshold we do a buffered file comparison and copy instead of the whole to memory approach.

Both these seems more follow up material than something to do here though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'd like to second this request. I think it can merge but with my virtual-seciruty-hat on I'd say this is a way to DOS snapd. Just put up a large enough .png file full of noise and snapd goes down.

CC @jdstrand for possible validation angle at the store.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As a separate idea, we can extend EnsureDirState to support streaming. Instead of handing out bytes we could hand out bytes or file references that it can then efficiently use to avoid holding the entire file in memory.

Choose a reason for hiding this comment

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

Yeah, I'd like to second this request. I think it can merge but with my virtual-seciruty-hat on I'd say this is a way to DOS snapd. Just put up a large enough .png file full of noise and snapd goes down.

CC @jdstrand for possible validation angle at the store.

I'm working on this in the review-tools now. Please don't block this PR on that.

if err != nil {
return nil, err
}
// rename icons to match snap instance name
if strings.HasPrefix(base, snapPrefix) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this if always be true? because of the Match in findIcons ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I guess I was thinking about one of the possibilities brought up in the forum thread to allow installing icons matching a well known name. There's nothing else in this branch handling that though, so I've converted this to an error when the prefix doesn't match.

base = instancePrefix + base[len(snapPrefix):]
}
dirContent[base] = &osutil.FileState{
Content: data,
Mode: 0644,
}
}
return content, nil
}

func AddSnapIcons(s *snap.Info) error {
if err := os.MkdirAll(dirs.SnapDesktopIconsDir, 0755); err != nil {
return err
}

rootDir := filepath.Join(s.MountDir(), "meta", "gui", "icons")
icons, err := findIconFiles(s, rootDir)
if err != nil {
return err
}

content, err := deriveIconContent(s, rootDir, icons)
if err != nil {
return err
}
iconGlob := fmt.Sprintf("snap.%s.*", s.InstanceName())
_, _, err = osutil.EnsureTreeState(dirs.SnapDesktopIconsDir, []string{iconGlob}, content)
return err
}

func RemoveSnapIcons(s *snap.Info) error {
if !osutil.IsDirectory(dirs.SnapDesktopIconsDir) {
return nil
}
iconGlob := fmt.Sprintf("snap.%s.*", s.InstanceName())
_, _, err := osutil.EnsureTreeState(dirs.SnapDesktopIconsDir, []string{iconGlob}, nil)
return err
}