WIP: PoC for Recording seccomp profiles

[rhafer]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PoC adds a controller to the controller that will collect seccomp profiles generated by the seccomp-bpf oci-hook. It works by watching for pods carrying the "io.containers.trace-syscall" annotation which is used by the above oci hook to enable tracing of syscalls issued by a pod and generating a seccomp profiles from that.
After pod completion the controller will collect the generated profile and create a "SeccompProfile" resource in the namespace that the traced pod was running in.

This is still in it's early stages and there are still quite a few things to work on:

• Better error handling
• Documentation
• Tests

And a ton of open questions to be detailed out:

• What to do with the generated profiles? Currently the collected profiles will be just created as a "SeccompProfile" resource and will unconditionally be installed on every node. For that point the profile is generally usable by everybody who can access it. Should we maybe add some flag (label?, annotation?) to the SeccompProfile CRD that will mark a profile as being disabled until e.g. a manual review?
• In the non-namespaced deployment the operatore now needs pretty wide (read) access to resources (namely pods and nodes). It also needs to be able to create "SeccompProfiles" in any namespace. Can this be further restricted?
• Deeper integration with the oci-hook? Currently the integration with the tracing mechanism is pretty rough and based on specifying the right filename/directory in the .trace-syscall annotation. This is not only a bad UX but also raises question about how to cleanup the directory and what to when e.g. to pods specify the same output file.
  • Instead of watching for the hook annotation and giving the user control over the output file I guess we could create a mutating webhook that triggers on a different annotation (something specific to the seccomp operator) and injects the oci-hook annotation into the pod, which would give us better control over the output.
  • The oci-hook could be modified to "submit" the profile directly to the operator (e.g. via grpc) instead of writing a file to disk
• Trying out different approach for the syscall tracing. (See also Record seccomp profiles #46 (comment)) and https://kubernetes.slack.com/archives/C013FQNB0A2/p1600971007007100)

Which issue(s) this PR fixes:

Fixes #46

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rhafer
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[pjbgf]

@rhafer really good job so far! Here's my 2 pence on the open questions:

What to do with the generated profiles? Currently the collected profiles will be just created as a "SeccompProfile" resource and will unconditionally be installed on every node. For that point the profile is generally usable by everybody who can access it. Should we maybe add some flag (label?, annotation?) to the SeccompProfile CRD that will mark a profile as being disabled until e.g. a manual review?

Very good question. Maybe it would be worth keeping it simple for now and just create the CRD profile, which will lead to sync/saving into all nodes. At this point, the user would always need to actively map a profile to a workload, which they should be allowed to. We can refine this further once we have the auto apply in place.

On that point, it would be nice to auto populate the new field that defines what image is this profile for. This is still WIP, just linking here for future reference: #138 (comment)

In the non-namespaced deployment the operatore now needs pretty wide (read) access to resources (namely pods and nodes). It also needs to be able to create "SeccompProfiles" in any namespace. Can this be further restricted?

My understanding is that now that we are deprecating the ConfigMap support, we won't have cluster level profiles anymore. So we probably want to have a default target namespace for the profiles to be generated into so we can have a tight RBAC by default. @cmurphy please correct me if my namespace comment is gibberish. 😄

[rhafer]

On that point, it would be nice to auto populate the new field that defines what image is this profile for. This is still WIP, just linking here for future reference: #138 (comment)

👍

My understanding is that now that we are deprecating the ConfigMap support, we won't have cluster level profiles anymore. So we probably want to have a default target namespace for the profiles to be generated into so we can have a tight RBAC by default.

Hm, not sure I completely understand that. But I think the recorded profile should endup in the same namespace as the pod that it was recorded from. Otherwise it would just be too easy to have two pods from different namespaces overwrite each other's recorded profiles.

[cmurphy]

In the non-namespaced deployment the operatore now needs pretty wide (read) access to resources (namely pods and nodes). It also needs to be able to create "SeccompProfiles" in any namespace. Can this be further restricted?

My understanding is that now that we are deprecating the ConfigMap support, we won't have cluster level profiles anymore.

I didn't fully understand this before, but I guess the "cluster level profiles" are, e.g. the seccomp-operator-profile configmap which gets mounted and copied by the initcontainer to /var/lib/kubelet/seccomp/seccomp-operator.json? Right now SeccompProfile doesn't do that, but maybe it should?

So we probably want to have a default target namespace for the profiles to be generated into so we can have a tight RBAC by default.

I agree with @rhafer that the profile should go in the namespace it was recorded from, and as such I'm not sure how the operator could be further restricted: it needs broad permissions to write SeccompProfiles to any namespace.

[saschagrunert]

Did you use the CRI-O annotation patch to make this work? I'm thinking if we can choose another way to pass the data than the annotation…

[rhafer]

Yeah. For now this depends on the annotation patch. (For reference cri-o/cri-o#4138)

[pjbgf]

Hm, not sure I completely understand that. But I think the recorded profile should endup in the same namespace as the pod that it was recorded from. Otherwise it would just be too easy to have two pods from different namespaces overwrite each other's recorded profiles.

@rhafer I am not opposed to get the recorded profile into the namespace the recorded pod was running from, I just thought this probably would happen as two separate steps to start with:

1. A pod is selected for profile recording, a temporary seccomp profile may be created and will be amended through this process.
2. A user promotes the generated profile by pushing it to a given namespace and applying it to a workload.

Given how difficult it is to get a complete profile, I was thinking on restricting the first part to a given namespace, which we could then enforce simply by RBAC. Then second step would be the user's responsibility until this feature matured.

However, I think it is more important to have an end-to-end process we can play around with then get bog down on this details, so feel free to go ahead and we will then tackle such concerns as they appear.

[rhafer]

rebased

[saschagrunert]

/ok-to-test

[k8s-ci-robot]

@rhafer: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-security-profiles-operator-verify	308ffdc	link	/test pull-security-profiles-operator-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[saschagrunert]

rebased

Do you think that we can demo this at the community meeting this Thursday?

[rhafer]

Do you think that we can demo this at the community meeting this Thursday?

Yeah, that should work. Even if just for discussing some of the road blocks I've hit recently with the current approach :-)

[saschagrunert]

@rhafer I reused most of your work in #247, thank you again for the contribution. I assume that you will not continue this PR now. Feel free to reopen if that is not the case. 🙏

/close

[k8s-ci-robot]

@saschagrunert: Closed this PR.

In response to this:

@rhafer I reused most of your work in #247, thank you again for the contribution. I assume that you will not continue this PR now. Feel free to reopen if that is not the case. 🙏

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

@rhafer: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.