virtcontainers: Pass seccomp profile inside VM #689

nitkon · 2018-09-03T03:48:16Z

Seccomp profile was dropped as agent, inside the VM,
was not capable of handling it. Allow it to be passed
as agent binary is now build with -tags "seccomp" and
the image on the guest would have libseccomp installed.

Fixes: #688

Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

opendev-zuul · 2018-09-03T04:38:38Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : FAILURE in 44m 17s

jodh-intel · 2018-09-03T13:37:34Z

lgtm.

But blocked on:

sboeuf · 2018-09-04T17:04:50Z

Not sure the agent support seccomp. We need to fix the agent, and I think we need libseccomp to be part of the VM image if we want to be able to use seccomp inside the VM.

nitkon · 2018-09-04T17:55:16Z

@sboeuf : PTAL kata-containers/agent#353 (comment)

opendev-zuul · 2018-09-11T19:48:22Z

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

kata-runsh : RETRY_LIMIT in 9m 16s

sboeuf · 2018-09-14T15:42:10Z

@jodh-intel @nitkon I was thinking, shouldn't this PR introduce a new config option inside configuration.toml under the [agent] section so that we can say if the agent supports or not libseccomp.
And of course, by default the agent will not support it, so we won't go through this code path.

jodh-intel · 2018-09-17T08:57:31Z

@sboeuf - ah you mean we should have that option so the runtime is clear that seccomp support must be provided by the agent in the image, or else it needs to fail?

Yes, I agree. This is the same sort of issue as:

agent: add support for guest-hooks agent#365 (comment) (and the runtime component which adds the new option cli: add guest hook path option #720).

nitkon · 2018-09-17T10:03:57Z

@sboeuf @jodh-intel :
Case 1:
I just want to understand this. As of today if the agent does not provide the seccomp support we fail with the apt message. Isn't that a sufficient message for the user?

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.

Case 2:

If we are thinking of implementing via configuration.toml, then ....

The configuration.toml file would have something like this

[agent.kata]
#Set this to true if  seccomp support is provided by the agent
#Seccomp = true

If seccomp is not set or set to false in config.toml, then disable the seccomp

runtime/virtcontainers/kata_agent.go

Line 724 in a5e82c1

grpcSpec.Linux.Seccomp = nil

runtime/virtcontainers/kata_agent_test.go

Line 454 in a5e82c1

assert.Nil(g.Linux.Seccomp)

and error out saying "agent does not support seccomp. "
If agent was built with seccomp support and not set in sonfig.toml, still it would fail, which would not have been the case in CASE 1.

If seccomp is set to true in config.toml, then enable the seccomp

runtime/virtcontainers/kata_agent.go

Line 724 in a5e82c1

grpcSpec.Linux.Seccomp = nil

runtime/virtcontainers/kata_agent_test.go

Line 454 in a5e82c1

assert.Nil(g.Linux.Seccomp)

nitkon · 2018-09-17T16:48:28Z

@jodh-intel @sboeuf :

