From 8459390cdd81ef1c8180948566e893b06233923c Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Thu, 28 Jun 2018 09:56:23 -0700 Subject: Error out if spec is invalid Closes #66 PiperOrigin-RevId: 202496258 Change-Id: Ib9287c5bf1279ffba1db21ebd9e6b59305cddf34 --- runsc/boot/loader.go | 2 +- runsc/cmd/boot.go | 6 +- runsc/cmd/cmd.go | 2 +- runsc/cmd/gofer.go | 2 +- runsc/container/container.go | 9 ++- runsc/container/container_test.go | 22 -------- runsc/specutils/BUILD | 1 + runsc/specutils/specutils.go | 27 ++++++++- runsc/specutils/specutils_test.go | 113 ++++++++++++++++++++++++++++++++++++++ 9 files changed, 150 insertions(+), 34 deletions(-) (limited to 'runsc') diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index da95fa0e7..f359a0eb0 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -267,7 +267,7 @@ func newProcess(spec *specs.Spec, conf *Config, ioFDs []int, console bool, creds Filename: exec, Argv: spec.Process.Args, Envv: spec.Process.Env, - WorkingDirectory: spec.Process.Cwd, + WorkingDirectory: spec.Process.Cwd, // Defaults to '/' if empty. Credentials: creds, Umask: 0022, Limits: ls, diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go index 0d0e6b63f..685cb6f00 100644 --- a/runsc/cmd/boot.go +++ b/runsc/cmd/boot.go @@ -23,6 +23,7 @@ import ( "context" "flag" "github.com/google/subcommands" + specs "github.com/opencontainers/runtime-spec/specs-go" "gvisor.googlesource.com/gvisor/pkg/log" "gvisor.googlesource.com/gvisor/runsc/boot" "gvisor.googlesource.com/gvisor/runsc/specutils" @@ -116,6 +117,9 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) if b.applyCaps { caps := spec.Process.Capabilities + if caps == nil { + caps = &specs.LinuxCapabilities{} + } if conf.Platform == boot.PlatformPtrace { // Ptrace platform requires extra capabilities. const c = "CAP_SYS_PTRACE" @@ -131,7 +135,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) args = append(args, arg) } } - if err := setCapsAndCallSelf(spec, args, caps); err != nil { + if err := setCapsAndCallSelf(args, caps); err != nil { Fatalf("%v", err) } panic("setCapsAndCallSelf must never return success") diff --git a/runsc/cmd/cmd.go b/runsc/cmd/cmd.go index 940c8cd14..44ebd7165 100644 --- a/runsc/cmd/cmd.go +++ b/runsc/cmd/cmd.go @@ -72,7 +72,7 @@ func (i *intFlags) Set(s string) error { // setCapsAndCallSelf sets capabilities to the current thread and then execve's // itself again with the arguments specified in 'args' to restart the process // with the desired capabilities. -func setCapsAndCallSelf(spec *specs.Spec, args []string, caps *specs.LinuxCapabilities) error { +func setCapsAndCallSelf(args []string, caps *specs.LinuxCapabilities) error { // Keep thread locked while capabilities are changed. runtime.LockOSThread() defer runtime.UnlockOSThread() diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index 8e1060a35..55315c0e8 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -95,7 +95,7 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) // Note: minimal argument handling for the default case to keep it simple. args := os.Args args = append(args, "--apply-caps=false") - if err := setCapsAndCallSelf(spec, args, lc); err != nil { + if err := setCapsAndCallSelf(args, lc); err != nil { Fatalf("Unable to apply caps: %v", err) } panic("unreachable") diff --git a/runsc/container/container.go b/runsc/container/container.go index 428aa5c62..c7dc6ec10 100644 --- a/runsc/container/container.go +++ b/runsc/container/container.go @@ -193,9 +193,6 @@ func Create(id string, spec *specs.Spec, conf *boot.Config, bundleDir, consoleSo if err := validateID(id); err != nil { return nil, err } - if err := specutils.ValidateSpec(spec); err != nil { - return nil, err - } containerRoot := filepath.Join(conf.RootDir, id) if _, err := os.Stat(containerRoot); err == nil { @@ -434,8 +431,10 @@ func (c *Container) Destroy() error { log.Debugf("Destroy container %q", c.ID) // First stop the container. - if err := c.Sandbox.Stop(c.ID); err != nil { - return err + if c.Sandbox != nil { + if err := c.Sandbox.Stop(c.ID); err != nil { + return err + } } // "If any poststop hook fails, the runtime MUST log a warning, but the diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go index de487ea97..11285a123 100644 --- a/runsc/container/container_test.go +++ b/runsc/container/container_test.go @@ -812,28 +812,6 @@ func TestConsoleSocket(t *testing.T) { } } -func TestSpecUnsupported(t *testing.T) { - spec := testutil.NewSpecWithArgs("/bin/true") - spec.Process.SelinuxLabel = "somelabel" - - // These are normally set by docker and will just cause warnings to be logged. - spec.Process.ApparmorProfile = "someprofile" - spec.Linux = &specs.Linux{Seccomp: &specs.LinuxSeccomp{}} - - rootDir, bundleDir, conf, err := testutil.SetupContainer(spec) - if err != nil { - t.Fatalf("error setting up container: %v", err) - } - defer os.RemoveAll(rootDir) - defer os.RemoveAll(bundleDir) - - id := testutil.UniqueContainerID() - _, err = container.Create(id, spec, conf, bundleDir, "", "", "") - if err == nil || !strings.Contains(err.Error(), "is not supported") { - t.Errorf("container.Create() wrong error, got: %v, want: *is not supported, spec.Process: %+v", err, spec.Process) - } -} - // TestRunNonRoot checks that sandbox can be configured when running as // non-privileged user. func TestRunNonRoot(t *testing.T) { diff --git a/runsc/specutils/BUILD b/runsc/specutils/BUILD index 1b6d265bc..34c952bdf 100644 --- a/runsc/specutils/BUILD +++ b/runsc/specutils/BUILD @@ -22,4 +22,5 @@ go_test( size = "small", srcs = ["specutils_test.go"], embed = [":specutils"], + deps = ["@com_github_opencontainers_runtime-spec//specs-go:go_default_library"], ) diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go index c552111f2..0d9e09e9d 100644 --- a/runsc/specutils/specutils.go +++ b/runsc/specutils/specutils.go @@ -47,10 +47,28 @@ func LogSpec(spec *specs.Spec) { // ValidateSpec validates that the spec is compatible with runsc. func ValidateSpec(spec *specs.Spec) error { + // Mandatory fields. if spec.Process == nil { - return fmt.Errorf("Process must be defined") + return fmt.Errorf("Spec.Process must be defined: %+v", spec) } - if spec.Process.SelinuxLabel != "" { + if len(spec.Process.Args) == 0 { + return fmt.Errorf("Spec.Process.Arg must be defined: %+v", spec.Process) + } + if spec.Root == nil { + return fmt.Errorf("Spec.Root must be defined: %+v", spec) + } + if len(spec.Root.Path) == 0 { + return fmt.Errorf("Spec.Root.Path must be defined: %+v", spec.Root) + } + + // Unsupported fields. + if spec.Solaris != nil { + return fmt.Errorf("Spec.Solaris is not supported: %+v", spec) + } + if spec.Windows != nil { + return fmt.Errorf("Spec.Windows is not supported: %+v", spec) + } + if len(spec.Process.SelinuxLabel) != 0 { return fmt.Errorf("SELinux is not supported: %s", spec.Process.SelinuxLabel) } @@ -64,7 +82,7 @@ func ValidateSpec(spec *specs.Spec) error { log.Warningf("Seccomp spec is being ignored") } - // 2 annotations are use by containerd to support multi-container pods. + // Two annotations are use by containerd to support multi-container pods. // "io.kubernetes.cri.container-type" // "io.kubernetes.cri.sandbox-id" containerType, hasContainerType := spec.Annotations[ContainerdContainerTypeAnnotation] @@ -98,6 +116,9 @@ func ReadSpec(bundleDir string) (*specs.Spec, error) { if err := json.Unmarshal(specBytes, &spec); err != nil { return nil, fmt.Errorf("error unmarshaling spec from file %q: %v\n %s", specFile, err, string(specBytes)) } + if err := ValidateSpec(&spec); err != nil { + return nil, err + } return &spec, nil } diff --git a/runsc/specutils/specutils_test.go b/runsc/specutils/specutils_test.go index ef293e608..959be3af3 100644 --- a/runsc/specutils/specutils_test.go +++ b/runsc/specutils/specutils_test.go @@ -20,6 +20,8 @@ import ( "strings" "testing" "time" + + specs "github.com/opencontainers/runtime-spec/specs-go" ) func TestWaitForReadyHappy(t *testing.T) { @@ -94,3 +96,114 @@ func TestWaitForReadyTimeout(t *testing.T) { } cmd.Process.Kill() } + +func TestSpecInvalid(t *testing.T) { + for _, test := range []struct { + name string + spec specs.Spec + error string + }{ + { + name: "valid", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + }, + }, + error: "", + }, + { + name: "valid+warning", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + // This is normally set by docker and will just cause warnings to be logged. + ApparmorProfile: "someprofile", + }, + // This is normally set by docker and will just cause warnings to be logged. + Linux: &specs.Linux{Seccomp: &specs.LinuxSeccomp{}}, + }, + error: "", + }, + { + name: "no root", + spec: specs.Spec{ + Process: &specs.Process{ + Args: []string{"/bin/true"}, + }, + }, + error: "must be defined", + }, + { + name: "empty root", + spec: specs.Spec{ + Root: &specs.Root{}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + }, + }, + error: "must be defined", + }, + { + name: "no process", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + }, + error: "must be defined", + }, + { + name: "empty args", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{}, + }, + error: "must be defined", + }, + { + name: "selinux", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + SelinuxLabel: "somelabel", + }, + }, + error: "is not supported", + }, + { + name: "solaris", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + }, + Solaris: &specs.Solaris{}, + }, + error: "is not supported", + }, + { + name: "windows", + spec: specs.Spec{ + Root: &specs.Root{Path: "/"}, + Process: &specs.Process{ + Args: []string{"/bin/true"}, + }, + Windows: &specs.Windows{}, + }, + error: "is not supported", + }, + } { + err := ValidateSpec(&test.spec) + if len(test.error) == 0 { + if err != nil { + t.Errorf("ValidateSpec(%q) failed, err: %v", test.name, err) + } + } else { + if err == nil || !strings.Contains(err.Error(), test.error) { + t.Errorf("ValidateSpec(%q) wrong error, got: %v, want: .*%s.*", test.name, err, test.error) + } + } + } +} -- cgit v1.2.3