Use seccomp library for adding seccomp configuration flags #159

grantseltzer · 2016-07-28T00:29:57Z

I added functionality to specify syscalls and their arguments for all actions. Also changed default action and archs flags to ociseccompgen library.

Now we can do things like:

ocitools generate --seccomp-errno clone,write --seccomp-trace getcwd --seccomp-trap umount:0:1:2:NE

The behavior mimics that of: https://github.com/GrantSeltzer/Manhattan

Also updated man page to reflect this change.

Signed-off-by: Grantseltzer grantseltzer@gmail.com

grantseltzer · 2016-07-28T00:46:35Z

~~Failing for a different reason, fixing right now.~~
@mrunalp :D 👍

wking · 2016-07-28T03:48:54Z

cmd/ocitools/generate.go

 		if err != nil {
 			return err
 		}
 		return nil
 	},
 }

-func setupSpec(g *generate.Generator, context *cli.Context) error {
+func setupSpec(g generate.Generator, context *cli.Context) error {


Why are we removing the pointer here?

They just landed via #138.

Hmmm, Must have accidentally cut it when making the new branch. Thanks for the call out.

@wking Fixed

wking · 2016-08-02T19:47:11Z

dfe2722 → cfb6d90 talks about rebasing, and pulls in some recent
master changes like #170, but doesn't seem to have actually rebased
anything.

$ git log --graph --oneline --decorate origin/pr/159 | head -n4

cfb6d90 (origin/pr/159) Rebase and clean up seccomp generate api
dfe2722 Change generator receivers to pointers, other fixes
22752db Use seccomp library for adding seccomp configuration flags
3c2125c Merge pull request generate: fix tmpfs adding based on manpage #155 from Mashimiao/generate-fix-tmpfs-adding

You probably intended to rebase onto the current master.

grantseltzer · 2016-08-02T19:55:57Z

@wking Cleaning up now

wking · 2016-08-02T20:32:11Z

On Tue, Aug 02, 2016 at 12:55:58PM -0700, Grant Seltzer Richman wrote:

@wking Cleaning up now

cfb6d90 → a9b3569 looks like it rebases onto master and then merges
your old tip:

$ git glog a9b3569 | head -n7

a9b3569 (origin/pr/159) Fix rebase
|\
| * cfb6d90 Rebase and clean up seccomp generate api
| * dfe2722 Change generator receivers to pointers, other fixes
| * 22752db Use seccomp library for adding seccomp configuration flags
| a116901 Use seccomp library for adding seccomp configuration flags
| 9d3881d (origin/master) Merge pull request update urfave/cli package #174 from hmeng-19/update_cli

The difference between a116901 and a9b3569 is that the latter removes
duplicate entries for setupLinuxNamespaces, parseIDMapping, etc. I
expect you:

Rebased cfb6d90 onto master to create a116901.
Tried to push a116901 to your public add-seccomp-cli-features, but
Git warned you that it wasn't a fast-forward and suggested you merge.
Merged your old tip (cfb6d90) to get a9b3569.
Tried to push again (which succeeded).

What you probably wanted to do instead was use a --force push in (2)
to say “I know this isn't a fast forward, and I'm ok with that.
Please update the remote branch anyway”.

So I think you want to recover with something like:

$ git checkout add-seccomp-cli-features
$ git reset --hard a116901
$ git checkout a9b3569 -- cmd/ocitools/generate.go
$ git commit --amend --no-edit cmd/ocitools/generate.go
$ git push --force GrantSeltzer add-seccomp-cli-features

grantseltzer · 2016-08-02T20:37:51Z

@wking Wow thank you very much, My git skills have much improving to be done, I appreciate your help!

(Looking into build error right now)

mrunalp · 2016-08-02T21:04:17Z

cmd/ocitools/generate.go

-	}
-
-	return nil
+	err := addSeccomp(g, context)


pass context first

grantseltzer · 2016-08-03T16:23:13Z

@mrunalp PTAL

mrunalp · 2016-08-04T19:54:41Z

generate/seccomp/consts.go

+const (
+	seccompOverwrite = "overwrite"
+	seccompAppend    = "append"
+	nothing          = "nothing"


need to run gofmt on this file?

mrunalp · 2016-08-04T19:57:30Z

I think this looks fine overall. @hmeng-19 Could you test/review as well?

haiyanmeng · 2016-08-04T20:41:23Z

@grantseltzer , man/ocitools-generate.1.md also needs to be updated to reflect the new options.

haiyanmeng · 2016-08-04T20:46:48Z

cmd/ocitools/generate.go

+	seccompArch := context.String("seccomp-arch")
+	architectureArgs := strings.Split(seccompArch, ",")
+	for _, arg := range architectureArgs {
+		err = g.SetSeccompArchitecture(arg)


@grantseltzer , it is not a real problem. But I believe you want to err := ....

Just curious, what difference does it make in this context? Both appear to work

This works when you declare err before using var err error or err := .....
I pointed out because all the other for loops in this function seem to declare its own err.

For consistency, let's change this to err :=

rhatdan · 2016-08-12T18:07:37Z

Ok that works for me.

grantseltzer · 2016-08-23T15:35:23Z

@mrunalp @rhatdan @wking

Would it be preferred to have the default white list be a part of the default ocitools generate or should a flag be specified to add it?

It makes sense to have it be included by default since other options work this way, the list is 4000+ lines though.

mrunalp · 2016-08-23T16:10:47Z

Should be part of default

Sent from my iPhone

On Aug 23, 2016, at 8:35 AM, Grant Seltzer Richman notifications@github.com wrote:

@mrunalp @rhatdan @wking

Would it be preferred to have the default white list be a part of the default ocitools generate or should a flag be specified to add it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

rhatdan · 2016-08-24T12:10:29Z

Part of default.

grantseltzer · 2016-09-01T19:19:16Z

A default seccomp config is now a part of the output of ocitools generate, including detection of the system architecture.

The --seccomp-* flags edit on top of that

mrunalp · 2016-09-01T22:40:53Z

I just tried this out and the runtime tests fail:

-----------------------------------------------------------------------------------
VALIDATING RUNTIME: runc
-----------------------------------------------------------------------------------
runtime: failed to create new OS thread (have 2 already; errno=1)
fatal error: newosproc

runtime stack:
runtime.throw(0x57c714, 0x9)
        /usr/lib/golang/src/runtime/panic.go:566 +0x95
runtime.newosproc(0xc420028000, 0xc420037fc0)
        /usr/lib/golang/src/runtime/os_linux.go:160 +0x194
runtime.newm(0x5907f8, 0x0)
        /usr/lib/golang/src/runtime/proc.go:1572 +0x132
runtime.main.func1()
        /usr/lib/golang/src/runtime/proc.go:126 +0x36
runtime.systemstack(0x63bb00)
        /usr/lib/golang/src/runtime/asm_amd64.s:298 +0x79
runtime.mstart()
        /usr/lib/golang/src/runtime/proc.go:1079

goroutine 1 [running]:
runtime.systemstack_switch()
        /usr/lib/golang/src/runtime/asm_amd64.s:252 fp=0xc420022768 sp=0xc420022760
runtime.main()
        /usr/lib/golang/src/runtime/proc.go:127 +0x6c fp=0xc4200227c0 sp=0xc420022768
runtime.goexit()
        /usr/lib/golang/src/runtime/asm_amd64.s:2086 +0x1 fp=0xc4200227c8 sp=0xc4200227c0
Runtime runc failed validation

grantseltzer · 2016-09-01T23:59:24Z

Which test is that? I can't reproduce that. I think what could be happening is there's a system call not in the white-list that is falling under the default action to errno

grantseltzer · 2016-09-06T16:27:51Z

I've added the syscalls to the default whitelist based on allowed capabilities, pretty sure that was the issue.

grantseltzer · 2016-09-12T19:37:17Z

@mrunalp PTAL

mrunalp · 2016-09-29T21:32:46Z

generate/seccomp/parse_action.go

+	return a, nil
+}
+
+//ParseDefaultAction simply sets the default action of the seccomp configuration


nit: space after //

mrunalp · 2016-09-29T21:44:15Z

From my testing:

 oci-runtime-tool generate --output config.json --tty --seccomp-errno getcwd

I still see the default seccomp options with getcwd being allowed.

mrunalp · 2016-09-29T21:46:00Z

Next I tried to use a template with no syscalls set.

[root@dhcp-16-129 busybox]# oci-runtime-tool generate --output config.json --tty --seccomp-errno getcwd --template template.json                                                                                                                              
[root@dhcp-16-129 busybox]# vim config.json 
[root@dhcp-16-129 busybox]# tail -n 20 config.json 
                        {
                                "type": "ipc"
                        },
                        {
                                "type": "uts"
                        },
                        {
                                "type": "mount"
                        }
                ],
                "seccomp": {
                        "defaultAction": "SCMP_ACT_ERRNO",
                        "architectures": [
                                "SCMP_ARCH_X86_64",
                                "SCMP_ARCH_X86",
                                "SCMP_ARCH_X32"
                        ]
                }
        }

