Remove dependency in dockerd on libseccomp by cpuguy83 · Pull Request #41395 · moby/moby

cpuguy83 · 2020-08-27T23:25:22Z

This was just using libseccomp to get the right arch, but we can use
GOARCH to get this.
The nativeToSeccomp map needed to be adjusted a bit for mipsle vs mipsel
since that's go how refers to it. Also added some other arches to it.

Closes #41299

AkihiroSuda · 2020-08-28T09:10:50Z

Should be removed from vendor?

Also Dockerfile should no longer need aptgetting libseccomp

Still need libseccomp to build runc at least.

I may push some images for runc and containerd since these rarely change and just consume those in our Dockerfile...

thaJeztah · 2020-08-28T12:12:33Z

Build is failing;

[2020-08-27T23:32:31.365Z] === Errors
[2020-08-27T23:32:31.365Z] profiles/seccomp/seccomp.go:102:37: undefined: arch
[2020-08-27T23:32:31.365Z] profiles/seccomp/seccomp.go:121:38: undefined: arch
[2020-08-27T23:32:31.365Z] profiles/seccomp/seccomp.go:102: undefined: arch
[2020-08-27T23:32:31.365Z] profiles/seccomp/seccomp.go:121: undefined: arch

cpuguy83 · 2020-08-28T19:17:22Z

Updated to remove seccomp build tag and instead check at runtime if seccomp is supported.
Also removes vendor and moves libseccomp-dev install to just run runc build stage.

thaJeztah · 2020-08-28T20:30:19Z

wondering; what would happen if we didn't even check, and just passed this down to containerd/runc (having them deal with wether or not to do something with it)?

Looks like it would error out: https://github.com/opencontainers/runc/blob/2c632d1a2de0192c3f18a2542ccb6f30a8719b1f/libcontainer/seccomp/seccomp_unsupported.go#L16

Sorry that's different... when runc is not compiled with seccomp support.

Most likely what will happen here is the kernel will throw an error.

That's if runc applies it; I wonder if runc should do the check, and skip that part of the spec if it's not supported 🤔

Basically; if could have multiple OCI runtimes configured, and the docker daemon doesn't know what they support (hope you follow my train of thought)

I follow, but I think at the runc level you just want it to execute what you told it to.
If you pass a seccomp profile and it doesn't load it but continues anyway, seems like this would cause some security issues at the very least.

cpuguy83 · 2020-09-03T18:53:08Z

Hold off on merging... realized there's an issue here where some files are still depending on the 'seccomp' build tag.

cpuguy83 · 2020-09-03T22:48:32Z

I pulled back on those buildtag changes just because there's a ton of places where it's used.
I can deal with this in a follow up since it's really a different change.

tianon

LGTM (although test failure looks weird)

thaJeztah · 2020-09-10T23:51:21Z

@cpuguy83 tests are still failing, so looks related

cpuguy83 · 2020-09-11T20:10:22Z

Old profile -> New profile... 🤔