We can pass the seccomp profile either explicitly using --security-opt option OR docker by default passes a seccomp profile (https://github.com/moby/moby/blob/master/profiles/seccomp/default.json)
In either case if seccomp is not supported by agent, it would fail with the following error:

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.

Thus, shouldn't we have agent built with seccomp support by default rather than make it optional?

sboeuf · 2018-09-17T18:12:11Z

@nitkon the problem with enabling this by default is that even when you're not passing any seccomp from the CLI, the spec won't be nil, and this will trigger libcontainer to try to use seccomp.

Please try to look into the way seccomp is actually carried from the CLI to the agent, and identify where it's providing a LinuxSeccomp structure, even when nothing is provided. This way, we could override the "empty" structure with a nil pointer.

My advice would be to dump the spec in various places so that you can figure this out.

nitkon · 2018-09-17T18:39:34Z

@nitkon the problem with enabling this by default is that even when you're not passing any seccomp from the CLI, the spec won't be nil, and this will trigger libcontainer to try to use seccomp.

If the user does not provide any seccomp profile explicitly, then do we not want the default seccomp provided by docker to be applied to our container? If we want the default seccomp profile to be applied to each container, we should have both agent and rootfs seccomp capable always and not optional. It might affect size and boot time, but will provide additional security.

Please try to look into the way seccomp is actually carried from the CLI to the agent, and identify where it's providing a LinuxSeccomp structure, even when nothing is provided. This way, we could override the "empty" structure with a nil pointer.

You mean to say when no seccomp profile is provided by the user, make sure the default seccomp is dropped inside the agent, resulting in libcontainer not trying to use the default seccomp?
My question is, why to drop default seccomp, isn't it good for the security of Kata Containers? Are we trying to save boot time and rootfs size?

My advice would be to dump the spec in various places so that you can figure this out.

Sure. Thanks for the input.

sboeuf · 2018-09-17T19:01:04Z

As you mentioned, we don't want to affect our boot time and memory footprint by enabling seccomp. At least for now. But I understand that some people might want this extra feature and that's what you're trying to enable here.

Also, because the spec that we get might have a default seccomp, then we have no other choice than letting the user decide if he wants to use seccomp or not. And in our case, this translates as a configuration option.

jodh-intel · 2018-09-18T10:58:17Z

I'd appreciate it if the team could review the following (very carefully), but I think this summarises the main scenarios we need to handle in code (and tests!!)...

seccomp profile provided? (¹)	runtime seccomp support (²)	runtime seccomp enabled (³)	agent/image seccomp support (⁴)	seccomp enforced? (⁵)	Run workload? (⁶)	Notes
no	no	-	no	no	yes
no	no	-	yes	no	yes
no	yes	no	no	no	yes
no	yes	yes	no	no	?	Config option enabled so runtime should warn or error ⁷
no	yes	no	yes	no	yes
no	yes	yes	yes	no	?	Config option enabled so runtime should warn or error ⁷
yes	no	-	no	no	yes	⁸
yes	no	-	yes	no	yes	⁸
yes	yes	no	no	no	?	Profile provided, but config disabled so runtime should warn or error ⁷
yes	yes	no	yes	no	no?	seccomp disabled by config, so runtime should warn or error ⁷
yes	yes	yes	no	no	no?	Cannot enforce seccomp, so error ⁷
yes	yes	yes	yes	yes	yes	Finally, we got there! 😄

Footnotes:

1: Implicitly or explicitly ("docker run --security-opt seccomp=..."). If provided, the
optional seccomp object will be present in the OCI config.json file.
2: Whether the runtime version can "handle" seccomp fully (it cannot today).
3: Whether seccomp support is enabled in the runtime's configuration.toml configuration file.
4: Whether the agent can "handle" seccomp fully.
5: Whether the seccomp rules will be applied.
6: Whether the runtime workload will be executed.
7: We could make the runtime seccomp option value either enabled or enforced to modify the behaviour (log or error respectively)?
8: Runtime sets (or should set) seccomp profile to nil before sending to agent.

grahamwhaley · 2018-09-18T12:34:16Z

@jodh-intel - would it be easy for you to add maybe an (S) after the run workload column yes, where it was run with seccomp enabled? Just so we can distinguish when seccomp is enforced or not?

jodh-intel · 2018-09-18T13:02:07Z

@grahamwhaley - you mean drop the existing "seccomp enforced?" column and bundle into the next column?

jodh-intel · 2018-09-18T13:30:22Z

Table updated with more details on the column meanings.

grahamwhaley · 2018-09-18T14:27:06Z

Ah, that is what that column means! ;-) - then, np @jodh-intel as it is. thx!

nitkon · 2018-09-18T14:41:38Z

Hi @jodh-intel , thanks for the table 👍

