-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add an option to add and drop capabilities in the Docker driver #3754
Changes from 2 commits
7f072ab
8f23c95
4080aac
b509b0a
45d53ca
0ec61a8
35d9331
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import ( | |
"github.com/docker/docker/cli/config/configfile" | ||
"github.com/docker/docker/reference" | ||
"github.com/docker/docker/registry" | ||
"github.com/moby/moby/daemon/caps" | ||
|
||
"github.com/hashicorp/go-multierror" | ||
"github.com/hashicorp/go-plugin" | ||
|
@@ -99,6 +100,9 @@ const ( | |
dockerImageRemoveDelayConfigOption = "docker.cleanup.image.delay" | ||
dockerImageRemoveDelayConfigDefault = 3 * time.Minute | ||
|
||
dockerCapsWhitelistConfigOption = "docker.caps.whitelist" | ||
dockerCapsWhitelistConfigDefault = dockerBasicCaps | ||
|
||
// dockerTimeout is the length of time a request can be outstanding before | ||
// it is timed out. | ||
dockerTimeout = 5 * time.Minute | ||
|
@@ -109,6 +113,9 @@ const ( | |
// dockerAuthHelperPrefix is the prefix to attach to the credential helper | ||
// and should be found in the $PATH. Example: ${prefix-}${helper-name} | ||
dockerAuthHelperPrefix = "docker-credential-" | ||
|
||
dockerBasicCaps = "CHOWN,DAC_OVERRIDE,FSETID,FOWNER,MKNOD," + | ||
"NET_RAW,SETGID,SETUID,SETFCAP,SETPCAP,NET_BIND_SERVICE,SYS_CHROOT,KILL,AUDIT_WRITE" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment documenting what this is for and how the options were chosen |
||
) | ||
|
||
type DockerDriver struct { | ||
|
@@ -202,6 +209,8 @@ type DockerDriverConfig struct { | |
MacAddress string `mapstructure:"mac_address"` // Pin mac address to container | ||
SecurityOpt []string `mapstructure:"security_opt"` // Flags to pass directly to security-opt | ||
Devices []DockerDevice `mapstructure:"devices"` // To allow mounting USB or other serial control devices | ||
CapAdd []string `mapstructure:"cap_add"` // Flags to pass directly to cap-add | ||
CapDrop []string `mapstructure:"cap_drop"` // Flags to pass directly to cap-drop | ||
} | ||
|
||
func sliceMergeUlimit(ulimitsRaw map[string]string) ([]docker.ULimit, error) { | ||
|
@@ -304,6 +313,8 @@ func NewDockerDriverConfig(task *structs.Task, env *env.TaskEnv) (*DockerDriverC | |
dconf.ExtraHosts = env.ParseAndReplace(dconf.ExtraHosts) | ||
dconf.MacAddress = env.ReplaceEnv(dconf.MacAddress) | ||
dconf.SecurityOpt = env.ParseAndReplace(dconf.SecurityOpt) | ||
dconf.CapAdd = env.ParseAndReplace(dconf.CapAdd) | ||
dconf.CapDrop = env.ParseAndReplace(dconf.CapDrop) | ||
|
||
for _, m := range dconf.SysctlRaw { | ||
for k, v := range m { | ||
|
@@ -644,6 +655,12 @@ func (d *DockerDriver) Validate(config map[string]interface{}) error { | |
"devices": { | ||
Type: fields.TypeArray, | ||
}, | ||
"cap_add": { | ||
Type: fields.TypeArray, | ||
}, | ||
"cap_drop": { | ||
Type: fields.TypeArray, | ||
}, | ||
}, | ||
} | ||
|
||
|
@@ -1115,6 +1132,32 @@ func (d *DockerDriver) createContainerConfig(ctx *ExecContext, task *structs.Tas | |
} | ||
hostConfig.Privileged = driverConfig.Privileged | ||
|
||
// set capabilities | ||
hostCapsWhitelist := d.config.ReadDefault(dockerCapsWhitelistConfigOption, dockerCapsWhitelistConfigDefault) | ||
hostCapsWhitelist = strings.ToLower(hostCapsWhitelist) | ||
if !strings.Contains(hostCapsWhitelist, "all") { | ||
effectiveCaps, err := caps.TweakCapabilities( | ||
strings.Split(dockerBasicCaps, ","), | ||
driverConfig.CapAdd, | ||
driverConfig.CapDrop, | ||
) | ||
if err != nil { | ||
return c, err | ||
} | ||
var missingCaps []string | ||
for _, cap := range effectiveCaps { | ||
cap = strings.ToLower(cap[len("CAP_"):]) | ||
if !strings.Contains(hostCapsWhitelist, cap) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is susceptible to imprecise substring matches. Despite no existing capability being a substring of another capability, I still feel like this risks a future exploit by allowing "admin" to match "net_admin" in the whitelist. Can we either parse the cap whitelist into a map to do key lookups or surround the whitelist with delimiters ( I know capabilities don't change often (ever anymore?), but I'd still rather be safe than sorry when doing any sort of whitelist lookups. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, absolutely :) This is just a quick poc to get some feedback. I'll try finish this over the weekend. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! Yeah, your general approach looks great. |
||
missingCaps = append(missingCaps, cap) | ||
} | ||
} | ||
if len(missingCaps) > 0 { | ||
return c, fmt.Errorf("Docker driver doesn't have the following caps whitelisted on this Nomad agent: %s", missingCaps) | ||
} | ||
} | ||
hostConfig.CapAdd = driverConfig.CapAdd | ||
hostConfig.CapDrop = driverConfig.CapDrop | ||
|
||
// set SHM size | ||
if driverConfig.ShmSize != 0 { | ||
hostConfig.ShmSize = driverConfig.ShmSize | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -324,6 +324,32 @@ The `docker` driver supports the following configuration in the job spec. Only | |
} | ||
``` | ||
|
||
* `cap_add` - (Optional) A list of string flags to pass directly to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And then make sure to document Note you don't need to use the exact text I wrote. Just make sure to mention the whitelist and the word capabilities just in case someone is searching for that particular word. :) |
||
[`--cap-add`](https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities). | ||
For example: | ||
|
||
|
||
```hcl | ||
config { | ||
cap_add = [ | ||
"SYS_TIME", | ||
] | ||
} | ||
``` | ||
|
||
* `cap_drop` - (Optional) A list of string flags to pass directly to | ||
[`--cap-drop`](https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities). | ||
For example: | ||
|
||
|
||
```hcl | ||
config { | ||
cap_drop = [ | ||
"MKNOD", | ||
] | ||
} | ||
``` | ||
|
||
### Container Name | ||
|
||
Nomad creates a container after pulling an image. Containers are named | ||
|
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.
add a comment documenting these