Merge pull request #445 from kzys/jailer-patch-drive

Fix bind-mount for additional drive files

Author: Kazuyoshi Kato

runtime/jailer.go

@@ -105,5 +105,5 @@ func newJailer(
 		CgroupPath:        request.JailerConfig.CgroupPath,
 		DriveExposePolicy: request.JailerConfig.DriveExposePolicy,
 	}
-	return newRuncJailer(ctx, l, service.vmID, config)
+	return newRuncJailer(ctx, l, service.vmID, config, request.DriveMounts)
 }

runtime/jailer_integ_test.go

@@ -25,10 +25,12 @@ import (
 	"github.com/containerd/containerd/namespaces"
 	"github.com/containerd/containerd/oci"
 	"github.com/containerd/containerd/pkg/ttrpcutil"
+	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	_ "github.com/firecracker-microvm/firecracker-containerd/firecracker-control"
+	"github.com/firecracker-microvm/firecracker-containerd/internal"
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 	fccontrol "github.com/firecracker-microvm/firecracker-containerd/proto/service/fccontrol/ttrpc"
 	"github.com/firecracker-microvm/firecracker-containerd/runtime/cpuset"
@@ -55,6 +57,10 @@ func TestJailer_Isolated(t *testing.T) {
 	})
 }
 
