-
Notifications
You must be signed in to change notification settings - Fork 206
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
Enable SECCOMP_FILTER_FLAG_SPEC_ALLOW
per default
#938
Enable SECCOMP_FILTER_FLAG_SPEC_ALLOW
per default
#938
Conversation
It’s a breaking change for runc :( |
58c547d
to
84a0a51
Compare
is this still a breaking change for runc or can we merge it? |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Let's wait for opencontainers/runc#3390 to be merged first. Then we need a new runc release. |
// SECCOMP_RET_ALLOW should be logged. An administrator may override this | ||
// filter flag by preventing specific actions from being logged via the | ||
// /proc/sys/kernel/seccomp/actions_logged file. (since Linux 4.14) | ||
SeccompFilterFlagLog = "SECCOMP_FILTER_FLAG_LOG" |
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.
is there a risk that a malicious container could fill the log if this is enabled by default?
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.
The log message always has a limited line length accordingly to: https://github.com/torvalds/linux/blob/23d04328444a8fa0ca060c5e532220dac8e8bc26/kernel/auditsc.c#L2946-L2970
The kernel has an audit rate limit as well as backlog limit. Auditd has a file rotation in place as well.
In theory users can still specify SCMP_ACT_LOG
as default action, which would log all syscalls and not exclude the allowed ones.
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.
The log filter level may have a negative performance impact, I'll do some testing around this.
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.
Benchmark results
Environment
- Linux 5.16.10
- cri-o 1.23.1
- Kubernetes 1.23.4
Test pod
apiVersion: v1
kind: Pod
metadata:
name: test-pod
spec:
containers:
- name: test-container
securityContext:
seccompProfile:
type: RuntimeDefault
image: nginx:1.21.6
Test
ab -n 100000 -c 100 http://$HOST/index.html
Results
Seccomp Profile | Test time (in sec) | Requests per second |
---|---|---|
RuntimeDefault | 5.0 | 19913.1 |
RuntimeDefault (modified to log sendfile 1x per request) |
46.3 | 2156.4 |
RuntimeDefault (modified to log close 2x per request) |
93.6 | 1068.7 |
{ "defaultAction": "SCMP_ACT_LOG" } |
521.9 | 191.5 |
Unconfined | 5.1 | 19323.4 |
It's interesting to see the impact of logging, which is executed in the kernel there:
https://github.com/torvalds/linux/blob/7e57714cd0ad2d5bb90e50b5096a0e671dec1ef3/kernel/auditsc.c#L2946-L2970
My local machine spikes audit CPU usage during the test, for example with SCMP_ACT_LOG
:
Audit settings:
> sudo auditctl -s
enabled 1
failure 1
pid 803
rate_limit 0
backlog_limit 64
lost 163
backlog 65
backlog_wait_time 60000
loginuid_immutable 0 unlocked
Just curious: why enable |
It's boosting performance, see: http://mamememo.blogspot.com/2020/05/cpu-intensive-rubypython-code-runs.html |
SECCOMP_FILTER_FLAG_LOG
and SECCOMP_FILTER_FLAG_SPEC_ALLOW
per defaultSECCOMP_FILTER_FLAG_SPEC_ALLOW
per default
84a0a51
to
bba0885
Compare
We now enable the flag for the default seccomp profile. Signed-off-by: Sascha Grunert <[email protected]>
bba0885
to
3cd4e02
Compare
@saschagrunert cool. And being vulnerable by default is not a problem, I guess? |
If we add this, should we make it more optional, so users could easily turn it on and off? One issue I have with SECCOMP is that it is almost impossible for normal users to customize. If this is a security versus performance change, is there a way we could add a flag to --security-opt seccomp:SPEC_ALLOW ... |
Hm, it may be better than to leave it as it is for RuntimeDefault. crun is using it as a default as well: I also found some referring discussions in moby/moby#41389 and opencontainers/runc#2430. |
@saschagrunert thanks! |
We now enable the flag for the default seccomp profile.
/hold