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.

Sign up for GitHub

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

Jump to bottom

Allow setting securityContext in ephemeral containers #99023

Merged
merged 3 commits into from Jul 10, 2021
Merged

Allow setting securityContext in ephemeral containers #99023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70765fa
Allow securityContext in EphemeralContainers
verb Feb 12, 2021
aff49ca
Generated code for securityContext in EphemeralContainers
verb Feb 12, 2021
babebf7
Apply PSP container tests to EphemeralContainers
verb Jun 25, 2021
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
2 changes: 1 addition & 1 deletion api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/api/pod/util_test.go
View file
Open in desktop
Expand Up @@ -1359,12 +1359,12 @@ func TestDropEphemeralContainers(t *testing.T) {
pod func() *api.Pod
}{
{
description: "has subpaths",
description: "has ephemeral containers",
hasEphemeralContainers: true,
pod: podWithEphemeralContainers,
},
{
description: "does not have subpaths",
description: "does not have ephemeral containers",
hasEphemeralContainers: false,
pod: podWithoutEphemeralContainers,
},
Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/core/types.go
View file
Open in desktop
Expand Up @@ -3140,7 +3140,8 @@ type EphemeralContainerCommon struct {
TerminationMessagePolicy TerminationMessagePolicy
// Required: Policy for pulling images for this container
ImagePullPolicy PullPolicy
// SecurityContext is not allowed for ephemeral containers.
// Optional: SecurityContext defines the security options the ephemeral container should be run with.
// If set, the fields of SecurityContext override the equivalent fields of PodSecurityContext.
// +optional
SecurityContext *SecurityContext

Expand Down
1 change: 1 addition & 0 deletions pkg/apis/core/validation/validation.go
View file
Open in desktop
Expand Up @@ -85,6 +85,7 @@ var allowedEphemeralContainerFields = map[string]bool{
"TerminationMessagePath": true,
"TerminationMessagePolicy": true,
"ImagePullPolicy": true,
"SecurityContext": true,
Copy link
Member

Choose a reason for hiding this comment

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

is there any coverage of this field for PodSecurity and PodSecurityPolicy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For PodSecurityPolicy I added tests to podsecuritypolicy/provider_test.go to run the Container tests on EphemeralContainer as well.

I wasn't following the PSP replacement and didn't know about PodSecurity until you mentioned it. I'm happy to add test coverage there as well once I bring myself up to speed. If the PSP coverage looks sufficient, how do you feel about addressing PodSecurity in a follow-up PR since it's still in alpha?

Copy link
Member

Choose a reason for hiding this comment

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

as long as there's double opt-in and we have test coverage for the interaction before either this feature or PodSecurity graduates from alpha, that's ok

Copy link
Member

Choose a reason for hiding this comment

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

including test coverage for ephemeral containers is in the PodSecurity KEP for alpha → beta graduation

fwiw, running with PodSecurity enabled, I just verified the PodSecurity enforce level already applies to ephemeral containers as well:

FEATURE_GATES=PodSecurity=true,EphemeralContainers=true hack/local-up-cluster.sh 

kubectl label ns default pod-security.kubernetes.io/enforce=restricted

kubectl apply -f ~/snippets/pods/restricted_pod.json 

kubectl debug restricted -it --image=busybox

Defaulting debug container name to debugger-86nkb.
Error from server (Forbidden): allowPrivilegeEscalation != false (container "debugger-86nkb" must set securityContext.allowPrivilegeEscalation=false), unrestricted capabilities (container "debugger-86nkb" must set securityContext.capabilities.drop=["ALL"]), runAsNonRoot != true (pod or container "debugger-86nkb" must set securityContext.runAsNonRoot=true), seccompProfile (pod or container "debugger-86nkb" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")

"Stdin": true,
"StdinOnce": true,
"TTY": true,
Expand Down
23 changes: 14 additions & 9 deletions pkg/apis/core/validation/validation_test.go
View file
Open in desktop
Expand Up @@ -5822,7 +5822,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
TargetContainerName: "ctr",
},
},
"All Whitelisted Fields": {
"All allowed Fields": {
{
EphemeralContainerCommon: core.EphemeralContainerCommon{

Expand All @@ -5848,9 +5848,14 @@ func TestValidateEphemeralContainers(t *testing.T) {
TerminationMessagePath: "/dev/termination-log",
verb marked this conversation as resolved.
Show resolved Hide resolved
TerminationMessagePolicy: "File",
ImagePullPolicy: "IfNotPresent",
Stdin: true,
StdinOnce: true,
TTY: true,
SecurityContext: &core.SecurityContext{
Capabilities: &core.Capabilities{
Add: []core.Capability{"SYS_ADMIN"},
},
},
Stdin: true,
StdinOnce: true,
TTY: true,
},
},
},
Expand Down Expand Up @@ -5923,7 +5928,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
field.Error{Type: field.ErrorTypeNotFound, Field: "ephemeralContainers[0].targetContainerName"},
},
{
"Container uses non-whitelisted field: Lifecycle",
"Container uses disallowed field: Lifecycle",
[]core.EphemeralContainer{
{
EphemeralContainerCommon: core.EphemeralContainerCommon{
Expand All @@ -5942,7 +5947,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].lifecycle"},
},
{
"Container uses non-whitelisted field: LivenessProbe",
"Container uses disallowed field: LivenessProbe",
[]core.EphemeralContainer{
{
EphemeralContainerCommon: core.EphemeralContainerCommon{
Expand All @@ -5962,7 +5967,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].livenessProbe"},
},
{
"Container uses non-whitelisted field: Ports",
"Container uses disallowed field: Ports",
[]core.EphemeralContainer{
{
EphemeralContainerCommon: core.EphemeralContainerCommon{
Expand All @@ -5979,7 +5984,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].ports"},
},
{
"Container uses non-whitelisted field: ReadinessProbe",
"Container uses disallowed field: ReadinessProbe",
[]core.EphemeralContainer{
{
EphemeralContainerCommon: core.EphemeralContainerCommon{
Expand All @@ -5998,7 +6003,7 @@ func TestValidateEphemeralContainers(t *testing.T) {
field.Error{Type: field.ErrorTypeForbidden, Field: "ephemeralContainers[0].readinessProbe"},
},
{
"Container uses non-whitelisted field: Resources",
"Container uses disallowed field: Resources",
[]core.EphemeralContainer{
{
EphemeralContainerCommon: core.EphemeralContainerCommon{
Expand Down
50 changes: 50 additions & 0 deletions pkg/security/podsecuritypolicy/provider_test.go
View file
Open in desktop
Expand Up @@ -104,6 +104,8 @@ func TestMutatePodNonmutating(t *testing.T) {
}

func TestMutateContainerNonmutating(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)()

untrue := false
tests := []struct {
security *api.SecurityContext
Expand All @@ -120,6 +122,11 @@ func TestMutateContainerNonmutating(t *testing.T) {
Containers: []api.Container{{
SecurityContext: tc.security,
}},
EphemeralContainers: []api.EphemeralContainer{{
EphemeralContainerCommon: api.EphemeralContainerCommon{
SecurityContext: tc.security,
},
}},
},
}
}
Expand Down Expand Up @@ -546,6 +553,8 @@ func allowFlexVolumesPSP(allowAllFlexVolumes, allowAllVolumes bool) *policy.PodS
}

func TestValidateContainerFailures(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)()

// fail user strategy
failUserPSP := defaultPSP()
uid := int64(999)
Expand Down Expand Up @@ -689,6 +698,13 @@ func TestValidateContainerFailures(t *testing.T) {
errs := provider.ValidatePod(test.pod)
require.NotEmpty(t, errs, "expected validation failure but did not receive errors")
assert.Contains(t, errs[0].Error(), test.expectedError, "unexpected error")

// We want EphemeralContainers to behave the same as regular containers, so move the
// containers to ephemeralContainers and validate again.
ecPod := moveContainersToEphemeral(test.pod)
errs = provider.ValidatePod(ecPod)
require.NotEmpty(t, errs, "expected validation failure for ephemeral containers but did not receive errors")
assert.Contains(t, errs[0].Error(), test.expectedError, "unexpected error")
})
}
}
Expand Down Expand Up @@ -1062,6 +1078,8 @@ func TestValidatePodSuccess(t *testing.T) {
}

func TestValidateContainerSuccess(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)()

// success user strategy
userPSP := defaultPSP()
uid := int64(999)
Expand Down Expand Up @@ -1221,6 +1239,12 @@ func TestValidateContainerSuccess(t *testing.T) {
require.NoError(t, err, "unable to create provider")
errs := provider.ValidatePod(test.pod)
assert.Empty(t, errs, "expected validation pass but received errors")

// We want EphemeralContainers to behave the same as regular containers, so move the
// containers to ephemeralContainers and validate again.
ecPod := moveContainersToEphemeral(test.pod)
errs = provider.ValidatePod(ecPod)
assert.Empty(t, errs, "expected validation pass for ephemeral containers but received errors")
})
}
}
Expand Down Expand Up @@ -1377,6 +1401,17 @@ func defaultV1Pod() *v1.Pod {
}
}

func moveContainersToEphemeral(in *api.Pod) *api.Pod {
out := in.DeepCopy()
for _, c := range out.Spec.Containers {
out.Spec.EphemeralContainers = append(out.Spec.EphemeralContainers, api.EphemeralContainer{
EphemeralContainerCommon: api.EphemeralContainerCommon(c),
})
}
out.Spec.Containers = nil
return out
}

// TestValidateAllowedVolumes will test that for every field of VolumeSource we can create
// a pod with that type of volume and deny it, accept it explicitly, or accept it with
// the FSTypeAll wildcard.
Expand Down Expand Up @@ -1490,6 +1525,8 @@ func TestValidateProjectedVolume(t *testing.T) {
}

func TestAllowPrivilegeEscalation(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)()

ptr := pointer.BoolPtr
tests := []struct {
pspAPE bool // PSP AllowPrivilegeEscalation
Expand Down Expand Up @@ -1528,6 +1565,7 @@ func TestAllowPrivilegeEscalation(t *testing.T) {
t.Run(fmt.Sprintf("pspAPE:%t_pspDAPE:%s_podAPE:%s", test.pspAPE, fmtPtr(test.pspDAPE), fmtPtr(test.podAPE)), func(t *testing.T) {
pod := defaultPod()
pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation = test.podAPE
ecPod := moveContainersToEphemeral(pod)

psp := defaultPSP()
psp.Spec.AllowPrivilegeEscalation = &test.pspAPE
Expand All @@ -1547,6 +1585,18 @@ func TestAllowPrivilegeEscalation(t *testing.T) {
ape := pod.Spec.Containers[0].SecurityContext.AllowPrivilegeEscalation
assert.Equal(t, test.expectAPE, ape, "expected pod AllowPrivilegeEscalation")
}

err = provider.MutatePod(ecPod)
require.NoError(t, err)

errs = provider.ValidatePod(ecPod)
if test.expectErr {
assert.NotEmpty(t, errs, "expected validation error for ephemeral containers")
} else {
assert.Empty(t, errs, "expected no validation errors for ephemeral containers")
ape := ecPod.Spec.EphemeralContainers[0].SecurityContext.AllowPrivilegeEscalation
assert.Equal(t, test.expectAPE, ape, "expected pod AllowPrivilegeEscalation for ephemeral container")
}
})
}
}
Expand Down
3 changes: 2 additions & 1 deletion staging/src/k8s.io/api/core/v1/generated.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion staging/src/k8s.io/api/core/v1/types.go
View file
Open in desktop
Expand Up @@ -3525,7 +3525,8 @@ type EphemeralContainerCommon struct {
// More info: https://kubernetes.io/docs/concepts/containers/images#updating-images
// +optional
ImagePullPolicy PullPolicy `json:"imagePullPolicy,omitempty" protobuf:"bytes,14,opt,name=imagePullPolicy,casttype=PullPolicy"`
// SecurityContext is not allowed for ephemeral containers.
// Optional: SecurityContext defines the security options the ephemeral container should be run with.
// If set, the fields of SecurityContext override the equivalent fields of PodSecurityContext.
// +optional
SecurityContext *SecurityContext `json:"securityContext,omitempty" protobuf:"bytes,15,opt,name=securityContext"`

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.