Skip to content

Commit f4d81e2

Browse files
php-coderphp-coder
committed
SecurityContextConstraints: avoid unnecessary securitycontext mutation.
1 parent d07223b commit f4d81e2

File tree

2 files changed

+65
-119
lines changed

2 files changed

+65
-119
lines changed

‎pkg/security/securitycontextconstraints/provider.go

+56-81
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import (
1313
api "k8s.io/kubernetes/pkg/apis/core"
1414
"k8s.io/kubernetes/pkg/apis/extensions"
1515
"k8s.io/kubernetes/pkg/security/podsecuritypolicy/sysctl"
16+
"k8s.io/kubernetes/pkg/securitycontext"
17+
"k8s.io/kubernetes/pkg/util/maps"
1618

1719
securityapi "github.com/openshift/origin/pkg/security/apis/security"
1820
)
@@ -103,49 +105,33 @@ func NewSimpleProvider(scc *securityapi.SecurityContextConstraints) (SecurityCon
103105
// Create a PodSecurityContext based on the given constraints. If a setting is already set
104106
// on the PodSecurityContext it will not be changed. Validate should be used after the context
105107
// is created to ensure it complies with the required restrictions.
106-
//
107-
// NOTE: this method works on a copy of the PodSecurityContext. It is up to the caller to
108-
// apply the PSC if validation passes.
109108
func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurityContext, map[string]string, error) {
110-
var sc *api.PodSecurityContext = nil
111-
if pod.Spec.SecurityContext != nil {
112-
// work with a copy
113-
copy := *pod.Spec.SecurityContext
114-
sc = &copy
115-
} else {
116-
sc = &api.PodSecurityContext{}
117-
}
118-
119-
var annotationsCopy map[string]string = nil
120-
if pod.Annotations != nil {
121-
annotationsCopy = make(map[string]string, len(pod.Annotations))
122-
for k, v := range pod.Annotations {
123-
annotationsCopy[k] = v
124-
}
125-
}
109+
sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext)
110+
111+
annotationsCopy := maps.CopySS(pod.Annotations)
126112