+func fsSafeTestName(tb testing.TB) string {
+	return strings.Replace(tb.Name(), "/", "-", -1)
+}
+
 func testJailer(t *testing.T, jailerConfig *proto.JailerConfig) {
 	require := require.New(t)
 
@@ -72,26 +78,39 @@ func testJailer(t *testing.T, jailerConfig *proto.JailerConfig) {
 
 	vmID := testNameToVMID(t.Name())
 
+	additionalDrive := internal.CreateFSImg(ctx, t, "ext4", internal.FSImgFile{
+		Subpath:  "dir/hello",
+		Contents: "additional drive\n",
+	})
+
 	request := proto.CreateVMRequest{
 		VMID:         vmID,
 		JailerConfig: jailerConfig,
+		DriveMounts: []*proto.FirecrackerDriveMount{
+			{HostPath: additionalDrive, VMPath: "/mnt", FilesystemType: "ext4"},
+		},
 	}
 
 	// If the drive files are bind-mounted, the files must be readable from the jailer's user.
 	if jailerConfig != nil && jailerConfig.DriveExposePolicy == proto.DriveExposePolicy_BIND {
-		f, err := ioutil.TempFile("", strings.Replace(t.Name(), "/", "-", -1))
+		f, err := ioutil.TempFile("", fsSafeTestName(t)+"_rootfs")
 		require.NoError(err)
 		defer f.Close()
 
 		dst := f.Name()
 
+		// Copy the root drive before chown, since the file is used by other tests.
 		err = copyFile(defaultRuntimeConfig.RootDrive, dst, 0400)
 		require.NoErrorf(err, "failed to copy a rootfs as %q", dst)
 
 		err = os.Chown(dst, int(jailerConfig.UID), int(jailerConfig.GID))
 		require.NoError(err, "failed to chown %q", dst)
 
 		request.RootDrive = &proto.FirecrackerRootDrive{HostPath: dst}
+
+		// The additional drive file is only used by this test.
+		err = os.Chown(additionalDrive, int(jailerConfig.UID), int(jailerConfig.GID))
+		require.NoError(err, "failed to chown %q", additionalDrive)
 	}
 
 	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
@@ -102,12 +121,22 @@ func testJailer(t *testing.T, jailerConfig *proto.JailerConfig) {
 		"container",
 		containerd.WithSnapshotter(defaultSnapshotterName),
 		containerd.WithNewSnapshot("snapshot", image),
-		containerd.WithNewSpec(oci.WithProcessArgs("/bin/echo", "-n", "hello"), firecrackeroci.WithVMID(vmID)),
+		containerd.WithNewSpec(
+			oci.WithProcessArgs(
+				"/bin/sh", "-c", "echo hello && cat /mnt/in-container/dir/hello",
+			),
+			firecrackeroci.WithVMID(vmID),
+			oci.WithMounts([]specs.Mount{{
+				Source:      "/mnt",
+				Destination: "/mnt/in-container",
+				Options:     []string{"bind"},
+			}}),
+		),
 	)
 	require.NoError(err)
 
 	stdout := startAndWaitTask(ctx, t, c)
-	require.Equal("hello", stdout)
+	require.Equal("hello\nadditional drive\n", stdout)
 
 	stat, err := os.Stat(filepath.Join(shimBaseDir(), "default", vmID))
 	require.NoError(err)

runtime/runc_jailer.go

@@ -51,6 +51,7 @@ type runcJailer struct {
 	vmID       string
 	configSpec specs.Spec
 	runcClient runc.Runc
+	started    bool
 }
 
 const firecrackerFileName = "firecracker"
@@ -69,7 +70,10 @@ type runcJailerConfig struct {
 	DriveExposePolicy proto.DriveExposePolicy
 }
 
-func newRuncJailer(ctx context.Context, logger *logrus.Entry, vmID string, cfg runcJailerConfig) (*runcJailer, error) {
+func newRuncJailer(
+	ctx context.Context, logger *logrus.Entry, vmID string, cfg runcJailerConfig,
+	mounts []*proto.FirecrackerDriveMount,
+) (*runcJailer, error) {
 	l := logger.WithField("ociBundlePath", cfg.OCIBundlePath).
 		WithField("runcBinaryPath", cfg.RuncBinPath)
 
@@ -102,9 +106,26 @@ func newRuncJailer(ctx context.Context, logger *logrus.Entry, vmID string, cfg r
 		return nil, errors.Wrapf(err, "%s failed to mkdirAndChown", rootPath)
 	}
 
+	if j.Config.DriveExposePolicy == proto.DriveExposePolicy_BIND {
+		err := j.prepareBindMounts(mounts)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	return j, nil
 }
 
+func (j *runcJailer) prepareBindMounts(mounts []*proto.FirecrackerDriveMount) error {
+	for _, m := range mounts {
+		err := j.bindMountFileToJail(m.HostPath, filepath.Join(j.RootPath(), m.HostPath))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // JailPath returns the base directory from where the jail binary will be ran
 // from
 func (j *runcJailer) OCIBundlePath() string {
@@ -242,6 +263,8 @@ func (j *runcJailer) BuildJailedRootHandler(cfg *config.Config, machineConfig *f
 			}
 
 			j.logger.Info("Successfully ran jailer handler")
+			j.started = true
+
 			return nil
 		},
 	}
@@ -341,7 +364,7 @@ func (j *runcJailer) ExposeFileToJail(srcPath string) error {
 // exposeFileToJail will make the file accessible from the jail.
 func (j *runcJailer) exposeFileToJail(src, dst string, mode os.FileMode) error {
 	if j.Config.DriveExposePolicy == proto.DriveExposePolicy_BIND {
-		return j.bindMountFileToJail(src, dst, mode)
+		return j.bindMountFileToJail(src, dst)
 	}
 	return j.copyFileToJail(src, dst, mode)
 }
@@ -359,7 +382,28 @@ func (j *runcJailer) copyFileToJail(src, dst string, mode os.FileMode) error {
 
 // bindMountFileToJail mounts a file from src to dst, and chown the new file to
 // the jail user. Note that actual mount is not happening until runc is invoked.
-func (j *runcJailer) bindMountFileToJail(src, dst string, _ os.FileMode) error {
+func (j *runcJailer) bindMountFileToJail(src, dst string) error {
+	// Once runc has started, the jailer cannot mount any new files.
+	if j.started {
+		_, err := os.Stat(dst)
+		if err != nil {
+			return errors.Wrapf(err, "%q must be created by runc", dst)
+		}
+		return nil
+	}
+
+	// The directory must be traversable from runc (running as root) and Firecracker (running as j.Config.UID).
+	// These two users don't have a shared group. Hence the permission must be 701, not 700.
+	err := os.MkdirAll(filepath.Dir(dst), 0701)
+	if err != nil {
+		return err
+	}
+
+	err = os.Chown(filepath.Dir(dst), int(j.Config.UID), int(j.Config.GID))
+	if err != nil {
+		return err
+	}
+
 	f, err := os.Create(dst)
 	if err != nil {
 		return err

runtime/runc_jailer_test.go

@@ -21,6 +21,8 @@ import (
 	"syscall"
 	"testing"
 
+	"github.com/firecracker-microvm/firecracker-containerd/proto"
+
 	"github.com/firecracker-microvm/firecracker-go-sdk"
 	models "github.com/firecracker-microvm/firecracker-go-sdk/client/models"
 	"github.com/sirupsen/logrus"
@@ -61,7 +63,7 @@ func TestBuildJailedRootHandler_Isolated(t *testing.T) {
 		GID:            456,
 	}
 	vmID := "foo"
-	jailer, err := newRuncJailer(context.Background(), l, vmID, runcConfig)
+	jailer, err := newRuncJailer(context.Background(), l, vmID, runcConfig, []*proto.FirecrackerDriveMount{})
 	require.NoError(t, err, "failed to create runc jailer")
 
 	cfg := config.Config{
@@ -132,3 +134,40 @@ func TestMkdirAllWithPermissions_Isolated(t *testing.T) {
 	assert.Equal(t, newuid, newlyCreatedPathStat.Sys().(*syscall.Stat_t).Uid)
 	assert.Equal(t, newgid, newlyCreatedPathStat.Sys().(*syscall.Stat_t).Gid)
 }
+
+func TestBindMountToJail_Isolated(t *testing.T) {
+	// The user must be root to call chown.
+	internal.RequiresIsolation(t)
+
+	dir, err := ioutil.TempDir("", t.Name())
+	require.NoError(t, err)
+	defer os.RemoveAll(dir)
+
+	f, err := os.Create(filepath.Join(dir, "src1"))
+	require.NoError(t, err)
+	defer f.Close()
+
+	j := &runcJailer{
+		started: false,
+		Config:  runcJailerConfig{OCIBundlePath: dir},
+	}
+
+	// Create the mount point. The mount point will be used by runc later.
+	err = j.bindMountFileToJail(
+		filepath.Join(dir, "src1"),
+		filepath.Join(dir, "dst1"),
+	)
+	require.NoError(t, err)
+
+	_, err = os.Stat(filepath.Join(dir, "dst1"))
+	require.NoError(t, err)
+
+	// Once runc has been started, it doesn't create a mount point and
+	// let the caller know the method cannot be used.
+	j.started = true
+	err = j.bindMountFileToJail(
+		filepath.Join(dir, "src2"),
+		filepath.Join(dir, "not-found"),
+	)
+	require.Error(t, err)
+}