grantseltzer · 2016-09-29T22:34:31Z

For anyone else involved, based on my discussion with @mrunalp

There's a check that will error out if a syscall rule is specified that has the same action as the defaultAction (I've added an error message instead of it failing silently).

I'm going to be adding a seccomp-syscall-remove flag for the edge case of a rule for a system call existing already but we want to change it to the default (For example, if getcwd is set to allow, and the defaultAction is errno, and we want to change getcwd to errno). I'm also adding a flag to remove all syscall rules.

The other edge case that needed to be addressed is when a new default action is specified but there are already system calls that match it. This should cause an error and to report the system calls that have this matching action.

mrunalp · 2016-09-29T22:36:46Z

@grantseltzer For changing the default action, we should add a force flag in the API, so it clears syscalls that match the new action.

grantseltzer · 2016-09-29T22:37:22Z

@mrunalp Right, got it.

grantseltzer · 2016-10-04T23:36:25Z

~~The build keeps failing for what seems to be a CI issue, I'm not sure if something can be reset or this issue will just go away on its own tomorrow.~~ Fixed: golint doesn't support Go 1.5 anymore, upgraded to 1.6

Regardless, I've added flags for removing specific syscall rules or just all of them. I've also added a force option for setting the default action without it deleting rules that already have the same action set.

