-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
agent/runtime: Add seccomp feature #1788
Conversation
Change history: Experiments of the seccomp featureApply a seccomp profile to kata container using crictl
For more details of the seccomp profile, please see the Conduct experiments using
|
Seccomp profile provided? 1 | Runtime seccomp enabled? 2 | Agent seccomp enabled? 3 | Seccomp enforced? 4 | Notes |
---|---|---|---|---|
no | no | no | no | No seccomp profile |
no | yes | no | no | No seccomp profile |
no | no | yes | no | No seccomp profile |
no | yes | yes | no | No seccomp profile |
yes | no | no | no | Seccomp profile is not passed to the runtime. Therefore, grpc.Linux.Seccomp is null . |
yes | yes | no | no | Profile provided and grpc.Linux.seccomp is passed to the agent, but the agent disables seccomp feature. The runtime exits conservatively with an error message. |
yes | no | yes | no | Seccomp profile is not passed to the runtime.Therefore, grpc.Linux.Seccomp is null . |
yes | yes | yes | yes | Now, seccomp rule is enforced. 😄 |
Footnotes:
- 1: Implicitly or explicitly. If the seccomp profile is provided, the seccomp object will be present in the OCI
config.json
file. - 2: Whether the seccomp support is enabled in the runtime's
configuration.toml
configuration file. - 3: Whether the seccomp feature of the agent is enabled in the agent's
Makefile
. - 4: Whether the seccomp rule will be applied.
Notes
If we use the libseccomp crate with statically linked the libsecomp library, we should comply with GNU LGPL-2.1 licensed the libseccomp library when we distribute the kata agent
binary.
Therefore, we must attach the complete source code for the libraries with annotations about the license in order to comply with the LGPL-2.1 (6(a)).
Next actions
- Modify CI test to verify this seccomp feature
- Remove the libseccomp package (
libseccomp
) in the rootfs- If the agent always uses the libseccomp crate with statically linked, we do not need the libseccomp in rootfs anymore.
- Update documents
- Discuss the type of linking the libseccomp library
- Need to include the libseccomp source code into artifacts in order to comply with
GNU LGPL-2.1
This is really cool, I'll take a closer look at this in the coming week, @ManaSugi! Thanks a lot for working on this one! |
/test |
f55d5d4
to
885b429
Compare
Sorry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 A couple of comments
|
||
// Set only if the agent is built with seccomp support and the guest | ||
// environment supports seccomp. | ||
bool supports_seccomp = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to update the agent API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I think that supports_seccomp
flag always will be true
, so the flag is not used anymore.
In addition, we can control whether the agent's seccomp support is enabled when building the agent.
If users want to control the flag through the agent API, we should leave it, but I'm not sure if there is such a use case.
Could you give me your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't use a field, we should leave it here for rpc protocol compatibility. Please don't remove it.
And in fact, the field is useful on the runtime side so that we can dynamically determine if seccomp info should be passed to the agent or not. Please look at src/runtime/virtcontainers/kata_agent.go
and see how it works currently.
#[cfg(feature = "seccomp")] | ||
if !oci_process.no_new_privileges { | ||
if let Some(ref scmp) = linux.seccomp { | ||
seccomp::init_seccomp(scmp)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shoud we implement the errror handling like below?
if !oci_process.no_new_privileges {
if let Some(ref scmp) = linux.seccomp {
#[cfg(feature = "seccomp")]
seccomp::init_seccomp(scmp)?;
#[cfg(not(feature = "seccomp"))]
return Err(anyhow!("Seccomp config provided but seccomp not supported"));
}
}
This error tells users that the runtime enables seccomp support, but the agent does not enable seccomp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is handled in the runtime by the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach is safer: if the admin asked for seccomp but it isn't available, I'd prefer to get a hard error telling me that immediately. The current code doesn't error, which may lead an admin to think seccomp is being enforced when it isn't (which would be surprising and potentially dangerous).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Hence, I have already added the error in kata-runtime at the following point.
https://github.com/kata-containers/kata-containers/pull/1788/files#diff-6fb7f551035031ed29ad100c61f1a7e92b0fa15d2f67e4c7ea7ebf3b715583f4R1424
If the user enables seccomp in the runtime using disable_guest_seccomp
, but the agent does not have seccomp capability, the runtime exits conservatively with the error message. Therefore, we can guarantee that the agent has seccomp capability when seccomp profiles are passed to the agent.
Thank you @ManaSugi for this great PR. |
@liubin |
@ManaSugi thank you for the clarification. |
Now, I'm creating unit test inside the |
This test runs only as a
In addition, bump |
82db087
to
4f8c213
Compare
What is the cost of running a seccomp capable agent but do not enforce it? I'm wondering if we should just always enable the capability, and optionally enforce it when starting the agent. |
@bergwolf From a performance point, the cost may be nothing. However, if we always enforce the seccomp feature in the Could you give me your thoughts? |
@ManaSugi Thanks for the explanation! Then how about building it by default, and we ship the agent with seccomp capability builtin but disabled by default? And we can provide a For those who don't have libseccomp dev library and want to build agent by themselves. The benefit is that we don't have to ship two versions of kata agent (which means two versions of guest images). |
@bergwolf
I have a question. I'm sorry for the lack of explanation and let me check we're on the same page here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManaSugi Sorry for the delay. By disabled by default
I mean disabling it by the runtime. The agent binary has the secomp capability built-in. The capability is exposed to the runtime and runtime can then decide if seccomp info needs to be passed to the agent. So here is my proposal:
- we add a runtime config option to enable seccomp, and disable it by default
- we add kata-agent build flag to control if seccomp it builtin, and enable it by default
- for those who want seccomp support, they can just use the prebuilt agent and alter runtime config to enable it
- for those who don't even want seccomp be built, they can build their own agent w/o seccomp support
Also please keep the supports_seccomp
bit in the agent protocol, so that runtime can know if seccomp is supported by agent when runtime config asks for it, and fail properly if the required feature is not supported.
wdyt?
|
||
// Set only if the agent is built with seccomp support and the guest | ||
// environment supports seccomp. | ||
bool supports_seccomp = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't use a field, we should leave it here for rpc protocol compatibility. Please don't remove it.
And in fact, the field is useful on the runtime side so that we can dynamically determine if seccomp info should be passed to the agent or not. Please look at src/runtime/virtcontainers/kata_agent.go
and see how it works currently.
@bergwolf, Thank you for your kind reply and proposal. I understand clearly and agree with your proposal. Also, I keep the
|
8fdd58e
to
4c60917
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Thanks @ManaSugi !
@ManaSugi Please rebase to solve code conflicts. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ManaSugi.
A few comments.
src/agent/rustjail/src/container.rs
Outdated
#[cfg(feature = "seccomp")] | ||
use crate::seccomp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this feature is optional, we should specify whether it is available in announce()
(for the logs). We already have a seccomp field in the GetGuestDetails()
ttRPC response so in fact it might be worth just adding the output of get_agent_details()
to the output of announce()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand.
I added the information of seccomp capability in the agent to AgentConfig
.
So, announce()
can output the information through AgentConfig
.
Could you give me your thoughts about that?
[features] | ||
seccomp = ["libseccomp"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need to modify the CI.
Fortunately, all distributions in CI already have libseccomp-dev
package and the agent enables seccomp capability by default.
Regarding a unit test, we need to install the libseccomp library and set the environment variables of the libseccomp crate because the agent has seccomp feature by default.
First, we have to build the libseccomp libraries for the <arch>-unknown-linux-musl
target to link the libseccomp statically and install it.
e.g.,
$ wget https://github.com/seccomp/libseccomp/releases/download/v2.5.1/libseccomp-2.5.1.tar.gz
$ tar xvf libseccomp-2.5.1.tar.gz
$ cd libseccomp-2.5.1
$ ./configure CPPFLAGS="-I/usr/include/x86_64-linux-musl" --enable-static --disable-shared
$ make
$ sudo make install
Second, set the environment variables for libseccomp crate before building the agent.
e.g.
$ export LIBSECCOMP_LINK_TYPE=static
$ export LIBSECCOMP_LIB_PATH=/usr/local/lib
Regarding an integration test with k8s, we have to add a seccomp profile for k8s in k8s-security-context.bats.
In that case, we also have to set disable_guest_seccomp
as false
in runtime's configuration like below.
$ sudo sed -i '/^disable_guest_seccomp/ s/true/false/' /etc/kata-containers/configuration.toml
I'm sorry if I'm wrong about CI.
src/agent/rustjail/src/seccomp.rs
Outdated
"architectures": [ | ||
"SCMP_ARCH_X86", | ||
"SCMP_ARCH_X32", | ||
"SCMP_ARCH_X86_64", | ||
"SCMP_ARCH_AARCH64", | ||
"SCMP_ARCH_ARM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kata also works on PPC64 and s390x so won't this test fail unless you add them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added PPC64
and S390X
in the unit test.
To avoid endian mismatch error of libseccomp
, I divided the architecture by endian.
Ref: seccomp/libseccomp@2745d8a
Any update on this @ManaSugi? |
I apologize for the late reply and thank you for the review @jodh-intel @bergwolf. |
@jongwu sorry, no -- I don't do a ton of musl obv 😁 |
@jongwu |
I'm not sure I can fix the error quickly. Maybe we can skip this feature on arm temporarily and get it fixed later.
I tried, but it doesn't work. I'm not sure we can fix it quickly. maybe we can skip it on arm temporarily and get it fixed later. |
@jongwu @Jakob-Naucke The reason why the failure does not occur in the case of s390x is that the agent uses gnu-libc on s390x (in the case of x86, the test is skipped). |
To run |
Hello @ManaSugi -, good new of finding the root cause. I prefer to skip it on arm and enable it back in the new PR. I don't want to block this so long. |
The kata-agent supports seccomp feature based on the OCI runtime specification. This seccomp capability in the kata-agent is enabled by default. However, it is not enforced by default: users need to enable that by setting `disable_guest_seccomp` to `false` in the main configuration file. Fixes: kata-containers#1476 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
The osbuilder needs to set up libseccomp library to build the kata-agent because the kata-agent supports seccomp currently. The library is built from the sources to create a static library for musl libc. In addition, environment variables for the libseccomp crate are set to link the library statically. Fixes: kata-containers#1476 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
This adds a step for installing libseccomp because the kata-agent supports seccomp feature. Fixes: kata-containers#1476 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
This adds explanation about how to enable seccomp in the kata-runtime and build the kata-agent with seccomp capability. Fixes: kata-containers#1476 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
In order to pass CI test of aarch64, it is necessary to run `ci/install_libseccomp.sh` before ruuning unit tests in `jenkins_job_build.sh`. However, `ci/install_libseccomp.sh` is not available until PR kata-containers#1788 including this commit is merged in the mainline. Therefore, we disable seccomp feature on aarch64 temporarily. After kata-containers#1788 lands and CI is fixed, this commit will be reverted. Fixes: kata-containers#1476 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
ce9a898
to
42add7f
Compare
@jongwu Thank you for your comment. I added a commit to disable seccomp feature on aarch64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ManaSugi.
lgtm
/test |
/test-clh-k8s-containerd |
Thank you all for the great reviews for a long time. |
Need to install libseccomp library from sources before running unit tests because Kata agent enables seccomp feature Depends-on: kata-containers/kata-containers#1788 Fixes: kata-containers#4123 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
@ManaSugi - Thank you for your patience and perisistence! 😄 |
Need to install libseccomp library from sources before running unit tests because Kata agent enables seccomp feature Depends-on: kata-containers/kata-containers#1788 Fixes: kata-containers#4123 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Need to install libseccomp library from sources before running unit tests because Kata agent enables seccomp feature Depends-on: kata-containers/kata-containers#1788 Fixes: kata-containers#4123 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Need to install libseccomp library from sources before running unit tests because Kata agent enables seccomp feature in the [kata-containers#1788](kata-containers/kata-containers#1788) Fixes: kata-containers#4123 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Need to install libseccomp library from sources before running unit tests because Kata agent enables seccomp feature in the kata-containers/kata-containers#1788 Fixes: kata-containers#4123 Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Kata agent supports seccomp feature from kata-containers#1788. Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
Kata agent supports seccomp feature from kata-containers#1788. Signed-off-by: Manabu Sugimoto <Manabu.Sugimoto@sony.com>
The agent supports seccomp capability based on the OCI runtime specification.
This seccomp capability in the agent is enabled by default.
However, it is not enforced by default: users need to enable that in the main configuration file.
Fixes: #1476
Signed-off-by: Manabu Sugimoto Manabu.Sugimoto@sony.com