So, why cannot we take a simple approach that:--- if a user mentions in configuration.toml file that agent/rootfs supports seccomp, then simply allow the config to be passed by the runtime to the agent else drop it.
When we say "Whether the runtime version can "handle" seccomp fully " , what does that mean exactly? Isn't the role of runtime to just pass/drop the seccomp profile?
What's the difference between enabled(yes/true in config file?) and enforced?
Could you please clarify the difference between "runtime seccomp support " and "runtime seccomp enabled" ?

jodh-intel · 2018-09-21T16:44:26Z

Hi @nitkon

So, why cannot we take a simple approach that:--- if a user mentions in configuration.toml file that agent/rootfs supports seccomp, then simply allow the config to be passed by the runtime to the agent
else drop it.

Having thought about this a little more, I'm not sure I like the idea of the config file determining whether seccomp support should be enabled. Instead, I think that when Kata fully supports seccomp, the config file should be used to tell the runtime how to behave depending on what sort of seccomp environment it's running in. And we should probe to determine whether the agent/image is seccomp-capable.

If we rely on the config file to determine whether seccomp should be used, there are three possible scenarios when a seccomp profile is provided but the config file disables seccomp:

the config setting is correct and the image really doesn't support seccomp.
the image in fact does support seccomp but the admin explicitly wanted to disable it for some reason [*].
the config file is mis-configured (the admin forgot to update it, forgetting or not realising that the image does support seccomp).

That last one is the nasty one of course. Also, it begs the question how will the admin know if the image supports seccomp? Even if the image itself has the seccomp libraries installed (which kata-collect-data.sh will show in the contents of the osbuilder.yaml file), that doesn't necessarily mean the agent was built for seccomp.

When we say "Whether the runtime version can "handle" seccomp fully " , what does that mean exactly? Isn't the role of runtime to just pass/drop the seccomp profile?

Yes, but it doesn't currently have that capability - it just drops it. So by "handling" I mean that the runtime can heuristically decide what to do.

What's the difference between enabled(yes/true in config file?) and enforced?

See above.

Could you please clarify the difference between "runtime seccomp support " and "runtime seccomp enabled" ?

Again, see above ;)

I think a better design might be to introduce a new gRPC agent call that the runtime can make before it sends over the OCI spec (including the seccomp profile). That gRPC call would tell the runtime whether the agent can deal with seccomp or not. That would be a pure agent call - again, we don't want libcontainer getting involved at this stage. Something like:

message AgentFeaturesResponse {
    bool supports_seccomp = 1;
};

rpc GetAgentFeatures(AgentFeaturesRequest) returns (AgentFeaturesResponse);

(Aside: In fact, I wonder if we want to replace Check() which is basically "ping for gRPC" with a call like this - we need to make a ping-type call before requesting the agent create containers, so why not get that call to return more information than just "I'm alive"? :)

Once that GetAgentFeatures() API is available:

If a seccomp profile is specified and the agent supports seccomp, pass the profile verbatim (no config settings checked [*]).
If a seccomp profile is specified and the agent does not support seccomp, the runtime can check the config file option:
- if seccomp = permissive, log a message and pass a nil profile to the agent (current behaviour).
- if seccomp = enforcing, log and error and fail.

The problem is that the "current behaviour" != "best behaviour" in the scenario where the agent does support seccomp since the default behaviour should probably be to enforce, so I think we'll need to break the current behaviour (by making seccomp enforced if available) to ensure the users get the safest possible default config.

[*] - However, we may need to come up with a config option to handle this scenario if the admin has requested that seccomp be disabled even though the agent supports it.

wdyt @sboeuf, @bergwolf, @egernst, @gnawux?

bergwolf · 2018-09-22T03:12:31Z

@jodh-intel Yes, I agree. GetAgentFeatures sounds good to me. It can be useful for many use cases.

jodh-intel · 2018-09-24T08:59:39Z

@nitkon - I've raised kata-containers/agent#381 for adding a new gRPC call. I'm happy to look at that if you want?

nitkon · 2018-09-24T09:01:23Z

@jodh-intel : Okay sure. Go ahead :-)

