From 70765fa24d569f5bd65f31b171cf899842adfc5e Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Fri, 12 Feb 2021 10:44:54 +0100 Subject: [PATCH 1/3] Allow securityContext in EphemeralContainers --- pkg/api/pod/util_test.go | 4 ++-- pkg/apis/core/types.go | 3 ++- pkg/apis/core/validation/validation.go | 1 + pkg/apis/core/validation/validation_test.go | 23 +++++++++++++-------- staging/src/k8s.io/api/core/v1/types.go | 3 ++- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/pkg/api/pod/util_test.go b/pkg/api/pod/util_test.go index a11439a52569..3124c8d63680 100644 --- a/pkg/api/pod/util_test.go +++ b/pkg/api/pod/util_test.go @@ -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, }, diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 3fdc87977a61..bf74884bd4cb 100644 --- a/pkg/apis/core/types.go +++ b/pkg/apis/core/types.go @@ -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 diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index c2e05ed61f0a..8ca1fc97a278 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -85,6 +85,7 @@ var allowedEphemeralContainerFields = map[string]bool{ "TerminationMessagePath": true, "TerminationMessagePolicy": true, "ImagePullPolicy": true, + "SecurityContext": true, "Stdin": true, "StdinOnce": true, "TTY": true, diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index d444b68b5646..a440b38c0a46 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -5822,7 +5822,7 @@ func TestValidateEphemeralContainers(t *testing.T) { TargetContainerName: "ctr", }, }, - "All Whitelisted Fields": { + "All allowed Fields": { { EphemeralContainerCommon: core.EphemeralContainerCommon{ @@ -5848,9 +5848,14 @@ func TestValidateEphemeralContainers(t *testing.T) { TerminationMessagePath: "/dev/termination-log", 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, }, }, }, @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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{ diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index ef4e433ac46c..dd662fcf61ff 100644 --- a/staging/src/k8s.io/api/core/v1/types.go +++ b/staging/src/k8s.io/api/core/v1/types.go @@ -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"` From aff49ca684613cdc86e32f3aa42ad067666d172e Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Fri, 12 Feb 2021 10:45:27 +0100 Subject: [PATCH 2/3] Generated code for securityContext in EphemeralContainers --- api/openapi-spec/swagger.json | 2 +- staging/src/k8s.io/api/core/v1/generated.proto | 3 ++- staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json index 8b6e129b4275..9f118cfff55f 100644 --- a/api/openapi-spec/swagger.json +++ b/api/openapi-spec/swagger.json @@ -5474,7 +5474,7 @@ }, "securityContext": { "$ref": "#/definitions/io.k8s.api.core.v1.SecurityContext", - "description": "SecurityContext is not allowed for ephemeral containers." + "description": "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." }, "startupProbe": { "$ref": "#/definitions/io.k8s.api.core.v1.Probe", diff --git a/staging/src/k8s.io/api/core/v1/generated.proto b/staging/src/k8s.io/api/core/v1/generated.proto index 7c4474842631..9d38ce07d2e9 100644 --- a/staging/src/k8s.io/api/core/v1/generated.proto +++ b/staging/src/k8s.io/api/core/v1/generated.proto @@ -1338,7 +1338,8 @@ message EphemeralContainerCommon { // +optional optional string imagePullPolicy = 14; - // 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 optional SecurityContext securityContext = 15; diff --git a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go index 12a7d489690d..07fc68ac796f 100644 --- a/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go +++ b/staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go @@ -607,7 +607,7 @@ var map_EphemeralContainerCommon = map[string]string{ "terminationMessagePath": "Optional: Path at which the file to which the container's termination message will be written is mounted into the container's filesystem. Message written is intended to be brief final status, such as an assertion failure message. Will be truncated by the node if greater than 4096 bytes. The total message length across all containers will be limited to 12kb. Defaults to /dev/termination-log. Cannot be updated.", "terminationMessagePolicy": "Indicate how the termination message should be populated. File will use the contents of terminationMessagePath to populate the container status message on both success and failure. FallbackToLogsOnError will use the last chunk of container log output if the termination message file is empty and the container exited with an error. The log output is limited to 2048 bytes or 80 lines, whichever is smaller. Defaults to File. Cannot be updated.", "imagePullPolicy": "Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: https://kubernetes.io/docs/concepts/containers/images#updating-images", - "securityContext": "SecurityContext is not allowed for ephemeral containers.", + "securityContext": "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.", "stdin": "Whether this container should allocate a buffer for stdin in the container runtime. If this is not set, reads from stdin in the container will always result in EOF. Default is false.", "stdinOnce": "Whether the container runtime should close the stdin channel after it has been opened by a single attach. When stdin is true the stdin stream will remain open across multiple attach sessions. If stdinOnce is set to true, stdin is opened on container start, is empty until the first client attaches to stdin, and then remains open and accepts data until the client disconnects, at which time stdin is closed and remains closed until the container is restarted. If this flag is false, a container processes that reads from stdin will never receive an EOF. Default is false", "tty": "Whether this container should allocate a TTY for itself, also requires 'stdin' to be true. Default is false.", From babebf76d39508104d4a3a43fb0bb3399245495c Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Fri, 25 Jun 2021 18:07:49 +0200 Subject: [PATCH 3/3] Apply PSP container tests to EphemeralContainers --- .../podsecuritypolicy/provider_test.go | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/pkg/security/podsecuritypolicy/provider_test.go b/pkg/security/podsecuritypolicy/provider_test.go index 14c7ab45782d..6e5085085bd4 100644 --- a/pkg/security/podsecuritypolicy/provider_test.go +++ b/pkg/security/podsecuritypolicy/provider_test.go @@ -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 @@ -120,6 +122,11 @@ func TestMutateContainerNonmutating(t *testing.T) { Containers: []api.Container{{ SecurityContext: tc.security, }}, + EphemeralContainers: []api.EphemeralContainer{{ + EphemeralContainerCommon: api.EphemeralContainerCommon{ + SecurityContext: tc.security, + }, + }}, }, } } @@ -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) @@ -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") }) } } @@ -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) @@ -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") }) } } @@ -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. @@ -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 @@ -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 @@ -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") + } }) } }