Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Commit

Permalink
Merge pull request #2713 from bergwolf/ro-shared-dir
Browse files Browse the repository at this point in the history
vc: make host shared path readonly
  • Loading branch information
teawater authored Jun 3, 2020
2 parents 5300c51 + a3dec26 commit 7a4925f
Show file tree
Hide file tree
Showing 14 changed files with 96 additions and 64 deletions.
3 changes: 0 additions & 3 deletions virtcontainers/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,6 @@ type agent interface {
// configureFromGrpc will update agent settings based on provided arguments which from Grpc
configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error

// getSharePath will return the agent 9pfs share mount path
getSharePath(id string) string

// reseedRNG will reseed the guest random number generator
reseedRNG(data []byte) error

Expand Down
4 changes: 2 additions & 2 deletions virtcontainers/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func TestStartSandboxKataAgentSuccessful(t *testing.T) {

// TODO: defaultSharedDir is a hyper var = /run/hyper/shared/sandboxes
// do we need to unmount sandboxes and containers?
err = bindUnmountAllRootfs(ctx, testDir, pImpl)
err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl)
assert.NoError(err)
}

Expand Down Expand Up @@ -474,7 +474,7 @@ func TestRunSandboxKataAgentSuccessful(t *testing.T) {
_, err = os.Stat(sandboxDir)
assert.NoError(err)

err = bindUnmountAllRootfs(ctx, testDir, pImpl)
err = bindUnmountAllRootfs(ctx, filepath.Join(testDir, p.ID()), pImpl)
assert.NoError(err)
}

