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

Improve securityContext implementation #1333

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Improve securityContext implementation #1333

merged 1 commit into from
Aug 15, 2022

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Aug 3, 2022

Is this a bug fix or adding new feature?

New feature
Fixes #1332
Fixes #1101

What is this PR about? / Why do we need it?

Implements securityContext for the node pod and all containers (including sidecar containers). Also switches to a more secure securityContext default of running as a non-root user and with a read-only root filesystem.

Currently, the node pod must run as a root user because /var/lib/kubelet/plugins/ebs.csi.aws.com/ and /var/lib/kubelet/plugins_registry/ are kubelet-managed and usually only writable by root (additionally, the node driver needs privileged for the bidirectional mount anyways, so root would not give extra security to that container anyways).

Also makes some minor improvements to make generate-kustomize so it doesn't generate useless diffs that have to be manually removed.

What testing is done?

Manually tested.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ConnorJC3 / name: ConnorJC (63adbf2)

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 3, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @ConnorJC3. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 3, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 3, 2022
@torredil torredil self-assigned this Aug 3, 2022
@ConnorJC3
Copy link
Contributor Author

/hold should also migrate the new defaults over to the kustomize configuration if they pass CI, also needs helm changelog

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2022
@torredil torredil removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 3, 2022
@torredil
Copy link
Member

torredil commented Aug 3, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 3, 2022
@pierluigilenoci
Copy link

Thank you @ConnorJC3!

@ConnorJC3
Copy link
Contributor Author

/unhold made CI happy, ready for review

@@ -104,6 +106,10 @@ spec:
resources:
{{- toYaml . | nindent 12 }}
{{- end }}
{{- with .Values.node.containerSecurityContext }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking for feedback: Was wondering if this should default to the ebs-plugin security context if not supplied, similar to how resources does (look a few lines up).

@torredil
Copy link
Member

torredil commented Aug 4, 2022

/lgtm

$ kubectl logs ebs-csi-node-pzr8x -n kube-system ebs-plugin

I0804 18:06:24.019852       1 metadata.go:85] retrieving instance data from ec2 metadata
W0804 18:06:27.184531       1 metadata.go:88] ec2 metadata is not available
I0804 18:06:27.184625       1 metadata.go:96] retrieving instance data from kubernetes api
I0804 18:06:27.186130       1 metadata.go:101] kubernetes api is available
$ kubectl logs ebs-csi-node-pzr8x -n kube-system node-driver-registrar

I0804 18:06:26.209987       1 main.go:166] Version: v2.5.1
I0804 18:06:26.210037       1 main.go:167] Running node-driver-registrar in mode=registration
I0804 18:06:26.210718       1 main.go:191] Attempting to open a gRPC connection with: "/csi/csi.sock"
I0804 18:06:27.211836       1 main.go:198] Calling CSI driver to discover driver name
I0804 18:06:27.215678       1 main.go:208] CSI driver name: "ebs.csi.aws.com"
I0804 18:06:27.216326       1 node_register.go:53] Starting Registration Server at: /registration/ebs.csi.aws.com-reg.sock
I0804 18:06:27.216710       1 node_register.go:62] Registration Server started at: /registration/ebs.csi.aws.com-reg.sock
I0804 18:06:27.216865       1 node_register.go:92] Skipping HTTP server because endpoint is set to: ""
I0804 18:06:27.375036       1 main.go:102] Received GetInfo call: &InfoRequest{}
E0804 18:06:27.375117       1 main.go:107] "Failed to create registration probe file" err="mkdir /var/lib/kubelet: read-only file system" registrationProbePath="/var/lib/kubelet/plugins/ebs.csi.aws.com/registration"
I0804 18:06:27.375130       1 main.go:109] "Kubelet registration probe created" path="/var/lib/kubelet/plugins/ebs.csi.aws.com/registration"
I0804 18:06:27.426540       1 main.go:120] Received NotifyRegistrationStatus call: &RegistrationStatus{PluginRegistered:true,Error:,}
  • Failed to create registration probe file" err="mkdir /var/lib/kubelet: read-only file system" is not of concern, only matters for kubelet-registration-probe which the driver does not use.
$ kubectl logs ebs-csi-node-pzr8x -n kube-system liveness-probe

