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

fix(deploy) Increase container security #1057

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

ubergesundheit
Copy link
Contributor

@ubergesundheit ubergesundheit commented Apr 17, 2024

Change default values to run containers with read-only file system and non-root user for both kong and postgres deployments.

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

  • PR is based off the current tip of the main branch.
  • Changes are documented under the "Unreleased" header in CHANGELOG.md
  • New or modified sections of values.yaml are documented in the README.md
  • Commits follow the Kong commit message guidelines

@ubergesundheit ubergesundheit requested a review from a team as a code owner April 17, 2024 06:48
@ubergesundheit ubergesundheit changed the title fix(deploy) increase container security fix(deploy) Increase container security Apr 17, 2024
@ubergesundheit
Copy link
Contributor Author

ubergesundheit commented Apr 17, 2024

The reason specifying runAsNonRoot only in the containerSecurityContext is that injected kuma-init initContainers seem to need elevated privileges.

@rainest
Copy link
Contributor

rainest commented Apr 22, 2024

Do you know of anything that's functionally different in terms of enforcement or compliance validation if like settings are set at the Pod versus container level?

The rough justification for having defaults only in the container section is more or less what you've mentioned: we apply those context constraints to any container the chart itself manages, and any other container in the Pod is an unknown sidecar (with Kuma having a special status as a well-known sidecar that we try to accommodate by default). Pod-level settings might thus break those unknown sidecars and AFAIK there's no difference when applying the same setting at both levels. If there's something that's Pod-level only, we should consider that in isolation.

The Postgres settings should be fine, though we've normally mostly ignored it since it's kind of a legacy wart we discourage use of and plan to ultimately remove.

@ubergesundheit
Copy link
Contributor Author

As you already wrote, the pods securityContext applies to all containers in the pod while the securityContext of a container only applies to the single container. There are some fields that can be only configured in the securityContext of the pod (fsGroup, fsGroupChangePolicy, supplementalGroups and sysctls) and some fields that are exclusive to the containers securityContext (allowPrivilegeEscalation, capabilities, privileged, procMount and readOnlyRootFilesystem). Fields runAsGroup, runAsNonRoot, runAsUser, seLinuxOptions, seccompProfile and windowsOptions are supported by both.

In terms of compliance, IMO it would be better to set most of the stuff in the pods securityContext. But that breaks injected containers. Not sure how much thought was put into kube-linters rules which we're trying to make happy in this PR.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Alright, incompatibility is less of a concern: https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container

Security settings that you specify for a Container apply only to the individual Container, and they override settings made at the Pod level when there is overlap

It is still a breaking change though, since any added sidecars that are incompatible will need to specify context where they didn't previously, so we'd need to note as much in the changelog.

What issue were you seeing with Kuma, on which version, and with what install configuration? At least with https://kuma.io/docs/2.7.x/production/dp-config/cni/#installation and the KIND instructions I was able to set up Kong and our test echo server with Pod-level runAsNonRoot: true. That may well have tripped it since AFAIK https://kuma.io/docs/2.7.x/production/dp-config/transparent-proxying/ actually does require root, but I'd think it'd set the override on sidecars in that case.

However, I'd expect UID 1000 at the Pod level to break that also, since even if the container can run as root, it wouldn't be.

charts/kong/values.yaml Outdated Show resolved Hide resolved
@ubergesundheit
Copy link
Contributor Author

What issue were you seeing with Kuma, on which version, and with what install configuration?

I was running tests locally with the versions specified in the test files. As said the injected Kuma initContainers were not happy with the runAsNonRoot setting as these were set explicitly to run with uid 0

@rainest
Copy link
Contributor

rainest commented Apr 26, 2024

Would the Pods come online if you manually added a runAsNonRoot: false to the kuma initContainers? I think that should work in namespaces that permit it.

Trying to do my own tests, but have been frustrated by some other issue interfering with my ability to run the transparent proxy mode locally.

@ubergesundheit
Copy link
Contributor Author

Would the Pods come online if you manually added a runAsNonRoot: false to the kuma initContainers? I think that should work in namespaces that permit it.

As long as the pods securityContext does not already specify runAsNonRoot: true, it should work. But I did not test this.

@ubergesundheit
Copy link
Contributor Author

@rainest sorry for letting this stall. I've rebased and ran make test.golden.update. I've also addressed your comment regarding securityContext

@ubergesundheit
Copy link
Contributor Author

@rainest is this still something worth merging?

@ubergesundheit
Copy link
Contributor Author

Hello @pmalek can you take a look at this PR? Thank you :)

Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

These changes look good. Please rebase on latest main. Left 1 question.

charts/kong/values.yaml Outdated Show resolved Hide resolved
charts/kong/values.yaml Show resolved Hide resolved
charts/kong/values.yaml Show resolved Hide resolved
@ubergesundheit ubergesundheit force-pushed the container-security branch 2 times, most recently from 0ec08d6 to f746ccb Compare February 4, 2025 21:58
@ubergesundheit
Copy link
Contributor Author

Thanks, rebased to current main 👍

@ubergesundheit
Copy link
Contributor Author

I've seen that kube-linter also failed for the kube-rbac-proxy in the gateway-operator chart. I rebased again and updated the containers securityContext.

The other test failures seem to be due to helm dependency failures. I think they should work after retrying

Change default values to run containers with read-only file system and
non-root user for both kong and gateway-operator charts.

Signed-off-by: Gerald Pape <[email protected]>
@pmalek pmalek merged commit f25f1db into Kong:main Feb 5, 2025
34 checks passed
@pmalek
Copy link
Member

pmalek commented Feb 5, 2025

Thanks @ubergesundheit 🙇

@ubergesundheit ubergesundheit deleted the container-security branch February 5, 2025 13:41
@czeslavo czeslavo mentioned this pull request Feb 6, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable all kube-linter rules
3 participants