From 5701b0823e342c6280e641268087bda25864a4f5 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Thu, 17 Jun 2021 16:21:50 -0500
Subject: [PATCH 01/10] interfaces/cups: add cups-socket attr, use to specify
 mount rules in backend

Make the cups interface slot have an optional attribute, cups-socket-dir, which
is used to specify the directory where the cups socket that the slotting snap
is going to share with client snaps.

This directory then is mounted into client snap's mount namespaces via the
mount backend at /var/cups/, such that client applications wishing to
print need to adjust the location of where they expect the cups socket to
be at all, but then will always end up sending print requests to the cups snap,
which will mediate requests to print based on whether or not the client snap
has a connected cups plug or not.

The primary advantage of this is that it means we don't need to make the cups
interface implictly slotted by the system snap, and it can always be provided
by the cups snap, and we can then make any snap with a cups interface plug
auto-connect to the slot from the cups snap unambiguously, providing all snap
clients the ability to print without any extra connections necessary,
regardless of their distro or what the client snap is.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 interfaces/builtin/cups.go      | 149 ++++++++++++++++++++++++++++++--
 interfaces/builtin/cups_test.go |  40 +++++++--
 2 files changed, 176 insertions(+), 13 deletions(-)

diff --git a/interfaces/builtin/cups.go b/interfaces/builtin/cups.go
index 210477f6b81..af18d4fbfec 100644
--- a/interfaces/builtin/cups.go
+++ b/interfaces/builtin/cups.go
@@ -19,6 +19,17 @@
 
 package builtin
 
+import (
+	"fmt"
+	"strings"
+
+	"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/snap"
+)
+
 // On systems where the slot is provided by an app snap, the cups interface is
 // the companion interface to the cups-control interface. The design of these
 // interfaces is based on the idea that the slot implementation (eg cupsd) is
@@ -48,15 +59,139 @@ const cupsConnectedPlugAppArmor = `
 
 #include <abstractions/cups-client>
 /{,var/}run/cups/printcap r,
+
+# allow talking to the snap cups socket
+/var/cups/cups.sock rw,
 `
 
+type cupsInterface struct {
+	commonInterface
+}
+
+func (iface *cupsInterface) AppArmorConnectedSlot(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	return nil
+}
+
+func validateCupsSocketDirAttr(a interfaces.Attrer, snapInfo *snap.Info) (string, error) {
+	// Allow an empty specification for the slot, in which case we don't perform
+	// any mounts, etc. This is mainly to prevent errors in systems which still
+	// have the old cups snap installed that haven't been updated to use the new
+	// snap with the new slot declaration
+	if _, ok := a.Lookup("cups-socket-dir"); !ok {
+		return "", nil
+	}
+
+	var cupsdSocketSourceDir string
+	if err := a.Attr("cups-socket-dir", &cupsdSocketSourceDir); err != nil {
+		return "", err
+	}
+
+	if !cleanSubPath(cupsdSocketSourceDir) {
+		return "", fmt.Errorf("cups-socket-dir is not clean: %q", cupsdSocketSourceDir)
+	}
+
+	// validate that the setting for cups-socket-dir is in $SNAP_DATA or
+	// $SNAP_COMMON, we don't allow any other directories for the slot socket
+	// dir
+	// TODO: should we also allow /run/$SNAP_INSTANCE_NAME/ too ?
+	if !strings.HasPrefix(cupsdSocketSourceDir, "$SNAP_COMMON") && !strings.HasPrefix(cupsdSocketSourceDir, "$SNAP_DATA") {
+		return "", fmt.Errorf("cups-socket-dir must be a directory of $SNAP_COMMON or $SNAP_DATA")
+	}
+	// otherwise it must have a prefix of either SNAP_COMMON or SNAP_DATA,
+	// validate that it has no other variables in it
+	err := snap.ValidatePathVariables(cupsdSocketSourceDir)
+	if err != nil {
+		return "", err
+	}
+
+	// The path starts with $ and ValidatePathVariables() ensures
+	// path contains only $SNAP, $SNAP_DATA, $SNAP_COMMON, and no
+	// other $VARs are present. It is ok to use
+	// ExpandSnapVariables() since it only expands $SNAP, $SNAP_DATA
+	// and $SNAP_COMMON
+	return snapInfo.ExpandSnapVariables(cupsdSocketSourceDir), nil
+}
+
+func (iface *cupsInterface) BeforePrepareSlot(slot *snap.SlotInfo) error {
+	// verify that the snap has a cups-socket-dir interface attribute, which is
+	// needed to identify where to find the cups socket is located in the snap
+	// providing the cups socket
+	_, err := validateCupsSocketDirAttr(slot, slot.Snap)
+	return err
+}
+
+func (iface *cupsInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	cupsdSocketSourceDir, err := validateCupsSocketDirAttr(slot, slot.Snap())
+	if err != nil {
+		return err
+	}
+
+	// add the base snippet
+	spec.AddSnippet(cupsConnectedPlugAppArmor)
+
+	if cupsdSocketSourceDir == "" {
+		// no other rules, this is the legacy slot without the additional
+		// attribute
+		return nil
+	}
+
+	// add rules to access the socket dir from the slot location directly
+	// this is necessary otherwise clients get denials like this:
+	// apparmor="DENIED" operation="connect"
+	// profile="snap.test-snapd-cups-consumer.bin"
+	// name="/var/snap/test-snapd-cups-provider/common/cups.sock"
+	// pid=3195747 comm="nc" requested_mask="wr" denied_mask="wr" fsuid=0 ouid=0
+	// this denial is the same that would happen for the content interface, so
+	// we employ the same workaround from the content interface here too
+	spec.AddSnippet(fmt.Sprintf(`
+# In addition to the bind mount, add any AppArmor rules so that
+# snaps may directly access the slot implementation's files. Due
+# to a limitation in the kernel's LSM hooks for AF_UNIX, these
+# are needed for using named sockets within the exported
+# directory.
+%s/** mrwklix,`, cupsdSocketSourceDir))
+
+	// setup the snap-update-ns rules for bind mounting for the plugging snap
+	emit := spec.AddUpdateNSf
+
+	emit("  # Mount cupsd socket from cups snap to client snap\n")
+	emit("  mount options=(bind rw) %s -> /var/cups/,\n", cupsdSocketSourceDir)
+	emit("  umount /var/cups/,\n")
+
+	apparmor.GenWritableProfile(emit, cupsdSocketSourceDir, 1)
+	apparmor.GenWritableProfile(emit, "/var/cups", 1)
+
+	return nil
+}
+
+func (iface *cupsInterface) MountConnectedPlug(spec *mount.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
+	cupsdSocketSourceDir, err := validateCupsSocketDirAttr(slot, slot.Snap())
+	if err != nil {
+		return err
+	}
+
+	if cupsdSocketSourceDir == "" {
+		// no other rules, this is the legacy slot without the additional
+		// attribute
+		return nil
+	}
+
+	// add a bind mount of the cups-socket-dir to /var/cups of the plugging snap
+	return spec.AddMountEntry(osutil.MountEntry{
+		Name:    cupsdSocketSourceDir,
+		Dir:     "/var/cups/",
+		Options: []string{"bind", "rw"},
+	})
+}
+
 func init() {
-	registerIface(&commonInterface{
-		name:                  "cups",
-		summary:               cupsSummary,
-		implicitOnCore:        false,
-		implicitOnClassic:     false,
-		baseDeclarationSlots:  cupsBaseDeclarationSlots,
-		connectedPlugAppArmor: cupsConnectedPlugAppArmor,
+	registerIface(&cupsInterface{
+		commonInterface: commonInterface{
+			name:                 "cups",
+			summary:              cupsSummary,
+			implicitOnCore:       false,
+			implicitOnClassic:    false,
+			baseDeclarationSlots: cupsBaseDeclarationSlots,
+		},
 	})
 }
diff --git a/interfaces/builtin/cups_test.go b/interfaces/builtin/cups_test.go
index 223c10a3607..3fc67d9514f 100644
--- a/interfaces/builtin/cups_test.go
+++ b/interfaces/builtin/cups_test.go
@@ -31,11 +31,16 @@ import (
 )
 
 type cupsSuite struct {
-	iface            interfaces.Interface
-	plugInfo         *snap.PlugInfo
-	plug             *interfaces.ConnectedPlug
+	iface interfaces.Interface
+
+	plugInfo *snap.PlugInfo
+	plug     *interfaces.ConnectedPlug
+
 	providerSlotInfo *snap.SlotInfo
 	providerSlot     *interfaces.ConnectedSlot
+
+	providerLegacySlotInfo *snap.SlotInfo
+	providerLegacySlot     *interfaces.ConnectedSlot
 }
 
 var _ = Suite(&cupsSuite{iface: builtin.MustInterface("cups")})
@@ -49,14 +54,26 @@ apps:
 
 const cupsProviderYaml = `name: provider
 version: 0
+slots:
+  cups-socket:
+    interface: cups
+    cups-socket-dir: $SNAP_COMMON/foo-subdir
 apps:
  app:
-  slots: [cups]
+  slots: [cups-socket]
+`
+
+const cupsProviderLegacyYaml = `name: provider
+version: 0
+slots:
+  # no attribute
+  cups: {}
 `
 
 func (s *cupsSuite) SetUpTest(c *C) {
 	s.plug, s.plugInfo = MockConnectedPlug(c, cupsConsumerYaml, nil, "cups")
-	s.providerSlot, s.providerSlotInfo = MockConnectedSlot(c, cupsProviderYaml, nil, "cups")
+	s.providerSlot, s.providerSlotInfo = MockConnectedSlot(c, cupsProviderYaml, nil, "cups-socket")
+	s.providerLegacySlot, s.providerLegacySlotInfo = MockConnectedSlot(c, cupsProviderLegacyYaml, nil, "cups")
 }
 
 func (s *cupsSuite) TestName(c *C) {
@@ -65,6 +82,7 @@ func (s *cupsSuite) TestName(c *C) {
 
 func (s *cupsSuite) TestSanitizeSlot(c *C) {
 	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerSlotInfo), IsNil)
+	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerLegacySlotInfo), IsNil)
 }
 
 func (s *cupsSuite) TestSanitizePlug(c *C) {
@@ -74,7 +92,6 @@ func (s *cupsSuite) TestSanitizePlug(c *C) {
 func (s *cupsSuite) TestAppArmorSpec(c *C) {
 	for _, onClassic := range []bool{true, false} {
 		restore := release.MockOnClassic(onClassic)
-		defer restore()
 
 		// consumer to provider on core for ConnectedPlug
 		spec := &apparmor.Specification{}
@@ -82,7 +99,18 @@ func (s *cupsSuite) TestAppArmorSpec(c *C) {
 		c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "#include <abstractions/cups-client>")
+		// the special mount rules are present
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "/var/snap/provider/common/foo-subdir/** mrwklix,")
 		restore()
+
+		// consumer to legacy provider
+		specLegacy := &apparmor.Specification{}
+		c.Assert(specLegacy.AddConnectedPlug(s.iface, s.plug, s.providerLegacySlot), IsNil)
+		c.Assert(specLegacy.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
+		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
+		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "#include <abstractions/cups-client>")
+		// no special mounting rules
+		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "/var/snap/provider/common/foo-subdir/** mrwklix,")
 	}
 }
 

