@burnMyDread congrats of your first PR to Moby project 🎂

It looks to be that CI does not like your changes to TestContainerInitDNS test:

04:33:46 ?   	github.com/docker/docker/contrib/httpserver	[no test files]
04:34:06 time="2019-06-01T01:33:54Z" level=warning msg="Published ports are discarded when using host network mode"
04:34:06 --- FAIL: TestContainerInitDNS (0.00s)
04:34:06     daemon_test.go:208: invalid character 'C' looking for beginning of object key string
04:34:06 time="2019-06-01T01:33:54Z" level=warning msg="Changing requested CPUShares of 1 to minimum allowed of 2"
04:34:06 time="2019-06-01T01:33:54Z" level=warning msg="Changing requested CPUShares of 262145 to maximum allowed of 262144"
04:34:06 time="2019-06-01T01:33:54Z" level=error msg="Error replicating health state for container container_id: evalSymlinksInScope: /config.v2.json is not in /go/src/github.com/docker/docker/daemon"

You should be able to test/debug it locally with command:
TESTFLAGS="-test.run TestContainerInitDNS" make test-unit
more details about testing you can find from https://github.com/moby/moby/blob/master/TESTING.md

Also, plz use your real name on sign off message.

And last but not least it would be nice if you can describe some real world use case for this one so maintainers can more easily understand if this is correct way to solve that issue?

@olljanat Thanks for the feedback I will push a commit with an updated unit test on Monday and figure out what is going on with the signature.

As for your request for background on why we want this change. We would like to set a local policy for what the default capabilities for newly created docker containers as we would like a more restrictive set of capabilities than the current default. More details on the request are here.

Looking at the windows RS1 failure, it looks like all the runs today failed, and I see a PR up here: #39287 to fix what looks like the same issue.

@burnMyDread yea RS1 is broken and can be ignored for now. Last status of investigating it you can see on #39290 (comment)

Ok cool, thanks.

Derek add label: rebuild/windowsRS1

Derek add label: rebuild/windowsRS1

@burnMyDread only pre-defined person can send commands to Derek 😉
Howvever RS1 looks to be stuck on scheduling. Can you combine your commits to one, verify that it have valid commit message with correct signature and force push it here? It will re-trigger CI

Codecov Report

Merging #39297 into master will increase coverage by 0.87%.
The diff coverage is 41.91%.

@@            Coverage Diff             @@
##           master   #39297      +/-   ##
==========================================
+ Coverage   36.18%   37.05%   +0.87%     
==========================================
  Files         610      611       +1     
  Lines       45363    45522     +159     
==========================================
+ Hits        16413    16869     +456     
+ Misses      26707    26371     -336     
- Partials     2243     2282      +39     

@justincormack Are you ok with the test I added to cover configuring the capabilities? (I left the DNS test alone in this version of the code.)

@olljanat Are you happy with the commit message and the signoff? (Also do I need to do something to get the codecov report to work correctly?)

@burnMyDread assuming that it is valid email address then yes 👍 Codecov have some weird issues which happens time to time. You can ignore it.

@olljanat Ok sounds good. (That address is an alias, but it is my real email account.)

@olljanat What can I do to help get this accepted?

LGTM, @thaJeztah @justincormack ping

@thaJeztah @justincormack Any feedback for me? Thanks.

We should have a proper look at this when taking Swarm services into account; see the discussion on #25885 (comment)

@thaJeztah Thanks for the link. I am a little unclear on what you need from me, but I wanted to clarify one thing. With my change, I only update the capabilities if opts.params.HostConfig.Capabilities is nil. I believe that will allow swarm to set the capabilities however it sees fit and they will not get overwritten by the daemon's setting.

