-
Notifications
You must be signed in to change notification settings - Fork 187
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
security: Drop capabilities, set userid and enable seccomp #521
Conversation
# Required for AWS IAM Role bindings | ||
# https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts-technical-overview.html |
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.
Moved the comments here as they relate to fsGroup
and not PodSecurityContext
.
config/manager/deployment.yaml
Outdated
@@ -31,6 +31,12 @@ spec: | |||
securityContext: | |||
allowPrivilegeEscalation: false | |||
readOnlyRootFilesystem: true | |||
capabilities: | |||
drop: [ "ALL" ] | |||
runAsUser: 65534 |
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.
Setting a used ID here was proposed before, but we’ve rejected it due to breaking Flux on OpenShift and other Kubernetes distributions that enforce a specific ID range. I think this change should be made to the docs here https://fluxcd.io/docs/installation/#pod-security-policy
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.
Think that shipping a secure default configuration and instructing people to change this if it does not work for their distribution is better than hoping people will find your security notes (and then also apply the required configuration).
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.
@stefanprodan when this was last attempted, do you remember what user/group id was used?
The user ID picked here is linked to the user nobody
, which is meant to be the user with least privileges and is generally defined as part of all main distributions.
The upstream pause container relies on the same approach since Kubernetes 1.21. Other security focused projects targetting OpenShift also rely on it.
I completely agree with you that we should not prioritise security over reliability. On that point, would this be better received if accompanied by an OpenShift E2E automated test?
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.
I think it throws an error like this: unable to validate against any security context constraint: [fsGroup: Invalid value: []int64{65534}: 65534 is not an allowed group spec.containers[0].securityContext.securityContext.runAsUser: Invalid value: 1001: must be in the ranges: [1000810000, 1000819999]]
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.
We do have e2e tests for OpenShift, Flux is on OperatorHub. Adding fs group or user ID to each controller repo, just to remove them in flux CLI makes zero sense to me.
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.
Why would we remove them again in the Flux CLI if the OpenShift / OperatorHub setup has custom patches / instructions to make it work anyway?
(Aiming at the instructions here https://fluxcd.io/docs/use-cases/openshift/#security-context-constraints, which could be rewritten to something that deals with the error shared earlier).
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.
They get removed here, not in CLI my bad.
seccompProfile: | ||
type: RuntimeDefault |
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 syntax requires Kubernetes 1.19
. I am assuming this is OK as it aligns with flux check --pre
which checks for >=1.19.0
.
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.
We do support older versions, see: https://fluxcd.io/docs/installation/#prerequisites
Rebased with changes of the statically build PR for testing in ARM64. Will rebase after that is merged. |
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.
LGTM
I see no issues with secomp being supported by newer Kubernetes version as we've announced that the next Flux release will drop support for Kubernetes EOL versions.
Thanks @pjbgf 🏅
This was tested on Kubernetes
|
@pjbgf please rebase |
Further restricts the SecurityContext that the controller runs under, by enabling the default seccomp profile and dropping all linux capabilities. This was set at container-level to ensure backwards compatibility with use cases in which sidecars are injected into the source-controller pod without setting less restrictive settings. BREAKING CHANGE: The use of new seccomp API requires Kubernetes 1.19. Co-authored-by: Sanskar Jaiswal <sanskar.jaiswal@weave.works> Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
BREAKING CHANGE: the controller container is now executed under 65534:65534 (userid:groupid). This change may break deployments that hard-coded the user name 'controller' in their PodSecurityPolicy. Signed-off-by: Paulo Gomes <paulo.gomes@weave.works>
Further restricts the
SecurityContext
that the controller runs under, by enabling the default seccomp profile, dropping all linux capabilities.This was set at container-level to ensure backwards compatibility with use cases in which more privileged sidecars are injected into the
source-controller
pod without setting less restrictive settings.Note that seccomp will only be enabled if the container runtime and operational system supports it, otherwise the container will run
unconfined
(aka fail-open), which is the same behaviour as not setting theseccompProfile
in the first place.Relates to fluxcd/flux2#2014
Impacts weaveworks/flux2-openshift#10
BREAKING CHANGE:
65534:65534
(userid:groupid). This change may break deployments that hard-coded the user name 'controller' in theirPodSecurityPolicy
.