Helm charts:: Security Contexts

[pasindutennage-da]

Fix https://github.com/DACH-NY/canton-network-internal/issues/5825

[moritzkiefer-da]

The change itself looks fairly reasonable. But there is a very large number of configuration flags on security contexts and security contexts on various levels. They did mention this explicitly but their message sounds fairly ambigous and they may just have used this an example. Can we check with them what they expect?

[martinflorian-da]

I think there are two parts here:

1. What are reasonable defaults that we would (eventually) like everyone to have.
2. What is a cheap way to allow arbitrary customizations.

For 1. we need some research; did we do that somewhere?

For 2. I'm not sure if this PR goes in the right direction. Why not just allow overriding the securityContext: field completely? Then people can configure whatever they want, whenever they want.

[isegall-da]

Can we use something other than docs? We know the docs are going away soon

[pasindutennage-da]

@martinflorian-da

For 2. I'm not sure if this PR goes in the right direction. Why not just allow overriding the securityContext: field completely? Then people can configure whatever they want, whenever they want.

I agree with your point. We can let one override securityContext, however, we (I) should make sure that whatever they put inside that object is supported in our helm charts; else, it will break.

What I would do is; define securityContext in schema as an object with our selected subset (not just allowPrivilegeEscalation) of allowed options. Then in Helm helper, I will set only those supported options.

[moritzkiefer-da]

however, we (I) should make sure that whatever they put inside that object is supported in our helm charts; else, it will break.

I wouldn't try to enforce this in the schema. Trying to figure out all the various options that may be safe is a lost cause and some options may be safe in some usecase but not others. Just set the ones that are always valid by default and let people overwrite with whatever they want.

[martinflorian-da]

Totally agree with @moritzkiefer-da ; for the defaults it's IMO fine enough if we ensure validity via our cluster testing. For everything else - up to the users to pick the right strings. If they want to override this it's reasonable to assume they have some idea what they are doing.

[pasindutennage-da]

@martinflorian-da @moritzkiefer-da

I will allow overriding the containerSecurityContext field, so users can put whatever they want. We will set a few defaults that won't break our deployments.

Full list of securitycontext: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.36/#securitycontext-v1-core

Defaults

allowPrivilegeEscalation: false (controls whether a process can gain more privileges than its parent

privileged: false (container does not run in privileged mode)

runAsNonRoot: true (ensures the container must run as a non-root user) -- cluster_test will say if this breaks some postgress pods

Testing

I will update the tests to verify both the default state (no overrides) and the arbitrary overrides. Just extending my current 2 test per chart strcuture.

To define these default securitycontext, we can either put them in values-template.yaml, or directly in the Helm _helpers.tpl . Which one do you think is clearer?

[moritzkiefer-da]

any reason not to set something like

  seccompProfile:
    type: RuntimeDefault

and maybe switch to explicit capabilities whitelists?

it seems like runAsUser, runAsGroup and fsGroup are also set quite frequently from a quick search but I also can't come up with all that clear of a reason why it meaningfully improves things over runAsNonRoot: false so fine to skip for now.

To define these default securitycontext, we can either put them in values-template.yaml, or directly in the Helm _helpers.tpl . Which one do you think is clearer?

I think I prefer values-templates. Then you also get automatic merging with user-supplied values.

Note that there is also both a pod and a container security context. I think we want to set both.

[pasindutennage-da]

@moritzkiefer-da @martinflorian-da

I made the changes for scan helm chart. To make sure that we are on the same page, can you please have a look and see if scan helm chart (only scan helm chart for now) supports all the security contexts?

The remaining work is to just duplicate this across all helm charts, and I will do it next.

[moritzkiefer-da]

thx, it looks good in configurability, the big question ofc is if it really works so definitely make sure to test this