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>
runpod · Apr 28, 2023 · 3eebf4d · 3eebf4d
1 parent e22758b
commit 3eebf4d
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 91 deletions.
diff --git 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,20 +97,27 @@ 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:"-"`
 	SharedEndpointList       []string          `json:"-"`
 	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
@@ -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
@@ -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
@@ -190,20 +190,20 @@ 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
 	)
 
 	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
@@ -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
@@ -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
@@ -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,
 					},