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

mount-control: step 3 #10864

Merged
merged 3 commits into from
Jan 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions interfaces/builtin/mount_control.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,12 @@ func validateMountInfo(mountInfo *MountInfo) error {
return fmt.Errorf(`mount-control "what" attribute must be "none" with "tmpfs"; found %q instead`, mountInfo.what)
}

// Until we have a clear picture of how this should work, disallow creating
// persistent mounts into $SNAP_DATA
if mountInfo.persistent && strings.HasPrefix(mountInfo.where, "$SNAP_DATA") {
return errors.New(`mount-control "persistent" attribute cannot be used to mount onto $SNAP_DATA`)
}

return nil
}

Expand Down
4 changes: 4 additions & 0 deletions interfaces/builtin/mount_control_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,10 @@ apps:
"mount:\n - what: /\n where: /media/*\n options: [rw]\n type: [tmpfs]",
`mount-control "what" attribute must be "none" with "tmpfs"; found "/" instead`,
},
{
"mount:\n - what: /\n where: $SNAP_DATA/foo\n options: [ro]\n persistent: true",
`mount-control "persistent" attribute cannot be used to mount onto \$SNAP_DATA`,
},
}

for _, testData := range data {
Expand Down
237 changes: 237 additions & 0 deletions overlord/hookstate/ctlcmd/mount.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2022 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 ctlcmd

import (
"fmt"
"strings"

"github.com/snapcore/snapd/i18n"
"github.com/snapcore/snapd/interfaces"
"github.com/snapcore/snapd/interfaces/utils"
"github.com/snapcore/snapd/logger"
"github.com/snapcore/snapd/overlord/hookstate"
"github.com/snapcore/snapd/overlord/ifacestate"
"github.com/snapcore/snapd/overlord/snapstate"
"github.com/snapcore/snapd/snap"
"github.com/snapcore/snapd/strutil"
"github.com/snapcore/snapd/systemd"
)

var (
shortMountHelp = i18n.G("Create a temporary or permanent mount")
longMountHelp = i18n.G(`
The mount command mounts the given source onto the given destination path,
provided that the snap has a plug for the mount-control interface which allows
this operation.`)
)

func init() {
addCommand("mount", shortMountHelp, longMountHelp, func() command { return &mountCommand{} })
}

type mountCommand struct {
baseCommand
Positional struct {
What string `positional-arg-name:"<what>" required:"yes" description:"path to the resource to be mounted"`
Where string `positional-arg-name:"<where>" required:"yes" description:"path to the destination mount point"`
} `positional-args:"yes" required:"yes"`
Persistent bool `long:"persistent" description:"make the mount persist across reboots"`
Type string `long:"type" short:"t" description:"filesystem type"`
Options string `long:"options" short:"o" description:"comma-separated list of mount options"`
snapInfo *snap.Info
optionsList []string
}

func matchMountPathAttribute(path string, attribute interface{}, snapInfo *snap.Info) bool {
pattern, ok := attribute.(string)
if !ok {
return false
}

expandedPattern := snapInfo.ExpandSnapVariables(pattern)
Copy link
Collaborator

Choose a reason for hiding this comment

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

afaict the unit tests don't exercise this code, we need test for $SNAP_DATA and $SNAP_COMMON and the valid/invalid combinations with persistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added more tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see them, forgot to push something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is now tested (it appears green in the coverage report), and the combinatio with persistent is tested here. You didn't mention $SNAP_COMMON in your other comment, so I didn't treat it specially. Let me know if I should.

Copy link
Collaborator

Choose a reason for hiding this comment

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

coverage and tested are not the same thing. I haven't rechecked but if this line is removed no tests fail, so this is not tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I added one test now. I wanted to mock snapInfo.ExpandSnapVariables() but snap.Info is not an interface, so I fell back on using the production code and just compose the "where" in such a way that it would replicate the logic of info.DataDir().


pp, err := utils.NewPathPattern(expandedPattern)
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
}

if !matchMountPathAttribute(m.Positional.Where, attributes["where"], m.snapInfo) {
return false
}

if m.Type != "" {
if types, ok := attributes["type"].([]interface{}); ok {
found := false
for _, iface := range types {
if typeString, ok := iface.(string); ok && typeString == m.Type {
found = true
break
}
}
if !found {
return false
}
} else {
return false
}
} else {
// The filesystem type was not given; we let it through only if the
Copy link
Collaborator

@pedronis pedronis Dec 16, 2021

Choose a reason for hiding this comment

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

is this scenario tested?

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, one branch was tested by the existing tests, and this new else is tested with this small addition

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a test for the happy case where both the plug and the command don't have type

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 newly added TestHappyWithVariableExpansion() incidentally tests that. Let me know if you'd rather have a separate test.

// plug also did not specify a type.
if _, typeIsSet := attributes["type"]; typeIsSet {
return false
}
}

if optionsIfaces, ok := attributes["options"].([]interface{}); ok {
var allowedOptions []string
for _, iface := range optionsIfaces {
if option, ok := iface.(string); ok {
allowedOptions = append(allowedOptions, option)
}
}
for _, option := range m.optionsList {
if !strutil.ListContains(allowedOptions, option) {
return false
}
}
}

if m.Persistent {
if allowedPersistent, ok := attributes["persistent"].(bool); !ok || !allowedPersistent {
return false
}
}

return true
}

// checkConnections checks whether the established connections give the snap
// permission to execute the mount command
func (m *mountCommand) checkConnections(context *hookstate.Context) error {
snapName := context.InstanceName()

st := context.State()
st.Lock()
defer st.Unlock()

conns, err := ifacestate.ConnectionStates(st)
Copy link
Collaborator

Choose a reason for hiding this comment

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

for a follow up but now I think we have 2 or 3 places in ctlcmd doing matches against this, maybe they can share a helper? worth checking at least

if err != nil {
return fmt.Errorf("internal error: cannot get connections: %s", err)
}

m.snapInfo, err = snapstate.CurrentInfo(st, snapName)
if err != nil {
return fmt.Errorf("internal error: cannot get snap info: %s", err)
}

for connId, connState := range conns {
if connState.Interface != "mount-control" {
continue
}

if connState.Undesired || connState.HotplugGone {
continue
}

connRef, err := interfaces.ParseConnRef(connId)
if err != nil {
return err
}

if connRef.PlugRef.Snap != snapName {
continue
}

mounts, ok := connState.StaticPlugAttrs["mount"].([]interface{})
if !ok {
continue
}

for _, mountAttributes := range mounts {
if m.matchConnection(mountAttributes.(map[string]interface{})) {
return nil
}
}
}
return fmt.Errorf("no matching mount-control connection found")
}

func (m *mountCommand) createMountUnit(sysd systemd.Systemd) (string, error) {
snapName := m.snapInfo.InstanceName()
revision := m.snapInfo.SnapRevision().String()
lifetime := systemd.Transient
if m.Persistent {
lifetime = systemd.Persistent
}
return sysd.AddMountUnitFileWithOptions(&systemd.MountUnitOptions{
Lifetime: lifetime,
SnapName: snapName,
Revision: revision,
What: m.Positional.What,
Where: m.Positional.Where,
Fstype: m.Type,
Options: m.optionsList,
Origin: "mount-control",
})
}

func (m *mountCommand) Execute([]string) error {
context, err := m.ensureContext()
if err != nil {
return err
}

// Parse the mount options into an array
for _, option := range strings.Split(m.Options, ",") {
if option != "" {
m.optionsList = append(m.optionsList, option)
}
}

if err := m.checkConnections(context); err != nil {
snapName := context.InstanceName()
return fmt.Errorf("snap %q lacks permissions to create the requested mount: %v", snapName, err)
}

sysd := systemd.New(systemd.SystemMode, nil)
name, err := m.createMountUnit(sysd)
if err != nil {
return fmt.Errorf("cannot create mount unit: %v", err)
}

if err := sysd.Start(name); err != nil {
// There's no point in keeping the mount unit if it doesn't work. Even
// if the problem is transient, the next invocation of "snapctl mount"
// will create a new unit anyway.
if err := sysd.RemoveMountUnitFile(m.Positional.Where); err != nil {
// Not much we can do about it, other than logging.
logger.Noticef("cannot remove mount unit on %q which failed to start: %v", m.Positional.Where, err)
}
return fmt.Errorf("cannot start mount unit: %v", err)
}
return nil
}
Loading