127-
if sc.SupplementalGroups == nil {
113+
if sc.SupplementalGroups() == nil {
128114
supGroups, err := s.supplementalGroupStrategy.Generate(pod)
129115
if err != nil {
130116
return nil, nil, err
131117
}
132-
sc.SupplementalGroups = supGroups
118+
sc.SetSupplementalGroups(supGroups)
133119
}
134120

135-
if sc.FSGroup == nil {
121+
if sc.FSGroup() == nil {
136122
fsGroup, err := s.fsGroupStrategy.GenerateSingle(pod)
137123
if err != nil {
138124
return nil, nil, err
139125
}
140-
sc.FSGroup = fsGroup
126+
sc.SetFSGroup(fsGroup)
141127
}
142128

143-
if sc.SELinuxOptions == nil {
129+
if sc.SELinuxOptions() == nil {
144130
seLinux, err := s.seLinuxStrategy.Generate(pod, nil)
145131
if err != nil {
146132
return nil, nil, err
147133
}
148-
sc.SELinuxOptions = seLinux
134+
sc.SetSELinuxOptions(seLinux)
149135
}
150136

151137
// we only generate a seccomp annotation for the entire pod. Validation
@@ -166,93 +152,83 @@ func (s *simpleProvider) CreatePodSecurityContext(pod *api.Pod) (*api.PodSecurit
166152
}
167153
}
168154

169-
return sc, annotationsCopy, nil
155+
return sc.PodSecurityContext(), annotationsCopy, nil
170156
}
171157

172158
// Create a SecurityContext based on the given constraints. If a setting is already set on the
173159
// container's security context then it will not be changed. Validation should be used after
174160
// the context is created to ensure it complies with the required restrictions.
175-
//
176-
// NOTE: this method works on a copy of the SC of the container. It is up to the caller to apply
177-
// the SC if validation passes.
178161
func (s *simpleProvider) CreateContainerSecurityContext(pod *api.Pod, container *api.Container) (*api.SecurityContext, error) {
179-
var sc *api.SecurityContext = nil
180-
if container.SecurityContext != nil {
181-
// work with a copy of the original
182-
copy := *container.SecurityContext
183-
sc = &copy
184-
} else {
185-
sc = &api.SecurityContext{}
186-
}
187-
if sc.RunAsUser == nil {
162+
sc := securitycontext.NewEffectiveContainerSecurityContextMutator(
163+
securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext),
164+
securitycontext.NewContainerSecurityContextMutator(container.SecurityContext),
165+
)
166+
if sc.RunAsUser() == nil {
188167
uid, err := s.runAsUserStrategy.Generate(pod, container)
189168
if err != nil {
190169
return nil, err
191170
}
192-
sc.RunAsUser = uid
171+
sc.SetRunAsUser(uid)
193172
}
194173

195-
if sc.SELinuxOptions == nil {
174+
if sc.SELinuxOptions() == nil {
196175
seLinux, err := s.seLinuxStrategy.Generate(pod, container)
197176
if err != nil {
198177
return nil, err
199178
}
200-
sc.SELinuxOptions = seLinux
179+
sc.SetSELinuxOptions(seLinux)
201180
}
202181

203182
// if we're using the non-root strategy set the marker that this container should not be
204183
// run as root which will signal to the kubelet to do a final check either on the runAsUser
205184
// or, if runAsUser is not set, the image
206-
if sc.RunAsNonRoot == nil && sc.RunAsUser == nil && s.scc.RunAsUser.Type == securityapi.RunAsUserStrategyMustRunAsNonRoot {
185+
if sc.RunAsNonRoot() == nil && sc.RunAsUser() == nil && s.scc.RunAsUser.Type == securityapi.RunAsUserStrategyMustRunAsNonRoot {
207186
nonRoot := true
208-
sc.RunAsNonRoot = &nonRoot
187+
sc.SetRunAsNonRoot(&nonRoot)
209188
}
210189

211190
caps, err := s.capabilitiesStrategy.Generate(pod, container)
212191
if err != nil {
213192
return nil, err
214193
}
215-
sc.Capabilities = caps
194+
sc.SetCapabilities(caps)
216195

217196
// if the SCC requires a read only root filesystem and the container has not made a specific
218197
// request then default ReadOnlyRootFilesystem to true.
219-
if s.scc.ReadOnlyRootFilesystem && sc.ReadOnlyRootFilesystem == nil {
198+
if s.scc.ReadOnlyRootFilesystem && sc.ReadOnlyRootFilesystem() == nil {
220199
readOnlyRootFS := true
221-
sc.ReadOnlyRootFilesystem = &readOnlyRootFS
200+
sc.SetReadOnlyRootFilesystem(&readOnlyRootFS)
222201
}
223202

224-
return sc, nil
203+
return sc.ContainerSecurityContext(), nil
225204
}
226205

227206
// Ensure a pod's SecurityContext is in compliance with the given constraints.
228207
func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field.Path) field.ErrorList {
229208
allErrs := field.ErrorList{}
230209

231-
if pod.Spec.SecurityContext == nil {
232-
allErrs = append(allErrs, field.Invalid(fldPath.Child("securityContext"), pod.Spec.SecurityContext, "No security context is set"))
233-
return allErrs
234-
}
210+
sc := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext)
235211

236212
fsGroups := []int64{}
237-
if pod.Spec.SecurityContext.FSGroup != nil {
238-
fsGroups = append(fsGroups, *pod.Spec.SecurityContext.FSGroup)
213+
if fsGroup := sc.FSGroup(); fsGroup != nil {
214+
fsGroups = append(fsGroups, *fsGroup)
239215
}
240216
allErrs = append(allErrs, s.fsGroupStrategy.Validate(pod, fsGroups)...)
241-
allErrs = append(allErrs, s.supplementalGroupStrategy.Validate(pod, pod.Spec.SecurityContext.SupplementalGroups)...)
217+
allErrs = append(allErrs, s.supplementalGroupStrategy.Validate(pod, sc.SupplementalGroups())...)
242218
allErrs = append(allErrs, s.seccompStrategy.ValidatePod(pod)...)
243219

244-
allErrs = append(allErrs, s.seLinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, nil, pod.Spec.SecurityContext.SELinuxOptions)...)
220+
allErrs = append(allErrs, s.seLinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, nil, sc.SELinuxOptions())...)
245221

246-
if !s.scc.AllowHostNetwork && pod.Spec.SecurityContext.HostNetwork {
247-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), pod.Spec.SecurityContext.HostNetwork, "Host network is not allowed to be used"))
222+
if !s.scc.AllowHostNetwork && sc.HostNetwork() {
223+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), sc.HostNetwork(), "Host network is not allowed to be used"))
248224
}
249225