-                                 "arch_prctl"
-                         ],
-                         "action": "SCMP_ACT_ALLOW"
-                 },
-                 {
-                         "names": [
-                                 "modify_ldt"
-                         ],
-                         "action": "SCMP_ACT_ALLOW"
-                 },
-                 {
-                         "names": [

cpuguy83 · 2020-09-11T21:39:13Z

Should be fixed now.
Had to add a mapping from goarch -> libseccomp string -> docker arch type

thaJeztah · 2020-09-11T21:54:55Z

nice catch that this was inside the loop 👍

thaJeztah · 2020-09-11T21:58:49Z

couldn't we change this to goarchToSeccomp ?

so change this to "386" ?

ah, nevermind; looks like arch is used further down (didn't show in the diff)

Guess we could still do it, but we would need both mapping (but it would change to GOARCH -> native, and GOARCH -> seccomp)

Yeah this was pretty much why it was failing. Didn't realize we were using the libseccomp arch string.

Do you think it will be cleaner to have GOARCH -> nativeArch and GOARCh -> seccompArch, so that they can be used independently?

diff --git a/profiles/seccomp/seccomp_linux.go b/profiles/seccomp/seccomp_linux.go index 7847085a0a..ac6441d6c3 100644 --- a/profiles/seccomp/seccomp_linux.go +++ b/profiles/seccomp/seccomp_linux.go @@ -27,16 +27,16 @@ func LoadProfile(body string, rs *specs.Spec) (*specs.LinuxSeccomp, error) { return setupSeccomp(&config, rs) } -// libseccomp string => seccomp arch -var nativeToSeccomp = map[string]types.Arch{ - "x86": types.ArchX86, +// GOARCH => seccomp arch +var goToSeccomp = map[string]types.Arch{ + "386": types.ArchX86, "amd64": types.ArchX86_64, "arm": types.ArchARM, "arm64": types.ArchAARCH64, "mips64": types.ArchMIPS64, - "mips64n32": types.ArchMIPS64N32, - "mipsel64": types.ArchMIPSEL64, - "mips3l64n32": types.ArchMIPSEL64N32, + "mips64p32": types.ArchMIPS64N32, + "mips64le": types.ArchMIPSEL64, + "mips64p32le": types.ArchMIPSEL64N32, "mipsle": types.ArchMIPSEL, "ppc": types.ArchPPC, "ppc64": types.ArchPPC64, @@ -97,8 +97,7 @@ func setupSeccomp(config *types.Seccomp, rs *specs.Spec) (*specs.LinuxSeccomp, e } } - arch := goToNative[runtime.GOARCH] - seccompArch, archExists := nativeToSeccomp[arch] + seccompArch, archExists := goToSeccomp[runtime.GOARCH] if len(config.ArchMap) != 0 && archExists { for _, a := range config.ArchMap { @@ -115,6 +114,8 @@ func setupSeccomp(config *types.Seccomp, rs *specs.Spec) (*specs.LinuxSeccomp, e newConfig.DefaultAction = specs.LinuxSeccompAction(config.DefaultAction) Loop: + arch := goToNative[runtime.GOARCH] + // Loop through all syscall blocks and convert them to libcontainer format after filtering them for _, call := range config.Syscalls { if len(call.Excludes.Arches) > 0 {

It's different, I don't think it's better or worse.

Yeah, it's not a blocker, as non of this is exported. Thought it would be slightly easier to understand, as we wouldn't be mapping twice (GOOS -> native -> seccomp).

I can open a follow-up to discuss

This was just using libseccomp to get the right arch, but we can use GOARCH to get this. The nativeToSeccomp map needed to be adjusted a bit for mipsle vs mipsel since that's go how refers to it. Also added some other arches to it. Signed-off-by: Brian Goff <[email protected]>

Signed-off-by: Brian Goff <[email protected]>

As it turns out, we call this function every time someone calls `docker info`, every time a contianer is created, and every time a container is started. Certainly this should be refactored as a whole, but for now, memoize the seccomp value. Signed-off-by: Brian Goff <[email protected]>

thaJeztah

LGTM

cpuguy83 requested review from AkihiroSuda, justincormack and thaJeztah August 27, 2020 23:36

AkihiroSuda reviewed Aug 28, 2020

View reviewed changes

thaJeztah added this to the 20.03.0 milestone Aug 28, 2020

thaJeztah added area/security/seccomp kind/refactor PR's that refactor, or clean-up code status/2-code-review labels Aug 28, 2020

cpuguy83 force-pushed the no_libseccomp branch from 77ed42c to c5d66fe Compare August 28, 2020 19:13

cpuguy83 force-pushed the no_libseccomp branch 2 times, most recently from 003d82b to de6600f Compare August 28, 2020 19:39

thaJeztah reviewed Aug 28, 2020

View reviewed changes

cpuguy83 requested review from AkihiroSuda and thaJeztah September 2, 2020 00:26

AkihiroSuda approved these changes Sep 2, 2020

View reviewed changes

cpuguy83 force-pushed the no_libseccomp branch from de6600f to 841d085 Compare September 3, 2020 18:30

tianon approved these changes Sep 3, 2020

View reviewed changes

cpuguy83 force-pushed the no_libseccomp branch from 841d085 to 1dcd4ee Compare September 3, 2020 22:48

tianon approved these changes Sep 4, 2020

View reviewed changes

cpuguy83 force-pushed the no_libseccomp branch from 1dcd4ee to 0ff9eae Compare September 11, 2020 21:37

thaJeztah reviewed Sep 11, 2020

View reviewed changes

Comment thread profiles/seccomp/seccomp_linux.go Outdated

cpuguy83 added 2 commits September 11, 2020 22:48

Remove libseccomp-golang from vendor

b83dc8e

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the no_libseccomp branch from 0ff9eae to df7031b Compare September 11, 2020 22:49

thaJeztah approved these changes Sep 15, 2020

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Sep 15, 2020

thaJeztah merged commit 1155b6b into moby:master Sep 15, 2020

cpuguy83 deleted the no_libseccomp branch September 15, 2020 16:21

AkihiroSuda added the impact/changelog label Sep 29, 2020

Uh oh!

Conversation

cpuguy83 commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 28, 2020

Uh oh!

cpuguy83 commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Sep 3, 2020

Uh oh!

cpuguy83 commented Sep 3, 2020

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 10, 2020

Uh oh!

cpuguy83 commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cpuguy83 commented Sep 11, 2020 •

edited

Loading