-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 explicit securitycontexts to controllers #7831
✨ add explicit securitycontexts to controllers #7831
Conversation
Welcome @tuminoid! |
Hi @tuminoid. 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 Once the patch is verified, the new status will be reflected by the 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. |
Hello maintainers! I'd like to hear your opinion on adding explicit securityContexts to CAPI controller manager.
|
P.S. I assume you wanted to clarify those points first. Eventually we should modify the manager manifests of CABPK, KCP and CAPD in this PR as well (CAPD won't work as non-root because of the docker socket mount, but let's explicitly add what we can) |
Thanks for the reply! We had a discussion in CAPM3 that we want to align with you (CAPI) first on this, and then replicate the approach to CAPM3, BMO, IPAM and elsewhere where necessary. As for you question about seccompProfiles, the link in the PR description describes them. In short, all (or almost all) container runtimes come with a default profile that limits some of the syscalls from the containers. They're typically not used by non-root workloads, so enabling it changes nothing. In case container calls restricted syscall, it is prevented and app will crash. In our testing, I've enabled it for CAPI, CAPM3, BMO, IPAM without any issues.
I can amend the PR for sure. Would you ok-to-test the PR so I can verify CAPI tests pass before I do that? |
/ok-to-test |
The interesting question is what happens if the container runtime doesn't come with a default profile. If I understand it correctly starting with Kubernetes 1.25 the SeccompDefault (https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/) feature gate will be enabled per default which sets the default profile automatically. So I think overall we could have the following problems:
Given that starting with Kubernetes 1.25 Kubernetes takes care of setting the default seccomp profile automatically I would prefer not setting it in ClusterAPI to not run into issues in the edge cases. (but just my opinion, no objection if folks want to enable it and think it's safe) |
SeccompDefault feature gate enables kubelet to use
If the container runtime doesn't have a default profile, then it'll be no different than Unconfined, as the profile filters syscalls, or depending on your runtime configuration, it may already be using runtime defaults. https://github.com/cri-o/cri-o/blob/main/docs/crio.conf.5.md
https://docs.docker.com/engine/security/seccomp/ for Docker. Good list of syscalls filtered, showing most of them are namespaced, and not available anyways as we drop all capabilities, + few obsolete or non-namespaced calls. containerd follows Docker profile: containerd/containerd#5924 and https://github.com/containerd/containerd/blob/f0a32c66dad1e9de716c9960af806105d691cd78/contrib/seccomp/seccomp_default.go#L51 and so on. Of course, if you think seccomp is too risky to enable by default, I can leave it out of this PR. |
/retest |
Thx for the additional context! Sounds okay to me, but I would like to hear more opinions from others. |
Setting a security context in the manifest will break tilt This means for each code update, the pod won't be updated and tilt will report this error:
From what I understood from tilt-dev/tilt#3060, |
@bengentil Nice catch, thx! Given how our tilt setup works I think that is something that we can do in our tilt-prepare binary (roughly here). But would be good to have verification that dropping it there works. |
@sbueringer you're right it's way easier and less error prone to implement it in tilt-prepare I've successfully tested this fix: bengentil@d5eb23a Don't know the process if it has to be in the same PR, but it should be merged before or at the same time ideally, feel free to take my commit in this branch if needed. |
EDIT: I think it's okay to open this as a separate PR and merge it before. Especially given that this makes our tilt setup compatible with infra providers setting securityContext |
PR created: #7846 |
3eb44bc
to
7d77c00
Compare
Thanks @bengentil for spotting the tilt issue and the PR to fix it! It'd be good to have it merged first. I've updated the PR by splitting the indentation fix to separate commit and by adding securityContext's to KCP and CABPK. CAPD is privileged and none of the securityContexts make sense or have effect when privileged, so I did not touch it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice change, I have modified the PR title so it stands out on release notes!
Can we document this change in upgrade notes for the providers as well?
- https://main.cluster-api.sigs.k8s.io/developer/providers/v1.3-to-v1.4.html#other --> Cluster API controllers are now using an explicit security context by default.
- https://main.cluster-api.sigs.k8s.io/developer/providers/v1.3-to-v1.4.html#suggested-changes-for-providers --> Providers should add an explicit security context to their controllers deployment, see ✨ add explicit securitycontexts to controllers #7831 for reference
Also, it will be great if we apply security policies to our test extension in https://github.com/kubernetes-sigs/cluster-api/blob/main/test/extension/config/default/manager.yaml (It should be pretty straightforward, but if I'm wrong this could be done also a follow-up PR)
I think lint will have to be retriggered by someone with the correct rights for the repo, but I think we should be happy to merge if an approver verifies that lint is working locally - not sure why it's stuck. |
Cool. It should not fail, as lint action is linting Go files, and this PR doesn't change any Go sources. |
I had to "re-trigger" it. Fyi it was not the "new contributor" case where someone with write access has to click a button. This was the case where "somehow" GitHub just doesn't trigger the GitHub action. I triggered the linter by essentially creating a "PR edited" event by adding a new empty line somewhere in the PR description... Only had this 2-3 times up until now and it was always on dependabot PRs |
/lgtm /assign @fabriziopandini |
Add explicit, secure securityContexts for all managers except CAPD, which is privileged and for testing purposes. These securityContexts do not change the configuration, just make it explicit and enforced, except for the seccompPolicy which changes from Unconfined to RuntimeDefault. Syscalls filtered by RuntimeDefault policy are 95% namespaced and require capabilities (which we drop) in the first place, so no practical change there either.
c101a2e
to
9ac1f05
Compare
Rebased due changelog conflict. Please re-review and let's get this merged. This already has had approved/lgtm from everyone required. |
Thx! /lgtm |
LGTM label has been added. Git tree hash: d55651945044364f8715f476a681aa010d950fc7
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
Adding explicit securitycontext ensures the CAPO controller will run as non-root, without special capabilities. Those are often also the defaults but being explicit avoids reliance on fallback values. In addition, adding seccompProfile of RuntimeDefault adds runtime specific syscall filtering (mostly off-limit by not having capability in the first place) but also couple other, non-namespaced syscalls. There is good discussion and reference links in similar CAPI PR at: kubernetes-sigs/cluster-api#7831
Adding explicit securitycontext ensures the CAPO controller will run as non-root, without special capabilities. Those are often also the defaults but being explicit avoids reliance on fallback values. In addition, adding seccompProfile of RuntimeDefault adds runtime specific syscall filtering (mostly off-limit by not having capability in the first place) but also couple other, non-namespaced syscalls. There is good discussion and reference links in similar CAPI PR at: kubernetes-sigs/cluster-api#7831
Adding explicit securitycontext ensures the CAPO controller will run as non-root, without special capabilities. Those are often also the defaults but being explicit avoids reliance on fallback values. In addition, adding seccompProfile of RuntimeDefault adds runtime specific syscall filtering (mostly off-limit by not having capability in the first place) but also couple other, non-namespaced syscalls. There is good discussion and reference links in similar CAPI PR at: kubernetes-sigs/cluster-api#7831
Set explicit, secure securityContexts for the controller manager deployment and containers instead of relying on defaults or fallbacks.
Only actual change here is enabling runtimeDefault seccompPolicy, instead of running as Unconfined.
https://kubernetes.io/docs/tutorials/security/seccomp/
Also, reindent poorly indented command block.