Skip to content

Commit 3eebf4d

Browse files
thaJeztahthaJeztah
committed
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 <[email protected]>
1 parent e22758b commit 3eebf4d

8 files changed

Lines changed: 118 additions & 91 deletions

File tree

‎container/container.go‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,7 @@ type Container struct {
7979
Name string
8080
Driver string
8181
OS string
82-
// MountLabel contains the options for the 'mount' command
83-
MountLabel string
84-
ProcessLabel string
82+
8583
RestartCount int
8684
HasBeenStartedBefore bool
8785
HasBeenManuallyStopped bool // used for unless-stopped restart policy
@@ -99,20 +97,27 @@ type Container struct {
9997
attachContext *attachContext
10098

10199
// Fields here are specific to Unix platforms
102-
AppArmorProfile string
103-
HostnamePath string
104-
HostsPath string
105-
ShmPath string
106-
ResolvConfPath string
107-
SeccompProfile string
108-
NoNewPrivileges bool
100+
SecurityOptions
101+
HostnamePath string
102+
HostsPath string
103+
ShmPath string
104+
ResolvConfPath string
109105

110106
// Fields here are specific to Windows
111107
NetworkSharedContainerID string `json:"-"`
112108
SharedEndpointList []string `json:"-"`
113109
LocalLogCacheMeta localLogCacheMeta `json:",omitempty"`
114110
}
115111

112+
type SecurityOptions struct {
113+
// MountLabel contains the options for the "mount" command.
114+
MountLabel string
115+
ProcessLabel string
116+
AppArmorProfile string
117+
SeccompProfile string
118+
NoNewPrivileges bool
119+
}
120+
116121
type localLogCacheMeta struct {
117122
HaveNotifyEnabled bool
118123
}

‎daemon/container.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ func (daemon *Daemon) generateHostname(id string, config *containertypes.Config)
209209
func (daemon *Daemon) setSecurityOptions(container *container.Container, hostConfig *containertypes.HostConfig) error {
210210
container.Lock()
211211
defer container.Unlock()
212-
return daemon.parseSecurityOpt(container, hostConfig)
212+
return daemon.parseSecurityOpt(&container.SecurityOptions, hostConfig)
213213
}
214214

215215
func (daemon *Daemon) setHostConfig(container *container.Container, hostConfig *containertypes.HostConfig) error {

‎daemon/container_linux.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ func (daemon *Daemon) saveAppArmorConfig(container *container.Container) error {
1515
return nil // if apparmor is disabled there is nothing to do here.
1616
}
1717

18-
if err := parseSecurityOpt(container, container.HostConfig); err != nil {
18+
if err := parseSecurityOpt(&container.SecurityOptions, container.HostConfig); err != nil {
1919
return errdefs.InvalidParameter(err)
2020
}
2121

‎daemon/daemon_unix.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -190,20 +190,20 @@ func getBlkioWeightDevices(config containertypes.Resources) ([]specs.LinuxWeight
190190
return blkioWeightDevices, nil
191191
}
192192

193-
func (daemon *Daemon) parseSecurityOpt(container *container.Container, hostConfig *containertypes.HostConfig) error {
194-
container.NoNewPrivileges = daemon.configStore.NoNewPrivileges
195-
return parseSecurityOpt(container, hostConfig)
193+
func (daemon *Daemon) parseSecurityOpt(securityOptions *container.SecurityOptions, hostConfig *containertypes.HostConfig) error {
194+
securityOptions.NoNewPrivileges = daemon.configStore.NoNewPrivileges
195+
return parseSecurityOpt(securityOptions, hostConfig)
196196
}
197197

198-
func parseSecurityOpt(container *container.Container, config *containertypes.HostConfig) error {
198+
func parseSecurityOpt(securityOptions *container.SecurityOptions, config *containertypes.HostConfig) error {
199199
var (
200200
labelOpts []string
201201
err error
202202
)
203203

204204
for _, opt := range config.SecurityOpt {
205205
if opt == "no-new-privileges" {
206-
container.NoNewPrivileges = true
206+
securityOptions.NoNewPrivileges = true
207207
continue
208208
}
209209
if opt == "disable" {
@@ -227,21 +227,21 @@ func parseSecurityOpt(container *container.Container, config *containertypes.Hos
227227
case "label":
228228
labelOpts = append(labelOpts, v)
229229
case "apparmor":
230-
container.AppArmorProfile = v
230+
securityOptions.AppArmorProfile = v
231231
case "seccomp":
232-
container.SeccompProfile = v
232+
securityOptions.SeccompProfile = v
233233
case "no-new-privileges":
234234
noNewPrivileges, err := strconv.ParseBool(v)
235235
if err != nil {
236236
return fmt.Errorf("invalid --security-opt 2: %q", opt)
237237
}
238-
container.NoNewPrivileges = noNewPrivileges
238+
securityOptions.NoNewPrivileges = noNewPrivileges
239239
default:
240240
return fmt.Errorf("invalid --security-opt 2: %q", opt)
241241
}
242242
}
243243

244-
container.ProcessLabel, container.MountLabel, err = label.InitLabels(labelOpts)
244+
securityOptions.ProcessLabel, securityOptions.MountLabel, err = label.InitLabels(labelOpts)
245245
return err
246246
}
247247

‎daemon/daemon_unix_test.go‎

Lines changed: 78 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/docker/docker/container"
1515
"github.com/docker/docker/daemon/config"
1616
"github.com/docker/docker/pkg/sysinfo"
17+
"github.com/opencontainers/selinux/go-selinux"
1718
"golang.org/x/sys/unix"
1819
"gotest.tools/v3/assert"
1920
is "gotest.tools/v3/assert/cmp"
@@ -138,115 +139,136 @@ func TestAdjustCPUSharesNoAdjustment(t *testing.T) {
138139

139140
// Unix test as uses settings which are not available on Windows
140141
func TestParseSecurityOptWithDeprecatedColon(t *testing.T) {
141-
ctr := &container.Container{}
142+
opts := &container.SecurityOptions{}
142143
cfg := &containertypes.HostConfig{}
143144

144145
// test apparmor
145146
cfg.SecurityOpt = []string{"apparmor=test_profile"}
146-
if err := parseSecurityOpt(ctr, cfg); err != nil {
147+
if err := parseSecurityOpt(opts, cfg); err != nil {
147148
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
148149
}
149-
if ctr.AppArmorProfile != "test_profile" {
150-
t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", ctr.AppArmorProfile)
150+
if opts.AppArmorProfile != "test_profile" {
151+
t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", opts.AppArmorProfile)
151152
}
152153

153154
// test seccomp
154155
sp := "/path/to/seccomp_test.json"
155156
cfg.SecurityOpt = []string{"seccomp=" + sp}
156-
if err := parseSecurityOpt(ctr, cfg); err != nil {
157+
if err := parseSecurityOpt(opts, cfg); err != nil {
157158
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
158159
}
159-
if ctr.SeccompProfile != sp {
160-
t.Fatalf("Unexpected AppArmorProfile, expected: %q, got %q", sp, ctr.SeccompProfile)
160+
if opts.SeccompProfile != sp {
161+
t.Fatalf("Unexpected AppArmorProfile, expected: %q, got %q", sp, opts.SeccompProfile)
161162
}
162163

163164
// test valid label
164165
cfg.SecurityOpt = []string{"label=user:USER"}
165-
if err := parseSecurityOpt(ctr, cfg); err != nil {
166+
if err := parseSecurityOpt(opts, cfg); err != nil {
166167
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
167168
}
168169

169170
// test invalid label
170171
cfg.SecurityOpt = []string{"label"}
171-
if err := parseSecurityOpt(ctr, cfg); err == nil {
172+
if err := parseSecurityOpt(opts, cfg); err == nil {
172173
t.Fatal("Expected parseSecurityOpt error, got nil")
173174
}
174175

175176
// test invalid opt
176177
cfg.SecurityOpt = []string{"test"}
177-
if err := parseSecurityOpt(ctr, cfg); err == nil {
178+
if err := parseSecurityOpt(opts, cfg); err == nil {
178179
t.Fatal("Expected parseSecurityOpt error, got nil")
179180
}
180181
}
181182

182183
func TestParseSecurityOpt(t *testing.T) {
183-
ctr := &container.Container{}
184-
cfg := &containertypes.HostConfig{}
185-
186-
// test apparmor
187-
cfg.SecurityOpt = []string{"apparmor=test_profile"}
188-
if err := parseSecurityOpt(ctr, cfg); err != nil {
189-
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
190-
}
191-
if ctr.AppArmorProfile != "test_profile" {
192-
t.Fatalf("Unexpected AppArmorProfile, expected: \"test_profile\", got %q", ctr.AppArmorProfile)
193-
}
194-
195-
// test seccomp
196-
sp := "/path/to/seccomp_test.json"
197-
cfg.SecurityOpt = []string{"seccomp=" + sp}
198-
if err := parseSecurityOpt(ctr, cfg); err != nil {
199-
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
200-
}
201-
if ctr.SeccompProfile != sp {
202-
t.Fatalf("Unexpected SeccompProfile, expected: %q, got %q", sp, ctr.SeccompProfile)
203-
}
204-
205-
// test valid label
206-
cfg.SecurityOpt = []string{"label=user:USER"}
207-
if err := parseSecurityOpt(ctr, cfg); err != nil {
208-
t.Fatalf("Unexpected parseSecurityOpt error: %v", err)
209-
}
210-
211-
// test invalid label
212-
cfg.SecurityOpt = []string{"label"}
213-
if err := parseSecurityOpt(ctr, cfg); err == nil {
214-
t.Fatal("Expected parseSecurityOpt error, got nil")
215-
}
216-
217-
// test invalid opt
218-
cfg.SecurityOpt = []string{"test"}
219-
if err := parseSecurityOpt(ctr, cfg); err == nil {
220-
t.Fatal("Expected parseSecurityOpt error, got nil")
221-
}
184+
t.Run("apparmor", func(t *testing.T) {
185+
secOpts := &container.SecurityOptions{}
186+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
187+
SecurityOpt: []string{"apparmor=test_profile"},
188+
})
189+
assert.Check(t, err)
190+
assert.Equal(t, secOpts.AppArmorProfile, "test_profile")
191+
})
192+
t.Run("apparmor using legacy separator", func(t *testing.T) {
193+
secOpts := &container.SecurityOptions{}
194+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
195+
SecurityOpt: []string{"apparmor:test_profile"},
196+
})
197+
assert.Check(t, err)
198+
assert.Equal(t, secOpts.AppArmorProfile, "test_profile")
199+
})
200+
t.Run("seccomp", func(t *testing.T) {
201+
secOpts := &container.SecurityOptions{}
202+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
203+
SecurityOpt: []string{"seccomp=/path/to/seccomp_test.json"},
204+
})
205+
assert.Check(t, err)
206+
assert.Equal(t, secOpts.SeccompProfile, "/path/to/seccomp_test.json")
207+
})
208+
t.Run("valid label", func(t *testing.T) {
209+
secOpts := &container.SecurityOptions{}
210+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
211+
SecurityOpt: []string{"label=user:USER"},
212+
})
213+
assert.Check(t, err)
214+
if selinux.GetEnabled() {
215+
// TODO(thaJeztah): set expected labels here (or "partial" if depends on host)
216+
// assert.Check(t, is.Equal(secOpts.MountLabel, ""))
217+
// assert.Check(t, is.Equal(secOpts.ProcessLabel, ""))
218+
} else {
219+
assert.Check(t, is.Equal(secOpts.MountLabel, ""))
220+
assert.Check(t, is.Equal(secOpts.ProcessLabel, ""))
221+
}
222+
})
223+
t.Run("invalid label", func(t *testing.T) {
224+
secOpts := &container.SecurityOptions{}
225+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
226+
SecurityOpt: []string{"label"},
227+
})
228+
assert.Error(t, err, `invalid --security-opt 1: "label"`)
229+
})
230+
t.Run("invalid option (no value)", func(t *testing.T) {
231+
secOpts := &container.SecurityOptions{}
232+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
233+
SecurityOpt: []string{"unknown"},
234+
})
235+
assert.Error(t, err, `invalid --security-opt 1: "unknown"`)
236+
})
237+
t.Run("unknown option", func(t *testing.T) {
238+
secOpts := &container.SecurityOptions{}
239+
err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
240+
SecurityOpt: []string{"unknown=something"},
241+
})
242+
assert.Error(t, err, `invalid --security-opt 2: "unknown=something"`)
243+
})
222244
}
223245