jodh-intel · 2018-09-24T09:23:14Z

@nitkon - ok, I've assigned it to myself and hope to look at that today/tomorrow "with a following wind" ;)

raravena80 · 2018-09-28T16:35:19Z

@nitkon @jodh-intel ping looks like #381 is closed. Was that what we needed for this?

jodh-intel · 2018-09-28T16:38:03Z

Yes, kata-containers/agent#381 is now closed. I'm probably going to raise a PR to move that call in the runtime, but the call will still be made.

As such, @nitkon can re-vendor the agent into the runtime to get the latest version of GetGuestDetails() which includes whether seccomp is available in the image or not.

bergwolf · 2018-10-09T07:25:14Z

ping @nitkon

raravena80 · 2018-10-22T16:38:10Z

@nitkon ping (From your weekly Kata herder)

gnawux · 2018-12-05T13:01:52Z

/test On Dec 5, 2018 20:43, "nitkon" <notifications@github.com> wrote: Travis-CI is passing. Can someone start the jenkins CI test? ping for review @jodh-intel <https://github.com/jodh-intel> @bergwolf <https://github.com/bergwolf> @egernst <https://github.com/egernst> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#689 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADTbLrh0M6VeHfm7KumdTORX6ve14-Nks5u179-gaJpZM4WW7cg> .

amshinde · 2018-12-05T21:29:22Z

/retest

nitkon · 2018-12-06T07:03:10Z

The test failures are not related to the PR. Let's have a retest.
/retest

bergwolf

Just one comment. Otherwise looks good to me. Thanks @nitkon !

bergwolf · 2018-12-06T08:57:34Z

virtcontainers/kata_agent.go

@@ -986,9 +987,18 @@ func (k *kataAgent) createContainer(sandbox *Sandbox, c *Container) (p *Process,
 		return nil, err
 	}

