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 <github@gone.nl>

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
+	SecurityOptions
 	HostnamePath   string
 	HostsPath      string
 	ShmPath        string
 	ResolvConfPath string
-	SeccompProfile  string
-	NoNewPrivileges bool
 
 	// 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
 }

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 {

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)
 	}
 

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
 }
 

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)
 	}
 }
 

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
 }
 

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,
 					},

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,
 				},