250-
if !s.scc.AllowHostPID && pod.Spec.SecurityContext.HostPID {
251-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), pod.Spec.SecurityContext.HostPID, "Host PID is not allowed to be used"))
226+
if !s.scc.AllowHostPID && sc.HostPID() {
227+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), sc.HostPID(), "Host PID is not allowed to be used"))
252228
}
253229

254-
if !s.scc.AllowHostIPC && pod.Spec.SecurityContext.HostIPC {
255-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), pod.Spec.SecurityContext.HostIPC, "Host IPC is not allowed to be used"))
230+
if !s.scc.AllowHostIPC && sc.HostIPC() {
231+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), sc.HostIPC(), "Host IPC is not allowed to be used"))
256232
}
257233

258234
allErrs = append(allErrs, s.sysctlsStrategy.Validate(pod)...)
@@ -303,24 +279,22 @@ func (s *simpleProvider) ValidatePodSecurityContext(pod *api.Pod, fldPath *field
303279
func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, container *api.Container, fldPath *field.Path) field.ErrorList {
304280
allErrs := field.ErrorList{}
305281

306-
if container.SecurityContext == nil {
307-
allErrs = append(allErrs, field.Invalid(fldPath.Child("securityContext"), container.SecurityContext, "No security context is set"))
308-
return allErrs
309-
}
282+
podSC := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext)
283+
sc := securitycontext.NewEffectiveContainerSecurityContextAccessor(podSC, securitycontext.NewContainerSecurityContextMutator(container.SecurityContext))
310284

311-
sc := container.SecurityContext
312-
allErrs = append(allErrs, s.runAsUserStrategy.Validate(fldPath.Child("securityContext"), pod, container, sc.RunAsNonRoot, sc.RunAsUser)...)
313-
allErrs = append(allErrs, s.seLinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions)...)
285+
allErrs = append(allErrs, s.runAsUserStrategy.Validate(fldPath.Child("securityContext"), pod, container, sc.RunAsNonRoot(), sc.RunAsUser())...)
286+
allErrs = append(allErrs, s.seLinuxStrategy.Validate(fldPath.Child("seLinuxOptions"), pod, container, sc.SELinuxOptions())...)
314287
allErrs = append(allErrs, s.seccompStrategy.ValidateContainer(pod, container)...)
315288

316-
if !s.scc.AllowPrivilegedContainer && sc.Privileged != nil && *sc.Privileged {
317-
allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *sc.Privileged, "Privileged containers are not allowed"))
289+
privileged := sc.Privileged()
290+
if !s.scc.AllowPrivilegedContainer && privileged != nil && *privileged {
291+
allErrs = append(allErrs, field.Invalid(fldPath.Child("privileged"), *privileged, "Privileged containers are not allowed"))
318292
}
319293

320-
allErrs = append(allErrs, s.capabilitiesStrategy.Validate(pod, container, sc.Capabilities)...)
294+
allErrs = append(allErrs, s.capabilitiesStrategy.Validate(pod, container, sc.Capabilities())...)
321295

