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.

Sign up for GitHub

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

interfaces: mount host system fonts in desktop interface #3889

Merged
merged 5 commits into from
Sep 19, 2017
Merged

interfaces: mount host system fonts in desktop interface #3889

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba94f4e
interfaces: mount host system fonts in desktop interface
jhenstridge Sep 8, 2017
36f4877
interfaces: changes from jdstrand's review
jhenstridge Sep 14, 2017
ff3c33e
interfaces, dirs: respect GlobalRootDir
jhenstridge Sep 14, 2017
cb60ba4
cmd/snap-confine: allow mounting of fonts
jhenstridge Sep 14, 2017
1d76910
interfaces: fix formatting
jhenstridge Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 61 additions & 8 deletions interfaces/builtin/desktop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@

package builtin

import (
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/mount"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/release"
)

const desktopSummary = `allows access to basic graphical desktop resources`

const desktopBaseDeclarationSlots = `
Expand Down Expand Up @@ -117,13 +125,58 @@ dbus (send)
deny /{dev,run,var/run}/shm/lttng-ust-* rw,
`

var desktopFontconfigDirs = []string{
"/usr/share/fonts",
"/usr/local/share/fonts",
"/var/cache/fontconfig",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these need to be set in dirs, so they inherit the global root dir.

Also, how cross-distro are these locations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what happens if a different base snap doesn't have the mount point location available? Does it fail gracefully, or does it break the mounting sequence? @zyga?

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'll have a look at defining these through the dirs package.

The locations are fairly consistent. /usr/share/fonts is fontconfig's default font location. The /usr/local/share/fonts directory isn't part of upstream fontconfig, but is commonly configured by major distros:

https://sources.debian.net/src/fontconfig/2.12.3-0.2/debian/rules/#L20
https://src.fedoraproject.org/rpms/fontconfig/blob/master/f/fontconfig.spec#_69
https://build.opensuse.org/package/view_file/openSUSE:Factory/fontconfig/fontconfig.spec?expand=1
https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/fontconfig#n44

The fontconfig cache directory follows autoconf's $localstatedir directory, which is generally always configured as /var on Linux.

As for base snaps, I can see two solutions: (1) require base snaps to create these mount points in the same way they'd create /etc, /home, etc, or (2) rely on the upcoming layouts feature to ensure the mount points are available.

}

type desktopInterface struct{}

func (iface *desktopInterface) Name() string {
return "desktop"
}

func (iface *desktopInterface) StaticInfo() interfaces.StaticInfo {
return interfaces.StaticInfo{
Summary: desktopSummary,
ImplicitOnClassic: true,
BaseDeclarationSlots: desktopBaseDeclarationSlots,
}
}

func (iface *desktopInterface) SanitizeSlot(slot *interfaces.Slot) error {
return sanitizeSlotReservedForOS(iface, slot)
}

func (iface *desktopInterface) AutoConnect(*interfaces.Plug, *interfaces.Slot) bool {
return true
Copy link

Choose a reason for hiding this comment

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

Can you add a comment here like we do in other interfaces:

// allow what declarations allowed

Copy link

Choose a reason for hiding this comment

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

I wonder when we can get rid of AutoConnect altogether...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.

}

func (iface *desktopInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
spec.AddSnippet(desktopConnectedPlugAppArmor)
return nil
}

func (iface *desktopInterface) MountConnectedPlug(spec *mount.Specification, plug *interfaces.Plug, plugAttrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
if !release.OnClassic {
// There is nothing to expose on an all-snaps system
return nil
}

for _, dir := range desktopFontconfigDirs {
if !osutil.IsDirectory(dir) {
continue
}
spec.AddMountEntry(mount.Entry{
Name: "/var/lib/snapd/hostfs" + dir,
Dir: dir,
Options: []string{"bind", "ro"},
})
}
return nil
}

func init() {
registerIface(&commonInterface{
name: "desktop",
summary: desktopSummary,
implicitOnClassic: true,
baseDeclarationSlots: desktopBaseDeclarationSlots,
connectedPlugAppArmor: desktopConnectedPlugAppArmor,
reservedForOS: true,
})
registerIface(&desktopInterface{})
}
37 changes: 37 additions & 0 deletions interfaces/builtin/desktop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ import (
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/apparmor"
"github.com/snapcore/snapd/interfaces/builtin"
"github.com/snapcore/snapd/interfaces/mount"
"github.com/snapcore/snapd/osutil"
"github.com/snapcore/snapd/release"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/testutil"
)
Expand Down Expand Up @@ -91,6 +94,40 @@ func (s *DesktopInterfaceSuite) TestAppArmorSpec(c *C) {
c.Assert(spec.SecurityTags(), HasLen, 0)
}

func (s *DesktopInterfaceSuite) TestMountSpec(c *C) {
restore := release.MockOnClassic(false)
defer restore()

// On all-snaps systems, no mount entries are added
spec := &mount.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.coreSlot, nil), IsNil)
c.Check(spec.MountEntries(), HasLen, 0)

// On classic systems, a number of font related directories
// are bind mounted from the host system if they exist.
release.OnClassic = true
Copy link

Choose a reason for hiding this comment

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

Typically we'd do this in the testsuite instead:

// on a classic system with desktop slot coming from the core snap,
// a number of font related directories are bind mounted from the
// host system if they exist.
restore = release.MockOnClassic(true)
defer restore()

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'm already using MockOnClassic earlier to save the state. I'll switch this case to use MockOnClassic too, just in case someone splits the test later and leaks state.

spec = &mount.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, nil, s.coreSlot, nil), IsNil)
expectedMountPoints := []string{
"/usr/share/fonts",
"/usr/local/share/fonts",
"/var/cache/fontconfig",
}
expected := 0
for _, dir := range expectedMountPoints {
if osutil.IsDirectory(dir) {
expected += 1
}
}
entries := spec.MountEntries()
c.Check(entries, HasLen, expected)
for _, entry := range entries {
c.Check(expectedMountPoints, testutil.Contains, entry.Dir)
c.Check(entry.Name, Equals, "/var/lib/snapd/hostfs"+entry.Dir)
c.Check(entry.Options, DeepEquals, []string{"bind", "ro"})
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably need corresponding snap-confine apparmor profile updates. While I recall adding one for /usr/share/fonts well mid last year I'd like to check if all of those are present.

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 didn't notice any problems in my manual testing. I wonder if snap-update-ns got in first so I never hit that code path in snap-confine?

}
}

func (s *DesktopInterfaceSuite) TestStaticInfo(c *C) {
si := interfaces.StaticInfoOf(s.iface)
c.Assert(si.ImplicitOnCore, Equals, false)
Expand Down