Use oci-seccomp-gen library for adding seccomp configurations

[grantseltzer]

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.

[grantseltzer]

@mrunalp Not sure how to fix this error.

[mrunalp]

@grantseltzer Your commit is missing a Signed-off-by.

[wking]

ae08020 has recursive Godeps :p. I'm not sure if that's an expected thing, but 103 vendored files and 10k lines is a lot to pull in for this change. Maybe we can get a smaller library with the bits we need without Manhattan → runc → docker → libtrust and similar dependency chains?

[wking]

You probably didn't intend to remove this (it landed in #122). Maybe a busted rebase?

[grantseltzer]

@wking Not sure what's causing that godeps recursive chain. Do you know why that's happening?

[wking]

On Fri, Jul 01, 2016 at 04:52:03PM -0700, Grant Seltzer Richman wrote:

@wking Not sure what's causing that godeps recursive chain. Do you
know why that's happening?

I'm not familiar with Godeps, but if it is just based on Git clones
maybe it always pulls in the whole repository (and not just a
particular referenced package)? If so, the fix is probably “get
whatever portion of $DEPENDENCY you need pulled out into an
independent Git repository”, ideally by getting upstream to split
things up for you. But maybe the nesting is happening for a different
reason.

[grantseltzer]

I'm not familiar with Godeps, but if it is just based on Git clones maybe it always pulls in the whole repository (and not just a particular referenced package)? If so, the fix is probably “get whatever portion of $DEPENDENCY you need pulled out into an independent Git repository”, ideally by getting upstream to split things up for you. But maybe the nesting is happening for a different reason.

Yea, the issue is that I have the library ociseccompgen and the tool manhattan in the same git repo. I didn't expect go get to pull the whole repo instead of just the package.

For the sake of this this PR at least Godeps.json should be updated to include the ociseccompgen library, have the actual library stored and remove the embedded Godeps directory which is holding dependencies that ocitools already has (cli, logrus, runtime-sepc)

I can do that along with the fixing spacing and re-adding cgroup flag that I accidentally removed. Thoughts?

[wking]

On Fri, Jul 01, 2016 at 05:12:30PM -0700, Grant Seltzer Richman wrote:

I can do that along with the fixing spacing and re-adding cgroup
flag that I accidentally removed. Thoughts?

I don't know enough about Godeps to know what the right fix is ;).
But fixing these other two nits would be good.

[liangchenye]

remove this line.

[liangchenye]

@grantseltzer I follow this instruction to update your dependency, it works, no extra 'Godeps' added. I'm not sure if it also helps in your environment.

[liangchenye]

maybe put all the five ParseSyscallFlag functions to one loop?

[liangchenye]

Hi @grantseltzer , ocitools generate is a small and basic tool, oci-seccomp-gen looks more like a part of it. So personally I don't like ocitools generete to depend on oci-seccomp-gen. But I think it is good to have oci-seccomp-gen as an independent pkg since it could be used in different scenarios (now we have ocitools and docker). Would you mind/like to 'donate' your project to ocitools?

@mrunalp@wking@Mashimiao what do you think about this? The code struct could look like this:

ocitools/pkg/seccomp
ocitools/contrib/seccomp-generete (originial from https://github.com/GrantSeltzer/Manhattan/blob/master/main.go)

[grantseltzer]

@rhatdan@runcom@mheon what do you think of that idea? Manhattan can import its functionality from ocitools instead of inside it's own repo. I'm personally fine with it.

[runcom]

I'm +1k to include that library directly in ocitools and have Manhattan or ocitools itself to manage and generate seccomp profiles or OCI configs

[wking]

On Mon, Jul 04, 2016 at 02:22:52AM -0700, 梁辰晔 (Liang Chenye) wrote:

ocitools/pkg/seccomp

Is ‘pkg’ more idiomatic than ‘lib’? I don't read enough Go to know,
but for a C project I'd expect ‘lib’.

ocitools/contrib/seccomp-generete (originial from
https://github.com/GrantSeltzer/Manhattan/blob/master/main.go)

Why keep this executable separate? Isn't this PR just rolling it into
the bigger ‘ocitool generate …’ picture?

[runcom]

No pkg nor lib, just the name of the library is go idiomatic

[runcom]

+1 also on having the main tool work with the library directly instead of creating a contrib dir with binaries in there. So just:

./seccomp

And code from that used in the main ocitools

[grantseltzer]

I like the idea of having the ociseccompgen be part of the ocitools library, that makes a lot of sense.

Just to clarify, are we in disagreement that seccomp should be a flag for the ./ocitools generate command? That way it's consistent with other configuration options that generate supports (i.e. --cgroups-path or --hostname)

Manhattan is fully functioning for generated just the seccomp configurations: https://github.com/GrantSeltzer/Manhattan

[mrunalp]

Could add a flag --seccomp-only to ocitools generate

Sent from my iPhone

On Jul 4, 2016, at 4:07 PM, Grant Seltzer Richman notifications@github.com wrote:

I like the idea of having the ociseccompgen be part of the ocitools library, that makes a lot of sense.

Just to clarify, are we in disagreement that seccomp should be a flag for the ./ocitools generate command? That way it's consistent with other configuration options that generate supports (i.e. --cgroups-path or --hostname)

Manhattan is fully functioning for generated just the seccomp configurations: https://github.com/GrantSeltzer/Manhattan

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

[Mashimiao]

ocitools/contrib/seccomp-generete (originial from https://github.com/GrantSeltzer/Manhattan/blob/master/main.go)

I'm +1 for this.

[liangchenye]

OK, +1 for a flag --seccomp-only to ocitools generate.

[grantseltzer]

Alright, I'm working on the following points:

1. Rehost the ociseccompgen library into the ocitools repo

2. Add all of the flags like seccomp-allow that I added in this PR

3. Add --seccomp-only flag to just export a seccomp file

Should this be it's own PR (this would be closed) or squashed with the commit for this one?

[wking]

On Tue, Jul 05, 2016 at 12:23:18PM -0700, Grant Seltzer Richman wrote:

3. Add --seccomp-only flag to just export a seccomp file

What is the use-case for this? Can we accomplish the same thing by
having ‘ocitools generate’ write (optionally?) to stdout and use:

$ ocitools generate … | jq .linux.seccomp

[grantseltzer]

The use case is for use with other tools like docker that you can pass a seccomp configuration file to a container or (soon) the daemon. And yes piping into jq would do the same thing, But I think this would be more user friendly and feel less like a hack.

[wking]

On Tue, Jul 05, 2016 at 12:39:19PM -0700, Grant Seltzer Richman wrote:

The use case is for use with other tools like docker that you can
pass a seccomp configuration file to a container or (soon) the
daemon. And yes piping into jq would do the same thing, But I think
this would be more user friendly and feel less like a hack.

I'm just not sure how generically we should scope the solution. I'd
rather not end up with an API that has lots of --*-only flags. If we
don't want to use jq, we could also ‘--subsection linux.seccomp’ or
some such to handle the drilling internally.

And I think using jq for JSON dicing is using the proper (already
written) tool for the task. It doesn't feel like a hack ;).

And regardless of how we address this, always writing to config.json
doesn't seem appropriate for the seccomp-only use case. What
command-line API do we want for “please write the generated config to
stdout”? I'd rather do that by default, with an ‘--output PATH’
option to write to a file (for situations where shell redirection is
not available).

[grantseltzer]

I think having a flag for generate to rename the output file should be added, although that's a more general issue than this PR. Certainly the default for seccomp output could be something like "config.seccomp".

I'm not sure what other configurations can be independently used besides seccomp but I think the seccomp-only flag would definitely be used and makes more sense than having an independent tool (Manhattan). I definitely concede that using jq wouldn't be a hack but having this feature would give more visibility to the idea of using ocitools for it. I also think the feature adds minimal change to the code base.

[wking]

You don't need these now that you've dropped it from Generator.

[wking]

Actually, it looks like you forgot to drop it from Generator ;).

[grantseltzer]

fixing now :P

[grantseltzer]

@mrunalp@rhatdan@runcom

I'd like to open a fresh branch/PR if it's alright, the commit's for this one have gotten unmanageable because of multiple merge commits in the time it's been sitting. Besides the fail for the non-dco, everything seems good to me. Let me know whenever you're ready to look at this, Mrunal.

[wking]

On Tue, Jul 26, 2016 at 12:33:19PM -0700, Grant Seltzer Richman wrote:

I'd like to open a fresh branch/PR if it's alright, the commit's for
this one have gotten unmanageable because of multiple merge commits
in the time it's been sitting.

No need for a new PR. Once you have a squashed commit, just
force-push it to your seccomp-cli-features branch.

[wking]

Ah, and the base for the new commit/PR should be somewhere before #140 landed
in f0ecc98 so we can merge this into the v1.0.0.rc1 branch too. If
you do open a new PR, my personal preference would be for you to open
it on top of and targetting v1.0.0.rc1 [1,2].

[runcom]

Ah, and the base for the new commit/PR should be somewhere before #140 landed
in f0ecc98 so we can merge this into the v1.0.0.rc1 branch too. If
you do open a new PR, my personal preference would be for you to open
it on top of and targetting v1.0.0.rc1 [1,2].

I believe the last commit is rebased on top of the current ocitools master branch

[grantseltzer]

@runcom just helped me out, ignore my last comment.

@mrunalp can you please review :)

[wking]

On Tue, Jul 26, 2016 at 01:01:39PM -0700, Antonio Murdaca wrote:

Ah, and the base for the new commit/PR should be somewhere before
#140 landed in f0ecc98 so we can merge this into the v1.0.0.rc1
branch too. If you do open a new PR, my personal preference would
be for you to open it on top of and targetting v1.0.0.rc1 [1,2].

I believe the last commit is rebased on top of the current ocitools
master branch

I've rebased it backwards with:

$ git rebase --onto 564b30e HEAD^

After resolving some conflicts in code you'd deleted (but which had
been updated in master), I ended up with 69526b9, which I've pushed to
1. You can compare with your current commit using:

