Add Seccomp Notify support using UNIX sockets and container metadata

[rata]

Hi!

This is a follow-up of PR #1073 using the alternative discussed here. The reasoning to go this way over the others is here

I’m a co-worker of @alban and the commit has his sign-off too :)

This adds the specification for Seccomp Userspace Notification and the
Golang bindings. This contains:

• New fields in the seccomp section to use with seccomp userspace
  notification.
• Additional SeccompState struct containing the container state and file
  descriptors passed for seccomp.

This was discussed in the OCI Weekly Discussion on September 16th,
2020. After review on github, this implementation was changed to the
"Proposal with listenerPath and listenerExtraMetadata". For more
information see:

• Add Seccomp Notify support #1073 (comment)

Docs presented on the community meeting (for the old implementation
using hooks):

• https://hackmd.io/El8Dd2xrTlCaCG59ns5cwg#September-16-2020
• https://docs.google.com/document/d/1xHw5GQjMj6ZKR-40aKmTWZRkvlPuzMGQRu-YpOFQc30/edit

Documentation for this feature:

• https://www.kernel.org/doc/html/v5.0/userspace-api/seccomp_filter.html#userspace-notification
• man pages: seccomp_user_notif.2 at
  https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/log/?h=seccomp_user_notif
• brauner's blog:
  https://brauner.github.io/2020/07/23/seccomp-notify.html

This PR is an alternative proposal to PR #1038 . While similar in nature,
the main difference is that this PR adds optional metadata to be sent to
the seccomp agent and specifies how the UNIX socket MUST be used.

Signed-off-by: Alban Crequy alban@kinvolk.io
Signed-off-by: Rodrigo Campos rodrigo@kinvolk.io

---

We will make a PR for runc implementing this shortly.

Please also note that this change doesn’t imply a fork+exec, as @giuseppe wanted to avoid

Thanks again and don't hesitate to ask any questions! (I'll answer the ones in Alban's PR shortly :))

cc @KentaTada @ManaSugi @giuseppe @AkihiroSuda @brauner @alban @mauriciovasquezbernal

[giuseppe]

I am not sure about implementing pidFd as part of seccomp notifications.

There are cases where pidfd is useful also without seccomp notifications, should we have a separate way for passing the pidfd?

[alban]

I would be fine if the spec define another separate way for passing pidfds.

But I feel it is useful here as well. Note that this pidFd is not necessarily referring to the "pid 1" of the container, but in the case of "runc exec", it is the pid that entered in the container. I don't see how we can have a separate way for passing the pidfd in that case.

[giuseppe]

what do you think if we had something not tied to seccomp like?

{
    "ociVersion": "0.2.0",
    "fds" : {
       "seccompFd" : 0,
       "pidFd": 1,
       "futureFeatureFd" : 2
    },
    "pid": 4422,
    "state": {
        "ociVersion": "0.2.0",
        "id": "oci-container1",
        "status": "creating",
        "pid": 4422,
        "bundle": "/containers/redis",
        "annotations": {
            "myKey": "myValue"
        }
    }
}

Would that work as well?

I am thinking we already have a way to pass the tty fd (with --console-socket) and we could perhaps cover it as part of the OCI configuration instead of having it as a CLI option

[rata]

@giuseppe maybe this is a silly question, I think I'm a bit lost. How will the seccomp agent know about the new containers? Do you mean to keep the listenerPath and listenerMatadata in the seccomp section but send this more generic state instead of a seccomp state? In that case, how will be useful for other (non-seccomp) uses? Or do you mean to add fields to the state (not as your example json, as they are not part of the state) and use something like this?

I'm not sure I follow what you mean, or how we can use it from a seccomp agent that doesn't know when runc creates a container or "runc exec" is run in a container, so we have two (or more) pidfds to report: the fd for pid 1 of the container and the fd for the process that run "runc exec".

I'm fine with another mechanism, just not sure I understand it. I'll try to wake up and read your message again :)

[giuseppe]

my suggestion was just about using a map for holding the file descriptors and pass the same metadata. In this way we could extend it in future, if we'll need any new kind of fd.

[alban]

