Allow container securityContext to be set

[tarquin-the-brave]

#602

Changes proposed in this PR

• Allow the container securityContext, for containers in the client and server Pods, to be configured by the Helm chart.

This PR doesn't add in any default values for the containers' securityContext.

How I've tested this PR

• Tested for regression by using the default values.yaml and helm template:
  • Diff'ing this against the manifest produced in the same way on master found no difference.
  • helm template . | kubeval --strict also passed.
  • I'm confident this hasn't regressed the Helm chart.
• Tested giving values to the new fields and running helm template:
  • The output passed kubeval --strict. This confirms the templated manifest is valid YAML and valid k8s objects, confirming the indentation of the securityContexts is correct.

Testing with kubeaudit

The motivation for the PR was to allow the Helm chart to configure all the fields required to satisfy kubeaudit.

Using this Helm chart and the values:

global:
  tls:
    enabled: true
  acls:
    manageSystemACLs: true
server:
  annotations: |
    "container.apparmor.security.beta.kubernetes.io/consul": "runtime/default"
    "seccomp.security.alpha.kubernetes.io/pod": "runtime/default"
  containerSecurityContext:
    privileged: false
    capabilities:
      drop:
        - ALL
    allowPrivilegeEscalation: false
    readOnlyRootFilesystem: true
client:
  annotations: |
    "container.apparmor.security.beta.kubernetes.io/consul": "runtime/default"
    "seccomp.security.alpha.kubernetes.io/pod": "runtime/default"
  containerSecurityContext:
    client:
      privileged: false
      capabilities:
        drop:
          - ALL
      allowPrivilegeEscalation: false
      readOnlyRootFilesystem: true
    aclInit:
      privileged: false
      capabilities:
        drop:
          - ALL
      allowPrivilegeEscalation: false
      readOnlyRootFilesystem: true
    tlsInit:
      privileged: false
      capabilities:
        drop:
          - ALL
      allowPrivilegeEscalation: false
      readOnlyRootFilesystem: true
tests:
  enabled: false

I was able to get kubeaudit all to pass.

This isn't to say that these values above are correct in terms of Consul running successfully with these set, or that they are the right "secure defaults", but it does show that this Helm chart gives users access to all the k8s API fields needed to satisfy kubeaudit.

How I expect reviewers to test this PR

TODO

Checklist

• Bats tests added
• CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

[hashicorp-cla]

All committers have signed the CLA.

[lkysow]

Thanks for the PR! Can you also write some bats tests for this change? We should test that you can set securityContext for each container and that if openshift is enabled the settings are ignored.

[kschoche]

One additional comment is that we do need to run in escalated modes when transparent proxy is enabled, this is done in part of the webhook (

consul-k8s/control-plane/connect-inject/container_init.go

Line 203 in 3787573

if tproxyEnabled {

) where the sidecars are attached, I do not think that the tool you're using would catch this since it resides in the consul-k8s binary.
I think in order for this to actually work at runtime we'd need to gate it on transparent proxy being disabled, somehow? Unfortunately that is challenging because you can enable transparent proxy at runtime via a pod annotation. @lkysow thoughts?

[lkysow]

@tarquin-the-brave you should be able to rebase off of master to get the builds passing.

@kschoche makes a good point. This only applies to some of the pods we create through the helm chart, not the containers we create via injection.

[tarquin-the-brave]

One additional comment is that we do need to run in escalated modes when transparent proxy is enabled, this is done in part of the webhook (

consul-k8s/control-plane/connect-inject/container_init.go

Line 203 in 3787573

if tproxyEnabled {

) where the sidecars are attached, I do not think that the tool you're using would catch this since it resides in the consul-k8s binary.
I think in order for this to actually work at runtime we'd need to gate it on transparent proxy being disabled, somehow? Unfortunately that is challenging because you can enable transparent proxy at runtime via a pod annotation. @lkysow thoughts?

This change is only affecting the Pods deployed directly by the Helm chart. Init and sidecar containers injected by the webhook server won't be affected.

Whether that's something you'd want to make configurable is another question. There's a strong case to not want the initContainer's securityContext configurable: You need the capabilities it sets to manipulate the iptables (I assume) for transparent proxying. For the Envoy sidecar proxy you're also doing some container securityContext hardening

consul-k8s/control-plane/connect-inject/envoy_sidecar.go

Lines 69 to 74 in 3787573

	container.SecurityContext = &corev1.SecurityContext{
	RunAsUser: pointerToInt64(envoyUserAndGroupID),
	RunAsGroup: pointerToInt64(envoyUserAndGroupID),
	RunAsNonRoot: pointerToBool(true),
	ReadOnlyRootFilesystem: pointerToBool(true),
	}

These are the fields that overlap with the Pod securityContext. I imagine they're set here so that even if the the Pod that's being injected hasn't set them we ensure the sidecar proxy being injected has them.

The motivation for this PR was "there's further security hardening you can only do in the container securityContext, the Pods defined in this Helm chart don't set any fields in the container securityContext. Make it configurable so we can set some more fields and harden our Pods". This PR doesn't do any security hardening in itself. Where the injected Pods are already setting some fields in the container securityContext it would be messy to take values from the users and correct/overlay them so the application can run. A better solution in that case would be "harden the injected containers to the maximum that kubernetes allows, only giving containers the capabilities they need". That is pretty much our security requirements. This guidance from the NSA has a lot of good stuff in it on that.

In our use case we're actually running "Native Integration Mode" so we're not using the sidecar injection. I've also not made this change to the sidecar-injector Pod either, but can do so that this change covers "all Pods deployed by Helm chart". For our purposes, this PR with that addition would allow us to set the hardening fields we need. It's very much "make it configurable, we'll do the hardening". But the paragraph above got me thinking whether or not this is the right change for this project...

Every container that's deployed is running functionality delivered by "the Consul project". You know what permissions and capabilities different components need, and thus what "max hardened" would look like (at least in terms of k8s API field values). Does it make sense to make "Pod hardening fields" configurable at all? Unless you want the Helm chart to accommodate people swapping in their own rebuilds of components that might need more permissions. But that does come at the risk to all the other users of "they make their Pods less secure".

I suppose, like with the Pod securityContexts, this PR could be followed up with one to set defaults for these values that provide max hardening. As you mentioned in #602, you were still discussing internally what those defaults should be. When we do our testing, we could feed back to you "we tried max hardening the containers and this is what we found...".

What do you think? Would this PR with the recommended tests added, and having the webhook server container with securityContext configurable (for completeness), be something you'd want to take?

[lkysow]

If you can also determine the correct settings and submit that via PR then that would be welcome. The reason we're not doing that ourselves is simply due to time/priority constraints.

[tarquin-the-brave]

Yeah. As part of integrating this change we're going to do the testing to work out what the maximum container securityContext is that won't break any of the components. We'll then be configuring this chart with those values, so we're more than happy to follow up with a PR to set them as defaults.

[lkysow]

This is great work. Just one small comment and we're good to go.

[tarquin-the-brave]

@lkysow Thanks for your help!

Looks like I leave this with you to merge.

It'll be a couple of weeks yet, but when we do our Pod security hardening exercise I'll make sure to follow this PR up with one to set some strong defaults for these values.