$ diff -u <(git show --color=always cbd3a2e) <(git show --color=always 69526b9)

If that looks right to you, I'd like to see 69526b9 pushed to this PR
so it can be merged into both master and v1.0.0.rc1. The alternative
is for me to PR 69526b9 into v1.0.0.rc1, but then we get to unify
those slightly-different versions of the commit at some later point
when we merge v1.0.0.rc1 into master to pick up other
v1.0.0.rc1-compatible changes.

[runcom]

I've rebased it backwards with:

$ git rebase --onto 564b30e HEAD^

After resolving some conflicts in code you'd deleted (but which had
been updated in master), I ended up with 69526b9, which I've pushed to
[1]. You can compare with your current commit using:

$ diff -u <(git show --color=always cbd3a2e) <(git show --color=always 69526b9)

If that looks right to you, I'd like to see 69526b9 pushed to this PR
so it can be merged into both master and v1.0.0.rc1. The alternative
is for me to PR 69526b9 into v1.0.0.rc1, but then we get to unify
those slightly-different versions of the commit at some later point
when we merge v1.0.0.rc1 into master to pick up other
v1.0.0.rc1-compatible changes.

sorry, can't be just wait to land this in master and cherry-pick for the release? or create another pr for the release branch? it's so confusing for everyone like this, why are you rushing?

[runcom]

@grantseltzer will of course take care of opening a PR against the release branch once this is finished and afaict this isn't yet fully reviewed and lgtm'ed

[wking]

On Tue, Jul 26, 2016 at 01:13:53PM -0700, Antonio Murdaca wrote:

I've rebased it backwards with:

$ git rebase --onto 564b30e HEAD^

After resolving some conflicts in code you'd deleted (but which had
been updated in master), I ended up with 69526b9, which I've pushed to
1. You can compare with your current commit using:

$ diff -u <(git show --color=always cbd3a2e) <(git show --color=always 69526b9)

If that looks right to you, I'd like to see 69526b9 pushed to this PR
so it can be merged into both master and v1.0.0.rc1. The alternative
is for me to PR 69526b9 into v1.0.0.rc1, but then we get to unify
those slightly-different versions of the commit at some later point
when we merge v1.0.0.rc1 into master to pick up other
v1.0.0.rc1-compatible changes.

sorry, can't be just wait to land this in master and cherry-pick for
the release? or create another pr for the release branch?

