-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat: add support to add and drop linux capabilities #1702
base: main
Are you sure you want to change the base?
Conversation
@@ -25,6 +25,7 @@ require ( | |||
github.com/klauspost/compress v1.17.11 | |||
github.com/klauspost/pgzip v1.2.6 | |||
github.com/kubescape/go-git-url v0.0.30 | |||
github.com/moby/moby v27.4.0+incompatible |
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 is the cost of tracking and aligning to Docker default capabilities.
pkg/container/config.go
Outdated
@@ -46,6 +46,7 @@ type Config struct { | |||
PackageName string | |||
Mounts []BindMount | |||
Capabilities Capabilities | |||
CapAdd []string // List of kernel capabilities to add to the container. |
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.
Capability drop instead I think should be responsibility of the single runner (e.g. always dropping cap_sys_admin).
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.
Can we fold this into the Capabilities field above?
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.
Sure @jonjohnsonjr. Addressed with f2a5a53
pkg/config/config.go
Outdated
type Configuration struct { | ||
// Package metadata | ||
Package Package `json:"package" yaml:"package"` | ||
// The specification for the packages build environment | ||
Environment apko_types.ImageConfiguration `json:"environment" yaml:"environment"` | ||
// TODO: move them a runtime specialized section. | ||
// Linux process capabilities to add to the pipeline container. | ||
CapAdd []string `json:"cap-add,omitempty" yaml:"cap-add,omitempty"` |
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.
Can you introduce a new Capabilities object to track what's in Build? We probably want the ability to "drop" capabilities too and it would be nice if they lived in the same struct and mirrored Build for clarity. I'd also make this just "add" so it'd look something like:
capabilities:
add: ["SETUID"]
And in the future:
capabilities:
add: ["SETUID"]
drop: ["NET_ADMIN"]
Or whatever.
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 like it @jonjohnsonjr . I just addressed it, alongside the capability drop option.
Do we want to add a specialized test for this? In the meantime I used the following fixture:
package:
name: caps
description: caps add-drop feature test
version: 0.1.0
capabilities:
add:
- CAP_NET_ADMIN
drop:
- CAP_SYS_ADMIN
- CAP_SYS_CHROOT
environment:
contents:
packages:
- busybox
- cmd:capsh
pipeline:
- name: Test default effective capability
runs: |
capsh --decode=$(grep CapEff /proc/self/status | cut -d ':' -f2 | xargs) | grep -i cap_dac_override
- name: Test added non-default effective capability
runs: |
capsh --decode=$(grep CapEff /proc/self/status | cut -d ':' -f2 | xargs) | grep -i cap_net_admin
- name: Test dropped default effective capability
runs: |
capsh --decode=$(grep CapEff /proc/self/status | cut -d ':' -f2 | xargs) | grep -vi cap_sys_chroot
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 is great, yes, can you add this as a test and in the examples/ ?
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 @jonjohnsonjr Just added e2e tests for both build and test.
Plus a pipeline example with iproute
.
449ec12
to
a19c214
Compare
This commit adds the configuration to add and drop Linux capabilies to the container for bubblewrap and docker runners, enabling a declarative way to do least privilege at both build and test time. Furthermore, the minimum set of process capabilities in Bubblewrap container is now the same of Docker default capabilities. Signed-off-by: Massimiliano Giovagnoli <[email protected]>
a19c214
to
8e46080
Compare
Signed-off-by: Massimiliano Giovagnoli <[email protected]>
d66f8b7
to
cbf421c
Compare
Signed-off-by: Massimiliano Giovagnoli <[email protected]>
This commit adds the configuration to add and drop Linux capabilies to the container for bubblewrap and docker runners, enabling a declarative way to do least privilege. The capabilities added or dropped are relative to the minimum set of capabilities Docker sets. They are applied to all both build and test pipelines.
Furthermore, the minimum set of process capabilities in Bubblewrap container is now the same of Docker default capabilities, to improve result consistency.
Fixes #1703
Melange Pull Request Template
Functional Changes
Notes:
SCA Changes
Notes:
Linter
Notes: