Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add init containers to PSP admission #25830

Merged
merged 2 commits into from
May 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions plugin/pkg/admission/alwayspullimages/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func (a *alwaysPullImages) Admit(attributes admission.Attributes) (err error) {
return apierrors.NewBadRequest("Resource was marked with kind Pod but was unable to be converted")
}

for i := range pod.Spec.InitContainers {
pod.Spec.InitContainers[i].ImagePullPolicy = api.PullAlways
}

for i := range pod.Spec.Containers {
pod.Spec.Containers[i].ImagePullPolicy = api.PullAlways
}
Expand Down
11 changes: 11 additions & 0 deletions plugin/pkg/admission/alwayspullimages/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ func TestAdmission(t *testing.T) {
pod := api.Pod{
ObjectMeta: api.ObjectMeta{Name: "123", Namespace: namespace},
Spec: api.PodSpec{
InitContainers: []api.Container{
{Name: "init1", Image: "image"},
{Name: "init2", Image: "image", ImagePullPolicy: api.PullNever},
{Name: "init3", Image: "image", ImagePullPolicy: api.PullIfNotPresent},
{Name: "init4", Image: "image", ImagePullPolicy: api.PullAlways},
},
Containers: []api.Container{
{Name: "ctr1", Image: "image"},
{Name: "ctr2", Image: "image", ImagePullPolicy: api.PullNever},
Expand All @@ -44,6 +50,11 @@ func TestAdmission(t *testing.T) {
if err != nil {
t.Errorf("Unexpected error returned from admission handler")
}
for _, c := range pod.Spec.InitContainers {
if c.ImagePullPolicy != api.PullAlways {
t.Errorf("Container %v: expected pull always, got %v", c, c.ImagePullPolicy)
}
}
for _, c := range pod.Spec.Containers {
if c.ImagePullPolicy != api.PullAlways {
t.Errorf("Container %v: expected pull always, got %v", c, c.ImagePullPolicy)
Expand Down
8 changes: 8 additions & 0 deletions plugin/pkg/admission/exec/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,14 @@ func (d *denyExec) Admit(a admission.Attributes) (err error) {

// isPrivileged will return true a pod has any privileged containers
func isPrivileged(pod *api.Pod) bool {
for _, c := range pod.Spec.InitContainers {
if c.SecurityContext == nil {
continue
}
if *c.SecurityContext.Privileged {
return true
}
}
for _, c := range pod.Spec.Containers {
if c.SecurityContext == nil {
continue
Expand Down
30 changes: 30 additions & 0 deletions plugin/pkg/admission/exec/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,29 @@ func TestAdmission(t *testing.T) {
for _, tc := range testCases {
testAdmission(t, tc.pod, handler, true)
}

// run against an init container
handler = &denyExec{
Handler: admission.NewHandler(admission.Connect),
hostIPC: true,
hostPID: true,
privileged: true,
}

for _, tc := range testCases {
tc.pod.Spec.InitContainers = tc.pod.Spec.Containers
tc.pod.Spec.Containers = nil
testAdmission(t, tc.pod, handler, tc.shouldAccept)
}

// run with a permissive config and all cases should pass
handler.privileged = false
handler.hostPID = false
handler.hostIPC = false

for _, tc := range testCases {
testAdmission(t, tc.pod, handler, true)
}
}

func testAdmission(t *testing.T, pod *api.Pod, handler *denyExec, shouldAccept bool) {
Expand Down Expand Up @@ -173,6 +196,13 @@ func TestDenyExecOnPrivileged(t *testing.T) {
for _, tc := range testCases {
testAdmission(t, tc.pod, handler, tc.shouldAccept)
}

// test init containers
for _, tc := range testCases {
tc.pod.Spec.InitContainers = tc.pod.Spec.Containers
tc.pod.Spec.Containers = nil
testAdmission(t, tc.pod, handler, tc.shouldAccept)
}
}

func validPod(name string) *api.Pod {
Expand Down
24 changes: 24 additions & 0 deletions plugin/pkg/admission/security/podsecuritypolicy/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ func (c *podSecurityPolicyPlugin) Admit(a admission.Attributes) error {
// the same psp or is not considered valid.
func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.Path) field.ErrorList {
generatedSCs := make([]*api.SecurityContext, len(pod.Spec.Containers))
var generatedInitSCs []*api.SecurityContext

errs := field.ErrorList{}

Expand All @@ -201,6 +202,26 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P
pod.Spec.SecurityContext = psc
errs = append(errs, provider.ValidatePodSecurityContext(pod, field.NewPath("spec", "securityContext"))...)

// Note: this is not changing the original container, we will set container SCs later so long
// as all containers validated under the same PSP.
for i, containerCopy := range pod.Spec.InitContainers {
// We will determine the effective security context for the container and validate against that
// since that is how the sc provider will eventually apply settings in the runtime.
// This results in an SC that is based on the Pod's PSC with the set fields from the container
// overriding pod level settings.
containerCopy.SecurityContext = sc.DetermineEffectiveSecurityContext(pod, &containerCopy)

sc, err := provider.CreateContainerSecurityContext(pod, &containerCopy)
if err != nil {
errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error()))
continue
}
generatedInitSCs = append(generatedInitSCs, sc)

containerCopy.SecurityContext = sc
errs = append(errs, provider.ValidateContainerSecurityContext(pod, &containerCopy, field.NewPath("spec", "initContainers").Index(i).Child("securityContext"))...)
}

// Note: this is not changing the original container, we will set container SCs later so long
// as all containers validated under the same PSP.
for i, containerCopy := range pod.Spec.Containers {
Expand Down Expand Up @@ -229,6 +250,9 @@ func assignSecurityContext(provider psp.Provider, pod *api.Pod, fldPath *field.P

// if we've reached this code then we've generated and validated an SC for every container in the
// pod so let's apply what we generated. Note: the psc is already applied.
for i, sc := range generatedInitSCs {
pod.Spec.InitContainers[i].SecurityContext = sc
}
for i, sc := range generatedSCs {
pod.Spec.Containers[i].SecurityContext = sc
}
Expand Down
34 changes: 34 additions & 0 deletions plugin/pkg/admission/security/podsecuritypolicy/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ func NewTestAdmission(store cache.Store, kclient clientset.Interface) kadmission
}
}

func useInitContainers(pod *kapi.Pod) *kapi.Pod {
pod.Spec.InitContainers = pod.Spec.Containers
pod.Spec.Containers = []kapi.Container{}
return pod
}

func TestAdmitPrivileged(t *testing.T) {
createPodWithPriv := func(priv bool) *kapi.Pod {
pod := goodPod()
Expand Down Expand Up @@ -203,6 +209,18 @@ func TestAdmitCaps(t *testing.T) {
}
}
}

for k, v := range tc {
useInitContainers(v.pod)
testPSPAdmit(k, v.psps, v.pod, v.shouldPass, v.expectedPSP, t)

if v.expectedCapabilities != nil {
if !reflect.DeepEqual(v.expectedCapabilities, v.pod.Spec.InitContainers[0].SecurityContext.Capabilities) {
t.Errorf("%s resulted in caps that were not expected - expected: %v, received: %v", k, v.expectedCapabilities, v.pod.Spec.InitContainers[0].SecurityContext.Capabilities)
}
}
}

}

func TestAdmitVolumes(t *testing.T) {
Expand Down Expand Up @@ -235,6 +253,10 @@ func TestAdmitVolumes(t *testing.T) {
// expect a denial for this PSP
testPSPAdmit(fmt.Sprintf("%s denial", string(fsType)), []*extensions.PodSecurityPolicy{psp}, pod, false, "", t)

// also expect a denial for this PSP if it's an init container
useInitContainers(pod)
testPSPAdmit(fmt.Sprintf("%s denial", string(fsType)), []*extensions.PodSecurityPolicy{psp}, pod, false, "", t)

// now add the fstype directly to the psp and it should validate
psp.Spec.Volumes = []extensions.FSType{fsType}
testPSPAdmit(fmt.Sprintf("%s direct accept", string(fsType)), []*extensions.PodSecurityPolicy{psp}, pod, true, psp.Name, t)
Expand Down Expand Up @@ -304,6 +326,18 @@ func TestAdmitHostNetwork(t *testing.T) {
}
}
}

// test again with init containers
for k, v := range tests {
useInitContainers(v.pod)
testPSPAdmit(k, v.psps, v.pod, v.shouldPass, v.expectedPSP, t)

if v.shouldPass {
if v.pod.Spec.SecurityContext.HostNetwork != v.expectedHostNetwork {
t.Errorf("%s expected hostNetwork to be %t", k, v.expectedHostNetwork)
}
}
}
}

func TestAdmitHostPorts(t *testing.T) {
Expand Down
11 changes: 11 additions & 0 deletions plugin/pkg/admission/securitycontext/scdeny/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,17 @@ func (p *plugin) Admit(a admission.Attributes) (err error) {
return apierrors.NewForbidden(a.GetResource().GroupResource(), pod.Name, fmt.Errorf("SecurityContext.FSGroup is forbidden"))
}

for _, v := range pod.Spec.InitContainers {
if v.SecurityContext != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not extract a method here denyContainerSecurityContext?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do it here and technically save lines but it's not worth much.

if v.SecurityContext.SELinuxOptions != nil {
return apierrors.NewForbidden(a.GetResource().GroupResource(), pod.Name, fmt.Errorf("SecurityContext.SELinuxOptions is forbidden"))
}
if v.SecurityContext.RunAsUser != nil {
return apierrors.NewForbidden(a.GetResource().GroupResource(), pod.Name, fmt.Errorf("SecurityContext.RunAsUser is forbidden"))
}
}
}

for _, v := range pod.Spec.Containers {
if v.SecurityContext != nil {
if v.SecurityContext.SELinuxOptions != nil {
Expand Down
22 changes: 18 additions & 4 deletions plugin/pkg/admission/securitycontext/scdeny/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,25 @@ func TestAdmission(t *testing.T) {
}

for _, tc := range cases {
pod := pod()
pod.Spec.SecurityContext = tc.podSc
pod.Spec.Containers[0].SecurityContext = tc.sc
p := pod()
p.Spec.SecurityContext = tc.podSc
p.Spec.Containers[0].SecurityContext = tc.sc

err := handler.Admit(admission.NewAttributesRecord(pod, api.Kind("Pod").WithVersion("version"), "foo", "name", api.Resource("pods").WithVersion("version"), "", "ignored", nil))
err := handler.Admit(admission.NewAttributesRecord(p, api.Kind("Pod").WithVersion("version"), "foo", "name", api.Resource("pods").WithVersion("version"), "", "ignored", nil))
if err != nil && !tc.expectError {
t.Errorf("%v: unexpected error: %v", tc.name, err)
} else if err == nil && tc.expectError {
t.Errorf("%v: expected error", tc.name)
}

// verify init containers are also checked
p = pod()
p.Spec.SecurityContext = tc.podSc
p.Spec.Containers[0].SecurityContext = tc.sc
p.Spec.InitContainers = p.Spec.Containers
p.Spec.Containers = nil

err = handler.Admit(admission.NewAttributesRecord(p, api.Kind("Pod").WithVersion("version"), "foo", "name", api.Resource("pods").WithVersion("version"), "", "ignored", nil))
if err != nil && !tc.expectError {
t.Errorf("%v: unexpected error: %v", tc.name, err)
} else if err == nil && tc.expectError {
Expand Down
24 changes: 24 additions & 0 deletions plugin/pkg/admission/serviceaccount/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,16 @@ func (s *serviceAccount) limitSecretReferences(serviceAccount *api.ServiceAccoun
}
}

for _, container := range pod.Spec.InitContainers {
for _, env := range container.Env {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extract method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message would be different

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, agree it's not worth it here.

if env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil {
if !mountableSecrets.Has(env.ValueFrom.SecretKeyRef.Name) {
return fmt.Errorf("Init container %s with envVar %s referencing secret.secretName=\"%s\" is not allowed because service account %s does not reference that secret", container.Name, env.Name, env.ValueFrom.SecretKeyRef.Name, serviceAccount.Name)
}
}
}
}

for _, container := range pod.Spec.Containers {
for _, env := range container.Env {
if env.ValueFrom != nil && env.ValueFrom.SecretKeyRef != nil {
Expand Down Expand Up @@ -394,6 +404,20 @@ func (s *serviceAccount) mountServiceAccountToken(serviceAccount *api.ServiceAcc

// Ensure every container mounts the APISecret volume
needsTokenVolume := false
for i, container := range pod.Spec.InitContainers {
existingContainerMount := false
for _, volumeMount := range container.VolumeMounts {
// Existing mounts at the default mount path prevent mounting of the API token
if volumeMount.MountPath == DefaultAPITokenMountPath {
existingContainerMount = true
break
}
}
if !existingContainerMount {
pod.Spec.InitContainers[i].VolumeMounts = append(pod.Spec.InitContainers[i].VolumeMounts, volumeMount)
needsTokenVolume = true
}
}
for i, container := range pod.Spec.Containers {
existingContainerMount := false
for _, volumeMount := range container.VolumeMounts {
Expand Down