interfaces/builtin/mount_control: add support for nfs mounts (#14694)

* interfaces/builtin/mount_control: add support for nfs mounts Extend the mount-control interface with support for nfs mounts. The nfs entries cannot specify any other filesystem types, nor contain the 'what' attribute. Signed-off-by: Maciej Borzecki <[email protected]> * tests/lib/snaps/store/test-snapd-mount-control-nfs: add test snap Add snap for testing mount-control with NFS Signed-off-by: Maciej Borzecki <[email protected]> * tests/main/interfaces-mount-control-nfs: spread test Signed-off-by: Maciej Borzecki <[email protected]> * overlord/hookstate: support for mount-control with nfs Add support for performing NFS mounts through `snapctl mount` command. Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin/mount-control: improve validation and doc comments for nfs and tmpfs Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin: complain about the first occurrence of an exclusive FS type during validation Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin/mount_control: drop nfs4 The explicit 'nfs4' filesystem is deprecated and mounts should use the 'nfs' type which defaults to NFSv4 and supports all required options for handling NFSv3 and NFSv4. Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin/mount_control: improve coverage of validation code Signed-off-by: Maciej Borzecki <[email protected]> * tests/lib/snaps/store/test-snapd-mount-control-nfs: tweak snapcraft Signed-off-by: Maciej Borzecki <[email protected]> * tests/main/interfaces-mount-control-nfs: improve cleanup Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin: comment tweak Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin/mount_control: naming tweak Signed-off-by: Maciej Borzecki <[email protected]> * interfaces/builtin/mount_control: consider nfs4 to be deprecated Signed-off-by: Maciej Borzecki <[email protected]> * tests/main/interfaces-mount-control-nfs: enable Ubuntu 22.04, better retry Signed-off-by: Maciej Borzecki <[email protected]> --------- Signed-off-by: Maciej Borzecki <[email protected]>
canonical · Nov 18, 2024 · 0543dff · 0543dff
1 parent ede0457
commit 0543dff
Show file tree

Hide file tree

Showing 9 changed files with 307 additions and 23 deletions.
diff --git a/interfaces/builtin/mount_control.go b/interfaces/builtin/mount_control.go
@@ -125,7 +125,6 @@ var allowedFilesystemSpecificMountOptions = map[string][]string{
 	"jfs":        {"iocharset=", "resize=", "nointegrity", "integrity", "errors=", "noquota", "quota", "usrquota", "grpquota"},
 	"msdos":      {"blocksize=", "uid=", "gid=", "umask=", "dmask=", "fmask=", "allow_utime=", "check=", "codepage=", "conv=", "cvf_format=", "cvf_option", "debug", "discard", "dos1xfloppy", "errors=", "fat=", "iocharset=", "nfs=", "tz=", "time_offset=", "quiet", "rodir", "showexec", "sys_immutable", "flush", "usefree", "dots", "nodots", "dotsOK="},
 	"nfs":        {"nfsvers=", "vers=", "soft", "hard", "softreval", "nosoftreval", "intr", "nointr", "timeo=", "retrans=", "rsize=", "wsize=", "ac", "noac", "acregmin=", "acregmax=", "acdirmin=", "acdirmax=", "actimeo=", "bg", "fg", "nconnect=", "max_connect=", "rdirplus", "nordirplus", "retry=", "sec=", "sharecache", "nosharecache", "revsport", "norevsport", "lookupcache=", "fsc", "nofsc", "sloppy", "proto=", "udp", "tcp", "rdma", "port=", "mountport=", "mountproto=", "mounthost=", "mountvers=", "namlen=", "lock", "nolock", "cto", "nocto", "acl", "noacl", "local_lock=", "minorversion=", "clientaddr=", "migration", "nomigration"},
-	"nfs4":       {"nfsvers=", "vers=", "soft", "hard", "softreval", "nosoftreval", "intr", "nointr", "timeo=", "retrans=", "rsize=", "wsize=", "ac", "noac", "acregmin=", "acregmax=", "acdirmin=", "acdirmax=", "actimeo=", "bg", "fg", "nconnect=", "max_connect=", "rdirplus", "nordirplus", "retry=", "sec=", "sharecache", "nosharecache", "revsport", "norevsport", "lookupcache=", "fsc", "nofsc", "sloppy", "proto=", "minorversion=", "port=", "cto", "nocto", "clientaddr=", "migration", "nomigration"},
 	"ntfs":       {"iocharset=", "nls=", "utf8", "uni_xlate=", "posix=", "uid=", "gid=", "umask="},
 	"ntfs-3g":    {"acl", "allow_other", "big_writes", "compression", "debug", "delay_mtime", "delay_mtime=", "dmask=", "efs_raw", "fmask=", "force", "hide_dot_files", "hide_hid_files", "inherit", "locale=", "max_read=", "no_def_opts", "no_detach", "nocompression", "norecover", "permissions", "posix_nlink", "recover", "remove_hiberfile", "show_sys_files", "silent", "special_files=", "streams_interface=", "uid=", "gid=", "umask=", "usermapping=", "user_xattr", "windows_names"},
 	"lowntfs-3g": {"acl", "allow_other", "big_writes", "compression", "debug", "delay_mtime", "delay_mtime=", "dmask=", "efs_raw", "fmask=", "force", "hide_dot_files", "hide_hid_files", "ignore_case", "inherit", "locale=", "max_read=", "no_def_opts", "no_detach", "nocompression", "norecover", "permissions", "posix_nlink", "recover", "remove_hiberfile", "show_sys_files", "silent", "special_files=", "streams_interface=", "uid=", "gid=", "umask=", "usermapping=", "user_xattr", "windows_names"},
@@ -212,6 +211,13 @@ var disallowedFSTypes = []string{
 	"tracefs",
 }
 
+// THe filesystems which are considered deprecated and for which a better
+// alternative exists.
+var deprecatedFSTypes = []string{
+	// use "nfs"
+	"nfs4",
+}
+
 // mountControlInterface allows creating transient and persistent mounts
 type mountControlInterface struct {
 	commonInterface
@@ -244,6 +250,11 @@ type mountControlInterface struct {
 // nearly any path, and due to the super-privileged nature of this interface it
 // is expected that sensible values of what are enforced by the store manual
 // review queue and security teams.
+//
+// Certain filesystem types impose additional restrictions on the allowed values
+// for "what" attribute:
+// - "tmpfs" - "what" must be set to "none"
+// - "nfs" - "what" must be unset
 var (
 	whatRegexp  = regexp.MustCompile(`^(none|/[^"@]*)$`)
 	whereRegexp = regexp.MustCompile(`^(\$SNAP_COMMON|\$SNAP_DATA)?/[^\$"@]+$`)
@@ -253,8 +264,16 @@ var (
 // malicious string like
 //
 //	auto) options=() /malicious/content /var/lib/snapd/hostfs/...,\n mount fstype=(
+//
+// The "type" attribute is an optional list of expected filesystem types. It is
+// most useful in situations when it is known upfront that only a handful of
+// types are accepted for a given mount.
 var typeRegexp = regexp.MustCompile(`^[a-z0-9]+$`)
 
+// Because of additional rules imposed on mount attributes, some filesystems can
+// only be specified as a single "type" entry.
+var exclusiveFsTypes = []string{"tmpfs", "nfs"}
+
 type MountInfo struct {
 	what       string
 	where      string
@@ -298,8 +317,18 @@ func enumerateMounts(plug interfaces.Attrer, fn func(mountInfo *MountInfo) error
 	}
 
 	for _, mount := range mounts {
+		types, err := parseStringList(mount, "type")
+		if err != nil {
+			return err
+		}
+
+		disallowSource := false
+		if strutil.ListContains(types, "nfs") || strutil.ListContains(types, "nfs4") {
+			disallowSource = true
+		}
+
 		what, ok := mount["what"].(string)
-		if !ok {
+		if !ok && !disallowSource {
 			return fmt.Errorf(`mount-control "what" must be a string`)
 		}
 
@@ -316,11 +345,6 @@ func enumerateMounts(plug interfaces.Attrer, fn func(mountInfo *MountInfo) error
 			}
 		}
 
-		types, err := parseStringList(mount, "type")
-		if err != nil {
-			return err
-		}
-
 		options, err := parseStringList(mount, "options")
 		if err != nil {
 			return err
@@ -360,6 +384,14 @@ func validateWhatAttr(mountInfo *MountInfo) error {
 		return validateNoAppArmorRegexpWithError(`cannot use mount-control "what" attribute`, what)
 	}
 
+	if mountInfo.isType("nfs") {
+		if what != "" {
+			return fmt.Errorf(`mount-control "what" attribute must not be specified for nfs mounts`)
+		}
+		// that's it for nfs
+		return nil
+	}
+
 	if !whatRegexp.MatchString(what) {
 		return fmt.Errorf(`mount-control "what" attribute is invalid: must start with / and not contain special characters`)
 	}
@@ -404,22 +436,33 @@ func validateWhereAttr(where string) error {
 }
 
 func validateMountTypes(types []string) error {
-	includesTmpfs := false
+	exclusiveFsType := ""
+
+	// multiple types specified in "type" are useful when the accepted
+	// filesystem type is known upfront or the mount uses one of the special
+	// types, such as "nfs" or "tmpfs"
 	for _, t := range types {
 		if !typeRegexp.MatchString(t) {
 			return fmt.Errorf(`mount-control filesystem type invalid: %q`, t)
 		}
+
 		if strutil.ListContains(disallowedFSTypes, t) {
 			return fmt.Errorf(`mount-control forbidden filesystem type: %q`, t)
 		}
-		if t == "tmpfs" {
-			includesTmpfs = true
+
+		if strutil.ListContains(deprecatedFSTypes, t) {
+			return fmt.Errorf(`mount-control deprecated filesystem type: %q`, t)
+		}
+
+		if exclusiveFsType == "" && strutil.ListContains(exclusiveFsTypes, t) {
+			exclusiveFsType = t
 		}
 	}
 
-	if includesTmpfs && len(types) > 1 {
-		return errors.New(`mount-control filesystem type "tmpfs" cannot be listed with other types`)
+	if exclusiveFsType != "" && len(types) > 1 {
+		return fmt.Errorf(`mount-control filesystem type %q cannot be listed with other types`, exclusiveFsType)
 	}
+
 	return nil
 }
 
@@ -480,15 +523,15 @@ func isAllowedFilesystemSpecificMountOption(types []string, optionName string) b
 }
 
 func validateMountInfo(mountInfo *MountInfo) error {
-	if err := validateWhatAttr(mountInfo); err != nil {
+	if err := validateMountTypes(mountInfo.types); err != nil {
 		return err
 	}
 
-	if err := validateWhereAttr(mountInfo.where); err != nil {
+	if err := validateWhatAttr(mountInfo); err != nil {
 		return err
 	}
 
-	if err := validateMountTypes(mountInfo.types); err != nil {
+	if err := validateWhereAttr(mountInfo.where); err != nil {
 		return err
 	}
 
@@ -583,6 +626,15 @@ func (iface *mountControlInterface) AppArmorConnectedPlug(spec *apparmor.Specifi
 			target = expanded + target[variableEnd:]
 		}
 
+		if mountInfo.isType("nfs") {
+			// override NFS share source, also see 'nfs-mount' interface
+			source = "*:**"
+
+			// emit additional rule required by NFS
+			emit("  # Allow lookup of RPC program numbers (due to mount-control)\n")
+			emit("  /etc/rpc r,\n")
+		}
+
 		var typeRule string
 		if optionIncompatibleWithFsType(mountInfo.options) != "" {
 			// In this rule the FS type will not match unless it's empty

diff --git a/interfaces/builtin/mount_control_test.go b/interfaces/builtin/mount_control_test.go
@@ -78,6 +78,9 @@ plugs:
     where: $SNAP_COMMON/mnt/**
     type: [aufs]
     options: [br:/mnt/a, add:0:/mnt/b, dirwh=1, rw]
+  - type: [nfs]
+    where: /media/foo/**
+    options: [rw]
 apps:
  app:
   plugs: [mntctl]
@@ -307,6 +310,30 @@ func (s *MountControlInterfaceSuite) TestSanitizePlugUnhappy(c *C) {
 			"mount:\n  - what: diag\n    where: /dev/ffs-diag\n    type: [functionfs]\n    options: [rw,uid=*]",
 			`cannot use mount-control "option" attribute: "uid=\*" contains a reserved apparmor char from.*`,
 		},
+		{
+			"mount:\n  - what: diag\n    where: /media/foo\n    type: [nfs]\n    options: [rw]",
+			`mount-control "what" attribute must not be specified for nfs mounts.*`,
+		},
+		{
+			"mount:\n  - where: /media/foo\n    type: [nfs, ext4]\n    options: [rw]",
+			`mount-control filesystem type "nfs" cannot be listed with other types`,
+		},
+		{
+			"mount:\n  - where: /media/foo\n    type: [tmpfs, nfs, ext4]\n    options: [rw]",
+			`mount-control filesystem type "tmpfs" cannot be listed with other types`,
+		},
+		{
+			"mount:\n  - what: 123\n    where: /media/foo\n    type: [ext4]\n    options: [rw]",
+			`mount-control "what" must be a string`,
+		},
+		// the deprecated nfs4 isn't explicitly forbidden, but it is not
+		// possible construct a valid and useful specification using this
+		// type
+		{
+			// deprecated nfs4
+			"mount:\n  - where: /media/foo\n    type: [nfs4]\n    options: [rw]",
+			`mount-control deprecated filesystem type: "nfs4"`,
+		},
 	}
 
 	for _, testData := range data {
@@ -376,6 +403,14 @@ func (s *MountControlInterfaceSuite) TestAppArmorSpec(c *C) {
 	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, expectedMountLine7)
 	expectedUmountLine7 := `umount "/var/snap/consumer/common/mnt/**{,/}",`
 	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, expectedUmountLine7)
+
+	expectedMountLine8 := `mount fstype=(nfs) options=(rw) ` +
+		`"*:**" -> "/media/foo/**{,/}",`
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, expectedMountLine8)
+	expectedUmountLine8 := `umount "/media/foo/**{,/}",`
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, expectedUmountLine8)
+	expectedExtraLine8 := ` /etc/rpc r,`
+	c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, expectedExtraLine8)
 }
 
 func (s *MountControlInterfaceSuite) TestStaticInfo(c *C) {

diff --git a/overlord/hookstate/ctlcmd/mount.go b/overlord/hookstate/ctlcmd/mount.go
@@ -72,17 +72,29 @@ func matchMountPathAttribute(path string, attribute interface{}, snapInfo *snap.
 	return err == nil && pp.Matches(path)
 }
 
-// matchConnection checks whether the given mount connection attributes give
-// the snap permission to execute the mount command
-func (m *mountCommand) matchConnection(attributes map[string]interface{}) bool {
-	if !matchMountPathAttribute(m.Positional.What, attributes["what"], m.snapInfo) {
-		return false
-	}
+func matchMountSourceAttribute(path string, attribute interface{}, fsType string, snapInfo *snap.Info) bool {
+	if fsType == "nfs" {
+		// NFS mount source AppArmor profiles expects a match for "*:**", so
+		// make sure that the attribute is unset, and the path matches the
+		// format
+		if _, ok := attribute.(string); ok {
+			return false
+		}
 
-	if !matchMountPathAttribute(m.Positional.Where, attributes["where"], m.snapInfo) {
-		return false
+		host, share, found := strings.Cut(path, ":")
+		if !found || host == "" || strings.Contains(host, "/") || share == "" {
+			return false
+		}
+
+		return true
 	}
 
+	return matchMountPathAttribute(path, attribute, snapInfo)
+}
+
+// matchConnection checks whether the given mount connection attributes give
+// the snap permission to execute the mount command
+func (m *mountCommand) matchConnection(attributes map[string]interface{}) bool {
 	if m.Type != "" {
 		if types, ok := attributes["type"].([]interface{}); ok {
 			found := false
@@ -106,6 +118,14 @@ func (m *mountCommand) matchConnection(attributes map[string]interface{}) bool {
 		}
 	}
 
+	if !matchMountSourceAttribute(m.Positional.What, attributes["what"], m.Type, m.snapInfo) {
+		return false
+	}
+
+	if !matchMountPathAttribute(m.Positional.Where, attributes["where"], m.snapInfo) {
+		return false
+	}
+
 	if optionsIfaces, ok := attributes["options"].([]interface{}); ok {
 		var allowedOptions []string
 		for _, iface := range optionsIfaces {

diff --git a/overlord/hookstate/ctlcmd/mount_test.go b/overlord/hookstate/ctlcmd/mount_test.go
@@ -143,6 +143,11 @@ func (s *mountSuite) SetUpTest(c *C) {
 					"options":    []string{"ro"},
 					"persistent": false,
 				},
+				map[string]interface{}{
+					"where":   "/nfs-dest",
+					"options": []string{"rw"},
+					"type":    []string{"nfs"},
+				},
 			},
 		},
 	}
@@ -270,6 +275,15 @@ func (s *mountSuite) TestMissingProperPlug(c *C) {
 	_, _, err = ctlcmd.Run(s.mockContext, []string{"mount", "--persistent", "-o", "bind,rw", "/src", "/dest"}, 0)
 	c.Check(err, ErrorMatches, `.*no matching mount-control connection found`)
 	c.Check(s.sysd.EnsureMountUnitFileWithOptionsCalls, HasLen, 0)
+
+	// bad NFS source format
+	_, _, err = ctlcmd.Run(s.mockContext, []string{"mount", "-o", "rw", "-t", "nfs", "/src", "/dest"}, 0)
+	c.Check(err, ErrorMatches, `.*no matching mount-control connection found`)
+	_, _, err = ctlcmd.Run(s.mockContext, []string{"mount", "-o", "rw", "-t", "nfs", "/host:/src", "/dest"}, 0)
+	c.Check(err, ErrorMatches, `.*no matching mount-control connection found`)
+	_, _, err = ctlcmd.Run(s.mockContext, []string{"mount", "-o", "rw", "-t", "nfs", ":/share", "/dest"}, 0)
+	c.Check(err, ErrorMatches, `.*no matching mount-control connection found`)
+	c.Check(s.sysd.EnsureMountUnitFileWithOptionsCalls, HasLen, 0)
 }
 
 func (s *mountSuite) TestUnitCreationFailure(c *C) {
@@ -353,6 +367,27 @@ func (s *mountSuite) TestHappyWithCommasInPath(c *C) {
 	})
 }
 
+func (s *mountSuite) TestHappyNFS(c *C) {
+	s.injectSnapWithProperPlug(c)
+
+	s.sysd.EnsureMountUnitFileWithOptionsResult = ResultForEnsureMountUnitFileWithOptions{"/path/unit.mount", nil}
+
+	// Now try with commas in the paths
+	_, _, err := ctlcmd.Run(s.mockContext, []string{"mount", "-o", "rw", "-t", "nfs", "localhost:/var/share", "/nfs-dest"}, 0)
+	c.Check(err, IsNil)
+	c.Check(s.sysd.EnsureMountUnitFileWithOptionsCalls, DeepEquals, []*systemd.MountUnitOptions{
+		{
+			Lifetime:    systemd.Transient,
+			Description: "Mount unit for snap1, revision 1 via mount-control",
+			What:        "localhost:/var/share",
+			Where:       "/nfs-dest",
+			Fstype:      "nfs",
+			Options:     []string{"rw"},
+			Origin:      "mount-control",
+		},
+	})
+}
+
 func (s *mountSuite) TestEnsureMountUnitFailed(c *C) {
 	s.injectSnapWithProperPlug(c)
 

diff --git a/tests/lib/snaps/store/test-snapd-mount-control-nfs/bin/cmd b/tests/lib/snaps/store/test-snapd-mount-control-nfs/bin/cmd
@@ -0,0 +1,4 @@
+#!/bin/sh
+PS1='$ '
+
+exec "$@"
diff --git a/tests/lib/snaps/store/test-snapd-mount-control-nfs/snapcraft.yaml b/tests/lib/snaps/store/test-snapd-mount-control-nfs/snapcraft.yaml
@@ -0,0 +1,34 @@
+name: test-snapd-mount-control-nfs
+summary: Snap for testing mount-control with NFS
+description: Snap for testing mount-control with NFS
+version: "1.0"
+base: core22
+confinement: strict
+
+apps:
+    cmd:
+      command: bin/cmd
+      plugs:
+        - mntctl
+        - network
+        - removable-media
+
+plugs:
+    mntctl:
+        interface: mount-control
+        mount:
+            - type: [nfs]
+              where: /media/**
+              options: [rw]
+
+parts:
+  apps:
+    plugin: dump
+    source: .
+
+  network-shares:
+    plugin: nil
+    stage-packages:
+      - nfs-common
+    stage:
+      - -lib/systemd/system/nfs-common.service