That's the “The alternative is …” route above. I'd rather not
cherry-pick, because it makes the later merge of v1.0.0.rc1 into
master more confusing.

it's so confusing for everyone like this.

This is how lots of projects work. For example see Git's section on
picking which branch to base your work on 1.

At some point ocitools is (I think) going to have to support multiple
runtime-spec releases at the same time, so maybe we want to work out
how we see that working now, implement it, and drop the v1.0.0.rc1
branch? I have some thoughts on that in #98 for config validation,
but the thoughts there aren't all that useful for generate or runtime
validation.

[wking]

On Tue, Jul 26, 2016 at 01:14:36PM -0700, Antonio Murdaca wrote:

@grantseltzer will of course take care of opening a PR against the
release branch once this is finished and afaict this isn't yet fully
reviewed and lgtm'ed

Sounds good, and it is good to wait until the branch is reviewed and
merged into one branch before filing other PRs. If (for
v1.0.0.rc1-compatible features) we lead with a v1.0.0.rc1-based PR
1, there's no need for a second PR. The maintainer just merges
v1.0.0.rc1 into master after each v1.0.0.rc1 bump.

[mrunalp]

I think the API should be easy to use just like rest of generate.
For e.g.

g := generate.New()
g.SetSeccompDefault(...)
g.AddSeccompErrno(...)
g.AddSeccompErrno(...)
g.SaveToFile(...)

The command line option parsing should be left to integration with the cmd.

[grantseltzer]

@mrunalp How's that? I created a seccomp generator in the same way the generate library has.

[mrunalp]

@grantseltzer No, this doesn't help. User shouldn't have to deal with two different generators when trying to generate a config. You could create a seccomp generator API and then use it from the regular generate API. The generate API should be simple and comprehensive.

For e.g.

gen:=generate.New()
gen.SetSeccompDefault(..) // These calls could use your own seccomp API behind the scenes.
gen.AddSeccompErrno(..)
gen.AddSeccompKill(..)
gen.SetRootfs(..)
gen.AddCapability(..)
gen.SaveToFile(..)

[grantseltzer]

Just to clarify do you want all of the command line parsing done in the main package?

[mrunalp]

@grantseltzer Yes, it shouldn't be in the API.

[grantseltzer]

I think the check is failing because of vendoring, looking into it now, it's working locally

[mrunalp]

Can you open a new PR with your commit cherry picked to see if we can make the travis issue go away?

[wking]

On Wed, Jul 27, 2016 at 04:43:03PM -0700, Mrunal Patel wrote:

Can you open a new PR with your commit cherry picked to see if we
can make the travis issue go away?

New PR should not matter and this PR has already had its history
compacted. Current failures are pointer issues 1, so we're probably
missing an update around #150.

[mrunalp]

@wking It builds fine locally.

[grantseltzer]

@wking If you actually check the code you can see that it has been fixed but travis is not finding the correct commit.

[wking]

On Wed, Jul 27, 2016 at 04:53:37PM -0700, Grant Seltzer Richman wrote:

@wking If you actually check the code you can see that it has been
fixed but travis is not finding the correct commit.

Ah, in that case pushing an amended commit (to bump the commit
timestamp and get a new hash) may work. Although sometimes we've had
issues with Travis just being slow ;). Opening a new PR is fine too
if we don't want to wait for Travis to catch up.

[wking]

Actually, here's the merge commit Travis is testing not using a
pointer receiver 1. So I don't think any “kick Travis” fix will
work.

[wking]

And here's your current tip not using a pointer receiver: $ git grep addSeccomp d297bc1d297bc1:cmd/ocitools/generate.go: g, err := addSeccomp(g, context) d297bc1:cmd/ocitools/generate.go:func addSeccomp(g generate.Generator, context *cli.Context) (generate.Generator, error) {

[grantseltzer]

#159

[wking]

On Wed, Jul 27, 2016 at 05:56:21PM -0700, Grant Seltzer Richman wrote:

#159

So we should close this one?

[grantseltzer]

Yup @mrunalp

[wking]

On Thu, Jul 28, 2016 at 07:48:10PM -0700, Grant Seltzer Richman wrote:

Yup @mrunalp

@grantseltzer, as the PR author, you should be able to close it too.