Expand Down
4 changes: 2 additions & 2 deletions virtcontainers/clh.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ
clh.Logger().WithField("function", "createSandbox").Info("Sandbox already exist, loading from state")
clh.virtiofsd = &virtiofsd{
PID: clh.state.VirtiofsdPID,
sourcePath: filepath.Join(kataHostSharedDir(), clh.id),
sourcePath: filepath.Join(getSharePath(clh.id)),
debug: clh.config.Debug,
socketPath: virtiofsdSocketPath,
}
Expand Down Expand Up @@ -307,7 +307,7 @@ func (clh *cloudHypervisor) createSandbox(ctx context.Context, id string, networ

clh.virtiofsd = &virtiofsd{
path: clh.config.VirtioFSDaemon,
sourcePath: filepath.Join(kataHostSharedDir(), clh.id),
sourcePath: filepath.Join(getSharePath(clh.id)),
socketPath: virtiofsdSocketPath,
extraArgs: clh.config.VirtioFSExtraArgs,
debug: clh.config.Debug,
Expand Down
7 changes: 4 additions & 3 deletions virtcontainers/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ func (c *Container) shareFiles(m Mount, idx int, hostSharedDir, guestSharedDir s
}
} else {
// These mounts are created in the shared dir
mountDest := filepath.Join(hostSharedDir, c.sandbox.id, filename)
mountDest := filepath.Join(hostSharedDir, filename)
if err := bindMount(c.ctx, m.Source, mountDest, false, "private"); err != nil {
return "", false, err
}
Expand Down Expand Up @@ -850,7 +850,7 @@ func (c *Container) rollbackFailingContainerCreation() {
if err := c.unmountHostMounts(); err != nil {
c.Logger().WithError(err).Error("rollback failed unmountHostMounts()")
}
if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil {
if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil {
c.Logger().WithError(err).Error("rollback failed bindUnmountContainerRootfs()")
}
}
Expand All @@ -875,6 +875,7 @@ func (c *Container) create() (err error) {
// of rolling back all the actions previously performed.
defer func() {
if err != nil {
c.Logger().WithError(err).Error("container create failed")
c.rollbackFailingContainerCreation()
}
}()
Expand Down Expand Up @@ -1119,7 +1120,7 @@ func (c *Container) stop(force bool) error {
return err
}

if err := bindUnmountContainerRootfs(c.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err != nil && !force {
if err := bindUnmountContainerRootfs(c.ctx, getMountPath(c.sandbox.id), c.id); err != nil && !force {
return err
}

Expand Down
89 changes: 73 additions & 16 deletions virtcontainers/kata_agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,26 @@ var kataHostSharedDir = func() string {
return defaultKataHostSharedDir
}

// Shared path handling:
// 1. create two directories for each sandbox:
// -. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/, a directory to hold all host/guest shared mounts
// -. /run/kata-containers/shared/sandboxes/$sbx_id/shared/, a host/guest shared directory (9pfs/virtiofs source dir)
//
// 2. /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ is bind mounted readonly to /run/kata-containers/shared/sandboxes/$sbx_id/shared/, so guest cannot modify it
//
// 3. host-guest shared files/directories are mounted one-level under /run/kata-containers/shared/sandboxes/$sbx_id/mounts/ and thus present to guest at one level under /run/kata-containers/shared/sandboxes/$sbx_id/shared/
func getSharePath(id string) string {
return filepath.Join(kataHostSharedDir(), id, "shared")
}

func getMountPath(id string) string {
return filepath.Join(kataHostSharedDir(), id, "mounts")
}

func getSandboxPath(id string) string {
return filepath.Join(kataHostSharedDir(), id)
}

// The function is declared this way for mocking in unit tests
var kataGuestSharedDir = func() string {
if rootless.IsRootless() {
Expand All @@ -158,6 +178,10 @@ var kataGuestSandboxDir = func() string {
return defaultKataGuestSandboxDir
}

var kataGuestSandboxStorageDir = func() string {
return filepath.Join(defaultKataGuestSandboxDir, "storage")
}

func ephemeralPath() string {
if rootless.IsRootless() {
return filepath.Join(kataGuestSandboxDir(), kataEphemeralDevType)
Expand Down Expand Up @@ -223,10 +247,6 @@ func (k *kataAgent) Logger() *logrus.Entry {
return virtLog.WithField("subsystem", "kata_agent")
}

func (k *kataAgent) getSharePath(id string) string {
return filepath.Join(kataHostSharedDir(), id)
}

func (k *kataAgent) longLiveConn() bool {
return k.keepConn
}
Expand Down Expand Up @@ -356,7 +376,7 @@ func (k *kataAgent) capabilities() types.Capabilities {
return caps
}

func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error {
func (k *kataAgent) internalConfigure(h hypervisor, id string, builtin bool, config interface{}) error {
var err error
if config != nil {
switch c := config.(type) {
Expand All @@ -378,7 +398,7 @@ func (k *kataAgent) internalConfigure(h hypervisor, id, sharePath string, builti
}

func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool, config interface{}) error {
err := k.internalConfigure(h, id, sharePath, builtin, config)
err := k.internalConfigure(h, id, builtin, config)
if err != nil {
return err
}
Expand Down Expand Up @@ -424,14 +444,36 @@ func (k *kataAgent) configure(h hypervisor, id, sharePath string, builtin bool,
}

func (k *kataAgent) configureFromGrpc(h hypervisor, id string, builtin bool, config interface{}) error {
return k.internalConfigure(h, id, "", builtin, config)
return k.internalConfigure(h, id, builtin, config)
}

func (k *kataAgent) setupSharedPath(sandbox *Sandbox) error {
// create shared path structure
sharePath := getSharePath(sandbox.id)
mountPath := getMountPath(sandbox.id)
if err := os.MkdirAll(sharePath, DirMode); err != nil {
return err
}
if err := os.MkdirAll(mountPath, DirMode); err != nil {
return err
}

// slave mount so that future mountpoints under mountPath are shown in sharePath as well
if err := bindMount(context.Background(), mountPath, sharePath, true, "slave"); err != nil {
return err
}

return nil
}

func (k *kataAgent) createSandbox(sandbox *Sandbox) error {
span, _ := k.trace("createSandbox")
defer span.Finish()

return k.configure(sandbox.hypervisor, sandbox.id, k.getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig)
if err := k.setupSharedPath(sandbox); err != nil {
return err
}
return k.configure(sandbox.hypervisor, sandbox.id, getSharePath(sandbox.id), k.proxyBuiltIn, sandbox.config.AgentConfig)
}

func cmdToKataProcess(cmd types.Cmd) (process *grpc.Process, err error) {
Expand Down Expand Up @@ -1030,7 +1072,7 @@ func (k *kataAgent) replaceOCIMountsForStorages(spec *specs.Spec, volumeStorages
// Create a temporary location to mount the Storage. Mounting to the correct location
// will be handled by the OCI mount structure.
filename := fmt.Sprintf("%s-%s", uuid.Generate().String(), filepath.Base(m.Destination))
path := filepath.Join(kataGuestSharedDir(), filename)
path := filepath.Join(kataGuestSandboxStorageDir(), filename)

k.Logger().Debugf("Replacing OCI mount source (%s) with %s", m.Source, path)
ociMounts[index].Source = path
Expand Down Expand Up @@ -1245,7 +1287,7 @@ func (k *kataAgent) rollbackFailingContainerCreation(c *Container) {
k.Logger().WithError(err2).Error("rollback failed unmountHostMounts()")
}

if err2 := bindUnmountContainerRootfs(k.ctx, kataHostSharedDir(), c.sandbox.id, c.id); err2 != nil {
if err2 := bindUnmountContainerRootfs(k.ctx, getMountPath(c.sandbox.id), c.id); err2 != nil {
k.Logger().WithError(err2).Error("rollback failed bindUnmountContainerRootfs()")
}
}
Expand Down Expand Up @@ -1302,6 +1344,13 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat
rootfs.Options = []string{"nouuid"}
}

// Ensure container mount destination exists
// TODO: remove dependency on shared fs path. shared fs is just one kind of storage sources.
// we should not always use shared fs path for all kinds of storage. Stead, all storage
// should be bind mounted to a tmpfs path for containers to use.
if err := os.MkdirAll(filepath.Join(getMountPath(c.sandbox.id), c.id, c.rootfsSuffix), DirMode); err != nil {
return nil, err
}
return rootfs, nil
}

Expand All @@ -1313,7 +1362,7 @@ func (k *kataAgent) buildContainerRootfs(sandbox *Sandbox, c *Container, rootPat
// (kataGuestSharedDir) is already mounted in the
// guest. We only need to mount the rootfs from
// the host and it will show up in the guest.
if err := bindMountContainerRootfs(k.ctx, kataHostSharedDir(), sandbox.id, c.id, c.rootFs.Target, false); err != nil {
if err := bindMountContainerRootfs(k.ctx, getMountPath(sandbox.id), c.id, c.rootFs.Target, false); err != nil {
return nil, err
}

Expand Down Expand Up @@ -1346,6 +1395,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,
// takes care of rolling back actions previously performed.
defer func() {
if err != nil {
k.Logger().WithError(err).Error("createContainer failed")
k.rollbackFailingContainerCreation(c)
}
}()
Expand All @@ -1366,7 +1416,7 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,
}

// Handle container mounts
newMounts, ignoredMounts, err := c.mountSharedDirMounts(kataHostSharedDir(), kataGuestSharedDir())
newMounts, ignoredMounts, err := c.mountSharedDirMounts(getMountPath(sandbox.id), kataGuestSharedDir())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -2363,13 +2413,20 @@ func (k *kataAgent) markDead() {
}

func (k *kataAgent) cleanup(s *Sandbox) {
path := k.getSharePath(s.id)
// Unmount shared path
path := getSharePath(s.id)
k.Logger().WithField("path", path).Infof("cleanup agent")
if err := syscall.Unmount(path, syscall.MNT_DETACH|UmountNoFollow); err != nil {
k.Logger().WithError(err).Errorf("failed to unmount vm share path %s", path)
}

// Unmount mount path
path = getMountPath(s.id)
if err := bindUnmountAllRootfs(k.ctx, path, s); err != nil {
k.Logger().WithError(err).Errorf("failed to unmount container share path %s", path)
k.Logger().WithError(err).Errorf("failed to unmount vm mount path %s", path)
}
if err := os.RemoveAll(path); err != nil {
k.Logger().WithError(err).Errorf("failed to cleanup vm share path %s", path)
if err := os.RemoveAll(getSandboxPath(s.id)); err != nil {
k.Logger().WithError(err).Errorf("failed to cleanup vm path %s", getSandboxPath(s.id))
}
}

Expand Down
13 changes: 0 additions & 13 deletions virtcontainers/kata_agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -785,19 +785,6 @@ func TestHandlePidNamespace(t *testing.T) {
assert.False(testIsPidNamespacePresent(g))
}

func TestAgentPathAPI(t *testing.T) {
assert := assert.New(t)

k1 := &kataAgent{}
k2 := &kataAgent{}
id := "foobar"

// getSharePath
path1 := k1.getSharePath(id)
path2 := k2.getSharePath(id)
assert.Equal(path1, path2)
}

func TestAgentConfigure(t *testing.T) {
assert := assert.New(t)

Expand Down
14 changes: 7 additions & 7 deletions virtcontainers/mount.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,11 +275,11 @@ func remount(ctx context.Context, mountflags uintptr, src string) error {

// bindMountContainerRootfs bind mounts a container rootfs into a 9pfs shared
// directory between the guest and the host.
func bindMountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID, cRootFs string, readonly bool) error {
func bindMountContainerRootfs(ctx context.Context, shareDir, cid, cRootFs string, readonly bool) error {
span, _ := trace(ctx, "bindMountContainerRootfs")
defer span.Finish()

rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
rootfsDest := filepath.Join(shareDir, cid, rootfsDir)

return bindMount(ctx, cRootFs, rootfsDest, readonly, "private")
}
Expand Down Expand Up @@ -315,12 +315,12 @@ func isSymlink(path string) bool {
return stat.Mode()&os.ModeSymlink != 0
}

func bindUnmountContainerRootfs(ctx context.Context, sharedDir, sandboxID, cID string) error {
func bindUnmountContainerRootfs(ctx context.Context, sharedDir, cID string) error {
span, _ := trace(ctx, "bindUnmountContainerRootfs")
defer span.Finish()

rootfsDest := filepath.Join(sharedDir, sandboxID, cID, rootfsDir)
if isSymlink(filepath.Join(sharedDir, sandboxID, cID)) || isSymlink(rootfsDest) {
rootfsDest := filepath.Join(sharedDir, cID, rootfsDir)
if isSymlink(filepath.Join(sharedDir, cID)) || isSymlink(rootfsDest) {
logrus.Warnf("container dir %s is a symlink, malicious guest?", cID)
return nil
}
Expand All @@ -343,15 +343,15 @@ func bindUnmountAllRootfs(ctx context.Context, sharedDir string, sandbox *Sandbo

var errors *merr.Error
for _, c := range sandbox.containers {
if isSymlink(filepath.Join(sharedDir, sandbox.id, c.id)) {
if isSymlink(filepath.Join(sharedDir, c.id)) {
logrus.Warnf("container dir %s is a symlink, malicious guest?", c.id)
continue
}
c.unmountHostMounts()
if c.state.Fstype == "" {
// even if error found, don't break out of loop until all mounts attempted
// to be unmounted, and collect all errors
errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, sandbox.id, c.id))
errors = merr.Append(errors, bindUnmountContainerRootfs(c.ctx, sharedDir, c.id))
}
}
return errors.ErrorOrNil()
Expand Down
4 changes: 2 additions & 2 deletions virtcontainers/mount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ func TestBindUnmountContainerRootfsENOENTNotError(t *testing.T) {
assert.NoError(os.Remove(testPath))
}

err := bindUnmountContainerRootfs(context.Background(), testMnt, sID, cID)
err := bindUnmountContainerRootfs(context.Background(), filepath.Join(testMnt, sID), cID)
assert.NoError(err)
}

Expand All @@ -407,7 +407,7 @@ func TestBindUnmountContainerRootfsRemoveRootfsDest(t *testing.T) {
assert.NoError(err)
defer os.RemoveAll(filepath.Join(testDir, sID))

bindUnmountContainerRootfs(context.Background(), testDir, sID, cID)
bindUnmountContainerRootfs(context.Background(), filepath.Join(testDir, sID), cID)

if _, err := os.Stat(testPath); err == nil {
t.Fatal("empty rootfs dest should be removed")
Expand Down
5 changes: 0 additions & 5 deletions virtcontainers/noop_agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,6 @@ func (n *noopAgent) configureFromGrpc(h hypervisor, id string, builtin bool, con
return nil
}

// getSharePath is the Noop agent share path getter. It does nothing.
func (n *noopAgent) getSharePath(id string) string {
return ""
}

// reseedRNG is the Noop agent RND reseeder. It does nothing.
func (n *noopAgent) reseedRNG(data []byte) error {
return nil
Expand Down
7 changes: 0 additions & 7 deletions virtcontainers/noop_agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,13 +156,6 @@ func TestNoopAgentConfigure(t *testing.T) {
assert.NoError(err)
}

func TestNoopAgentGetSharePath(t *testing.T) {
n := &noopAgent{}
path := n.getSharePath("")
assert := assert.New(t)
assert.Empty(path)
}

func TestNoopAgentStartProxy(t *testing.T) {
assert := assert.New(t)
n := &noopAgent{}
Expand Down
2 changes: 1 addition & 1 deletion virtcontainers/qemu.go
Original file line number Diff line number Diff line change
Expand Up @@ -618,7 +618,7 @@ func (q *qemu) virtiofsdArgs(fd uintptr) []string {
// The daemon will terminate when the vhost-user socket
// connection with QEMU closes. Therefore we do not keep track
// of this child process after returning from this function.
sourcePath := filepath.Join(kataHostSharedDir(), q.id)
sourcePath := filepath.Join(getSharePath(q.id))
args := []string{
fmt.Sprintf("--fd=%v", fd),
"-o", "source=" + sourcePath,
Expand Down
Loading

0 comments on commit 7a4925f

Please # to comment.