This needs a documentation (at the very least an update to dockerd man page, this lives in https://github.com/docker/cli).

Also, can you please clean up the commit a bit to not remove those empty lines

@burnMyDread Looks like an error one might get from Docker For Mac just because sometimes the FS sucks.

This is a manjaro install not a mac.

I'll try an ubuntu VM for my build. I suspect the golang version.

I have a test working for the new code for the default case. I can't figure out how to pass in the daemon config. Any hints?

@cpuguy83 You good with this?

How do I replicate the tests in docker_cli_run_unix? (I tried running the complied daemon and running the same docker commands and they didn't fail.)

I ran the following test case and it passed:
root@8ea5dac37413:/go/src/github.com/docker/docker# ./bundles/dynbinary-daemon/dockerd --default-capabilities="CAP_CHOWN" --default-capabilities="CAP_DAC_OVERRIDE" &
[1] 1174
Then:
root@8ea5dac37413:/go/src/github.com/docker/docker# docker run --cap-add sys_admin alpine echo hello
time="2020-10-19T15:27:16.269413773Z" level=info msg="starting signal loop" namespace=moby path=/run/docker/containerd/daemon/io.containerd.runtime.v2.task/moby/2238caa1c8e26f26ee5ab79054b7e73d53218a7b6937573ae9b353a9b652c416 pid=1345
hello
INFO[2020-10-19T15:27:16.644204544Z] ignoring event module=libcontainerd namespace=moby topic=/tasks/delete type="*events.TaskDelete"
INFO[2020-10-19T15:27:16.644294840Z] shim disconnected id=2238caa1c8e26f26ee5ab79054b7e73d53218a7b6937573ae9b353a9b652c416

In the cli tests I see that we are calling this code:

Looking at the icmd docs it looks like the answer to my question was yes.
I did some more testing in the make shell and I can't get the cli to fail in the same way as the tests.

retest this please.

I got some details but it mostly raises more questions:

capsh --print
Current: = cap_chown,cap_dac_override,cap_fowner,cap_fsetid,cap_kill,cap_setgid,cap_setuid,cap_setpcap,cap_net_bind_service,cap_net_raw,cap_sys_chroot,cap_sys_admin,cap_mknod,cap_audit_write,cap_setfcap+eip

Note: The cap_sys_admin capability.

That is running on the same container with ns-test which is failing with clone failed: Operation not permitted
The docs say that you need cap_sys_admin to clone a process which implies that the process does not have cap_sys_admin (Or that something else is going wrong.) However, capsh does think that the container has cap_sys_admin. This is even when I launch it via non-interactive shell e.g.:
/usr/local/cli/docker run --cap-add sys_admin syscall-test /sbin/capsh --print

At this point I can change the tests to use capsh but that sounds dangerous. Do you guys have any idea why the tooling is giving me contradictory information like this?

I may have the issue sorted.

I gave this a quick try, and there's some things to address. I left some comments above, but there's some more;

1. Changing defaults should not affect existing containers

This needs some thinking what the best approach is. Currently, this pull request will modify capabilities on existing containers, which likely isn't desirable. Although this is already the case for the current "defaults", those defaults are hard-coded in the daemon, and would only change in case docker is updated and adds new capabilities (which would be somewhat more expected, and could be mentioned in changelogs).

However, I think that custom defaults should only apply to new containers, be part of the container's configuration, and be immutable after that.

Here's what's happens currently:

start the daemon:

dockerd

start a container in the background

docker run -dt --init --name test alpine

install capsh in the container, and check the capabilities in the container

docker exec test sh -c 'apk add --quiet --no-cache libcap`

docker exec test sh -c 'capsh --print'

Current: = cap_chown,cap_dac_override,cap_fowner,cap_fsetid,cap_kill,cap_setgid,cap_setuid,cap_setpcap,cap_net_bind_service,cap_net_raw,cap_sys_chroot,cap_mknod,cap_audit_write,cap_setfcap+eip
Bounding set =cap_chown,cap_dac_override,cap_fowner,cap_fsetid,cap_kill,cap_setgid,cap_setuid,cap_setpcap,cap_net_bind_service,cap_net_raw,cap_sys_chroot,cap_mknod,cap_audit_write,cap_setfcap
Ambient set =
Securebits: 00/0x0/1'b0
 secure-noroot: no (unlocked)
 secure-no-suid-fixup: no (unlocked)
 secure-keep-caps: no (unlocked)
 secure-no-ambient-raise: no (unlocked)
uid=0(root)
gid=0(root)
groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel),11(floppy),20(dialout),26(tape),27(video)

Now, stop the daemon, and start it again with custom default capabilities

dockerd --default-capabilities CAP_KILL --default-capabilities CAP_CHOWN

Start the container again

docker start test

Check capabilities in the container again, and notice that those were changed to the new defaults:

docker exec test sh -c 'capsh --print'

Current: = cap_chown,cap_kill+eip
Bounding set =cap_chown,cap_kill
Ambient set =
Securebits: 00/0x0/1'b0
 secure-noroot: no (unlocked)
 secure-no-suid-fixup: no (unlocked)
 secure-keep-caps: no (unlocked)
 secure-no-ambient-raise: no (unlocked)
uid=0(root)
gid=0(root)
groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel),11(floppy),20(dialout),26(tape),27(video)

I don't think changing defaults should affect existing containers, as that could easiliy lead to unexpected behavior for containers that were previously running fine, and all of a sudden no longer function properly.

2. missing validation

We should probably add validation on the configured defaults. Problem with this is that the daemon starts succesfully, but once a container is started, it will fail;

dockerd --default-capabilities=NO_SUCH_CAPABILITY

docker run --rm hello-world

docker: Error response from daemon: OCI runtime create failed: container_linux.go:370:
starting container process caused: unknown capability "NO_SUCH_CAPABILITY": unknown.
ERRO[0002] error waiting for container: context canceled

