From bc81f3fe4a042a15343d2eab44da32d818ac1ade Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Fri, 7 Sep 2018 12:27:44 -0700 Subject: Remove '--file-access=direct' option It was used before gofer was implemented and it's not supported anymore. BREAKING CHANGE: proxy-shared and proxy-exclusive options are now: shared and exclusive. PiperOrigin-RevId: 212017643 Change-Id: If029d4073fe60583e5ca25f98abb2953de0d78fd --- runsc/boot/BUILD | 3 ++ runsc/boot/config.go | 31 +++++------ runsc/boot/filter/config.go | 39 -------------- runsc/boot/filter/filter.go | 5 -- runsc/boot/fs.go | 19 ++----- runsc/boot/loader.go | 1 - runsc/boot/loader_test.go | 106 ++++++++++++++++++++------------------ runsc/cmd/boot.go | 7 --- runsc/container/container.go | 5 -- runsc/container/container_test.go | 6 +-- runsc/main.go | 6 +-- runsc/sandbox/sandbox.go | 12 ++--- runsc/test/testutil/testutil.go | 2 +- 13 files changed, 85 insertions(+), 157 deletions(-) (limited to 'runsc') diff --git a/runsc/boot/BUILD b/runsc/boot/BUILD index e96722069..a38a3a94e 100644 --- a/runsc/boot/BUILD +++ b/runsc/boot/BUILD @@ -85,8 +85,11 @@ go_test( deps = [ "//pkg/control/server", "//pkg/log", + "//pkg/p9", "//pkg/sentry/context/contexttest", "//pkg/sentry/fs", + "//pkg/unet", + "//runsc/fsgofer", "@com_github_opencontainers_runtime-spec//specs-go:go_default_library", ], ) diff --git a/runsc/boot/config.go b/runsc/boot/config.go index 28a1600cd..01da535af 100644 --- a/runsc/boot/config.go +++ b/runsc/boot/config.go @@ -60,28 +60,23 @@ func (p PlatformType) String() string { type FileAccessType int const ( - // FileAccessProxy sends IO requests to a Gofer process that validates the + // FileAccessShared sends IO requests to a Gofer process that validates the // requests and forwards them to the host. - FileAccessProxy FileAccessType = iota + FileAccessShared FileAccessType = iota - // FileAccessProxyExclusive is the same as FileAccessProxy, but enables + // FileAccessExclusive is the same as FileAccessShared, but enables // extra caching for improved performance. It should only be used if // the sandbox has exclusive access to the filesystem. - FileAccessProxyExclusive - - // FileAccessDirect connects the sandbox directly to the host filesystem. - FileAccessDirect + FileAccessExclusive ) // MakeFileAccessType converts type from string. func MakeFileAccessType(s string) (FileAccessType, error) { switch s { - case "proxy-shared": - return FileAccessProxy, nil - case "proxy-exclusive": - return FileAccessProxyExclusive, nil - case "direct": - return FileAccessDirect, nil + case "shared": + return FileAccessShared, nil + case "exclusive": + return FileAccessExclusive, nil default: return 0, fmt.Errorf("invalid file access type %q", s) } @@ -89,12 +84,10 @@ func MakeFileAccessType(s string) (FileAccessType, error) { func (f FileAccessType) String() string { switch f { - case FileAccessProxy: - return "proxy-shared" - case FileAccessProxyExclusive: - return "proxy-exclusive" - case FileAccessDirect: - return "direct" + case FileAccessShared: + return "shared" + case FileAccessExclusive: + return "exclusive" default: return fmt.Sprintf("unknown(%d)", f) } diff --git a/runsc/boot/filter/config.go b/runsc/boot/filter/config.go index f864b1f45..1a0c426ab 100644 --- a/runsc/boot/filter/config.go +++ b/runsc/boot/filter/config.go @@ -264,45 +264,6 @@ var allowedSyscalls = seccomp.SyscallRules{ }, } -// whitelistFSFilters returns syscalls made by whitelistFS. Using WhitelistFS -// is less secure because it runs inside the Sentry and must be able to perform -// file operations that would otherwise be disabled by seccomp when a Gofer is -// used. When whitelistFS is not used, openning new FD in the Sentry is -// disallowed. -func whitelistFSFilters() seccomp.SyscallRules { - return seccomp.SyscallRules{ - syscall.SYS_ACCESS: {}, - syscall.SYS_FCHMOD: {}, - syscall.SYS_FSTAT: {}, - syscall.SYS_FSYNC: {}, - syscall.SYS_FTRUNCATE: {}, - syscall.SYS_GETCWD: {}, - syscall.SYS_GETDENTS: {}, - syscall.SYS_GETDENTS64: {}, - syscall.SYS_LSEEK: {}, - syscall.SYS_LSTAT: {}, - syscall.SYS_MKDIR: {}, - syscall.SYS_MKDIRAT: {}, - syscall.SYS_NEWFSTATAT: {}, - syscall.SYS_OPEN: {}, - syscall.SYS_OPENAT: {}, - syscall.SYS_PREAD64: {}, - syscall.SYS_PWRITE64: {}, - syscall.SYS_READ: {}, - syscall.SYS_READLINK: {}, - syscall.SYS_READLINKAT: {}, - syscall.SYS_RENAMEAT: {}, - syscall.SYS_STAT: {}, - syscall.SYS_SYMLINK: {}, - syscall.SYS_SYMLINKAT: {}, - syscall.SYS_SYNC_FILE_RANGE: {}, - syscall.SYS_UNLINK: {}, - syscall.SYS_UNLINKAT: {}, - syscall.SYS_UTIMENSAT: {}, - syscall.SYS_WRITE: {}, - } -} - // hostInetFilters contains syscalls that are needed by sentry/socket/hostinet. func hostInetFilters() seccomp.SyscallRules { return seccomp.SyscallRules{ diff --git a/runsc/boot/filter/filter.go b/runsc/boot/filter/filter.go index 56d30f2a0..b656883ad 100644 --- a/runsc/boot/filter/filter.go +++ b/runsc/boot/filter/filter.go @@ -30,7 +30,6 @@ import ( // Options are seccomp filter related options. type Options struct { Platform platform.Platform - WhitelistFS bool HostNetwork bool ControllerFD int } @@ -44,10 +43,6 @@ func Install(opt Options) error { // when not enabled. s.Merge(instrumentationFilters()) - if opt.WhitelistFS { - Report("direct file access allows unrestricted file access!") - s.Merge(whitelistFSFilters()) - } if opt.HostNetwork { Report("host networking enabled: syscall filters less restrictive!") s.Merge(hostInetFilters()) diff --git a/runsc/boot/fs.go b/runsc/boot/fs.go index 772df40fe..3df276170 100644 --- a/runsc/boot/fs.go +++ b/runsc/boot/fs.go @@ -204,7 +204,7 @@ func createRootMount(ctx context.Context, spec *specs.Spec, conf *Config, fds *f ) switch conf.FileAccess { - case FileAccessProxy, FileAccessProxyExclusive: + case FileAccessShared, FileAccessExclusive: fd := fds.remove() log.Infof("Mounting root over 9P, ioFD: %d", fd) hostFS := mustFindFilesystem("9p") @@ -214,13 +214,6 @@ func createRootMount(ctx context.Context, spec *specs.Spec, conf *Config, fds *f return nil, fmt.Errorf("failed to generate root mount point: %v", err) } - case FileAccessDirect: - hostFS := mustFindFilesystem("whitelistfs") - rootInode, err = hostFS.Mount(ctx, rootDevice, mf, "root="+spec.Root.Path+",dont_translate_ownership=true") - if err != nil { - return nil, fmt.Errorf("failed to generate root mount point: %v", err) - } - default: return nil, fmt.Errorf("invalid file access type: %v", conf.FileAccess) } @@ -289,13 +282,10 @@ func getMountNameAndOptions(conf *Config, m specs.Mount, fds *fdDispenser) (stri case bind: switch conf.FileAccess { - case FileAccessProxy, FileAccessProxyExclusive: + case FileAccessShared, FileAccessExclusive: fd := fds.remove() fsName = "9p" opts = p9MountOptions(conf, fd) - case FileAccessDirect: - fsName = "whitelistfs" - opts = []string{"root=" + m.Source, "dont_translate_ownership=true"} default: err = fmt.Errorf("invalid file access type: %v", conf.FileAccess) } @@ -423,7 +413,7 @@ func p9MountOptions(conf *Config, fd int) []string { "wfdno=" + strconv.Itoa(fd), "privateunixsocket=true", } - if conf.FileAccess == FileAccessProxy { + if conf.FileAccess == FileAccessShared { opts = append(opts, "cache=remote_revalidating") } return opts @@ -503,9 +493,6 @@ func addRestoreMount(conf *Config, renv *fs.RestoreEnvironment, m specs.Mount, f // createRestoreEnvironment builds a fs.RestoreEnvironment called renv by adding the mounts // to the environment. func createRestoreEnvironment(spec *specs.Spec, conf *Config, fds *fdDispenser) (*fs.RestoreEnvironment, error) { - if conf.FileAccess == FileAccessDirect { - return nil, fmt.Errorf("host filesystem with whitelist not supported with S/R") - } renv := &fs.RestoreEnvironment{ MountSources: make(map[string][]fs.MountArgs), } diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 540cd6188..5fb489766 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -341,7 +341,6 @@ func (l *Loader) run() error { } else { opts := filter.Options{ Platform: l.k.Platform, - WhitelistFS: l.conf.FileAccess == FileAccessDirect, HostNetwork: l.conf.Network == NetworkHost, ControllerFD: l.ctrl.srv.FD(), } diff --git a/runsc/boot/loader_test.go b/runsc/boot/loader_test.go index 2396d52c8..d6bfe9ff1 100644 --- a/runsc/boot/loader_test.go +++ b/runsc/boot/loader_test.go @@ -20,14 +20,18 @@ import ( "os" "reflect" "sync" + "syscall" "testing" "time" specs "github.com/opencontainers/runtime-spec/specs-go" "gvisor.googlesource.com/gvisor/pkg/control/server" "gvisor.googlesource.com/gvisor/pkg/log" + "gvisor.googlesource.com/gvisor/pkg/p9" "gvisor.googlesource.com/gvisor/pkg/sentry/context/contexttest" "gvisor.googlesource.com/gvisor/pkg/sentry/fs" + "gvisor.googlesource.com/gvisor/pkg/unet" + "gvisor.googlesource.com/gvisor/runsc/fsgofer" ) func init() { @@ -39,7 +43,6 @@ func testConfig() *Config { return &Config{ RootDir: "unused_root_dir", Network: NetworkNone, - FileAccess: FileAccessDirect, DisableSeccomp: true, } } @@ -58,23 +61,62 @@ func testSpec() *specs.Spec { } } -func createLoader() (*Loader, error) { +// startGofer starts a new gofer routine serving 'root' path. It returns the +// sandbox side of the connection, and a function that when called will stop the +// gofer. +func startGofer(root string) (int, func(), error) { + fds, err := syscall.Socketpair(syscall.AF_UNIX, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0) + if err != nil { + return 0, nil, err + } + sandboxEnd, goferEnd := fds[0], fds[1] + + socket, err := unet.NewSocket(goferEnd) + if err != nil { + syscall.Close(sandboxEnd) + syscall.Close(goferEnd) + return 0, nil, fmt.Errorf("error creating server on FD %d: %v", goferEnd, err) + } + go func() { + at := fsgofer.NewAttachPoint(root, fsgofer.Config{ROMount: true}) + s := p9.NewServer(at) + if err := s.Handle(socket); err != nil { + log.Infof("Gofer is stopping. FD: %d, err: %v\n", goferEnd, err) + } + }() + // Closing the gofer FD will stop the gofer and exit goroutine above. + return sandboxEnd, func() { syscall.Close(goferEnd) }, nil +} + +func createLoader() (*Loader, func(), error) { fd, err := server.CreateSocket(ControlSocketAddr(fmt.Sprintf("%010d", rand.Int())[:10])) if err != nil { - return nil, err + return nil, nil, err } conf := testConfig() spec := testSpec() - return New(spec, conf, fd, nil, false) + + sandEnd, cleanup, err := startGofer(spec.Root.Path) + if err != nil { + return nil, nil, err + } + + l, err := New(spec, conf, fd, []int{sandEnd}, false) + if err != nil { + cleanup() + return nil, nil, err + } + return l, cleanup, nil } // TestRun runs a simple application in a sandbox and checks that it succeeds. func TestRun(t *testing.T) { - s, err := createLoader() + s, cleanup, err := createLoader() if err != nil { t.Fatalf("error creating loader: %v", err) } defer s.Destroy() + defer cleanup() // Start a goroutine to read the start chan result, otherwise Run will // block forever. @@ -106,11 +148,12 @@ func TestRun(t *testing.T) { // TestStartSignal tests that the controller Start message will cause // WaitForStartSignal to return. func TestStartSignal(t *testing.T) { - s, err := createLoader() + s, cleanup, err := createLoader() if err != nil { t.Fatalf("error creating loader: %v", err) } defer s.Destroy() + defer cleanup() // We aren't going to wait on this application, so the control server // needs to be shut down manually. @@ -330,7 +373,14 @@ func TestCreateMountNamespace(t *testing.T) { t.Run(tc.name, func(t *testing.T) { conf := testConfig() ctx := contexttest.Context(t) - mm, err := createMountNamespace(ctx, ctx, &tc.spec, conf, nil) + + sandEnd, cleanup, err := startGofer(tc.spec.Root.Path) + if err != nil { + t.Fatalf("failed to create gofer: %v", err) + } + defer cleanup() + + mm, err := createMountNamespace(ctx, ctx, &tc.spec, conf, []int{sandEnd}) if err != nil { t.Fatalf("createMountNamespace test case %q failed: %v", tc.name, err) } @@ -352,7 +402,6 @@ func TestRestoreEnvironment(t *testing.T) { testCases := []struct { name string spec *specs.Spec - fileAccess FileAccessType ioFDs []int errorExpected bool expectedRenv fs.RestoreEnvironment @@ -375,7 +424,6 @@ func TestRestoreEnvironment(t *testing.T) { }, }, }, - fileAccess: FileAccessProxy, ioFDs: []int{0}, errorExpected: false, expectedRenv: fs.RestoreEnvironment{ @@ -430,7 +478,6 @@ func TestRestoreEnvironment(t *testing.T) { }, }, }, - fileAccess: FileAccessProxy, ioFDs: []int{0, 1}, errorExpected: false, expectedRenv: fs.RestoreEnvironment{ @@ -489,7 +536,6 @@ func TestRestoreEnvironment(t *testing.T) { }, }, }, - fileAccess: FileAccessProxy, ioFDs: []int{0}, errorExpected: false, expectedRenv: fs.RestoreEnvironment{ @@ -534,48 +580,10 @@ func TestRestoreEnvironment(t *testing.T) { }, }, }, - { - name: "whitelist error test", - spec: &specs.Spec{ - Root: &specs.Root{ - Path: os.TempDir(), - Readonly: true, - }, - Mounts: []specs.Mount{ - { - Destination: "/dev/fd-foo", - Type: "bind", - }, - }, - }, - fileAccess: FileAccessDirect, - ioFDs: []int{0, 1}, - errorExpected: true, - }, - { - name: "bad options test", - spec: &specs.Spec{ - Root: &specs.Root{ - Path: os.TempDir(), - Readonly: true, - }, - Mounts: []specs.Mount{ - { - Destination: "/dev/fd-foo", - Type: "tmpfs", - Options: []string{"invalid_option=true"}, - }, - }, - }, - fileAccess: FileAccessDirect, - ioFDs: []int{0}, - errorExpected: true, - }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { conf := testConfig() - conf.FileAccess = tc.fileAccess fds := &fdDispenser{fds: tc.ioFDs} actualRenv, err := createRestoreEnvironment(tc.spec, conf, fds) if !tc.errorExpected && err != nil { diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go index 784baf23b..d8c7b9cd3 100644 --- a/runsc/cmd/boot.go +++ b/runsc/cmd/boot.go @@ -102,13 +102,6 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) conf := args[0].(*boot.Config) waitStatus := args[1].(*syscall.WaitStatus) - // sentry should run with a umask of 0 when --file-access=direct, because we want - // to preserve file modes exactly as set by the sentry, which will have applied - // its own umask. - if conf.FileAccess == boot.FileAccessDirect { - syscall.Umask(0) - } - if b.applyCaps { caps := spec.Process.Capabilities if caps == nil { diff --git a/runsc/container/container.go b/runsc/container/container.go index 5977fbd21..9a05a1dc5 100644 --- a/runsc/container/container.go +++ b/runsc/container/container.go @@ -615,11 +615,6 @@ func (c *Container) waitForStopped() error { } func (c *Container) createGoferProcess(spec *specs.Spec, conf *boot.Config, bundleDir string) ([]*os.File, error) { - if conf.FileAccess == boot.FileAccessDirect { - // Don't start a gofer. The sandbox will access host FS directly. - return nil, nil - } - if err := setupFS(spec, conf, bundleDir); err != nil { return nil, fmt.Errorf("failed to setup mounts: %v", err) } diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go index 9a94347b6..c45eb79a3 100644 --- a/runsc/container/container_test.go +++ b/runsc/container/container_test.go @@ -241,7 +241,7 @@ func configs(opts ...configOption) []*boot.Config { } c.Platform = boot.PlatformKVM case nonExclusiveFS: - c.FileAccess = boot.FileAccessProxy + c.FileAccess = boot.FileAccessShared default: panic(fmt.Sprintf("unknown config option %v", o)) @@ -1368,10 +1368,10 @@ func TestAbbreviatedIDs(t *testing.T) { // Check that modifications to a volume mount are propigated into and out of // the sandbox. func TestContainerVolumeContentsShared(t *testing.T) { - // Only run this test with shared proxy, since that is the only + // Only run this test with shared file access, since that is the only // behavior it is testing. conf := testutil.TestConfig() - conf.FileAccess = boot.FileAccessProxy + conf.FileAccess = boot.FileAccessShared t.Logf("Running test with conf: %+v", conf) // Main process just sleeps. We will use "exec" to probe the state of diff --git a/runsc/main.go b/runsc/main.go index c51b199aa..c30b29b81 100644 --- a/runsc/main.go +++ b/runsc/main.go @@ -58,7 +58,7 @@ var ( // Flags that control sandbox runtime behavior. platform = flag.String("platform", "ptrace", "specifies which platform to use: ptrace (default), kvm") network = flag.String("network", "sandbox", "specifies which network to use: sandbox (default), host, none. Using network inside the sandbox is more secure because it's isolated from the host network.") - fileAccess = flag.String("file-access", "proxy-exclusive", "specifies which filesystem to use: proxy-exclusive (default), proxy-shared, or direct. Using a proxy is more secure because it disallows the sandbox from opening files directly in the host. Setting 'proxy-shared' will disable caches and should be used if external modifications to the filesystem are expected.") + fileAccess = flag.String("file-access", "exclusive", "specifies which filesystem to use: exclusive (default), shared. Setting 'shared' will disable caches and should be used if external modifications to the filesystem are expected.") overlay = flag.Bool("overlay", false, "wrap filesystem mounts with writable overlay. All modifications are stored in memory inside the sandbox.") multiContainer = flag.Bool("multi-container", false, "enable *experimental* multi-container support.") watchdogAction = flag.String("watchdog-action", "log", "sets what action the watchdog takes when triggered: log (default), panic.") @@ -112,8 +112,8 @@ func main() { cmd.Fatalf("%v", err) } - if fsAccess == boot.FileAccessProxy && *overlay { - cmd.Fatalf("overlay flag is incompatible with proxy-shared file access") + if fsAccess == boot.FileAccessShared && *overlay { + cmd.Fatalf("overlay flag is incompatible with shared file access") } netType, err := boot.MakeNetworkType(*network) diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go index f6264d5b2..697210669 100644 --- a/runsc/sandbox/sandbox.go +++ b/runsc/sandbox/sandbox.go @@ -356,12 +356,8 @@ func (s *Sandbox) createSandboxProcess(spec *specs.Spec, conf *boot.Config, bund nss = append(nss, specs.LinuxNamespace{Type: specs.PIDNamespace}) } - if conf.FileAccess == boot.FileAccessDirect { - log.Infof("Sandbox will be started in the current mount namespace") - } else { - log.Infof("Sandbox will be started in new mount namespace") - nss = append(nss, specs.LinuxNamespace{Type: specs.MountNamespace}) - } + log.Infof("Sandbox will be started in new mount namespace") + nss = append(nss, specs.LinuxNamespace{Type: specs.MountNamespace}) // Joins the network namespace if network is enabled. the sandbox talks // directly to the host network, which may have been configured in the @@ -377,9 +373,7 @@ func (s *Sandbox) createSandboxProcess(spec *specs.Spec, conf *boot.Config, bund // User namespace depends on the following options: // - Host network/filesystem: requires to run inside the user namespace // specified in the spec or the current namespace if none is configured. - // - Gofer: when using a Gofer, the sandbox process can run isolated in a - // new user namespace with only the "nobody" user and group. - if conf.Network == boot.NetworkHost || conf.FileAccess == boot.FileAccessDirect { + if conf.Network == boot.NetworkHost { if userns, ok := specutils.GetNS(specs.UserNamespace, spec); ok { log.Infof("Sandbox will be started in container's user namespace: %+v", userns) nss = append(nss, userns) diff --git a/runsc/test/testutil/testutil.go b/runsc/test/testutil/testutil.go index 4f012a8ea..4d354de31 100644 --- a/runsc/test/testutil/testutil.go +++ b/runsc/test/testutil/testutil.go @@ -109,7 +109,7 @@ func TestConfig() *boot.Config { Network: boot.NetworkNone, Strace: true, MultiContainer: true, - FileAccess: boot.FileAccessProxyExclusive, + FileAccess: boot.FileAccessExclusive, TestOnlyAllowRunAsCurrentUserWithoutChroot: true, } } -- cgit v1.2.3