Jail/chroot nginx process inside controller container

[rikatz]

What this PR does / why we need it:

Isolate nginx program into its own userns inside the main controller container.

Using this technique, we can provide a sandbox inside a sandbox (beautiful, uh) so nginx program wont have access to sensitive files from the main container.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Which issue/s this PR fixes

So many...

How Has This Been Tested?

• Generated chroot container and ran inside a Kubernetes cluster (simple approach)
• Next step is to change the e2e tests and point them all to use the chrooted container instead of the non sandboxed container

TODO (Don't merge it before we pass through the TODO!!!)

• Plan the BETA for this, as this may be a breaking change
• We need to make sure it's clear that:
  • We are adding a new sec capability to the container, allowing it to do a unshare/create a new netns inside of it
  • If the admin adds / enables the seccomp feature with default profiles, ingress may stop working. This new approach NEEDS seccomp enabling "clone" syscalls

[rikatz]

/kind bug
/priority important-soon
/triage accepted

[tao12345666333]

wow! Great!

[longwuyuan]

This is definitely such a beautiful direction to take. Next dream is if nginx process or another process makes a unneccessary system call, then it gets SIGKILL. Even bigger dream is declare the sys calls a process is going to make and abide by it. Thanks, ; Long Wu Yuan

…

On 15-Mar-2022, at 9:11 AM, Jintao Zhang ***@***.***> wrote: wow! Great! — Reply to this email directly, view it on GitHub <#8337 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWVHAYUVHJTB5DQY52TVAABFLANCNFSM5QXLMBPQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.

[rikatz]

nope, because the resolv.conf is generated on runtime (when the Pod is bootstrapped) and may change from cluster to cluster :D

[tallclair]

Is there a reason the controller can't be separated out into a different container? Naively that seems like it would be a lot simpler than jailing it within the same container.

[rikatz]

Is there a reason the controller can't be separated out into a different container? Naively that seems like it would be a lot simpler than jailing it within the same container.

Hey @tallclair so let me try to clarify why we didn't followed this split, even this being actually our first attempt:

• One of the main problems is serviceAccount extraction. Mounting the serviceAccount token is a per pod, and not a per container configuration (I guess, at least I couldn't find a way to say "don't mount this SA on this container only"). So, even if we split the containers, the SA will still be there, usable if someone can add arbitrary configs to read the container FS. BTW, I can write a KEP on this to have automountServiceAccount per POD, although I promised myself to be away from KEPs for the next 3 years ;P

• Based on that, another approach was: "let's disable the SA, and mount/project a specific SA in a specific container" (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.22/#serviceaccounttokenprojection-v1-core). This works "partially". There are some weird workaround that needs to be made so client-go can use the token and behave as "inCluster". I'm not saying this is not impossible, I could make it work, but there are so many changes required just to use this token that it was a huge effort to change it.

• Now, considering that we could mount the SA only on one container and not on the other (based on any of the approaches above), there are some restriction in the way ingress-nginx was built that turns into a lot more code changes. Starting with the process monitoring. The ingress-controller process monitors, via a channel open with the nginx process start, if the process is respawning. To be more broad, ingress-controller process not only is responsible for that, but for all the control of the process. It uses nginx -t to test if a new template is valid. It reloads. It stops when requested to stop. So it needs not only to share the same PID namespace, but also to have access to that same binary. One thing we started to discuss here was to move all this process logic to another Go program, and receive the commands via a REST API or gRPC (like ingress controller calls the endpoint to start, stop, restart the container). This would take at least 15 days of implementation, as I'm not fully focused on ingress-nginx :)

• We can share the PID NS between both containers without any problem as well. But there are some caveats on that restart/reload/stop/etc process that wasn't that easy

Thinking on the same approach kube-proxyv2 is taking (aka kpng), we started to discuss about this as an opportunity to split CP/DP using a single gRPC controller and multiple proxies talking only with the controller and getting the right information. This is, for us, the new ingress-nginx and the desired state of art. But, again, at least myself stopped on the amount of time I have to develop vs amount of time required to make it work :)

So, we didn't either wanted to stop on the promise of a new implementation or a hard workaround, after all this needed to be fixed fast. And this is why the chroot/jail solution came on a weekend discussion and was implemented in like 4 hours :) I've spent some time as well researching the previous solutions, but all of them needed deep changes in the code not as easy as it seems (I though it was going to be easy the first time as well!)

Makes sense? :)

[tallclair]

@rikatz thanks for the detailed response.

I can write a KEP on this to have automountServiceAccount per POD

I might be misunderstanding you, but there's already a pod-level AutomountServiceAccountToken field: https://github.com/kubernetes/api/blob/master/core/v1/types.go#L3134-L3136

There are some weird workaround that needs to be made so client-go can use the token and behave as "inCluster".

Do you have any more details on what went wrong? I'm guessing you're missing some of the other projected data. Easiest thing is to just copy the projected volume from a pod with the SA auto mounted:

  - name: kube-api-access-7g4tx
    projected:
      defaultMode: 420
      sources:
      - serviceAccountToken:
          expirationSeconds: 3607
          path: token
      - configMap:
          items:
          - key: ca.crt
            path: ca.crt
          name: kube-root-ca.crt
      - downwardAPI:
          items:
          - fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
            path: namespace

So it needs not only to share the same PID namespace, but also to have access to that same binary.

I'm sure I don't understand all the nuances here, but if the issue is just that the controller needs access to the nginx binary, then you could consider just putting everything you need in a single container image, and run both containers off the same image (just with a different entrypoint / command).

We can share the PID NS between both containers without any problem as well. But there are some caveats on that restart/reload/stop/etc process that wasn't that easy

Yeah, that makes sense. I think stop should be straightforward? In place of restart, could you just have something that monitors the process (e.g. systemd, or just rely on the kubelet) and restarts it as soon as it's killed?

I'm just guessing here... I can see why you might need the gRPC interface instead.

---

On a separate topic, you might want to have a look at AppArmor hats, which are a way to change the AppArmor context, designed to change privileges on the fly. It might be a good fit for this use case (but only works on AppArmor systems).

[tao12345666333]

I understand that currently, we can complete image name conversion through helper functions, but once chroot is enabled, should we cancel digest?

The expected behavior is that we use a configuration item to allow users to switch images without perception, but digests are not the same. If the user configures digests, the image download will fail.

[rikatz]

nope, I think we may have digest for chroot as well. This can be another argument, it's fine. Because we don't have digest yet (no image published) I forgot this, but we can fix this on a followup

[tao12345666333]

OK,I agree

[tao12345666333]

nit: keep the previous import order

[rikatz]

🤦 fixing

[rikatz]

@tao12345666333 fixed the helm thing, please take a look into it

[tao12345666333]

I think this is great! If you want to merge this PR at any time please remove the hold

/lgtm
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, tao12345666333

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:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rikatz]

/hold cancel
So I can sleep fine and start working on another part tomorrow :)