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

KEP-127: update based on current PSS integration approach #4691

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

haircommander
Copy link
Contributor

  • One-line PR description:
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jun 4, 2024
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, mrunalp

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 Jun 5, 2024
@mrunalp
Copy link
Contributor

mrunalp commented Jun 5, 2024

@liggitt ptal

@haircommander
Copy link
Contributor Author

FYI: here's the implementation kubernetes/kubernetes#125198


The validation for capabilities can be relaxed in a baseline pod because capabilities
are user namespaced in the linux kernel, and any pod does not have a seccomp profile (as baseline
pods are not required to) has access to the `unshare` syscall, allowing a user to create a user
Copy link
Member

Choose a reason for hiding this comment

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

any pod does not have a seccomp profile (as baseline pods are not required to)

how does this interact with https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2413-seccomp-by-default where a pod that does not opt into a seccomp profile gets one applied at the kubelet level?

does this ability to add arbitrary capabilities undo protections a cluster administrator put in place by defaulting seccomp on at the node level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since PSA doesn't require RuntimeDefault in baseline, even if RuntimeDefault is on (which needs a kubelet flag to enable) a sufficiently knowledgeable user can just set it to Unconfined and still be in baseline.

Copy link
Member

Choose a reason for hiding this comment

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

a sufficiently knowledgeable user can just set it to Unconfined and still be in baseline

baseline disallows explicitly setting to Unconfined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see, I misunderstood that section. okay, so if kubelet sets SeccompDefault, then this would technically increase the capabilities of a container. That said, I still think user namespaced capabilities are safer than not user namespaced capabilities. summoning @giuseppe who can probably advocate for that better than me

Copy link
Contributor

Choose a reason for hiding this comment

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

It's odd that we allow nil or unset seccompProfile but not Unconfined. Is there a difference?

Copy link
Member

Choose a reason for hiding this comment

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

Unconfined explicitly opts out of seccomp

nil / unset allows https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2413-seccomp-by-default to take effect

Copy link
Contributor

@vinayakankugoyal vinayakankugoyal Jun 20, 2024

Choose a reason for hiding this comment

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

Ah fair point. Thanks! But what is the intent here? Seem to me to be to not allow Unconfined right? So should we only allow nil when --seccomp-default is set to RuntimeDefault?


The validation for capabilities can be relaxed in a baseline pod because capabilities
are user namespaced in the linux kernel, and any pod does not have a seccomp profile (as baseline
pods are not required to) has access to the `unshare` syscall, allowing a user to create a user
Copy link
Member

Choose a reason for hiding this comment

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

cc @tallclair @vinayakankugoyal for visibility / ack on the capabilities implications of pods without a seccomp profile that are in a user namespace

nodes will honor user namespaces is on the cluster admin, though.

The relaxation in detail means, that if user namespaces are enabled, then the following fields
will be accessible to pods in a restricted namespace:

- `spec.securityContext.runAsNonRoot`
Copy link
Contributor

Choose a reason for hiding this comment

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

are you saying that when userns is enabled

runAsNonRoot can be set to false and runAsUser/runAsGroup can be set to 0?

If so then lets just say it like that? WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks I reworded to be a bit less verbose, PTAL @vinayakankugoyal

Concretely, the "relaxation" means if a pod has `hostUsers` set to false, then the fields `runAsNonRoot` can be set to false and
`runAsUser` and `runAsGroup` can be set to any value for any `securityContext` in the pod (pod, container, initContainer, ephemeralContainer).

Further, validation will also be relaxed for pods with `hostUsers` to false in a baseline namespace, where the `capabilities` field can be set for
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to clarify that the relaxation applies to capabilities.add field and that any capability can be added in the case of non hostUser pods where as hostUser pods are allowed to add a certain set of capabilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! updated

@vinayakankugoyal
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 7836e05 into kubernetes:master Jun 21, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Jun 21, 2024
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

5 participants