Convert Seccomp support to use Libseccomp #70
Conversation
|
Nice! I will test this out today. |
|
@mheon Thanks! |
|
+1 |
|
Needs rebase. |
|
We now have an updated Makefile and a test Dockerfile so could update those with changes similar to the original PR. |
|
Rebased atop latest runc. Old tests from Libcontainer PR are included, and should be running - need to check that once they finish. Seccomp is no longer exposed via the Linux spec, as that has moved into the Specs repository. Might want a separate PR there to discuss how it should be presented in the spec. |
| @@ -53,6 +53,10 @@ | |||
| { | |||
| "ImportPath": "github.com/syndtr/gocapability/capability", | |||
| "Rev": "e55e5833692b49e49a0073ad5baf7803f21bebf4" | |||
| }, | |||
There was a problem hiding this comment.
I wonder if you added this by hands. Because it's not valid json.
There was a problem hiding this comment.
Fixed a merge conflict by hand, and didn't verify it still worked afterwards... Oops.
| @@ -0,0 +1,48 @@ | |||
| package seccomp | |||
There was a problem hiding this comment.
I don't think this API is as good as what we currently have. The current model is more clear and you have the ability to set the exit status that you want returned and not having alot of strings for the syscalls and operators. Those are both type safe currently. Can you not use the existing API and just swap out the implementation with libseccomp?
There was a problem hiding this comment.
I have a few issues with the current API, but nothing I would consider major.
- No ability to set default action - much harder to make a whitelist, you basically have to list every syscall. Should be simple to add to the Seccomp struct.
- I think representing syscalls by name, and not number, is superior. Architecture independence and clarity, mainly. Libseccomp gives us the ability to easily do architecture-independent resolution.
- I don't think a one-op masked equality operator makes much sense --- might as well use the libseccomp convention of two operands per syscall argument.
There was a problem hiding this comment.
Given this, I'd be willing move to the existing API, but I'd like to make some of these changes at the same time.
There was a problem hiding this comment.
+1 to adding a default action and removing the whitelist bool.
other changes sound ok for now
|
@crosbymichael Converted to use the old API, with the alterations discussed. |
| actAllow = libseccomp.ActAllow | ||
| actTrap = libseccomp.ActTrap | ||
| actKill = libseccomp.ActKill | ||
| actErrno = libseccomp.ActErrno.SetReturnCode(int16(syscall.EPERM)) |
There was a problem hiding this comment.
Should this be configurable?
There was a problem hiding this comment.
Can be made configurable pretty easily, if there's a demand for it. Though it would require modification to the API to let users configure the return code.
There was a problem hiding this comment.
I think we can change it later if there is a need.
|
@LK4D4 Could you take a look? |
|
@mheon what is necessity to have libseccomp 2.2.1? I'm not sure that all distributions have it. |
|
@LK4D4 v2.2.1 fixes a filter code generation bug which I found when writing the Go bindings. It's required for correct behavior in conditional syscall blocking. |
| @@ -4,7 +4,7 @@ TEST_DOCKERFILE=script/test_Dockerfile | |||
| export GOPATH:=$(CURDIR)/Godeps/_workspace:$(GOPATH) | |||
|
|
|||
| all: | |||
| go build -o runc . | |||
| go build -tags seccomp -o runc . | |||
There was a problem hiding this comment.
-tags should be variable, so you can disable seccomp
|
LGTM |
|
Rebased and squashed commits to clean up history. |
|
Another rebase to fix a merge conflict |
|
@crosbymichael @LK4D4 ping |
|
Recognizing that this is basically impossible to test at present, I've pushed a branch to my fork called seccomp_testing (https://github.com/mheon/runc/tree/seccomp_testing) that exposes Seccomp via the spec and generates spec files with sample Seccomp rules. I recommend testing with an image using Coreutils, not Busybox, if possible - Coreutils is a lot more dutiful about checkout return codes from system calls, even ones which theoretically can't return errors, which makes it easier to identify that Seccomp is doing its job. |
|
Rebased. My fix on the conflict in the Makefile is a bit questionable - someone should double-check it. |
|
Cool that my mention in a commit message on my runc branch popped up here. That fixes the same problem we were having with runc. See docker-archive/libcontainer#655 and #113. However, I haven't created a PR for it yet. How close is this to merging? If close, I can create that PR dependant on this one. |
|
Why isn't libsccomp 2.1.x supported? |
|
I found a bug in filter code generation when I was writing/testing the Go bindings for Libseccomp, and it was fixed in v2.2.1 - previous versions have issues with conditional syscall blocking in some circumstances (will discard all but the last rule in a chain, for example). I fixed the minimum required version at v2.2.1 to ensure that nobody would run into that issue. |
|
Ok, i'm not sure a hard failure is needed because all the packaged versions for libseccomp on ubuntu LTS's are 2.1.x making this patch unusable on those systems even if conditionals are broken. Do they know they are shipping a broken lib? |
|
After a quick conversation with the upstream in Debian, the package maintainer has agreed to bump the version to incorporate the bugfix - should start trickling out to distros soon. I would not be opposed to altering the patches to allow v2.1 to be used, but I'd want to disable conditional syscall filtering completely for versions of the library with the bug, to ensure nobody ends up with a known-broken config out of the box. |
|
@mheon I think that is better |
|
@mheon let us know after you have the chance to update the PR removing the build time restriction, should be an easy merge after that. |
…/70-for-docker-1.8.0-rc3 Apply PR opencontainers/runc#70 to test docker 1.8.0-rc3 on RPi.
|
To give a brief update, patches to the vendored libseccomp bindings to support v2.1 are presently being reviewed. Once they're merged, I can pull in the updated bindings here, and this should be good to merge. |
This removes the existing, native Go seccomp filter generation and replaces it with Libseccomp. Libseccomp is a C library which provides architecture independent generation of Seccomp filters for the Linux kernel. This adds a dependency on v2.2.1 or above of Libseccomp. Signed-off-by: Matthew Heon <[email protected]>
Signed-off-by: Matthew Heon <[email protected]>
As v2.1.0 is no longer required for successful testing, do not build it in the Dockerfile - instead just use the version Ubuntu ships. Signed-off-by: Matthew Heon <[email protected]>
|
New bindings made it upstream, so I've included them here. This should work with any library version >= 2.1.0, though with reduced functionality before the v2.2.1 conditional blocking fix. I did have to manually update the Godeps - running into tools/godep#117 as the only includes of the Seccomp code are conditional on the seccomp build tag. Could be solved by just removing said tag? |
|
Removed the build tag to fix Godeps. Easy enough to remove the commit if we want to keep the tag in. @crosbymichael @LK4D4 @mrunalp This should be ready for final testing & merging now |
Signed-off-by: Matthew Heon <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
Convert Seccomp support to use Libseccomp
|
Pretty new here. How do I apply #70 to my docker 1.8.1? |
|
@pavanpalaksha You could use the hack/vendor.sh script in Docker to vendor any commit of runc after the merge and the libseccomp-golang dependency added by this PR. After that, you'd need to edit the Dockerfile in the root directory of Docker and add libseccomp-devel to the dependencies installed by Apt. However, note that there is an incompatibility in required versions of the go-systemd library (v2 in 1.8.1, v3 in the latest runc master) that will cause compile errors if you go this route - you'd need to resolve those manually, or cherry-pick just the commits in this PR into the runc vendored into Docker. |
This is a continuation of docker-archive/libcontainer#633 (itself a continuation of a number of other pull requests).
The reasons for this were already stated in the previous PR, but I'll paraphrase what I said in that PR:
The library-based solution is at present a better solution than the current Golang version. Libseccomp is well-tested, well-supported, and high-performance, and removes the burden of supporting filter generation code.
The code is almost identical to the previous libcontainer PR, save a few import path changes, and exposing it via the runc Linux spec. My testing has been somewhat minimal, but everything still seems to work.
Closes #155