Helm charts:: Security Contexts

[pasindutennage-da]

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

Cluster test https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/72353

Deploy upgrade https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/72354

[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

[pasindutennage-da]

I didn't check this chart in scratch ; i have not provided default values for this chart.

[martinflorian-da]

I wonder if we should just remove that chart for now... (offtopic)

[pasindutennage-da]

I did not test this chart in scratch; and i did not set default context

[martinflorian-da]

Fair enough as we don't run this on MainNet, but at some point we should IMO harden that one too as it runs on DevNet. Let's add some TODO?

[pasindutennage-da]

this chart is not tested in scratch; and i have not set default context

[martinflorian-da]

Same as for load tester; let's maybe add a TODO to document that we do think it would be nice to harden these as well.

[pasindutennage-da]

i did not check this chart in scratch; i did not set default values

[martinflorian-da]

That one I don't fully get though; deploying splitwell is not that big of a deal?

[martinflorian-da]

Happy to also add a TODO here, mainly curious why you decided to skip it.

[pasindutennage-da]

i have not checked this chart in scratch; i have not set default values

[martinflorian-da]

same comment as for the other splitwell component

[pasindutennage-da]

@moritzkiefer-da

This PR is ready to be reviewed now. The approach i used

1. Started with the least privileges and then relaxed until containers could run without error.
2. Tested with cncluster apply --sv1-only
3. Avoided testing charts that we never use in production, but still included the contexts with empty defaults, just so that it is easy to reason about the charts at some later point in time.
4. Currently running cluster test and upgrade test. Links in the description.

[moritzkiefer-da]

Thanks! The changes lgtm, I do think @martinflorian-da's concerns on duplication are valid so let's try to factor out the base profile that's restricted. It seems like putting a placeholder in there and resolving it in the translation from values-template.yaml -> values.yaml is probably the easiest option.

I would also suggest to not get this into this week's release but instead merge directly after the release so we get maximum testing time in case there is some fallout.

[martinflorian-da]

Thanks for pushing this forward guys!

I'm going to continue being annoying sadly: Even with deduplicating the security context boilerplate itself I'm still not super excited about the maintainability of this change...

• the tests are very duplicate and boilerplate-y
• we need to remember to add the right thing on new charts we add

I guess these concerns are largely consistent with the state of the rest of our Helm. But I think that at some point we need to look into more sophisticated scanning / static checks to better guard against regressions and "forgot to add context to new thing we added".

@pasindutennage-da Did you happen to look into such tools already by any chance? So things like trivy, kyverno...

(TBC I'm thinking we should create a follow-up issue with the context we currently have; not that we should significantly change this PR.)

[martinflorian-da]

I wonder if we should just remove that chart for now... (offtopic)

[martinflorian-da]

Fair enough as we don't run this on MainNet, but at some point we should IMO harden that one too as it runs on DevNet. Let's add some TODO?

[martinflorian-da]

Same as for load tester; let's maybe add a TODO to document that we do think it would be nice to harden these as well.

[martinflorian-da]

That one I don't fully get though; deploying splitwell is not that big of a deal?

[martinflorian-da]

Happy to also add a TODO here, mainly curious why you decided to skip it.

[martinflorian-da]

same comment as for the other splitwell component

[martinflorian-da]

Suggested change

	- Added security contexts for Helm based deployments.
	- Added security contexts for all Helm-based deployments intended for production.

...or some better wording that avoids suggesting that we fixed all of them (we didn't). Also happy with a list (of charts we fixed or of the ones that remain unfixed).

[martinflorian-da]

That reads like "we got some error that we didn't understand and fixed it by reducing the security level"... can you give more context about the issue? Making it a TODO (pointing to a real issue on one of our security milestones) is also fine to me.

[martinflorian-da]

I'd also soften this to just "harden"... I don't think we can confidently claim "least possible privilege", and definitely not on "all pods and contains"