@ydjainopensource : @jodh-intel : Updated my PR

lgtm

@nitkon to avoid have possible image fragmentation I wonder if there is any big impact have just installed libseccomp but not used if not required.

@jcvenegas - the installed size of the seccomp package (for EulerOS) is ~200k fwics. The trouble is, if we add this to the default images we might end up setting a precedent and end up with @grahamwhaley's "death by a thousand cuts" where we keep on adding more and more small packages which results in a big increase in image size.

Suggestion (sorry about the huge letters folks! :)

How about this: We introduce a policy where we will add "security-related" packages to the default image only. But everything else would have to be conditional? That could still "bite" us if a particular security package is "large" but we can deal with such situations as they arise.

Determining what's in an image

It's worth reminding ourselves that -- conditional or not -- tools like https://github.com/kata-containers/runtime/blob/master/data/kata-collect-data.sh.in will still be able to show the difference between images due to osbuilder creating /var/lib/osbuilder/osbuilder.yaml.

/cc @grahamwhaley, @egernst, @sboeuf, @bergwolf.

Hi @jodh-intel, a possible approach would be to try to optimize the current image size by shaving out non essential packages, and then figure out what should be part of the image.

IMO any component that would improve security or extend support for a container engine (e.g. docker) should be considered for inclusion.

Right - we do try to keep the default images as small as possible, but if desired users can add whichever extra packages they need using the PACKAGES= variable in the config.sh files.

@jodh-intel in the case of that way to provide a "secure" image, I wonder an easy way to configure the runtime to use it. I think is not really user friendly ask the user change that in the config file by taking any of the images we provided.

I think is not an option let the runtime open the image and check the image capabilities all the time.

@jodh-intel : Are we concluding this discussion by saying that the seccomp packages should be included by default and not made conditional? I will need to update my PR accordingly.

@jodh-intel my point is that from osbuilder perspective this is not an issue, we it can generate the types of flavors we want, but a the problem comes on how to mange this from runtime side.

@jcvenegas - ah sorry, I'd misunderstood. Yes, we could add it to all images to make it simpler. Or, we could use the libcontainer calls to determine if the image+kernel has the support we need. If a seccomp profile is provided and the guest is not seccomp-capable, that should be an immediate error I think.

@jodh-intel : When a seccomp profile is passed and the image is not created with libseccomp support , it errors out today with the following error:

docker run -it --security-opt seccomp=sleep.json --runtime kata-runtime busybox sh
docker: Error response from daemon: OCI runtime create failed: rpc error: code = Internal desc = Could not run process: container_linux.go:348: starting container process caused "seccomp: config provided but seccomp not supported": unknown.

So, @jodh-intel should I update this PR with adding seccomp support by default and building agent by default with seccomp tag? (I was thinking of getting this feature landed this week :-) )

@nitkon - well I'd like input from other members of the team since although this change is small, we need to understand the implications of adding non-core packages like this.

There's nothing to stop you using an osbuilder image with seccomp support for testing other parts of the seccomp story you are working on meantime though :)

/cc @sboeuf, @bergwolf, @grahamwhaley.

@jodh-intel : Sure! 👍

@grahamwhaley @sboeuf: polite ping.

@nitkon - whilst we wait, one of your commits is failing checkcommits as you're using a different email address to your github-registered one fwics.

l g t m - would be good if we could get the euleros fix in as well - ping @liangchenye for some confirmation...

This is taking too long so I'm afraid if this PR doesn't work for EulerOS, it'll have to be fixed in a follow-up PR from @liangchenye.

@grahamwhaley, @jcvenegas, @sboeuf, @devimc, @amshinde - ptal so we can get this landed.

I think we understand the Jenkins unhappyness, but Travis is not happy either:

make -C cmd/checkcommits
make[1]: Entering directory '/home/travis/gopath/src/github.com/kata-containers/tests/cmd/checkcommits'
go test .
ok  	github.com/kata-containers/tests/cmd/checkcommits	0.009s
go install -ldflags "-X main.appCommit="a23bcc204ed588142b4b74a8898710fba094fa5c" -X main.appVersion=0.0.1" .
make[1]: Leaving directory '/home/travis/gopath/src/github.com/kata-containers/tests/cmd/checkcommits'
Running checkcommits version 0.0.1 (commit a23bcc204ed588142b4b74a8898710fba094fa5c)
Detected TravisCI Environment
Found 2 commits between commit a372cbbf0aa48ba12820bcdab45f7ee52f13417c and branch master
All commit checks passed.
INFO: Checking for SPDX license headers
fatal: ambiguous argument 'origin/': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
INFO: No files found
warning: "./..." matched no packages
INFO: Installing xurls utility
INFO: Checking documentation
INFO: Checking local branch for changed documents only
fatal: ambiguous argument 'origin/': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
INFO: No documentation to check
INFO: Skipping check_files: see https://github.com/kata-containers/tests/issues/469