I0804 18:06:30.575420       1 main.go:149] calling CSI driver to discover driver name
I0804 18:06:30.576854       1 main.go:155] CSI driver name: "ebs.csi.aws.com"
I0804 18:06:30.576863       1 main.go:183] ServeMux listening at "0.0.0.0:9808"
$ kubectl logs ebs-csi-controller-75684f4db-26zcd -n kube-system ebs-plugin

I0804 18:25:54.419726       1 metadata.go:85] retrieving instance data from ec2 metadata
W0804 18:25:57.573347       1 metadata.go:88] ec2 metadata is not available
I0804 18:25:57.573362       1 metadata.go:96] retrieving instance data from kubernetes api
I0804 18:25:57.574302       1 metadata.go:101] kubernetes api is available
$ kubectl logs ebs-csi-controller-75684f4db-26zcd -n kube-system csi-provisioner

W0804 18:25:54.562223       1 feature_gate.go:237] Setting GA feature gate Topology=true. It will be removed in a future release.
I0804 18:25:54.562405       1 feature_gate.go:245] feature gates: &{map[Topology:true]}
I0804 18:25:54.562442       1 csi-provisioner.go:139] Version: v3.1.0
I0804 18:25:54.562479       1 csi-provisioner.go:162] Building kube configs for running in cluster...
I0804 18:25:57.642598       1 common.go:111] Probing CSI driver for readiness
I0804 18:25:57.644500       1 csi-provisioner.go:206] Detected CSI driver ebs.csi.aws.com
$ kubectl logs ebs-csi-controller-75684f4db-26zcd -n kube-system csi-resizer

I0804 18:25:54.829060       1 main.go:93] Version : v1.4.0
I0804 18:25:54.829117       1 feature_gate.go:245] feature gates: &{map[]}
I0804 18:25:57.817799       1 common.go:111] Probing CSI driver for readiness
I0804 18:25:57.819005       1 main.go:141] CSI driver name: "ebs.csi.aws.com"
I0804 18:25:57.820014       1 common.go:111] Probing CSI driver for readiness
I0804 18:25:57.821784       1 controller.go:255] Starting external resizer ebs.csi.aws.com
$ kubectl logs ebs-csi-controller-75684f4db-26zcd -n kube-system liveness-probe

I0804 18:25:57.618983       1 main.go:149] calling CSI driver to discover driver name
I0804 18:25:57.624014       1 main.go:155] CSI driver name: "ebs.csi.aws.com"
I0804 18:25:57.624035       1 main.go:183] ServeMux listening at "0.0.0.0:9808"

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2022
@ConnorJC3
Copy link
Contributor Author

/remove-hold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2022
@ConnorJC3
Copy link
Contributor Author

ConnorJC3 commented Aug 4, 2022

As @torredil mentioned, the node-driver-registrar error is not relevant because the driver doesn't use the probe functionality in node-driver-registrar. But the error definitely is a bug so I went ahead and reported it to them.

@torredil
Copy link
Member

torredil commented Aug 4, 2022

We should also consider allowPrivilegeEscalation: false
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.24/#securitycontext-v1-core

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2022
@ConnorJC3
Copy link
Contributor Author

@torredil added to both. I don't think it will matter that much for the node as we already have to run as root there. Decided to leave out the default I asked about for now because it wouldn't be obvious which config to default from (the controller or the node?)

Implement securityContext for containers
Add securityContext for node pod
Utilize more secure defaults for securityContext
Improve `make generate-kustomize` to filter out namespace and tags
@pierluigilenoci
Copy link

pierluigilenoci commented Aug 8, 2022

@torredil @bertinatto @wongma7 can you please take a look?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorJC3, torredil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2022
@pierluigilenoci
Copy link

@torredil @bertinatto @wongma7 can someone approve running workflows?

@olemarkus
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2022
@k8s-ci-robot k8s-ci-robot merged commit c2eb966 into kubernetes-sigs:master Aug 15, 2022
@torredil
Copy link
Member

@pierluigilenoci Sorry for the delay on this. As a best practice, we strive for more than one review before merging changes. @olemarkus Thanks for taking a look.

@pierluigilenoci
Copy link

@torredil nothing to be sorry about. Thanks a lot to everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable SecurityContext on container level. Run containers with readOnlyRootFilesystem
5 participants