322-
if !s.scc.AllowHostNetwork && pod.Spec.SecurityContext.HostNetwork {
323-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), pod.Spec.SecurityContext.HostNetwork, "Host network is not allowed to be used"))
296+
if !s.scc.AllowHostNetwork && podSC.HostNetwork() {
297+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostNetwork"), podSC.HostNetwork(), "Host network is not allowed to be used"))
324298
}
325299

326300
if !s.scc.AllowHostPorts {
@@ -337,19 +311,20 @@ func (s *simpleProvider) ValidateContainerSecurityContext(pod *api.Pod, containe
337311
}
338312
}
339313

340-
if !s.scc.AllowHostPID && pod.Spec.SecurityContext.HostPID {
341-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), pod.Spec.SecurityContext.HostPID, "Host PID is not allowed to be used"))
314+
if !s.scc.AllowHostPID && podSC.HostPID() {
315+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostPID"), podSC.HostPID(), "Host PID is not allowed to be used"))
342316
}
343317

344-
if !s.scc.AllowHostIPC && pod.Spec.SecurityContext.HostIPC {
345-
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), pod.Spec.SecurityContext.HostIPC, "Host IPC is not allowed to be used"))
318+
if !s.scc.AllowHostIPC && podSC.HostIPC() {
319+
allErrs = append(allErrs, field.Invalid(fldPath.Child("hostIPC"), podSC.HostIPC(), "Host IPC is not allowed to be used"))
346320
}
347321

348322
if s.scc.ReadOnlyRootFilesystem {
349-
if sc.ReadOnlyRootFilesystem == nil {
350-
allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), sc.ReadOnlyRootFilesystem, "ReadOnlyRootFilesystem may not be nil and must be set to true"))
351-
} else if !*sc.ReadOnlyRootFilesystem {
352-
allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), *sc.ReadOnlyRootFilesystem, "ReadOnlyRootFilesystem must be set to true"))
323+
readOnly := sc.ReadOnlyRootFilesystem()
324+
if readOnly == nil {
325+
allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), readOnly, "ReadOnlyRootFilesystem may not be nil and must be set to true"))
326+
} else if !*readOnly {
327+
allErrs = append(allErrs, field.Invalid(fldPath.Child("readOnlyRootFilesystem"), *readOnly, "ReadOnlyRootFilesystem must be set to true"))
353328
}
354329
}
355330

‎pkg/security/securitycontextconstraints/provider_test.go

