Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Seccomp Notify support #1

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add Seccomp Notify support #1

wants to merge 3 commits into from

Conversation

alban
Copy link
Member

@alban alban commented Sep 29, 2020

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

  • A new OCI hook "sendSeccompFd" used to pass the seccompfd to an external seccomp agent via the hook.
  • 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, see:

Documentation for this feature:

This PR is an alternative proposal to PR 1038.

Signed-off-by: Alban Crequy [email protected]

@alban alban force-pushed the alban/seccomp branch 2 times, most recently from d53362d to 483dfbf Compare October 22, 2020 15:53
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments and suggestions.

config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
specs-go/state.go Outdated Show resolved Hide resolved
runtime.md Outdated Show resolved Hide resolved
config-linux.md Show resolved Hide resolved
@alban
Copy link
Member Author

alban commented Oct 23, 2020

Thanks for the review @mauriciovasquezbernal !

I addressed your comments. Another change I'd like to make is on the State, to make it look like in the proposal:

    “seccomp”: {
        "fds": {
            seccomp-fd: 3
        },
        pid: 4499    # this is the process on which the seccomp policy is attached to (!=pid1)
}

Also, the Travis tests failed, but I am not sure why. I'll check that.
Edit: Travis tests to be fixed in upstream PR 1072.

Without this, Travis CI only works on the main repository
(github.com/opencontainers/runtime-spec) but not on forks. It is useful
to make Travis CI work on forks for contributors to be able to test
their work before submitting a PR upstream.

See:
https://docs.travis-ci.com/user/languages/go#go-import-path

Symptoms:
```
$ make docs
go run ./.tool/version-doc.go > version.md
.tool/version-doc.go:10:2: cannot find package "github.com/opencontainers/runtime-spec/specs-go" in any of:
	/home/travis/.gimme/versions/go1.11.13.linux.amd64/src/github.com/opencontainers/runtime-spec/specs-go (from $GOROOT)
	/home/travis/gopath/src/github.com/opencontainers/runtime-spec/specs-go (from $GOPATH)
Makefile:53: recipe for target 'version.md' failed
make: *** [version.md] Error 1
The command "make docs" exited with 2.
```

Signed-off-by: Alban Crequy <[email protected]>
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more minor comments, one important about the exec phase.
Should this PR also update the Lifecycle ?

config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
config.md Outdated Show resolved Hide resolved
specs-go/state.go Outdated Show resolved Hide resolved
alban added 2 commits October 28, 2020 16:47
This adds the specification for Seccomp Userspace Notification and the
Golang bindings. This contains:
- A new OCI hook "sendSeccompFd" used to pass the seccompfd to an
  external seccomp agent via the hook.
- 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, see:
- 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.

Signed-off-by: Alban Crequy <[email protected]>
Fixup following reviews

Signed-off-by: Alban Crequy <[email protected]>
@alban
Copy link
Member Author

alban commented Oct 28, 2020

I updated the branch based on reviews.

Should this PR also update the Lifecycle ?

I am not sure. I didn't update it.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alban
Copy link
Member Author

alban commented Oct 29, 2020

Upstream PR 1073

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants