Add an option to add and drop capabilities in the Docker driver #3754
Conversation
There was a problem hiding this comment.
Looks great! You'll need to vendor moby/moby/daemon/caps but otherwise it looks fine to include to me!
If you've run make bootstrap or manually installed govendor, you should be able to add it by:
govendor update github.com/moby/moby/daemon/caps
govendor update github.com/syndtr/gocapability/capability
make vendorfmt # makes the vendor.json diff prettierThanks a lot of submitting this!
client/driver/docker.go
Outdated
| @@ -99,6 +100,9 @@ const ( | |||
| dockerImageRemoveDelayConfigOption = "docker.cleanup.image.delay" | |||
| dockerImageRemoveDelayConfigDefault = 3 * time.Minute | |||
|
|
|||
| dockerCapsWhitelistConfigOption = "docker.caps.whitelist" | |||
| dockerCapsWhitelistConfigDefault = dockerBasicCaps | |||
There was a problem hiding this comment.
add a comment documenting these
client/driver/docker.go
Outdated
| @@ -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.
Add a comment documenting what this is for and how the options were chosen
client/driver/docker.go
Outdated
| 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.
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 (,) and search for the delimited form: ,net_admin,.
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.
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.
Great! Yeah, your general approach looks great.
| @@ -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.
A list of Linux capabilities as strings to pass directly to . Only whitelisted capabilities may be added. The whitelist can be customized using the
docker.caps.whitelistkey in the client node's configuration.
And then make sure to document docker.caps.whitelist too.
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. :)
|
Travis should pass once you vendor the dependencies. |
|
Shoot, just noticed the moby package doesn't build on Windows. You'll have to use a wrapper func to access it and define the funcs in
func TweakCaps(...) ... { return ... }Where you just return zero values. And then in default just pass through to the |
|
@schmichael I updated the PR. |
| @@ -590,6 +620,13 @@ options](/docs/agent/configuration/client.html#options): | |||
| access to the host's devices. Note that you must set a similar setting on the | |||
| Docker daemon for this to work. | |||
|
|
|||
| * `docker.caps.whitelist`: A list of allowed Linux capabilities. Defaults to | |||
There was a problem hiding this comment.
Can you document the all case?
|
@filipochnik Great work and thanks! |
| @@ -625,7 +625,8 @@ options](/docs/agent/configuration/client.html#options): | |||
| which is the list of capabilities allowed by docker by default, as | |||
| [defined here](https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities). | |||
| Allows the operator to control which capabilities can be obtained by | |||
| tasks using `cap_add` and `cap_drop` options. | |||
| tasks using `cap_add` and `cap_drop` options. Supports the value `"ALL"` as a | |||
There was a problem hiding this comment.
How can I set the "cap_add" & "cap_drop" in the .nomad file?
I am using nomad v0.6.2 and if I add "cap_add" : ["NET_ADMIN"]" in Tasks, I get the error "Cap_add is an invalid field" in nomad.
There was a problem hiding this comment.
Which version of Nomad are you using? This will be available in 0.8.0, which hasn't been released yet.
If you are using your own development build of 0.8.0 and you find the docs unclear I suggest you write to Nomad mailing list or if you suspect a bug, submit a new issue. There's a much higher chance of getting a response that way :)
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Related to #3695
This PR adds an option to set
--cap-addand--cap-dropin the Docker driver.Since adding capabilities without restriction is insecure, I propose adding a whitelist defined in the client config. See the second commit for a sketch of that idea. It would be great if I could get some feedback whether it is the right direction to take this.
Note: I realize you might not want to pull github.com/moby/moby as a dependency, but the code I used from there is pretty short and can easily be copied.