There's existing validation functions for the --cap-add and -cap-drop flags that could possibly be reused). However those also accept ALL as capability. I don't think we should allow that to be used for defaults, as setting that would be a bit of a foot-gun, and containers would be started with all capabilities, which isn't a great thing to do (open to suggestions for that though).

3. daemon.json configuration not working correctly

The third thing I tried is to set default capabilities through daemon.json, as this is likely how most users would set the defaults;

Create /etc/docker and create a daemon.json with custom capabilities;

mkdir -p /etc/docker/
echo '{"default-capabilities":["CAP_KILL","CAP_CHOWN"]}' > /etc/docker/daemon.json

Start the daemon;

dockerd

Check capabilities in containers that are started:

docker run --rm alpine sh -c 'apk add --quiet --no-cache libcap && capsh --print'

Current: = cap_chown,cap_dac_override,cap_fowner,cap_fsetid,cap_kill,cap_setgid,cap_setuid,cap_setpcap,cap_net_bind_service,cap_net_raw,cap_sys_chroot,cap_mknod,cap_audit_write,cap_setfcap+eip
Bounding set =cap_chown,cap_dac_override,cap_fowner,cap_fsetid,cap_kill,cap_setgid,cap_setuid,cap_setpcap,cap_net_bind_service,cap_net_raw,cap_sys_chroot,cap_mknod,cap_audit_write,cap_setfcap
Ambient set =
Securebits: 00/0x0/1'b0
 secure-noroot: no (unlocked)
 secure-no-suid-fixup: no (unlocked)
 secure-keep-caps: no (unlocked)
 secure-no-ambient-raise: no (unlocked)
uid=0(root)
gid=0(root)
groups=0(root),1(bin),2(daemon),3(sys),4(adm),6(disk),10(wheel),11(floppy),20(dialout),26(tape),27(video)

Notice that the custom capabilities are not applied. However, trying to set the capabilities through the --default-capabilities flag correctly produces an error message that the option is set both as flag and through daemon.json;

dockerd --default-capabilities CAP_KILL --default-capabilities CAP_CHOWN

unable to configure the Docker daemon with file /etc/docker/daemon.json:
the following directives are specified both as a flag and in the configuration file:
default-capabilities: (from flag: [CAP_KILL CAP_CHOWN], from file: [CAP_KILL CAP_CHOWN])

