securityContext for app_service volumes #493
Conversation
ChuckHend
requested review from
sjmiller609,
nhudson and
ianstanton
as code owners
There was a problem hiding this comment.
I carefully reviewed this one because of the security related change.
I think the security aspect is OK, based on the following reasoning. "If you specify a fsGroup in the security context, for a (Linux) Pod, all processes in the pod's containers are part of the additional group that you specified." The group being granted here is a permission the container user already has, so that is not granting something new. Also the other security controls are not adjusted by this setting. By setting in fsGroup, this informs the CSI driver what permissions should be set, which is why it fixes the issue. (reference)
One feedback that could make the code slightly cleaner. It is unnecessary to have both container and pod security policies, could consolidate into only one or the other for a clearer policy.
| // Add any user provided volumes / volume mounts | ||
| if let Some(storage) = appsvc.storage.clone() { | ||
| // when there are user specified volumes, we need to let kubernetes modify permissions of those volumes | ||
| pod_security_context = Some(PodSecurityContext { | ||
| fs_group: Some(65534), |
There was a problem hiding this comment.
PodSecurityContext fsGroup
securityContext.fsGroup (int64)
A special supplemental group that applies to all containers in a pod. Some volume types allow the Kubelet to change the ownership of that volume to be owned by the pod:
The owning GID will be the FSGroup 2. The setgid bit is set (new files created in the volume will be owned by FSGroup) 3. The permission bits are OR'd with rw-rw----
If unset, the Kubelet will not modify the ownership and permissions of any volume. Note that this field cannot be set when spec.os.name is windows.
https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#security-context
There was a problem hiding this comment.
It is unnecessary to have both pod security policy and container security policy, we could maybe just define this setting inside one or the other. This comment has no security benefit it is only a question of organization.
There was a problem hiding this comment.
fsGroup isn't available on the container from what I can tell, so we'd need to move it all up to the pod I think...
There was a problem hiding this comment.
In this case I think good to go as-is, since that's nicer from the perspective of changing less.
| @@ -394,8 +395,14 @@ fn generate_deployment( | |||
| }; | |||
| volume_mounts.push(certs_volume_mount); | |||
|
|
|||
| let mut pod_security_context: Option<PodSecurityContext> = None; | |||
There was a problem hiding this comment.
Informational, no action requested:
Pod Security Context vs Container Security Context:
SecurityContext defines the security options the container should be run with. If set, the fields of SecurityContext override the equivalent fields of PodSecurityContext.
This means only the unset values such as fsGroup can be overwritten by Pod Security Context. So, we can constrain the security related implications only to the fsGroup configuration.
we need a pod security context so that app's can write to the ephemeral volume that gets attached