mrunalp · 2016-10-12T00:00:47Z

LGTM
@Mashimiao @liangchenye @wking If you want to give it a look.

Mashimiao · 2016-10-12T01:42:04Z

cmd/oci-runtime-tool/generate.go

+	cli.StringFlag{Name: "seccomp-trace", Usage: "specifies syscalls to respond with trace"},
+	cli.StringFlag{Name: "seccomp-kill", Usage: "specifies syscalls to respond with kill"},
+	cli.StringFlag{Name: "seccomp-remove", Usage: "specifies syscalls to remove seccomp rules for"},
+	cli.BoolFlag{Name: "seccomp-remove-all", Usage: "removes all syscall rules from seccomp configuration"},


I think the options should be in sorted list as #204 does. So we don't need to do it again.

I went ahead and reordered it. While not in strict alphabetical order, I separated it logically so that the syscall action flags were together (and alphabetical), the default flags together, etc...

I think the way it is now makes the most sense.

Personal opinion, separating them logically does not make too much sense. As seccomp related options all start with seccomp-*, easily to see they are in one group and find the needed one.
I think Following one standard(alphabetical order) makes more sense.
@wking @mrunalp @liangchenye any comments?

Sure, I can concede that. ~~Will fix now~~ Fixed

Mashimiao · 2016-10-12T01:43:12Z

cmd/oci-runtime-tool/generate.go