+9-38
Original file line numberDiff line numberDiff line change
@@ -32,27 +32,18 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) {
3232
ObjectMeta: metav1.ObjectMeta{
3333
Name: "scc-sa",
3434
},
35-
SeccompProfiles: []string{"foo"},
36-
DefaultAddCapabilities: []api.Capability{"foo"},
37-
RequiredDropCapabilities: []api.Capability{"bar"},
35+
SeccompProfiles: []string{"foo"},
3836
RunAsUser: securityapi.RunAsUserStrategyOptions{
3937
Type: securityapi.RunAsUserStrategyRunAsAny,
4038
},
4139
SELinuxContext: securityapi.SELinuxContextStrategyOptions{
4240
Type: securityapi.SELinuxStrategyRunAsAny,
4341
},
44-
// these are pod mutating strategies that are tested above
4542
FSGroup: securityapi.FSGroupStrategyOptions{
46-
Type: securityapi.FSGroupStrategyMustRunAs,
47-
Ranges: []securityapi.IDRange{
48-
{Min: 1, Max: 1},
49-
},
43+
Type: securityapi.FSGroupStrategyRunAsAny,
5044
},
5145
SupplementalGroups: securityapi.SupplementalGroupsStrategyOptions{
52-
Type: securityapi.SupplementalGroupsStrategyMustRunAs,
53-
Ranges: []securityapi.IDRange{
54-
{Min: 1, Max: 1},
55-
},
46+
Type: securityapi.SupplementalGroupsStrategyRunAsAny,
5647
},
5748
}
5849
}
@@ -64,21 +55,13 @@ func TestCreatePodSecurityContextNonmutating(t *testing.T) {
6455
if err != nil {
6556
t.Fatalf("unable to create provider %v", err)
6657
}
67-
sc, annotations, err := provider.CreatePodSecurityContext(pod)
58+
_, _, err = provider.CreatePodSecurityContext(pod)
6859
if err != nil {
6960
t.Fatalf("unable to create psc %v", err)
7061
}
7162

72-
// The generated security context should have filled in missing options, so they should differ
73-
if reflect.DeepEqual(sc, &pod.Spec.SecurityContext) {
74-
t.Error("expected created security context to be different than container's, but they were identical")
75-
}
76-
77-
if reflect.DeepEqual(annotations, pod.Annotations) {
78-
t.Error("expected created annotations to be different than container's, but they were identical")
79-
}
80-
8163
// Creating the provider or the security context should not have mutated the scc or pod
64+
// since all the strategies were permissive
8265
if !reflect.DeepEqual(createPod(), pod) {
8366
diff := diff.ObjectDiff(createPod(), pod)
8467
t.Errorf("pod was mutated by CreatePodSecurityContext. diff:\n%s", diff)
@@ -102,30 +85,22 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
10285

10386
// Create an SCC with strategies that will populate a blank security context
10487
createSCC := func() *securityapi.SecurityContextConstraints {
105-
var uid int64 = 1
10688
return &securityapi.SecurityContextConstraints{
10789
ObjectMeta: metav1.ObjectMeta{
10890
Name: "scc-sa",
10991
},
110-
DefaultAddCapabilities: []api.Capability{"foo"},
111-
RequiredDropCapabilities: []api.Capability{"bar"},
11292
RunAsUser: securityapi.RunAsUserStrategyOptions{
113-
Type: securityapi.RunAsUserStrategyMustRunAs,
114-
UID: &uid,
93+
Type: securityapi.RunAsUserStrategyRunAsAny,
11594
},
11695
SELinuxContext: securityapi.SELinuxContextStrategyOptions{
117-
Type: securityapi.SELinuxStrategyMustRunAs,
118-
SELinuxOptions: &api.SELinuxOptions{User: "you"},
96+
Type: securityapi.SELinuxStrategyRunAsAny,
11997
},
120-
// these are pod mutating strategies that are tested above
12198
FSGroup: securityapi.FSGroupStrategyOptions{
12299
Type: securityapi.FSGroupStrategyRunAsAny,
123100
},
124101
SupplementalGroups: securityapi.SupplementalGroupsStrategyOptions{
125102
Type: securityapi.SupplementalGroupsStrategyRunAsAny,
126103
},
127-
// mutates the container SC by defaulting to true if container sets nil
128-
ReadOnlyRootFilesystem: true,
129104
}
130105
}
131106

@@ -136,17 +111,13 @@ func TestCreateContainerSecurityContextNonmutating(t *testing.T) {
136111
if err != nil {
137112
t.Fatalf("unable to create provider %v", err)
138113
}
139-
sc, err := provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0])
114+
_, err = provider.CreateContainerSecurityContext(pod, &pod.Spec.Containers[0])
140115
if err != nil {
141116
t.Fatalf("unable to create container security context %v", err)
142117
}
143118

144-
// The generated security context should have filled in missing options, so they should differ
145-
if reflect.DeepEqual(sc, &pod.Spec.Containers[0].SecurityContext) {
146-
t.Error("expected created security context to be different than container's, but they were identical")
147-
}
148-
149119
// Creating the provider or the security context should not have mutated the scc or pod
120+
// since all the strategies were permissive
150121
if !reflect.DeepEqual(createPod(), pod) {
151122
diff := diff.ObjectDiff(createPod(), pod)
152123
t.Errorf("pod was mutated by CreateContainerSecurityContext. diff:\n%s", diff)

0 commit comments

Comments
 (0)