I suspect some logic is missing to load the capabilities; possibly because the flag and daemon.json values have the same name (but I haven't checked)

If we agree on 1. ("Changing defaults should not affect existing containers"), we should also allow these capabilities to be "live-reloadable", so that they can be updated without having to restart the daemon.

I gave this a quick try, and there's some things to address. I left some comments above, but there's some more;

1. Changing defaults should not affect existing containers

This needs some thinking what the best approach is. Currently, this pull request will modify capabilities on existing containers, which likely isn't desirable. Although this is already the case for the current "defaults", those defaults are hard-coded in the daemon, and would only change in case docker is updated and adds new capabilities (which would be somewhat more expected, and could be mentioned in changelogs).

However, I think that custom defaults should only apply to new containers, be part of the container's configuration, and be immutable after that.

...

How did you see this getting implemented? Is it something where after a container is created we store a list of caps for that container? How would we go about checking the current caps for a container?

Our current workflow is that we tell our users to drop all caps and add what they need. What we would like to be able do is set a known safe set of default caps and have our users add the caps they need. Our high level requirement is that we need to be able to tell what caps a container has without doing a docker exec or equivalent on the container(s) as we don't know in advance what is or is not installed on the container. Your alpine example is a good indication of why this is tricky.

For our use-case I am not as worried about containers that stop working due to capabilities changes because we are already pretty aggressively changing the default caps as it is. I would also like to note that this is an opt-in change the defaults are the same.

A penultimate note is that this is really about cap_net_raw at the end of the day. If we didn't have that in the default then this functionality is less of a priority. That being said I get the argument that removing that cap would cause some compatibility issues.

In short, I don't have a problem this approach in theory, but I want to make sure that I understand the behavior you are looking for in more depth so that I can insure our goals line up.
Cheers.

(P.S. I agree on your other two notes but I want to get this sorted before I start coding.)

@thaJeztah Just to clarify the behavior you are thinking is that once a container 'gets' capabilities it keeps them via something like this struct:

Note DCO check is throwing an error.

One concern I have here is I don't think we really should support expanding the default capability set, only reducing it as this is a major foot-gun with regard to builds... and of course buildkit has its own default caps.

WDYT @tonistiigi @tianon @justincormack

I am 100% onboard with only removing caps. I wanted to make the most flexibility in the approach.

On the subject of CAP_NET_RAW, #41030 is definitely related (the goal is to remove that from the default set).

So to that end, IMO adding extra caps to drop by default seems pretty reasonable, but I wonder if it's a temporary measure? What are the use cases beyond CAP_NET_RAW?

Sorry for the delay; catching up with my notifications

How did you see this getting implemented? Is it something where after a container is created we store a list of caps for that container? How would we go about checking the current caps for a container?

and

So that if a container is re-launched it has the same capabilities every-time it is launched.

Yes, it would likely have to be stored in the container's hostconfig that we store on disk. I must admit I haven't looked closely yet what the best approach would be. I saw that this PR progressed since the last time I looked, and wanted to verify what the behavior was for existing containers when changing defaults.
If we do proceed with this PR, we should design the expected behavior, but it might be a tricky one to get "right" (and to get the "least surprising" behavior);

• If no custom defaults are set, and if the container didn't specify custom defaults, then (I think) the expectation would be to keep the "current" behavior; setting a new custom default on the daemon won't modify the container's capabilities, but updating docker (which could have new capabilities) probably should.
• That said; if a future update of docker removes capabilities, that might be somewhat unexpected for existing containers as well, so perhaps we should consider "custom" and "default" defaults equal, and always bake those defaults in the container's configuration?

If so, if we pull a container do the caps come from the repo or the local daemon? (This makes a big difference for you we are trying to do.)

The caps should always be an option set at runtime (either by the daemon for defaults, or by the user). An image that's pulled from a registry cannot specify these (which would be a security risk, as then I could pull an image that gains additional privileges).

If we didn't have that in the default then this functionality is less of a priority. That being said I get the argument that removing that cap would cause some compatibility issues.

I know @justincormack has that one on his list; we definitely want to drop cap_net_raw if possible. IIRC, it would be possible for most distros (more specifically: base images), but there were some that still required it.
However, if upstream distros are modified, older versions likely wouldn't be updated, which would mean that (e.g.) using ping in an image using FROM ubuntu:16.04 wouldn't work, but FROM ubuntu:latest would.

(Wondering if it would be acceptable to diverge from upstream, and to patch the official images to make those changes @tianon)?

One concern I have here is I don't think we really should support expanding the default capability set, only reducing it as this is a major foot-gun with regard to builds... and of course buildkit has its own default caps.

and

So to that end, IMO adding extra caps to drop by default seems pretty reasonable, but I wonder if it's a temporary measure? What are the use cases beyond CAP_NET_RAW?

I also agree to the above; only being able to drop capabilities that are currently enabled by default would have my preference, unless there's clear use-cases where the opposite would be important.

And, yes, if dropping CAP_NET_RAW is the only use-case we know, I would definitely prefer not having the extra bells-and-whistles at all. Perhaps we should prioritise that effort instead, so that we can have a clearer timeframe within which that can be solved.

A penultimate note is that this is really about cap_net_raw at the end of the day. If we didn't have that in the default then this functionality is less of a priority. That being said I get the argument that removing that cap would cause some compatibility issues.

Let me clarify a bit here. We started this project because we had one of our internal analysis discovered cap_net_raw and told us we needed an audit and mitigation plan. One part of that was an audit approach that is outside this PR. The other was getting the ability to drop by default cap_net_raw upstream which is this PR. I opened this PR before I was aware of any talk of removing cap_net_raw. The goal here was to have docker's policy match our policy with respect to caps. If we don't have cap_net_raw the only other cap of concern is cap_dac_override for which we have mitigations in place.

On the subject of CAP_NET_RAW, #41030 is definitely related (the goal is to remove that from the default set).

So to that end, IMO adding extra caps to drop by default seems pretty reasonable, but I wonder if it's a temporary measure? What are the use cases beyond CAP_NET_RAW?

The only other case we have is dropping cap_dac_override to mitigate the damage from a container breakout. That being said we are also requiring Mandatory Access Controls (MAC) to mitigate container breakout so we can live with the MAC pulling double duty. I don't really see the value in a temporary fix if you guys are looking into more permanent fixes. So while technically, we care about cap_dac_override as well it is a very distant second from net_raw. So, if you guys didn't see the value in a special flag for one or two caps I can understand.

I also agree to the above; only being able to drop capabilities that are currently enabled by default would have my preference, unless there's clear use-cases where the opposite would be important.

And, yes, if dropping CAP_NET_RAW is the only use-case we know, I would definitely prefer not having the extra bells-and-whistles at all. Perhaps we should prioritise that effort instead, so that we can have a clearer timeframe within which that can be solved.

I think we are on the same page. I like the drop only approach and get your point on not wanting temporary features. What is your gut feeling on how long it will take to drop net_raw? Are we talking early next year or a year from now? If it is far out how does this sound as a stop gap: I add a flag that sets the default caps to not include the two caps we talked about.