travis_time:end:08461f3c:start=1536741945538893828,finish=1536742004349972513,duration=58811078685
�[0Ktravis_fold:end:before_script
�[0Ktravis_time:start:17453408
�[0K$ travis_wait 30 .ci/run.sh


Still running (1 of 30): .ci/run.sh

�[31;1mThe command .ci/run.sh exited with 2.�[0m

�[32;1mLog:�[0m

INFO: running tests for all distros

INFO: Running test: Can create and run fedora image

INFO: ERROR: test failed

INFO: AGENT_INIT: 'no'

INFO: images:

total 0
INFO: rootfs:

ls: cannot access '/tmp/osbuilder-test.cC0mUbh/rootfs-osbuilder': No such file or directory
/home/travis/.travis/job_stages: line 375:  5485 Terminated              travis_jigger "${!}" "${timeout}" "${cmd[@]}"

travis_time:end:17453408:start=1536742004354785269,finish=1536742004451881417,duration=97096148
�[0K
�[31;1mThe command "travis_wait 30 .ci/run.sh" exited with 2.�[0m

looks good but CI is not happy

@devimc @grahamwhaley @jodh-intel : Updated my PR. Travis-CI is happy now.

the concern that has been mentioned here is that we will increase the footprint by having libseccomp enabled in the rootfs, even when no seccomp profile is applied.

I wonder if we should ship two kata-agents

I wonder if we should ship two kata-agents

In the same image? I guess we could do that and re-exec "the other one" on demand. That would bloat the image of course - more than if we just had a single agent.

Rather than having multiple agents, I'm wondering instead if we could modularise [1] it and then dynamically load such extra features. The "fast path" would be to not load seccomp support. However, both approaches don't really help with the image bloat issue... unless the modules / 2nd agent were dynamically shared into the image.

You could argue we already have precedent for adding security-related features by default since we do this for the kernel - enabling new features and building them into the kernel. But that's almost the same issue as the agent one - maybe we should discuss again the possibility of dynamically loading kernel modules? The difference between the kernel and the agent / mini O/S though is that the kernel features tend to be significantly smaller wrt on-disk size. However, there is still the risk of death by 1000 cuts as @grahamwhaley has pointed out before.

One problem we'd have making the agent more dynamic to allow security modules to be loaded on demand... is (ironically) security: how could the agent trust the modules? If they are already part of the image, we can probably just apply the implicit trust level of the image/initrd (aka we trust it :)

Related:

• Should we provide multiple guest kernel config files? packaging#55
• Create Linux kernel configuration script packaging#8

[1] - or "modularize" for @egernst 😄

OK, time to get back to this one.

I think a first key point is to settle the 'default image security position' for the project. As others have noted, I think it does make a lot of sense for the project to have security items enabled by default. We could then detail/document how to build an optimised setup (image, agent etc.) for users who are more concerned about space than footprint.

@kata-containers/architecture-committee - I think this is a committee call. Let me at least add it to the next meeting agenda.

Hi @grahamwhaley @jodh-intel: Was this point discussed in the committee call? Whats the final call on this? :-)

Hi @nitkon It was discussed (you can see some notes here.
Yesterday I started to evaluate the cost (size and speed) of enabling the seccomp parts across agent/image/etc. - Ideally our goal is to build all parts with seccomp in them, but have it disabled by default. It is a balance of size/speed though. I will hopefully have some results next week we can then discuss further.

ping @nitkon

@grahamwhaley @nitkon what's the status on this PR?
I'm asking as I cannot recall the decision from the AC about having libseccomp in the image or not? And was it mentioned we could generate a different image?
Based on input, we need to move forward with this, as I believe some ppl might be interested in running Kata with libseccomp enabled.

Oh, I meant to update this on Friday (or maybe I updated the Issue or another relevant PR)....
I am re-evaluating the metrics to measure the impact - but I need to re-run with initrd as well as .img, as initially I saw little impact on .img, but Yash saw big impact on initrd.
I did that on Friday, but the results didn't look like I expected, so I am re-evaluation. I am working on this... (I will try to progress this week)

Thanks @grahamwhaley! We'll wait for your input then :)
Do you recall what's the AC said about having one or two different images? Just easier to track here in Github.

I'm afraid I don't recall the decision about multiple img's - but, that will hang on the impact findings - if the impact is acceptable, then maybe it won't be an issue ...

@grahamwhaley Fair enough :)

@nitkon can you give this a rebase pls, and then I think we are good to merge this (as discussed over at kata-containers/runtime#689 (comment)).

@egernst @grahamwhaley: Rebased my PR.

/test

and, oh, this CI fail I suspect is not related to this - maybe a recent CI merge has broken something....

Cloning into 'tests'...
/home/ubuntu/tests/.ci/lib.sh: line 14: GOPATH: unbound variable
Build step 'Execute shell' marked build as failure
Performing Post build task...

Ooh, OK, I see what the problem is with the CI and the unbound variable.... I think it will only affect this (the osbuilder) repo right now...

In our Jenkins master script for this repo we start with:

git clone https://github.com/kata-containers/tests.git
cd tests

.ci/install_go.sh -p

export GOPATH=${WORKSPACE}/go

but, install_go.sh has:

source "${script_dir}/lib.sh"

near the top, and lib.sh now has:

lib_script="${GOPATH}/src/${tests_repo}/lib/common.bash"
source "${lib_script}"

added by me yesterday for the bare metal cleanup refactor. Oops, use of GOPATH before it has been set.....

I don't see a non-trivial correct simple fix for this, as we are trying to use GOPATH before we've even installed GO, and the osbuilder Jenkins script is using git to get a repo clone locally (outside of the GOPATH).
What we (now) do in the other repos is presume golang is installed on the Jenkins slaves, and use go get in the Jenkins master scripts to fetch the tests repo to the 'one true GOPATH place' in the first place. Maybe that is the real fix here - to fix the Jenkins master script for osbuilder.
@chavafg @jcvenegas for input on what they think is the right fix here...

@grahamwhaley what about exporting GOPATH before install_go.sh?
I think it is the easiest way and that's how we do in the jenkins_job_build.sh Basically moving https://github.com/kata-containers/ci/blob/master/jenkins/jobs/kata-containers-osbuilder-centos-7-4-PR/config.xml#L111 a couple of lines above.

Sure we can @chavafg :-) My only thought was that then notionally we are setting up the GOPATH before we've theoretically installed it with install_go.sh - but, I think we can agree there is no harm here, so that should work fine. Can you do the change?

done, lets see how this goes.

/test