It sounds good to me to use a map[string]int instead of individual fields for seccompFd and pidFd.

If you want to make it more generic, I guess you want to rename type SeccompState in specs-go/state.go... Do you have an idea of new name?

[rata]

@giuseppe ohh, sorry, I see now! SGTM too, seems like a nice idea. I'll adapt to create a "fds" section in the seccomp state.

Let me know if you want to rename the seccomp state to some other (more generic) thing as alban suggested :)

[rata]

@giuseppe I've moved to the map as you suggested. I couldn't find a good name for the struct.

What name do you suggest? :)

[giuseppe]

can't really think of anything good to suggest. Maybe ContainerState or ContainerProcessState?

[mauriciovasquezbernal]

@giuseppe I just updated the PR to call it ContainerProcessState. Please let us know what you think. Thanks!

[alban]

I am adding some suggestions that I didn't notice before.

[alban]

I would be fine if the spec define another separate way for passing pidfds.

But I feel it is useful here as well. Note that this pidFd is not necessarily referring to the "pid 1" of the container, but in the case of "runc exec", it is the pid that entered in the container. I don't see how we can have a separate way for passing the pidfd in that case.

[alban]

runc has a draft PR (opencontainers/runc#2682) implementing this.

[sargun]

One question about the version negotiation / how many FDs to allocate on the receiving side prior to doing the receive, but otherwise lgtm.

[vbatts]

maybe where you have map as the type, put the state as a link to that struct definition

[mauriciovasquezbernal]

Good idea. Done.

[vbatts]

nit: bullet list need not have periods at the end

[mauriciovasquezbernal]

Isn't it the other way around? I see end periods everywhere 🤔

[vbatts]

On the whole, this seems fine and developed enough. (it helps to have worked through the implementation on runc, and related work already done in crun).
By being in the spec, we can have consistent behavior across runtimes. That's the point.

This is not a breaking change, as the usage and fields are optional.
👍

[mauriciovasquezbernal]

@giuseppe @AkihiroSuda @KentaTada @ManaSugi I discussed this very briefly at the last OCI meeting.

Since AFAIK you're the ones more interested on this, I'd like to get your feedback and if there are any concerns to move this forward. Thanks!

[giuseppe]

should seccompFd be optional as well?

[alban]

I don't mind if seccompFd is optional in the spec, but how would it work if the runtime does not pass the seccompFd? Do you want to make it optional for runtimes that don't want to implement seccomp-notify, or are you thinking of an alternative way to obtain the seccompFd?

For runtimes that use seccomp-notify but implements the seccomp agent internally, they can just not set the listenerPath field.

The "Container Process State" section is separate and does not mandate the seccompFd, but this section is in the description of "listenerPath".

[giuseppe]

could it be possible to use it for retrieving pidFd (or anything more that is added in future) but not seccompFd? e.g. what happens if the profile doesn't set any rule with SECCOMP_ACTION_NOTIFY? Does the OCI runtime still need to create a seccompFd?

[rata]

If the profile doesn't set any notify rule, we propose this field should be ignored by the runtime. See here: https://github.com/opencontainers/runtime-spec/pull/1074/files/d87dd02ab1435993552f72fd1d98c7779f67228a#diff-048d23d864e15683f516d2c1768965d546e87f8a59b2606cf2f2d52500ba5a32R633.

Our idea is to use this UNIX socket to send to a seccomp agent. Do you want to make this field more "generic"? Be aware that this is in the seccomp section, we can reuse the container state process in other parts of the spec, but this should be about seccomp.

[giuseppe]

so do you think we should move the listenerPath outside the seccomp section? I think it should be possible to use pidFd without seccomp notify, or seccomp in general

[alban]

so do you think we should move the listenerPath outside the seccomp section?

I think we should not; it would make it more difficult to use in Kubernetes (see #1073 (comment)).

I think it should be possible to use pidFd without seccomp notify, or seccomp in general

If you want a mechanism to get the pidFd without seccomp, I'd rather doing that separately in a separate field than seccomp.listenerPath, even if it reuses this new "Container Process State" type.

There can be only one seccomp agent receiving the seccompFd (by design from #1073 (comment)). But I see use cases where several processes could receive the pidFd, so maybe it could be an array of listeners. Could this be a follow-up PR for this? In your use cases, would you want to receive the pidFd at container-create time or at container-start time?

[rata]

@giuseppe friendly ping? :)

[rata]

@AkihiroSuda @KentaTada @ManaSugi Friendly ping? It would be super helpful for us if we can get a review for this :)

[rhatdan]

@runtime-spec PTAL

[rhatdan]

@mrunalp PTAL

[sargun]

So, originally, you needed to support pidfd, because seccompfd left you with no way to determine if the container had been terminated or not. Now that there is tracking for that, it might be required. Do you think we should keep it?

[mauriciovasquezbernal]

I removed it from this PR as it's not strictly necessary anymore.

[sargun]

LGTM. Previously, there was the issue that you needed the PIDFD in order to track the container's lifetime. Now that we no longer need to do that because the epoll call will return EPOLLHUP (https://github.com/torvalds/linux/blob/master/kernel/seccomp.c#L1697-L1698) on the fd if there are no more containers, do we still need that?

In addition to that, I fear that if we embed the pid into the metadata, it will be problematic if the notifier is running in its own pid ns (for example a daemonset?). Do we want to ensure that the process that is sending the is the one that matches the pid, therefore enabling the use of SCM_CREDENTIALS to find out the pid of the other side? Or at least being explicit and saying which process will send this?

[giuseppe]

same here, I'd move this outside the seccomp section so it can be used also without seccomp

[rata]

@sargun

LGTM.

Thanks for the review!

Previously, there was the issue that you needed the PIDFD in order to track the container's lifetime. Now that we no longer need to do that because the epoll call will return EPOLLHUP (https://github.com/torvalds/linux/blob/master/kernel/seccomp.c#L1697-L1698) on the fd if there are no more containers, do we still need that?

Good point. I think it can be useful anyways, though. The pidfd refers to the container 1 process if a container is being created, or to the process doing “kubectl exec”. I think in some cases (specially the pid of the process connected via an “exec” might be tricker to get) is useful, and if the agent will not use it can just close it.

In addition to that, I fear that if we embed the pid into the metadata, it will be problematic if the notifier is running in its own pid ns (for example a daemonset?). Do we want to ensure that the process that is sending the is the one that matches the pid, therefore enabling the use of SCM_CREDENTIALS to find out the pid of the other side? Or at least being explicit and saying which process will send this?

Well, the same thing happens with the hooks. Just running the agent in the host pid will do the trick. In a daemonset, just using hostPID: true in the daemon set will do it, we do that here for example.

Also, I don’t think we want to ensure that the sender is the pid. Runc implementation is not doing that. And that's not doable today if sendmsg() is a syscall blocked by the seccomp policy, for example. In addition, it can also create issues for rootless in the future (in runc, it is sent by the runc parent and you need to have CAP_SYS_ADMIN to send a different pid in SCM_CREDENTIALS, which will complicate rootless, etc.).

Furthermore, I don’t think the the spec needs to be explicit about which process is sending the message, it just needs to say that the pid is from the container runtime namespace. The PR is already explicit about that, so IMHO it is fine: https://github.com/opencontainers/runtime-spec/pull/1074/files#diff-048d23d864e15683f516d2c1768965d546e87f8a59b2606cf2f2d52500ba5a32R719

What do you think?

[giuseppe]

Having the listenerPath on the seccomp section, as it is now, seems nice because: seccomp can only have one supervisor, so no point in sending the seccomp-fd to another process that will pass it to one seccomp agent in the host (specially problematic if there are more than one agent, as different agents may implement handling for different syscalls) and we can later add a new listenerPath in the linux section too if needed. Both listenerPath can even point to the same path if we want, the message currently in the spec has all the metadata needed to know what it is about (annotations, which fds are sent, etc.).

yes I think this makes sense since we send also all the needed metadata and the same listenerPath can be used both for "seccomp" and future "linux" FDs.

[rata]

I squashed the commits and fixed the conflict!

[giuseppe]

LGTM

[vbatts]

Glad to see this evolve and that there is a corresponding implementation for it (opencontainers/runc#2682).

LGTM