From 50582034e31bd5effc18326ae7c186a411a80e5a Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Tue, 23 Nov 2021 22:22:21 -0600
Subject: [PATCH 02/10] cmd/snap-exec/main.go: set CUPS_SERVER when /var/cups
 directory exists

This is a temporary work-around to enable snaps to not need to set CUPS_SERVER
in their own environment and instead let them rely on snapd to set the
appropriate environment when they are using a cups plug that is connected to a
cups slot that exposes the cups socket from specifically the cups socket at
this directory.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 cmd/snap-exec/main.go      | 12 ++++++++++
 cmd/snap-exec/main_test.go | 49 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/cmd/snap-exec/main.go b/cmd/snap-exec/main.go
index e64ad8cac9e..adedfba13a2 100644
--- a/cmd/snap-exec/main.go
+++ b/cmd/snap-exec/main.go
@@ -194,6 +194,18 @@ func execApp(snapApp, revision, command string, args []string) error {
 		env.ExtendWithExpanded(eenv)
 	}
 
+	// this is a workaround for the lack of an environment backend in interfaces
+	// where we want certain interfaces when connected to add environment
+	// variables to plugging snap apps, but this is a lot simpler as a
+	// work-around
+	// we currently only handle the CUPS_SERVER environment variable, setting it
+	// to /var/cups/ if that directory exists - it should not exist anywhere
+	// except in a strictly confined snap where we setup the bind mount from the
+	// source cups slot snap to the plugging snap
+	if exists, _, _ := osutil.DirExists(dirs.GlobalRootDir + "/var/cups"); exists {
+		env["CUPS_SERVER"] = "/var/cups/cups.sock"
+	}
+
 	// strings.Split() is ok here because we validate all app fields and the
 	// whitelist is pretty strict (see snap/validate.go:appContentWhitelist)
 	// (see also overlord/snapstate/check_snap.go's normPath)