224246
func TestParseNNPSecurityOptions(t *testing.T) {
225247
daemon := &Daemon{
226248
configStore: &config.Config{NoNewPrivileges: true},
227249
}
228-
ctr := &container.Container{}
250+
opts := &container.SecurityOptions{}
229251
cfg := &containertypes.HostConfig{}
230252

231253
// test NNP when "daemon:true" and "no-new-privileges=false""
232254
cfg.SecurityOpt = []string{"no-new-privileges=false"}
233255

234-
if err := daemon.parseSecurityOpt(ctr, cfg); err != nil {
256+
if err := daemon.parseSecurityOpt(opts, cfg); err != nil {
235257
t.Fatalf("Unexpected daemon.parseSecurityOpt error: %v", err)
236258
}
237-
if ctr.NoNewPrivileges {
238-
t.Fatalf("container.NoNewPrivileges should be FALSE: %v", ctr.NoNewPrivileges)
259+
if opts.NoNewPrivileges {
260+
t.Fatalf("container.NoNewPrivileges should be FALSE: %v", opts.NoNewPrivileges)
239261
}
240262

241263
// test NNP when "daemon:false" and "no-new-privileges=true""
242264
daemon.configStore.NoNewPrivileges = false
243265
cfg.SecurityOpt = []string{"no-new-privileges=true"}
244266

245-
if err := daemon.parseSecurityOpt(ctr, cfg); err != nil {
267+
if err := daemon.parseSecurityOpt(opts, cfg); err != nil {
246268
t.Fatalf("Unexpected daemon.parseSecurityOpt error: %v", err)
247269
}
248-
if !ctr.NoNewPrivileges {
249-
t.Fatalf("container.NoNewPrivileges should be TRUE: %v", ctr.NoNewPrivileges)
270+
if !opts.NoNewPrivileges {
271+
t.Fatalf("container.NoNewPrivileges should be TRUE: %v", opts.NoNewPrivileges)
250272
}
251273
}
252274

‎daemon/daemon_windows.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func getPluginExecRoot(cfg *config.Config) string {
5555
return filepath.Join(cfg.Root, "plugins")
5656
}
5757

58-
func (daemon *Daemon) parseSecurityOpt(container *container.Container, hostConfig *containertypes.HostConfig) error {
58+
func (daemon *Daemon) parseSecurityOpt(securityOptions *container.SecurityOptions, hostConfig *containertypes.HostConfig) error {
5959
return nil
6060
}
6161

‎daemon/exec_linux_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func TestExecSetPlatformOptAppArmor(t *testing.T) {
7474
}
7575
t.Run(doc, func(t *testing.T) {
7676
c := &container.Container{
77-
AppArmorProfile: tc.appArmorProfile,
77+
SecurityOptions: container.SecurityOptions{AppArmorProfile: tc.appArmorProfile},
7878
HostConfig: &containertypes.HostConfig{
7979
Privileged: tc.privileged,
8080
},

0 commit comments

Comments
 (0)