@@ -73,7 +80,6 @@ var generateFlags = []cli.Flag{
 	cli.Uint64Flag{Name: "linux-mem-kernel-limit", Usage: "kernel memory limit (in bytes)"},
 	cli.Uint64Flag{Name: "linux-mem-kernel-tcp", Usage: "kernel memory limit for tcp (in bytes)"},
 	cli.Uint64Flag{Name: "linux-mem-swappiness", Usage: "how aggressive the kernel will swap memory pages (Range from 0 to 100)"},
-	cli.Int64Flag{Name: "linux-pids-limit", Usage: "maximum number of PIDs"},


need rebased

Woops, Thanks for catching that!

Mashimiao · 2016-10-13T02:06:26Z

@grantseltzer can you add new options to completions/bash/oci-runtime-tool.
If you are not familiar with that, I can add them at #167
And sorry for need another rebase.

grantseltzer · 2016-10-13T02:09:53Z

Sure no problem

@Mashimiao All fixed up, that reminded me to update the man page as well.

On Oct 12, 2016 10:06 PM, "Ma Shimiao" notifications@github.com wrote:

@grantseltzer https://github.com/GrantSeltzer can you add new options
to completions/bash/oci-runtime-tool.
If you are not familiar with that, I can add them at #167
#167
And sorry for need another rebase.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#159 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKWrhbjHv1gLpj7QKdYJDbGhTqaHA4b6ks5qzZIkgaJpZM4JWwml
.

mrunalp · 2016-10-14T15:41:43Z

@Mashimiao Does this look fine to you now?

Mashimiao · 2016-10-16T13:59:58Z

Though there are some minimal issues(like part of new options not in alpha order in man), almost looks good.

Signed-off-by: Grantseltzer <grantseltzer@gmail.com> Signed-off-by: grantseltzer <grantseltzer@gmail.com>

grantseltzer · 2016-10-17T01:33:09Z

@Mashimiao Just fixed the man page order

Mashimiao · 2016-10-17T01:38:16Z

Thanks.
60473d2 looks good to me.

mrunalp · 2016-10-17T17:43:10Z

LGTM

grantseltzer · 2016-10-17T17:44:03Z

Thanks for the merge!

On Oct 17, 2016 1:43 PM, "Mrunal Patel" notifications@github.com wrote:

Merged #159 #159.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#159 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKWrhe4lMMAmZvMrB3XWUl-V_fc1IpfSks5q07OygaJpZM4JWwml
.

grantseltzer mentioned this pull request Jul 28, 2016

Use oci-seccomp-gen library for adding seccomp configurations #128

Closed

wking reviewed Jul 28, 2016
View reviewed changes

wking mentioned this pull request Aug 1, 2016

generate: optimize namespace setup log and fix manpage #166

Merged

mrunalp reviewed Aug 2, 2016
View reviewed changes

mrunalp reviewed Aug 4, 2016
View reviewed changes

haiyanmeng reviewed Aug 4, 2016
View reviewed changes

mrunalp reviewed Sep 29, 2016

View reviewed changes

Mashimiao reviewed Oct 12, 2016

View reviewed changes

Use seccomp library for adding seccomp configuration flags

60473d2

Signed-off-by: Grantseltzer <grantseltzer@gmail.com> Signed-off-by: grantseltzer <grantseltzer@gmail.com>

mrunalp merged commit a7b6558 into opencontainers:master Oct 17, 2016

wking mentioned this pull request Nov 2, 2016

Set pids limit when set through the CLI #264

Merged

Use seccomp library for adding seccomp configuration flags #159

Use seccomp library for adding seccomp configuration flags #159

Conversation

grantseltzer commented Jul 28, 2016

grantseltzer commented Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Aug 2, 2016

grantseltzer commented Aug 2, 2016

wking commented Aug 2, 2016

grantseltzer commented Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

grantseltzer commented Aug 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Aug 4, 2016

haiyanmeng commented Aug 4, 2016

Choose a reason for hiding this comment

grantseltzer Aug 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 12, 2016

grantseltzer commented Aug 23, 2016 • edited Loading

mrunalp commented Aug 23, 2016

rhatdan commented Aug 24, 2016

grantseltzer commented Sep 1, 2016 • edited Loading

mrunalp commented Sep 1, 2016

grantseltzer commented Sep 1, 2016

grantseltzer commented Sep 6, 2016

grantseltzer commented Sep 12, 2016

Choose a reason for hiding this comment

mrunalp commented Sep 29, 2016

mrunalp commented Sep 29, 2016

grantseltzer commented Sep 29, 2016

mrunalp commented Sep 29, 2016

grantseltzer commented Sep 29, 2016

grantseltzer commented Oct 4, 2016 • edited Loading

mrunalp commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

grantseltzer Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Oct 13, 2016

grantseltzer commented Oct 13, 2016 • edited Loading

mrunalp commented Oct 14, 2016

Mashimiao commented Oct 16, 2016

grantseltzer commented Oct 17, 2016

Mashimiao commented Oct 17, 2016

mrunalp commented Oct 17, 2016

grantseltzer commented Oct 17, 2016

grantseltzer commented Jul 28, 2016 •

edited

Loading

grantseltzer commented Aug 2, 2016 •

edited

Loading

grantseltzer Aug 4, 2016 •

edited

Loading

grantseltzer commented Aug 23, 2016 •

edited

Loading

grantseltzer commented Sep 1, 2016 •

edited

Loading

grantseltzer commented Oct 4, 2016 •

edited

Loading

Mashimiao Oct 12, 2016 •

edited

Loading

grantseltzer Oct 12, 2016 •

edited

Loading

grantseltzer commented Oct 13, 2016 •

edited

Loading