From 3eebf4d1622a0d8fc60f30694ee99d2130db1f4b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 14 Apr 2023 09:27:20 +0200 Subject: [PATCH] container: split security options to a SecurityOptions struct - Split these options to a separate struct, so that we can handle them in isolation. - Change some tests to use subtests, and improve coverage Signed-off-by: Sebastiaan van Stijn --- container/container.go | 25 ++++--- daemon/container.go | 2 +- daemon/container_linux.go | 2 +- daemon/daemon_unix.go | 18 ++--- daemon/daemon_unix_test.go | 134 ++++++++++++++++++++--------------- daemon/daemon_windows.go | 2 +- daemon/exec_linux_test.go | 2 +- daemon/seccomp_linux_test.go | 24 +++---- 8 files changed, 118 insertions(+), 91 deletions(-) diff --git a/container/container.go b/container/container.go index 53e0cfe963..e04696bd65 100644 --- a/container/container.go +++ b/container/container.go @@ -79,9 +79,7 @@ type Container struct { Name string Driver string OS string - // MountLabel contains the options for the 'mount' command - MountLabel string - ProcessLabel string + RestartCount int HasBeenStartedBefore bool HasBeenManuallyStopped bool // used for unless-stopped restart policy @@ -99,13 +97,11 @@ type Container struct { attachContext *attachContext // Fields here are specific to Unix platforms - AppArmorProfile string - HostnamePath string - HostsPath string - ShmPath string - ResolvConfPath string - SeccompProfile string - NoNewPrivileges bool + SecurityOptions + HostnamePath string + HostsPath string + ShmPath string + ResolvConfPath string // Fields here are specific to Windows NetworkSharedContainerID string `json:"-"` @@ -113,6 +109,15 @@ type Container struct { LocalLogCacheMeta localLogCacheMeta `json:",omitempty"` } +type SecurityOptions struct { + // MountLabel contains the options for the "mount" command. + MountLabel string + ProcessLabel string + AppArmorProfile string + SeccompProfile string + NoNewPrivileges bool +} + type localLogCacheMeta struct { HaveNotifyEnabled bool } diff --git a/daemon/container.go b/daemon/container.go index 38a2d72764..0fa9475593 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -209,7 +209,7 @@ func (daemon *Daemon) generateHostname(id string, config *containertypes.Config) func (daemon *Daemon) setSecurityOptions(container *container.Container, hostConfig *containertypes.HostConfig) error { container.Lock() defer container.Unlock() - return daemon.parseSecurityOpt(container, hostConfig) + return daemon.parseSecurityOpt(&container.SecurityOptions, hostConfig) } func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig) error { diff --git a/daemon/container_linux.go b/daemon/container_linux.go index 61a6d0ba6a..2a0b928aea 100644 --- a/daemon/container_linux.go +++ b/daemon/container_linux.go @@ -15,7 +15,7 @@ func (daemon *Daemon) saveAppArmorConfig(container *container.Container) error { return nil // if apparmor is disabled there is nothing to do here. } - if err := parseSecurityOpt(container, container.HostConfig); err != nil { + if err := parseSecurityOpt(&container.SecurityOptions, container.HostConfig); err != nil { return errdefs.InvalidParameter(err) } diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index 3a1e46add7..4b504c871e 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -190,12 +190,12 @@ func getBlkioWeightDevices(config containertypes.Resources) ([]specs.LinuxWeight return blkioWeightDevices, nil } -func (daemon *Daemon) parseSecurityOpt(container *container.Container, hostConfig *containertypes.HostConfig) error { - container.NoNewPrivileges = daemon.configStore.NoNewPrivileges - return parseSecurityOpt(container, hostConfig) +func (daemon *Daemon) parseSecurityOpt(securityOptions *container.SecurityOptions, hostConfig *containertypes.HostConfig) error { + securityOptions.NoNewPrivileges = daemon.configStore.NoNewPrivileges + return parseSecurityOpt(securityOptions, hostConfig) } -func parseSecurityOpt(container *container.Container, config *containertypes.HostConfig) error { +func parseSecurityOpt(securityOptions *container.SecurityOptions, config *containertypes.HostConfig) error { var ( labelOpts []string err error @@ -203,7 +203,7 @@ func parseSecurityOpt(container *container.Container, config *containertypes.Hos for _, opt := range config.SecurityOpt { if opt == "no-new-privileges" { - container.NoNewPrivileges = true + securityOptions.NoNewPrivileges = true continue } if opt == "disable" { @@ -227,21 +227,21 @@ func parseSecurityOpt(container *container.Container, config *containertypes.Hos case "label": labelOpts = append(labelOpts, v) case "apparmor": - container.AppArmorProfile = v + securityOptions.AppArmorProfile = v case "seccomp": - container.SeccompProfile = v + securityOptions.SeccompProfile = v case "no-new-privileges": noNewPrivileges, err := strconv.ParseBool(v) if err != nil { return fmt.Errorf("invalid --security-opt 2: %q", opt) } - container.NoNewPrivileges = noNewPrivileges + securityOptions.NoNewPrivileges = noNewPrivileges default: return fmt.Errorf("invalid --security-opt 2: %q", opt) } } - container.ProcessLabel, container.MountLabel, err = label.InitLabels(labelOpts) + securityOptions.ProcessLabel, securityOptions.MountLabel, err = label.InitLabels(labelOpts) return err } diff --git a/daemon/daemon_unix_test.go b/daemon/daemon_unix_test.go index 8c7202c49a..f44c0be48f 100644 --- a/daemon/daemon_unix_test.go +++ b/daemon/daemon_unix_test.go @@ -14,6 +14,7 @@ import ( "github.com/docker/docker/container" "github.com/docker/docker/daemon/config" "github.com/docker/docker/pkg/sysinfo" + "github.com/opencontainers/selinux/go-selinux" "golang.org/x/sys/unix" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" @@ -138,115 +139,136 @@ func TestAdjustCPUSharesNoAdjustment(t *testing.T) { // Unix test as uses settings which are not available on Windows func TestParseSecurityOptWithDeprecatedColon(t *testing.T) { - ctr := &container.Container{} + opts := &container.SecurityOptions{} cfg := &containertypes.HostConfig{} // test apparmor cfg.SecurityOpt = []string{"apparmor=test_profile"} - if err := parseSecurityOpt(ctr, cfg); err != nil { + if err := parseSecurityOpt(opts, cfg); err != nil { t.Fatalf("Unexpected parseSecurityOpt error: %v", err) } - if ctr.AppArmorProfile != "test_profile" { - t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", ctr.AppArmorProfile) + if opts.AppArmorProfile != "test_profile" { + t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", opts.AppArmorProfile) } // test seccomp sp := "/path/to/seccomp_test.json" cfg.SecurityOpt = []string{"seccomp=" + sp} - if err := parseSecurityOpt(ctr, cfg); err != nil { + if err := parseSecurityOpt(opts, cfg); err != nil { t.Fatalf("Unexpected parseSecurityOpt error: %v", err) } - if ctr.SeccompProfile != sp { - t.Fatalf("Unexpected AppArmorProfile, expected: %q, got %q", sp, ctr.SeccompProfile) + if opts.SeccompProfile != sp { + t.Fatalf("Unexpected AppArmorProfile, expected: %q, got %q", sp, opts.SeccompProfile) } // test valid label cfg.SecurityOpt = []string{"label=user:USER"} - if err := parseSecurityOpt(ctr, cfg); err != nil { + if err := parseSecurityOpt(opts, cfg); err != nil { t.Fatalf("Unexpected parseSecurityOpt error: %v", err) } // test invalid label cfg.SecurityOpt = []string{"label"} - if err := parseSecurityOpt(ctr, cfg); err == nil { + if err := parseSecurityOpt(opts, cfg); err == nil { t.Fatal("Expected parseSecurityOpt error, got nil") } // test invalid opt cfg.SecurityOpt = []string{"test"} - if err := parseSecurityOpt(ctr, cfg); err == nil { + if err := parseSecurityOpt(opts, cfg); err == nil { t.Fatal("Expected parseSecurityOpt error, got nil") } } func TestParseSecurityOpt(t *testing.T) { - ctr := &container.Container{} - cfg := &containertypes.HostConfig{} - - // test apparmor - cfg.SecurityOpt = []string{"apparmor=test_profile"} - if err := parseSecurityOpt(ctr, cfg); err != nil { - t.Fatalf("Unexpected parseSecurityOpt error: %v", err) - } - if ctr.AppArmorProfile != "test_profile" { - t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", ctr.AppArmorProfile) - } - - // test seccomp - sp := "/path/to/seccomp_test.json" - cfg.SecurityOpt = []string{"seccomp=" + sp} - if err := parseSecurityOpt(ctr, cfg); err != nil { - t.Fatalf("Unexpected parseSecurityOpt error: %v", err) - } - if ctr.SeccompProfile != sp { - t.Fatalf("Unexpected SeccompProfile, expected: %q, got %q", sp, ctr.SeccompProfile) - } - - // test valid label - cfg.SecurityOpt = []string{"label=user:USER"} - if err := parseSecurityOpt(ctr, cfg); err != nil { - t.Fatalf("Unexpected parseSecurityOpt error: %v", err) - } - - // test invalid label - cfg.SecurityOpt = []string{"label"} - if err := parseSecurityOpt(ctr, cfg); err == nil { - t.Fatal("Expected parseSecurityOpt error, got nil") - } - - // test invalid opt - cfg.SecurityOpt = []string{"test"} - if err := parseSecurityOpt(ctr, cfg); err == nil { - t.Fatal("Expected parseSecurityOpt error, got nil") - } + t.Run("apparmor", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"apparmor=test_profile"}, + }) + assert.Check(t, err) + assert.Equal(t, secOpts.AppArmorProfile, "test_profile") + }) + t.Run("apparmor using legacy separator", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"apparmor:test_profile"}, + }) + assert.Check(t, err) + assert.Equal(t, secOpts.AppArmorProfile, "test_profile") + }) + t.Run("seccomp", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"seccomp=/path/to/seccomp_test.json"}, + }) + assert.Check(t, err) + assert.Equal(t, secOpts.SeccompProfile, "/path/to/seccomp_test.json") + }) + t.Run("valid label", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"label=user:USER"}, + }) + assert.Check(t, err) + if selinux.GetEnabled() { + // TODO(thaJeztah): set expected labels here (or "partial" if depends on host) + // assert.Check(t, is.Equal(secOpts.MountLabel, "")) + // assert.Check(t, is.Equal(secOpts.ProcessLabel, "")) + } else { + assert.Check(t, is.Equal(secOpts.MountLabel, "")) + assert.Check(t, is.Equal(secOpts.ProcessLabel, "")) + } + }) + t.Run("invalid label", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"label"}, + }) + assert.Error(t, err, `invalid --security-opt 1: "label"`) + }) + t.Run("invalid option (no value)", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"unknown"}, + }) + assert.Error(t, err, `invalid --security-opt 1: "unknown"`) + }) + t.Run("unknown option", func(t *testing.T) { + secOpts := &container.SecurityOptions{} + err := parseSecurityOpt(secOpts, &containertypes.HostConfig{ + SecurityOpt: []string{"unknown=something"}, + }) + assert.Error(t, err, `invalid --security-opt 2: "unknown=something"`) + }) } func TestParseNNPSecurityOptions(t *testing.T) { daemon := &Daemon{ configStore: &config.Config{NoNewPrivileges: true}, } - ctr := &container.Container{} + opts := &container.SecurityOptions{} cfg := &containertypes.HostConfig{} // test NNP when "daemon:true" and "no-new-privileges=false"" cfg.SecurityOpt = []string{"no-new-privileges=false"} - if err := daemon.parseSecurityOpt(ctr, cfg); err != nil { + if err := daemon.parseSecurityOpt(opts, cfg); err != nil { t.Fatalf("Unexpected daemon.parseSecurityOpt error: %v", err) } - if ctr.NoNewPrivileges { - t.Fatalf("container.NoNewPrivileges should be FALSE: %v", ctr.NoNewPrivileges) + if opts.NoNewPrivileges { + t.Fatalf("container.NoNewPrivileges should be FALSE: %v", opts.NoNewPrivileges) } // test NNP when "daemon:false" and "no-new-privileges=true"" daemon.configStore.NoNewPrivileges = false cfg.SecurityOpt = []string{"no-new-privileges=true"} - if err := daemon.parseSecurityOpt(ctr, cfg); err != nil { + if err := daemon.parseSecurityOpt(opts, cfg); err != nil { t.Fatalf("Unexpected daemon.parseSecurityOpt error: %v", err) } - if !ctr.NoNewPrivileges { - t.Fatalf("container.NoNewPrivileges should be TRUE: %v", ctr.NoNewPrivileges) + if !opts.NoNewPrivileges { + t.Fatalf("container.NoNewPrivileges should be TRUE: %v", opts.NoNewPrivileges) } } diff --git a/daemon/daemon_windows.go b/daemon/daemon_windows.go index f63900e56c..8cbcc20132 100644 --- a/daemon/daemon_windows.go +++ b/daemon/daemon_windows.go @@ -55,7 +55,7 @@ func getPluginExecRoot(cfg *config.Config) string { return filepath.Join(cfg.Root, "plugins") } -func (daemon *Daemon) parseSecurityOpt(container *container.Container, hostConfig *containertypes.HostConfig) error { +func (daemon *Daemon) parseSecurityOpt(securityOptions *container.SecurityOptions, hostConfig *containertypes.HostConfig) error { return nil } diff --git a/daemon/exec_linux_test.go b/daemon/exec_linux_test.go index 17df7e16ad..1375eb788b 100644 --- a/daemon/exec_linux_test.go +++ b/daemon/exec_linux_test.go @@ -74,7 +74,7 @@ func TestExecSetPlatformOptAppArmor(t *testing.T) { } t.Run(doc, func(t *testing.T) { c := &container.Container{ - AppArmorProfile: tc.appArmorProfile, + SecurityOptions: container.SecurityOptions{AppArmorProfile: tc.appArmorProfile}, HostConfig: &containertypes.HostConfig{ Privileged: tc.privileged, }, diff --git a/daemon/seccomp_linux_test.go b/daemon/seccomp_linux_test.go index 6b5d517b99..63c2201924 100644 --- a/daemon/seccomp_linux_test.go +++ b/daemon/seccomp_linux_test.go @@ -31,7 +31,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: true}, }, c: &container.Container{ - SeccompProfile: dconfig.SeccompProfileUnconfined, + SecurityOptions: container.SecurityOptions{SeccompProfile: dconfig.SeccompProfileUnconfined}, HostConfig: &containertypes.HostConfig{ Privileged: false, }, @@ -45,7 +45,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: true}, }, c: &container.Container{ - SeccompProfile: "{ \"defaultAction\": \"SCMP_ACT_LOG\" }", + SecurityOptions: container.SecurityOptions{SeccompProfile: `{"defaultAction": "SCMP_ACT_LOG"}`}, HostConfig: &containertypes.HostConfig{ Privileged: true, }, @@ -59,7 +59,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: true}, }, c: &container.Container{ - SeccompProfile: "", + SecurityOptions: container.SecurityOptions{SeccompProfile: ""}, HostConfig: &containertypes.HostConfig{ Privileged: true, }, @@ -71,10 +71,10 @@ func TestWithSeccomp(t *testing.T) { comment: "privileged container w/ daemon profile runs unconfined", daemon: &Daemon{ sysInfo: &sysinfo.SysInfo{Seccomp: true}, - seccompProfile: []byte("{ \"defaultAction\": \"SCMP_ACT_ERRNO\" }"), + seccompProfile: []byte(`{"defaultAction": "SCMP_ACT_ERRNO"}`), }, c: &container.Container{ - SeccompProfile: "", + SecurityOptions: container.SecurityOptions{SeccompProfile: ""}, HostConfig: &containertypes.HostConfig{ Privileged: true, }, @@ -88,7 +88,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: false}, }, c: &container.Container{ - SeccompProfile: "{ \"defaultAction\": \"SCMP_ACT_ERRNO\" }", + SecurityOptions: container.SecurityOptions{SeccompProfile: `{"defaultAction": "SCMP_ACT_ERRNO"}`}, HostConfig: &containertypes.HostConfig{ Privileged: false, }, @@ -103,7 +103,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: true}, }, c: &container.Container{ - SeccompProfile: "", + SecurityOptions: container.SecurityOptions{SeccompProfile: ""}, HostConfig: &containertypes.HostConfig{ Privileged: false, }, @@ -122,7 +122,7 @@ func TestWithSeccomp(t *testing.T) { sysInfo: &sysinfo.SysInfo{Seccomp: true}, }, c: &container.Container{ - SeccompProfile: "{ \"defaultAction\": \"SCMP_ACT_ERRNO\" }", + SecurityOptions: container.SecurityOptions{SeccompProfile: `{"defaultAction": "SCMP_ACT_ERRNO"}`}, HostConfig: &containertypes.HostConfig{ Privileged: false, }, @@ -141,10 +141,10 @@ func TestWithSeccomp(t *testing.T) { comment: "load daemon's profile", daemon: &Daemon{ sysInfo: &sysinfo.SysInfo{Seccomp: true}, - seccompProfile: []byte("{ \"defaultAction\": \"SCMP_ACT_ERRNO\" }"), + seccompProfile: []byte(`{"defaultAction": "SCMP_ACT_ERRNO"}`), }, c: &container.Container{ - SeccompProfile: "", + SecurityOptions: container.SecurityOptions{SeccompProfile: ""}, HostConfig: &containertypes.HostConfig{ Privileged: false, }, @@ -163,10 +163,10 @@ func TestWithSeccomp(t *testing.T) { comment: "load prioritise container profile over daemon's", daemon: &Daemon{ sysInfo: &sysinfo.SysInfo{Seccomp: true}, - seccompProfile: []byte("{ \"defaultAction\": \"SCMP_ACT_ERRNO\" }"), + seccompProfile: []byte(`{"defaultAction": "SCMP_ACT_ERRNO"}`), }, c: &container.Container{ - SeccompProfile: "{ \"defaultAction\": \"SCMP_ACT_LOG\" }", + SecurityOptions: container.SecurityOptions{SeccompProfile: `{"defaultAction": "SCMP_ACT_LOG"}`}, HostConfig: &containertypes.HostConfig{ Privileged: false, },