diff --git a/cmd/snap-exec/main_test.go b/cmd/snap-exec/main_test.go
index 770ef01f067..17bfca8c68a 100644
--- a/cmd/snap-exec/main_test.go
+++ b/cmd/snap-exec/main_test.go
@@ -181,6 +181,55 @@ func (s *snapExecSuite) TestSnapExecAppIntegration(c *C) {
 	// See also https://bugs.launchpad.net/snapd/+bug/1860369
 	c.Check(execEnv, Not(testutil.Contains), "TEST_PATH=/vanilla")
 	c.Check(execEnv, testutil.Contains, "TEST_PATH=/custom")
+
+	// ensure that CUPS_SERVER is absent since we didn't mock the /var/cups dir
+	c.Check(execEnv, Not(testutil.Contains), "CUPS_SERVER=/var/cups")
+}
+
+func (s *snapExecSuite) TestSnapExecAppIntegrationCupsServerWorkaround(c *C) {
+	dir := c.MkDir()
+	dirs.SetRootDir(dir)
+	snaptest.MockSnap(c, string(mockYaml), &snap.SideInfo{
+		Revision: snap.R("42"),
+	})
+
+	// mock the /var/cups dir so that we observe CUPS_SERVER set
+	err := os.MkdirAll(filepath.Join(dir, "/var/cups"), 0755)
+	c.Assert(err, IsNil)
+
+	execArgv0 := ""
+	execArgs := []string{}
+	execEnv := []string{}
+	restore := snapExec.MockSyscallExec(func(argv0 string, argv []string, env []string) error {
+		execArgv0 = argv0
+		execArgs = argv
+		execEnv = env
+		return nil
+	})
+	defer restore()
+
+	// FIXME: TEST_PATH was meant to be just PATH but this uncovers another
+	// bug in the test suite where mocking binaries misbehaves.
+	oldPath := os.Getenv("TEST_PATH")
+	os.Setenv("TEST_PATH", "/vanilla")
+	defer os.Setenv("TEST_PATH", oldPath)
+
+	// launch and verify its run the right way
+	err = snapExec.ExecApp("snapname.app", "42", "stop", []string{"arg1", "arg2"})
+	c.Assert(err, IsNil)
+	c.Check(execArgv0, Equals, fmt.Sprintf("%s/snapname/42/stop-app", dirs.SnapMountDir))
+	c.Check(execArgs, DeepEquals, []string{execArgv0, "arg1", "arg2"})
+	c.Check(execEnv, testutil.Contains, "BASE_PATH=/some/path")
+	c.Check(execEnv, testutil.Contains, "LD_LIBRARY_PATH=/some/path/lib")
+	c.Check(execEnv, testutil.Contains, fmt.Sprintf("MY_PATH=%s", os.Getenv("PATH")))
+	// TEST_PATH is properly handled and we only see one value, /custom, defined
+	// as an app-specific override.
+	// See also https://bugs.launchpad.net/snapd/+bug/1860369
+	c.Check(execEnv, Not(testutil.Contains), "TEST_PATH=/vanilla")
+	c.Check(execEnv, testutil.Contains, "TEST_PATH=/custom")
+
+	// ensure that CUPS_SERVER is now set since we did mock the /var/cups dir
+	c.Check(execEnv, testutil.Contains, "CUPS_SERVER=/var/cups/cups.sock")
 }
 
 func (s *snapExecSuite) TestSnapExecAppCommandChainIntegration(c *C) {

From a7ae6437482e84eec231030a104419e7437c8e0f Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Mon, 24 Jan 2022 10:59:43 -0600
Subject: [PATCH 03/10] interfaces/builtin/cups: validate the cups-socket-dir
 attr is not AARE

Thanks to Alberto for the suggestion.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 interfaces/builtin/cups.go      |  9 +++++++--
 interfaces/builtin/cups_test.go | 20 +++++++++++++++++++-
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/interfaces/builtin/cups.go b/interfaces/builtin/cups.go
index af18d4fbfec..78912b4ca4f 100644
--- a/interfaces/builtin/cups.go
+++ b/interfaces/builtin/cups.go
@@ -86,6 +86,11 @@ func validateCupsSocketDirAttr(a interfaces.Attrer, snapInfo *snap.Info) (string
 		return "", err
 	}
 
+	// make sure that the cups socket dir is not an AppArmor Regular expression
+	if err := apparmor.ValidateNoAppArmorRegexp(cupsdSocketSourceDir); err != nil {
+		return "", fmt.Errorf("cups-socket-dir is not usable: %v", err)
+	}
+
 	if !cleanSubPath(cupsdSocketSourceDir) {
 		return "", fmt.Errorf("cups-socket-dir is not clean: %q", cupsdSocketSourceDir)
 	}
@@ -149,13 +154,13 @@ func (iface *cupsInterface) AppArmorConnectedPlug(spec *apparmor.Specification,
 # to a limitation in the kernel's LSM hooks for AF_UNIX, these
 # are needed for using named sockets within the exported
 # directory.
-%s/** mrwklix,`, cupsdSocketSourceDir))
+"%s/**" mrwklix,`, cupsdSocketSourceDir))
 
 	// setup the snap-update-ns rules for bind mounting for the plugging snap
 	emit := spec.AddUpdateNSf
 
 	emit("  # Mount cupsd socket from cups snap to client snap\n")
-	emit("  mount options=(bind rw) %s -> /var/cups/,\n", cupsdSocketSourceDir)
+	emit("  mount options=(bind rw) \"%s\" -> /var/cups/,\n", cupsdSocketSourceDir)
 	emit("  umount /var/cups/,\n")
 
 	apparmor.GenWritableProfile(emit, cupsdSocketSourceDir, 1)
diff --git a/interfaces/builtin/cups_test.go b/interfaces/builtin/cups_test.go
index 3fc67d9514f..74e0be5c03c 100644
--- a/interfaces/builtin/cups_test.go
+++ b/interfaces/builtin/cups_test.go
@@ -41,6 +41,9 @@ type cupsSuite struct {
 
 	providerLegacySlotInfo *snap.SlotInfo
 	providerLegacySlot     *interfaces.ConnectedSlot
+
+	invalidSlotInfo *snap.SlotInfo
+	invalidSlot     *interfaces.ConnectedSlot
 }
 
 var _ = Suite(&cupsSuite{iface: builtin.MustInterface("cups")})
@@ -63,6 +66,18 @@ apps:
   slots: [cups-socket]
 `
 
+const invalidCupsProviderYaml = `name: provider
+version: 0
+slots:
+  cups-socket:
+    interface: cups
+    # regex not allowed
+    cups-socket-dir: $SNAP_COMMON/foo-subdir/*
+apps:
+ app:
+  slots: [cups-socket]
+`
+
 const cupsProviderLegacyYaml = `name: provider
 version: 0
 slots:
@@ -74,6 +89,7 @@ func (s *cupsSuite) SetUpTest(c *C) {
 	s.plug, s.plugInfo = MockConnectedPlug(c, cupsConsumerYaml, nil, "cups")
 	s.providerSlot, s.providerSlotInfo = MockConnectedSlot(c, cupsProviderYaml, nil, "cups-socket")
 	s.providerLegacySlot, s.providerLegacySlotInfo = MockConnectedSlot(c, cupsProviderLegacyYaml, nil, "cups")
+	s.invalidSlot, s.invalidSlotInfo = MockConnectedSlot(c, invalidCupsProviderYaml, nil, "cups-socket")
 }
 
 func (s *cupsSuite) TestName(c *C) {
@@ -83,6 +99,8 @@ func (s *cupsSuite) TestName(c *C) {
 func (s *cupsSuite) TestSanitizeSlot(c *C) {
 	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerSlotInfo), IsNil)
 	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerLegacySlotInfo), IsNil)
+	// the regex is not allowed however
+	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.invalidSlotInfo), ErrorMatches, "cups-socket-dir is not usable: .* contains a reserved apparmor char .*")
 }
 
 func (s *cupsSuite) TestSanitizePlug(c *C) {
@@ -100,7 +118,7 @@ func (s *cupsSuite) TestAppArmorSpec(c *C) {
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "#include <abstractions/cups-client>")
 		// the special mount rules are present
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "/var/snap/provider/common/foo-subdir/** mrwklix,")
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `"/var/snap/provider/common/foo-subdir/**" mrwklix,`)
 		restore()
 
 		// consumer to legacy provider

From 593021a47ca2ad06abd52c2295a4d3288f3194d9 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Mon, 24 Jan 2022 11:37:39 -0600
Subject: [PATCH 04/10] interfaces/builtin/cups.go: fix mount update-ns rule

This needs to have a trailing "/" so that AppArmor matches it. Without this,
AppArmor will confusingly issue a message about flags not matching, when in
reality it is just missing the "/" to identify that it is a directory being
mounted.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 interfaces/builtin/cups.go | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/interfaces/builtin/cups.go b/interfaces/builtin/cups.go
index 78912b4ca4f..85c743ab3f0 100644
--- a/interfaces/builtin/cups.go
+++ b/interfaces/builtin/cups.go
@@ -160,7 +160,10 @@ func (iface *cupsInterface) AppArmorConnectedPlug(spec *apparmor.Specification,
 	emit := spec.AddUpdateNSf
 
 	emit("  # Mount cupsd socket from cups snap to client snap\n")
-	emit("  mount options=(bind rw) \"%s\" -> /var/cups/,\n", cupsdSocketSourceDir)
+	// note the trailing "/" is needed - we ensured that cupsdSocketSourceDir is
+	// clean when we validated it, so it will not have a trailing "/" so we are
+	// safe to add this here
+	emit("  mount options=(rw bind) \"%s/\" -> /var/cups/,\n", cupsdSocketSourceDir)
 	emit("  umount /var/cups/,\n")
 
 	apparmor.GenWritableProfile(emit, cupsdSocketSourceDir, 1)

From 0d6a1c47e6c102ca39cfc792abbd2e6482b7e556 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Tue, 25 Jan 2022 15:49:37 -0600
Subject: [PATCH 05/10] tests/main/interfaces-cups: adjust for new interface,
 use snaps from the store

The new and improved version of these test snaps actually does real unix socket
communication to make sure that it works.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 tests/main/interfaces-cups/task.yaml          | 94 +++++++++++++------
 .../test-snapd-consumer/bin/sh                |  3 -
 .../test-snapd-consumer/meta/snap.yaml        | 12 ---
 .../test-snapd-provider/bin/sh                |  3 -
 .../test-snapd-provider/meta/snap.yaml        |  9 --
 5 files changed, 65 insertions(+), 56 deletions(-)
 delete mode 100755 tests/main/interfaces-cups/test-snapd-consumer/bin/sh
 delete mode 100644 tests/main/interfaces-cups/test-snapd-consumer/meta/snap.yaml
 delete mode 100755 tests/main/interfaces-cups/test-snapd-provider/bin/sh
 delete mode 100644 tests/main/interfaces-cups/test-snapd-provider/meta/snap.yaml

diff --git a/tests/main/interfaces-cups/task.yaml b/tests/main/interfaces-cups/task.yaml
index 63dbc11ec86..ef90e0ccd49 100644
--- a/tests/main/interfaces-cups/task.yaml
+++ b/tests/main/interfaces-cups/task.yaml
@@ -6,11 +6,27 @@ details: |
     This intentionally does not test the mediation properties of the cupsd
     server.
 
-systems: [ubuntu-*]
+# we run on all systems, since we want the cups snap to work for all systems
+# which use snapd, such that any snap desiring to print can declare the cups
+# interface and have it auto-connected to the cups snap on all systems for the
+# best user experience everywhere
+
+systems:
+    # TODO: change the bases such that we can have 32-bit support again
+    - -ubuntu-18.04-32
 
 prepare: |
-    "$TESTSTOOLS"/snaps-state install-local test-snapd-provider
-    "$TESTSTOOLS"/snaps-state install-local test-snapd-consumer
+    # install the test snaps
+
+    # the provider snap needs an assertion in order to have it's slot connected,
+    # but since this PR has not yet landed, we simply use the store as a way to
+    # download the built artifact - when this is fully supported in the 
+    # review-tools, etc. then this can install the provider from the store 
+    # directly instead
+    snap download --edge test-snapd-cups-provider --basename=test-snapd-cups-provider
+    snap install --dangerous test-snapd-cups-provider.snap
+
+    snap install --edge test-snapd-cups-consumer
 
     if [ -e /run/cups ]; then
         mv /run/cups /run/cups.orig
@@ -25,47 +41,67 @@ restore: |
 
 execute: |
     echo "The provider can create the socket and any other files"
-    test-snapd-provider.sh -c "echo slot > /run/cups/cups.sock"
-    test-snapd-provider.sh -c "echo slot > /run/cups/other"
+    test-snapd-cups-provider.sh -c "echo slot > /run/cups/cups.sock"
+    test-snapd-cups-provider.sh -c "echo slot > /run/cups/other"
 
     echo "Check the consumer's interface is not auto-connected"
-    not test-snapd-consumer.cups-control -c "head /run/cups/cups.sock"
-    not test-snapd-consumer.cups -c "head /run/cups/cups.sock"
+
+    if [ "$(snap debug confinement)" = "strict" ]; then
+        echo "When the consumer's interface is not connected we cannot read the sockets"
+        not test-snapd-cups-consumer.cups-control -c "head /run/cups/cups.sock"
+        not test-snapd-cups-consumer.cups -c "head /run/cups/cups.sock"
+    fi
 
     echo "When the cups-control interface is connected"
-    snap connect test-snapd-consumer:cups-control test-snapd-provider:cups-control
+    snap connect test-snapd-cups-consumer:cups-control test-snapd-cups-provider:cups-control
 
-    echo "Then the plug can't access arbitrary files"
-    not test-snapd-consumer.cups-control -c "head /run/cups/other"
+    if [ "$(snap debug confinement)" = "strict" ]; then
+        echo "Then the plug can't access arbitrary files"
+        not test-snapd-cups-consumer.cups-control -c "head /run/cups/other"
+    fi
 
     echo "The plug can write to the socket"
-    test-snapd-consumer.cups-control -c "echo cups-control-plug > /run/cups/cups.sock"
-    test-snapd-provider.sh -c "cat /run/cups/cups.sock" | MATCH cups-control-plug
+    test-snapd-cups-consumer.cups-control -c "echo cups-control-plug > /run/cups/cups.sock"
+    test-snapd-cups-provider.sh -c "cat /run/cups/cups.sock" | MATCH cups-control-plug
 
     echo "The plug can read from the socket"
-    test-snapd-provider.sh -c "echo slot > /run/cups/cups.sock"
-    test-snapd-consumer.cups-control -c "cat /run/cups/cups.sock" | MATCH slot
+    test-snapd-cups-provider.sh -c "echo slot > /run/cups/cups.sock"
+    test-snapd-cups-consumer.cups-control -c "cat /run/cups/cups.sock" | MATCH slot
 
     echo "When the cups-control interface is disconnected"
-    snap disconnect test-snapd-consumer:cups-control
+    snap disconnect test-snapd-cups-consumer:cups-control
 
-    echo "The plug cannot read from the socket"
-    not test-snapd-consumer.cups-control -c "head /run/cups/cups.sock"
+    if [ "$(snap debug confinement)" = "strict" ]; then
+        echo "The plug cannot read from the socket"
+        not test-snapd-cups-consumer.cups-control -c "head /run/cups/cups.sock"
+    fi
 
-    echo "When the the cups interface is connected again"
-    snap connect test-snapd-consumer:cups test-snapd-provider:cups
+    echo "When the the cups interface is connected"
+    snap connect test-snapd-cups-consumer:cups test-snapd-cups-provider:cups
 
-    echo "Then the plug can't access arbitrary files"
-    not test-snapd-consumer.cups -c "head /run/cups/other"
+    if [ "$(snap debug confinement)" = "strict" ]; then
+        echo "Then the plug can't access arbitrary files"
+        not test-snapd-cups-consumer.cups -c "head /run/cups/other"
 
-    echo "The plug can write to the socket"
-    test-snapd-consumer.cups -c "echo cups-plug > /run/cups/cups.sock"
-    test-snapd-provider.sh -c "cat /run/cups/cups.sock" | MATCH cups-plug
+        echo "The plug also can't access the host /run cups socket"
+        not test-snapd-cups-consumer.cups -c "head /run/cups/cups.sock"
+    fi
 
-    echo "The plug can read from the socket"
-    test-snapd-provider.sh -c "echo slot > /run/cups/cups.sock"
-    test-snapd-consumer.cups -c "cat /run/cups/cups.sock" | MATCH slot
+    echo "The plug can write to the special cups snap specific socket"
+    echo hello | sudo snap run --shell test-snapd-cups-consumer.cups -c 'nc -q 1 -U $CUPS_SERVER' > cups-server-response.txt
+
+    echo "And the cups server on the other side of the socket sends a response"
+    MATCH hello < cups-server-response.txt
+
+    echo "The plug has CUPS_SERVER environment variable set automatically"
+    test-snapd-cups-consumer.cups -c 'echo $CUPS_SERVER' | MATCH /var/cups/cups.sock
 
     echo "And the the cups interface can be disconnected again"
-    snap disconnect test-snapd-consumer:cups
-    not test-snapd-consumer.cups -c "head /run/cups/cups.sock"
+    snap disconnect test-snapd-cups-consumer:cups
+
+    if [ "$(snap debug confinement)" = "strict" ]; then
+        not test-snapd-cups-consumer.cups -c "head /var/cups/cups.sock"
+    fi
+
+    echo "And the environment variable is not set"
+    test-snapd-cups-consumer.bin -c 'echo $CUPS_SERVER' | NOMATCH /var/cups/cups.sock
diff --git a/tests/main/interfaces-cups/test-snapd-consumer/bin/sh b/tests/main/interfaces-cups/test-snapd-consumer/bin/sh
deleted file mode 100755
index 0f845e07c5a..00000000000
--- a/tests/main/interfaces-cups/test-snapd-consumer/bin/sh
+++ /dev/null
@@ -1,3 +0,0 @@
-#!/bin/sh
-PS1='$ '
-exec /bin/sh "$@"
diff --git a/tests/main/interfaces-cups/test-snapd-consumer/meta/snap.yaml b/tests/main/interfaces-cups/test-snapd-consumer/meta/snap.yaml
deleted file mode 100644
index b5b35a23a59..00000000000
--- a/tests/main/interfaces-cups/test-snapd-consumer/meta/snap.yaml
+++ /dev/null
@@ -1,12 +0,0 @@
-name: test-snapd-consumer
-version: 1
-architectures: [all]
-apps:
-  cups-control:
-    command: bin/sh
-    plugs:
-    - cups-control
-  cups:
-    command: bin/sh
-    plugs:
-    - cups
diff --git a/tests/main/interfaces-cups/test-snapd-provider/bin/sh b/tests/main/interfaces-cups/test-snapd-provider/bin/sh
deleted file mode 100755
index 0f845e07c5a..00000000000
--- a/tests/main/interfaces-cups/test-snapd-provider/bin/sh
+++ /dev/null
@@ -1,3 +0,0 @@
-#!/bin/sh
-PS1='$ '
-exec /bin/sh "$@"
diff --git a/tests/main/interfaces-cups/test-snapd-provider/meta/snap.yaml b/tests/main/interfaces-cups/test-snapd-provider/meta/snap.yaml
deleted file mode 100644
index 8b6502d740e..00000000000
--- a/tests/main/interfaces-cups/test-snapd-provider/meta/snap.yaml
+++ /dev/null
@@ -1,9 +0,0 @@
-name: test-snapd-provider
-version: 1
-architectures: [all]
-apps:
-  sh:
-    command: bin/sh
-    slots:
-    - cups-control
-    - cups

From e8f92f6783a62926aead1fc3acf2167c9167b038 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Tue, 25 Jan 2022 17:33:30 -0600
Subject: [PATCH 06/10] i/cups: prevent using the host cupsd sock by not using
 the AA cups abstractions

This is because by design we want snaps using the cups interface to _only_ have
access to the snapped version of cupsd's socket at /var/cups/cups.sock since we
know that is always safe for snap apps to talk to, as it will always implement
mediation.

We do however grant access to ~/.cups/lpoptions since this is where things like
default printer, etc. are configured and we do want client snaps to continue to
be able to read this.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 interfaces/builtin/cups.go      | 17 +++++++++++++++--
 interfaces/builtin/cups_test.go | 10 ++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/interfaces/builtin/cups.go b/interfaces/builtin/cups.go
index 85c743ab3f0..b58469cd8c1 100644
--- a/interfaces/builtin/cups.go
+++ b/interfaces/builtin/cups.go
@@ -57,10 +57,23 @@ const cupsBaseDeclarationSlots = `
 const cupsConnectedPlugAppArmor = `
 # Allow communicating with the cups server
 
-#include <abstractions/cups-client>
+# Do not allow reading the user or global client.conf for this snap, as this may
+# allow a user to point an application at an unconfined cupsd which could be 
+# used to load printer drivers etc. We only want client snaps with the cups 
+# interface plug connected to be able to talk to a version of cupsd which is
+# strictly confined and performs mediation. This means only allowing to talk to
+# /var/cups/cups.sock and not /run/cups/cups.sock since snapd has no way to know
+# if the latter cupsd is confined and performs mediation, but the upstream 
+# maintained cups snap providing a cups slot will always perform mediation.
+# As such, do not use the <abstractions/cups-client> include file here.
+
+# Allow reading the personal settings for cups like default printer, etc.
+owner @{HOME}/.cups/lpoptions r,
+
 /{,var/}run/cups/printcap r,
 
-# allow talking to the snap cups socket
+# Allow talking to the snap version of cupsd socket that we expose via bind 
+# mounts from a snap providing the cups slot to this snap.
 /var/cups/cups.sock rw,
 `
 
diff --git a/interfaces/builtin/cups_test.go b/interfaces/builtin/cups_test.go
index 74e0be5c03c..3d17e7c036f 100644
--- a/interfaces/builtin/cups_test.go
+++ b/interfaces/builtin/cups_test.go
@@ -116,7 +116,10 @@ func (s *cupsSuite) TestAppArmorSpec(c *C) {
 		c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.providerSlot), IsNil)
 		c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "#include <abstractions/cups-client>")
+		// no cups abstractions
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
+		// but has the lpoptions config file though
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
 		// the special mount rules are present
 		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `"/var/snap/provider/common/foo-subdir/**" mrwklix,`)
 		restore()
@@ -126,7 +129,10 @@ func (s *cupsSuite) TestAppArmorSpec(c *C) {
 		c.Assert(specLegacy.AddConnectedPlug(s.iface, s.plug, s.providerLegacySlot), IsNil)
 		c.Assert(specLegacy.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
 		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
-		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "#include <abstractions/cups-client>")
+		// no cups abstractions
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
+		// but has the lpoptions config file though
+		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
 		// no special mounting rules
 		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "/var/snap/provider/common/foo-subdir/** mrwklix,")
 	}

From 82f3d673af63f4235518470b20c2ec3f33e59264 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Tue, 25 Jan 2022 18:23:32 -0600
Subject: [PATCH 07/10] tests/main/interfaces-cups: fix shellcheck issues

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 tests/main/interfaces-cups/task.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/main/interfaces-cups/task.yaml b/tests/main/interfaces-cups/task.yaml
index ef90e0ccd49..83ed6d5eaa7 100644
--- a/tests/main/interfaces-cups/task.yaml
+++ b/tests/main/interfaces-cups/task.yaml
@@ -88,12 +88,14 @@ execute: |
     fi
 
     echo "The plug can write to the special cups snap specific socket"
-    echo hello | sudo snap run --shell test-snapd-cups-consumer.cups -c 'nc -q 1 -U $CUPS_SERVER' > cups-server-response.txt
+    #shellcheck disable=SC2016
+    echo hello | test-snapd-cups-consumer.cups -c 'nc -q 1 -U $CUPS_SERVER' > cups-server-response.txt
 
     echo "And the cups server on the other side of the socket sends a response"
     MATCH hello < cups-server-response.txt
 
     echo "The plug has CUPS_SERVER environment variable set automatically"
+    #shellcheck disable=SC2016
     test-snapd-cups-consumer.cups -c 'echo $CUPS_SERVER' | MATCH /var/cups/cups.sock
 
     echo "And the the cups interface can be disconnected again"
@@ -104,4 +106,5 @@ execute: |
     fi
 
     echo "And the environment variable is not set"
+    #shellcheck disable=SC2016
     test-snapd-cups-consumer.bin -c 'echo $CUPS_SERVER' | NOMATCH /var/cups/cups.sock

From b9514af26b19fe8d3669644311e159b26b464fbe Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Thu, 27 Jan 2022 11:56:08 -0600
Subject: [PATCH 08/10] tests/cmd/snap-exec: rm unnecessary duplicated checks

These are already checked in other unit tests so we don't need to check them
again here.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 cmd/snap-exec/main_test.go | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/cmd/snap-exec/main_test.go b/cmd/snap-exec/main_test.go
index 17bfca8c68a..40055cb977a 100644
--- a/cmd/snap-exec/main_test.go
+++ b/cmd/snap-exec/main_test.go
@@ -208,25 +208,11 @@ func (s *snapExecSuite) TestSnapExecAppIntegrationCupsServerWorkaround(c *C) {
 	})
 	defer restore()
 
-	// FIXME: TEST_PATH was meant to be just PATH but this uncovers another
-	// bug in the test suite where mocking binaries misbehaves.
-	oldPath := os.Getenv("TEST_PATH")
-	os.Setenv("TEST_PATH", "/vanilla")
-	defer os.Setenv("TEST_PATH", oldPath)
-
 	// launch and verify its run the right way
 	err = snapExec.ExecApp("snapname.app", "42", "stop", []string{"arg1", "arg2"})
 	c.Assert(err, IsNil)
 	c.Check(execArgv0, Equals, fmt.Sprintf("%s/snapname/42/stop-app", dirs.SnapMountDir))
 	c.Check(execArgs, DeepEquals, []string{execArgv0, "arg1", "arg2"})
-	c.Check(execEnv, testutil.Contains, "BASE_PATH=/some/path")
-	c.Check(execEnv, testutil.Contains, "LD_LIBRARY_PATH=/some/path/lib")
-	c.Check(execEnv, testutil.Contains, fmt.Sprintf("MY_PATH=%s", os.Getenv("PATH")))
-	// TEST_PATH is properly handled and we only see one value, /custom, defined
-	// as an app-specific override.
-	// See also https://bugs.launchpad.net/snapd/+bug/1860369
-	c.Check(execEnv, Not(testutil.Contains), "TEST_PATH=/vanilla")
-	c.Check(execEnv, testutil.Contains, "TEST_PATH=/custom")
 
 	// ensure that CUPS_SERVER is now set since we did mock the /var/cups dir
 	c.Check(execEnv, testutil.Contains, "CUPS_SERVER=/var/cups/cups.sock")

From 65a97c48983bfa3eba61849b62c693d94eb90024 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Thu, 27 Jan 2022 13:00:01 -0600
Subject: [PATCH 09/10] interfaces/cups: rename to cups-socket-directory and
 add more unit tests

Test the mount specification that is generated for connected plugs, add more
unhappy validation cases and also check the snap-update-ns snippets too.

Thanks to Samuele for noticing these and suggesting the new attribute name.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 interfaces/builtin/cups.go      |  24 ++--
 interfaces/builtin/cups_test.go | 203 ++++++++++++++++++++++++--------
 2 files changed, 167 insertions(+), 60 deletions(-)

diff --git a/interfaces/builtin/cups.go b/interfaces/builtin/cups.go
index b58469cd8c1..4bd3c5784f2 100644
--- a/interfaces/builtin/cups.go
+++ b/interfaces/builtin/cups.go
@@ -85,35 +85,35 @@ func (iface *cupsInterface) AppArmorConnectedSlot(spec *apparmor.Specification,
 	return nil
 }
 
-func validateCupsSocketDirAttr(a interfaces.Attrer, snapInfo *snap.Info) (string, error) {
+func validateCupsSocketDirSlotAttr(a interfaces.Attrer, snapInfo *snap.Info) (string, error) {
 	// Allow an empty specification for the slot, in which case we don't perform
 	// any mounts, etc. This is mainly to prevent errors in systems which still
 	// have the old cups snap installed that haven't been updated to use the new
 	// snap with the new slot declaration
-	if _, ok := a.Lookup("cups-socket-dir"); !ok {
+	if _, ok := a.Lookup("cups-socket-directory"); !ok {
 		return "", nil
 	}
 
 	var cupsdSocketSourceDir string
-	if err := a.Attr("cups-socket-dir", &cupsdSocketSourceDir); err != nil {
+	if err := a.Attr("cups-socket-directory", &cupsdSocketSourceDir); err != nil {
 		return "", err
 	}
 
 	// make sure that the cups socket dir is not an AppArmor Regular expression
 	if err := apparmor.ValidateNoAppArmorRegexp(cupsdSocketSourceDir); err != nil {
-		return "", fmt.Errorf("cups-socket-dir is not usable: %v", err)
+		return "", fmt.Errorf("cups-socket-directory is not usable: %v", err)
 	}
 
 	if !cleanSubPath(cupsdSocketSourceDir) {
-		return "", fmt.Errorf("cups-socket-dir is not clean: %q", cupsdSocketSourceDir)
+		return "", fmt.Errorf("cups-socket-directory is not clean: %q", cupsdSocketSourceDir)
 	}
 
-	// validate that the setting for cups-socket-dir is in $SNAP_DATA or
+	// validate that the setting for cups-socket-directory is in $SNAP_DATA or
 	// $SNAP_COMMON, we don't allow any other directories for the slot socket
 	// dir
 	// TODO: should we also allow /run/$SNAP_INSTANCE_NAME/ too ?
 	if !strings.HasPrefix(cupsdSocketSourceDir, "$SNAP_COMMON") && !strings.HasPrefix(cupsdSocketSourceDir, "$SNAP_DATA") {
-		return "", fmt.Errorf("cups-socket-dir must be a directory of $SNAP_COMMON or $SNAP_DATA")
+		return "", fmt.Errorf("cups-socket-directory must be a directory of $SNAP_COMMON or $SNAP_DATA")
 	}
 	// otherwise it must have a prefix of either SNAP_COMMON or SNAP_DATA,
 	// validate that it has no other variables in it
@@ -131,15 +131,15 @@ func validateCupsSocketDirAttr(a interfaces.Attrer, snapInfo *snap.Info) (string
 }
 
 func (iface *cupsInterface) BeforePrepareSlot(slot *snap.SlotInfo) error {
-	// verify that the snap has a cups-socket-dir interface attribute, which is
+	// verify that the snap has a cups-socket-directory interface attribute, which is
 	// needed to identify where to find the cups socket is located in the snap
 	// providing the cups socket
-	_, err := validateCupsSocketDirAttr(slot, slot.Snap)
+	_, err := validateCupsSocketDirSlotAttr(slot, slot.Snap)
 	return err
 }
 
 func (iface *cupsInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
-	cupsdSocketSourceDir, err := validateCupsSocketDirAttr(slot, slot.Snap())
+	cupsdSocketSourceDir, err := validateCupsSocketDirSlotAttr(slot, slot.Snap())
 	if err != nil {
 		return err
 	}
@@ -186,7 +186,7 @@ func (iface *cupsInterface) AppArmorConnectedPlug(spec *apparmor.Specification,
 }
 
 func (iface *cupsInterface) MountConnectedPlug(spec *mount.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
-	cupsdSocketSourceDir, err := validateCupsSocketDirAttr(slot, slot.Snap())
+	cupsdSocketSourceDir, err := validateCupsSocketDirSlotAttr(slot, slot.Snap())
 	if err != nil {
 		return err
 	}
@@ -197,7 +197,7 @@ func (iface *cupsInterface) MountConnectedPlug(spec *mount.Specification, plug *
 		return nil
 	}
 
-	// add a bind mount of the cups-socket-dir to /var/cups of the plugging snap
+	// add a bind mount of the cups-socket-directory to /var/cups of the plugging snap
 	return spec.AddMountEntry(osutil.MountEntry{
 		Name:    cupsdSocketSourceDir,
 		Dir:     "/var/cups/",
diff --git a/interfaces/builtin/cups_test.go b/interfaces/builtin/cups_test.go
index 3d17e7c036f..43ef814469a 100644
--- a/interfaces/builtin/cups_test.go
+++ b/interfaces/builtin/cups_test.go
@@ -20,12 +20,16 @@
 package builtin_test
 
 import (
+	"fmt"
+	"strings"
+
 	. "gopkg.in/check.v1"
 
 	"github.com/snapcore/snapd/interfaces"
 	"github.com/snapcore/snapd/interfaces/apparmor"
 	"github.com/snapcore/snapd/interfaces/builtin"
-	"github.com/snapcore/snapd/release"
+	"github.com/snapcore/snapd/interfaces/mount"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/testutil"
 )
@@ -41,9 +45,6 @@ type cupsSuite struct {
 
 	providerLegacySlotInfo *snap.SlotInfo
 	providerLegacySlot     *interfaces.ConnectedSlot
-
-	invalidSlotInfo *snap.SlotInfo
-	invalidSlot     *interfaces.ConnectedSlot
 }
 
 var _ = Suite(&cupsSuite{iface: builtin.MustInterface("cups")})
@@ -60,19 +61,7 @@ version: 0
 slots:
   cups-socket:
     interface: cups
-    cups-socket-dir: $SNAP_COMMON/foo-subdir
-apps:
- app:
-  slots: [cups-socket]
-`
-
-const invalidCupsProviderYaml = `name: provider
-version: 0
-slots:
-  cups-socket:
-    interface: cups
-    # regex not allowed
-    cups-socket-dir: $SNAP_COMMON/foo-subdir/*
+    cups-socket-directory: $SNAP_COMMON/foo-subdir
 apps:
  app:
   slots: [cups-socket]
@@ -89,7 +78,6 @@ func (s *cupsSuite) SetUpTest(c *C) {
 	s.plug, s.plugInfo = MockConnectedPlug(c, cupsConsumerYaml, nil, "cups")
 	s.providerSlot, s.providerSlotInfo = MockConnectedSlot(c, cupsProviderYaml, nil, "cups-socket")
 	s.providerLegacySlot, s.providerLegacySlotInfo = MockConnectedSlot(c, cupsProviderLegacyYaml, nil, "cups")
-	s.invalidSlot, s.invalidSlotInfo = MockConnectedSlot(c, invalidCupsProviderYaml, nil, "cups-socket")
 }
 
 func (s *cupsSuite) TestName(c *C) {
@@ -99,43 +87,162 @@ func (s *cupsSuite) TestName(c *C) {
 func (s *cupsSuite) TestSanitizeSlot(c *C) {
 	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerSlotInfo), IsNil)
 	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.providerLegacySlotInfo), IsNil)
-	// the regex is not allowed however
-	c.Assert(interfaces.BeforePrepareSlot(s.iface, s.invalidSlotInfo), ErrorMatches, "cups-socket-dir is not usable: .* contains a reserved apparmor char .*")
+}
+
+const invalidCupsProviderYamlFmt = `name: provider
+version: 0
+slots:
+  cups-socket:
+    interface: cups
+    # regex not allowed
+    cups-socket-directory: %s
+apps:
+ app:
+  slots: [cups-socket]
+`
+
+func (s *cupsSuite) TestSanitizeInvalidSlot(c *C) {
+	tt := []struct {
+		snippet string
+		err     string
+	}{
+		{
+			"$SNAP_COMMON/foo-subdir/*",
+			"cups-socket-directory is not usable: .* contains a reserved apparmor char .*",
+		},
+		{
+			"$SNAP_COMMON/foo-subdir/../../../",
+			`cups-socket-directory is not clean: \"\$SNAP_COMMON/foo-subdir/../../../\"`,
+		},
+		{
+			"$SNAP/foo",
+			`cups-socket-directory must be a directory of \$SNAP_COMMON or \$SNAP_DATA`,
+		},
+	}
+
+	for _, t := range tt {
+		yaml := fmt.Sprintf(invalidCupsProviderYamlFmt, t.snippet)
+
+		_, invalidSlotInfo := MockConnectedSlot(c, yaml, nil, "cups-socket")
+
+		err := interfaces.BeforePrepareSlot(s.iface, invalidSlotInfo)
+		c.Assert(err, ErrorMatches, t.err)
+	}
 }
 
 func (s *cupsSuite) TestSanitizePlug(c *C) {
 	c.Assert(interfaces.BeforePreparePlug(s.iface, s.plugInfo), IsNil)
 }
 
+const expSnapUpdateNsSnippet = `  # Mount cupsd socket from cups snap to client snap
+  mount options=(rw bind) "/var/snap/provider/common/foo-subdir/" -> /var/cups/,
+  umount /var/cups/,
+  # Writable directory /var/snap/provider/common/foo-subdir
+  /var/snap/provider/common/foo-subdir/ rw,
+  /var/snap/provider/common/ rw,
+  /var/snap/provider/ rw,
+  # Writable mimic /var
+  # .. permissions for traversing the prefix that is assumed to exist
+  # .. variant with mimic at /
+  # Allow reading the mimic directory, it must exist in the first place.
+  / r,
+  # Allow setting the read-only directory aside via a bind mount.
+  /tmp/.snap/ rw,
+  mount options=(rbind, rw) / -> /tmp/.snap/,
+  # Allow mounting tmpfs over the read-only directory.
+  mount fstype=tmpfs options=(rw) tmpfs -> /,
+  # Allow creating empty files and directories for bind mounting things
+  # to reconstruct the now-writable parent directory.
+  /tmp/.snap/*/ rw,
+  /*/ rw,
+  mount options=(rbind, rw) /tmp/.snap/*/ -> /*/,
+  /tmp/.snap/* rw,
+  /* rw,
+  mount options=(bind, rw) /tmp/.snap/* -> /*,
+  # Allow unmounting the auxiliary directory.
+  # TODO: use fstype=tmpfs here for more strictness (LP: #1613403)
+  mount options=(rprivate) -> /tmp/.snap/,
+  umount /tmp/.snap/,
+  # Allow unmounting the destination directory as well as anything
+  # inside.  This lets us perform the undo plan in case the writable
+  # mimic fails.
+  mount options=(rprivate) -> /,
+  mount options=(rprivate) -> /*,
+  mount options=(rprivate) -> /*/,
+  umount /,
+  umount /*,
+  umount /*/,
+  # .. variant with mimic at /var/
+  /var/ r,
+  /tmp/.snap/var/ rw,
+  mount options=(rbind, rw) /var/ -> /tmp/.snap/var/,
+  mount fstype=tmpfs options=(rw) tmpfs -> /var/,
+  /tmp/.snap/var/*/ rw,
+  /var/*/ rw,
+  mount options=(rbind, rw) /tmp/.snap/var/*/ -> /var/*/,
+  /tmp/.snap/var/* rw,
+  /var/* rw,
+  mount options=(bind, rw) /tmp/.snap/var/* -> /var/*,
+  mount options=(rprivate) -> /tmp/.snap/var/,
+  umount /tmp/.snap/var/,
+  mount options=(rprivate) -> /var/,
+  mount options=(rprivate) -> /var/*,
+  mount options=(rprivate) -> /var/*/,
+  umount /var/,
+  umount /var/*,
+  umount /var/*/,
+`
+
 func (s *cupsSuite) TestAppArmorSpec(c *C) {
-	for _, onClassic := range []bool{true, false} {
-		restore := release.MockOnClassic(onClassic)
-
-		// consumer to provider on core for ConnectedPlug
-		spec := &apparmor.Specification{}
-		c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.providerSlot), IsNil)
-		c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
-		// no cups abstractions
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
-		// but has the lpoptions config file though
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
-		// the special mount rules are present
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `"/var/snap/provider/common/foo-subdir/**" mrwklix,`)
-		restore()
-
-		// consumer to legacy provider
-		specLegacy := &apparmor.Specification{}
-		c.Assert(specLegacy.AddConnectedPlug(s.iface, s.plug, s.providerLegacySlot), IsNil)
-		c.Assert(specLegacy.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
-		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
-		// no cups abstractions
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
-		// but has the lpoptions config file though
-		c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
-		// no special mounting rules
-		c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "/var/snap/provider/common/foo-subdir/** mrwklix,")
-	}
+	// consumer to provider on core for ConnectedPlug
+	spec := &apparmor.Specification{}
+	c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.providerSlot), IsNil)
+	c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
+	// no cups abstractions
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
+	// but has the lpoptions config file though
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
+	// the special mount rules are present
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `"/var/snap/provider/common/foo-subdir/**" mrwklix,`)
+	// the writable mimic profile for snap-update-ns is generated as well
+	c.Assert(strings.Join(spec.UpdateNS(), ""), Equals, expSnapUpdateNsSnippet)
+
+	// consumer to legacy provider
+	specLegacy := &apparmor.Specification{}
+	c.Assert(specLegacy.AddConnectedPlug(s.iface, s.plug, s.providerLegacySlot), IsNil)
+	c.Assert(specLegacy.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
+	c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, "# Allow communicating with the cups server")
+	// no cups abstractions
+	c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "#include <abstractions/cups-client>")
+	// but has the lpoptions config file though
+	c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), testutil.Contains, `owner @{HOME}/.cups/lpoptions r,`)
+	// no special mounting rules
+	c.Assert(specLegacy.SnippetForTag("snap.consumer.app"), Not(testutil.Contains), "/var/snap/provider/common/foo-subdir/** mrwklix,")
+	// no writable mimic profile for snap-update-ns
+	c.Assert(specLegacy.UpdateNS(), HasLen, 0)
+}
+
+func (s *cupsSuite) TestMountSpec(c *C) {
+	// consumer to provider on core for ConnectedPlug
+	spec := &mount.Specification{}
+	c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.providerSlot), IsNil)
+	// mount entry for /var/cups/ for all namespaces
+	c.Assert(spec.MountEntries(), DeepEquals, []osutil.MountEntry{
+		{
+			Name:    "/var/snap/provider/common/foo-subdir",
+			Dir:     "/var/cups/",
+			Options: []string{"bind", "rw"},
+		},
+	})
+	// no user specific mounts
+	c.Assert(spec.UserMountEntries(), HasLen, 0)
+
+	// consumer to legacy provider has no mounts at all
+	specLegacy := &mount.Specification{}
+	c.Assert(specLegacy.AddConnectedPlug(s.iface, s.plug, s.providerLegacySlot), IsNil)
+	c.Assert(specLegacy.MountEntries(), HasLen, 0)
+	c.Assert(specLegacy.UserMountEntries(), HasLen, 0)
 }
 
 func (s *cupsSuite) TestStaticInfo(c *C) {

From 513c7d051306dc1c627db1833bd4f00822e71de5 Mon Sep 17 00:00:00 2001
From: Ian Johnson <ian.johnson@canonical.com>
Date: Thu, 27 Jan 2022 13:05:33 -0600
Subject: [PATCH 10/10] tests/main/interfaces-cups: add notes about test snap
 sources

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
---
 tests/main/interfaces-cups/task.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/main/interfaces-cups/task.yaml b/tests/main/interfaces-cups/task.yaml
index 83ed6d5eaa7..be5013eb595 100644
--- a/tests/main/interfaces-cups/task.yaml
+++ b/tests/main/interfaces-cups/task.yaml
@@ -18,15 +18,20 @@ systems:
 prepare: |
     # install the test snaps
 
+    # source for this snap:
+    # https://github.com/anonymouse64/test-snapd-cups-consumer
+    # needs to be built with snapcraft and hence cannot be snap pack'd here
+    snap install --edge test-snapd-cups-consumer
+
     # the provider snap needs an assertion in order to have it's slot connected,
     # but since this PR has not yet landed, we simply use the store as a way to
     # download the built artifact - when this is fully supported in the 
     # review-tools, etc. then this can install the provider from the store 
     # directly instead
+    # source for this snap: https://github.com/anonymouse64/test-snapd-cups-provider
     snap download --edge test-snapd-cups-provider --basename=test-snapd-cups-provider
     snap install --dangerous test-snapd-cups-provider.snap
 
-    snap install --edge test-snapd-cups-consumer
 
     if [ -e /run/cups ]; then
         mv /run/cups /run/cups.orig