+	request := &grpc.GuestDetailsRequest{


We already query guest details in getAndStoreGuestDetails(). Please just save the secomp results got from there and then we can use it everywhere. By saving it, please add it to Sandbox and also save it on disk so that we can access it again after the very first create sandbox call.

nitkon · 2018-12-06T09:22:17Z

/retest

nitkon · 2018-12-06T12:23:13Z

/retest

nitkon · 2018-12-06T14:24:45Z

/retest

raravena80 · 2018-12-18T05:18:56Z

@nitkon ready to merge?

ghost · 2018-12-18T09:53:27Z

ping @nitkon

nitkon · 2019-01-03T11:55:55Z

@raravena80 : Sorry for the late reply. I was caught up in some personal work. Yes the PR is good to merge. @bergwolf I shall optimize it in a follow-up PR later ...

nitkon · 2019-01-07T04:39:31Z

Updated my PR. Not started the tests as all tests are failing for a couple of days...

nitkon · 2019-01-07T05:30:42Z

Re-pushed the PR as there was some maligned structs warning while doing static check.

nitkon · 2019-01-07T09:34:35Z

/test

Pass Seccomp profile to the agent only if the configuration.toml allows it to be passed and the agent/image is seccomp capable. Fixes: kata-containers#688 Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

nitkon · 2019-01-08T04:57:41Z

/test

nitkon · 2019-01-08T08:26:06Z

CI's are passing now. All green. @jodh-intel @grahamwhaley

…anch-bump # Kata Containers 1.10.0-alpha1

nitkon force-pushed the seccomp branch from ba88362 to b8c4d1d Compare September 11, 2018 18:35

sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018

jodh-intel mentioned this pull request Sep 24, 2018

Create GetAgentFeatures gRPC call kata-containers/agent#381

Closed

nitkon force-pushed the seccomp branch from ae49101 to a93de5d Compare December 5, 2018 12:50

amshinde approved these changes Dec 5, 2018

View reviewed changes

bergwolf reviewed Dec 6, 2018

View reviewed changes

nitkon force-pushed the seccomp branch from a93de5d to b05e075 Compare December 6, 2018 09:14

nitkon force-pushed the seccomp branch from b05e075 to 22f8167 Compare December 6, 2018 14:19

nitkon mentioned this pull request Jan 6, 2019

Limitations: Remove seccomp limitation. kata-containers/documentation#341

Closed

nitkon force-pushed the seccomp branch from 22f8167 to bea3c57 Compare January 7, 2019 04:19

nitkon force-pushed the seccomp branch 2 times, most recently from d69eca3 to 8086cad Compare January 7, 2019 05:12

grahamwhaley mentioned this pull request Jan 7, 2019

Limitations: Remove seccomp limitation. kata-containers/documentation#342

Merged

virtcontainers: Conditionally pass seccomp profile

c2c9c84

Pass Seccomp profile to the agent only if the configuration.toml allows it to be passed and the agent/image is seccomp capable. Fixes: kata-containers#688 Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

nitkon force-pushed the seccomp branch from 8086cad to c2c9c84 Compare January 8, 2019 04:52

jodh-intel merged commit 38c9cd2 into kata-containers:master Jan 8, 2019

jodh-intel mentioned this pull request Jul 22, 2019

kernel/config: enable security features kata-containers/packaging#639

Closed

egernst pushed a commit to egernst/runtime that referenced this pull request Feb 9, 2021

Merge pull request kata-containers#689 from amshinde/1.10.0-alpha1-br…

c87f497

…anch-bump # Kata Containers 1.10.0-alpha1

ManaSugi mentioned this pull request Mar 1, 2021

agent: Add seccomp support kata-containers/kata-containers#1476

Closed

virtcontainers: Pass seccomp profile inside VM #689

virtcontainers: Pass seccomp profile inside VM #689

Conversation

nitkon commented Sep 3, 2018

opendev-zuul bot commented Sep 3, 2018

jodh-intel commented Sep 3, 2018 • edited by amshinde Loading

sboeuf commented Sep 4, 2018

nitkon commented Sep 4, 2018

opendev-zuul bot commented Sep 11, 2018

sboeuf commented Sep 14, 2018

jodh-intel commented Sep 17, 2018

nitkon commented Sep 17, 2018 • edited Loading

nitkon commented Sep 17, 2018

sboeuf commented Sep 17, 2018

nitkon commented Sep 17, 2018

sboeuf commented Sep 17, 2018

jodh-intel commented Sep 18, 2018 • edited Loading

grahamwhaley commented Sep 18, 2018

jodh-intel commented Sep 18, 2018

jodh-intel commented Sep 18, 2018

grahamwhaley commented Sep 18, 2018

nitkon commented Sep 18, 2018

jodh-intel commented Sep 21, 2018

bergwolf commented Sep 22, 2018

jodh-intel commented Sep 24, 2018

nitkon commented Sep 24, 2018

jodh-intel commented Sep 24, 2018

raravena80 commented Sep 28, 2018

jodh-intel commented Sep 28, 2018

bergwolf commented Oct 9, 2018

raravena80 commented Oct 22, 2018

gnawux commented Dec 5, 2018 via email

amshinde commented Dec 5, 2018

nitkon commented Dec 6, 2018

bergwolf left a comment

Choose a reason for hiding this comment

bergwolf Dec 6, 2018

Choose a reason for hiding this comment

nitkon commented Dec 6, 2018

nitkon commented Dec 6, 2018

nitkon commented Dec 6, 2018

raravena80 commented Dec 18, 2018

ghost commented Dec 18, 2018

nitkon commented Jan 3, 2019

nitkon commented Jan 7, 2019

nitkon commented Jan 7, 2019

nitkon commented Jan 7, 2019

nitkon commented Jan 8, 2019

nitkon commented Jan 8, 2019

jodh-intel commented Sep 3, 2018 •

edited by amshinde

Loading

nitkon commented Sep 17, 2018 •

edited

Loading

jodh-intel commented Sep 18, 2018 •

edited

Loading