Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" #42481

thaJeztah · 2021-06-07T16:45:45Z

waiting for #42393, then I can add some tests.
Possibly will be splitting this PR in some chunks as well, in case we want to backport the "unconfined" daemon configuration fix.

Add const for "unconfined" and "default" seccomp profiles
Fix daemon.json and daemon --seccomp-profile not accepting "unconfined"
Commit b237189 implemented an option to
set the default seccomp profile in the daemon configuration. When that PR
was reviewed, it was discussed to have the option accept the path to a custom
profile JSON file; daemon: add a flag to override the default seccomp profile #26276 (comment)

However, in the implementation, the special "unconfined" value was not taken into
account. The "unconfined" value is meant to disable seccomp (more factually:
run with an empty profile).

While it's likely possible to achieve this by creating a file with an an empty
({}) profile, and passing the path to that file, it's inconsistent with the
--security-opt seccomp=unconfined option on docker run and docker create,
which is both confusing, and makes it harder to use (especially on Docker Desktop,
where there's no direct access to the VM's filesystem).

This patch adds the missing check for the special "unconfined" value.
daemon: allow "builtin" as valid value for seccomp profiles
This allows containers to use the embedded default profile if a different
default is set (e.g. "unconfined") in the daemon configuration. Without this
option, users would have to copy the default profile to a file in order to
use the default.
daemon: move custom seccomp profile warning from CLI to daemon side

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

tianon · 2021-06-24T18:49:33Z

From a security perspective, I agree this isn't super great, but IMO it's not too much different from having a host that doesn't support either AppArmor or SELinux (users generally need to know/trust the security characteristics of the host/daemon they're running on), so I'm +1 on consistency here.

tianon · 2021-06-24T19:03:46Z

daemon/config/config.go

@@ -58,6 +58,12 @@ const (
 	LinuxV1RuntimeName = "io.containerd.runtime.v1.linux"
 	// LinuxV2RuntimeName is the runtime used to specify the containerd v2 runc shim
 	LinuxV2RuntimeName = "io.containerd.runc.v2"
+
+	// SeccompProfileDefault is the built-in default seccomp profile.
+	SeccompProfileDefault = "default"


I think this naming choice is probably a little bit ambiguous -- if I set the daemon-wide --secomp-profile to something other than the built-in profile, does --security-opt seccomp=default get me the daemon-wide default, or the Docker built-in profile? (Maybe builtin or docker are better name options if it's the latter?)

tianon · 2021-06-24T19:04:31Z

cmd/dockerd/config_unix.go

@@ -58,7 +58,7 @@ func installConfigFlags(conf *config.Config, flags *pflag.FlagSet) error {
 	flags.StringVar(&conf.InitPath, "init-path", "", "Path to the docker-init binary")
 	flags.Int64Var(&conf.CPURealtimePeriod, "cpu-rt-period", 0, "Limit the CPU real-time period in microseconds for the parent cgroup for all containers")
 	flags.Int64Var(&conf.CPURealtimeRuntime, "cpu-rt-runtime", 0, "Limit the CPU real-time runtime in microseconds for the parent cgroup for all containers")
-	flags.StringVar(&conf.SeccompProfile, "seccomp-profile", "", "Path to seccomp profile")
+	flags.StringVar(&conf.SeccompProfile, "seccomp-profile", "", `Path to seccomp profile. Use "unconfined" to disable the default seccomp profile`)


Should this default to config.SeccompProfileDefault so that the effective default value shows up in the help output?

I was a bit in doubt; I think for most options we consider "" (default string value) to also be default. The ambiguity was a bit in that when using docker info, it will show default (if nothing is set), so yes, from that perspective perhaps it makes sense to use it here as well. 😅

daemon/info.go

integration/daemon/daemon_test.go

thaJeztah · 2021-07-07T11:27:34Z

I applied two of your suggestions.

Remaining questions are (see above);

do we want the command-line flag to show "default" as default value, or do we prefer "" (empty string) to indicate "use the default" ?
word of choice for "default" as it is ambiguous when using on docker run; in that case "default" could be interpreted as either:
- "default" (as configured on the daemon, which may not be the "default")
- "default" (compiled-in, docker's default profile
Alternative names for the above: builtin or docker (or perhaps docker-default?)
If we decide to change the name, do we want to change what's shown in docker info as well? (Likely: "yes")

The other question to answer is: "do we want to allow unconfined" as a default? This was brought up by @tonistiigi. Motivation for that question was that (already possible since #26276) being able to set a non-standard seccomp profile as default on the daemon, causes inconsistency. A container may be able to run on "daemon A", but fail to run on "daemon B", because it uses a different configuration.

While custom profiles are "one thing", disabling seccomp altogether (using "unconfined" as a default) may be another thing, as it will be a less-secure configuration.

As said;

this is already the case (and a "custom profile" could already be an empty profile, in which case the default is (roughly) the equivalent of "unconfined". If that's a real concern, then the only option would be to revert daemon: add a flag to override the default seccomp profile #26276
also to add: while it's possible to make these changes in the daemon configuration, it's not the default, so it will be a conscious decision of the user. (not taking into account some distributions in which packagers include a custom profile and set this as default). In such a setup, docker info should print a WARNING that a non-standard profile is in use (so it's not "invisible").

Happy to get input / feedback on the above: @justincormack @tonistiigi @tianon @AkihiroSuda

tianon · 2021-07-30T20:16:46Z

Happy to get input / feedback on the above

I feel like I already opined, but I'll do so again 😇

do we want the command-line flag to show "default" as default value, or do we prefer "" (empty string) to indicate "use the default" ?

IMO that depends a little bit on whether users can specify the empty string to get the default back (and thus override a config file or similar) -- I think showing our explicit default string is probably clearer though in either case.

Alternative names for the above: builtin or docker (or perhaps docker-default?)

I like builtin, personally -- I think it clearly conveys what this value means.

If we decide to change the name, do we want to change what's shown in docker info as well?

yes 😄

do we want to allow unconfined as a default?

If users want this foot-cannon, it's something they can already accomplish, so I don't really see a compelling reason to add a special case in our code to try and stop it; at most, I'd think a warning -- do we do anything in particular when AppArmor or SELinux are either disabled or unavailable?

There are already a lot of settings available (both in Docker and outside Docker) that can affect the ability of arbitrary contains to successfully run, and so I don't think that's a very strong justification for denying it (especially when it's only a cosmetic denial that then simply requires more effort to accomplish via an empty profile). By that argument, we really shouldn't be allowing users to specify a new default profile at all. 😅

cpuguy83

Agree with @tianon here.

builtin is a good name.
We shouldn't try to prevent people from doing what they want to do

I'll add that while we like to make secure containers more accessible, it is not a security product and shouldn't be trying to restrict people from running it the way they need to.

thaJeztah · 2021-08-03T17:06:23Z

I started rebasing this one, and renaming s/default/builtin/, then realized we have the client-side warning that would need to be updated (I saw @cpuguy83 already found it 😅). Will continue the rename later today

thaJeztah · 2021-08-04T16:24:11Z

@tianon @cpuguy83 I updated this:

changed default to builtin (do we want it to be built-in or one word?)
changed the --seccomp-profile flag to show the default (builtin) as default value

tianon

LGTM; I think builtin is fine 🤷 😇

cpuguy83 · 2021-08-06T18:41:55Z

=== RUN   TestDockerSuite/TestInfoSecurityOptions
    docker_cli_info_unix_test.go:30: assertion failed: [name=apparmor name=seccomp,profile=builtin] does not contain name=seccomp,profile=default
    --- FAIL: TestDockerSuite/TestInfoSecurityOptions (0.01s)

thaJeztah · 2021-08-06T19:19:56Z

=== RUN TestDockerSuite/TestInfoSecurityOptions
docker_cli_info_unix_test.go:30: assertion failed: [name=apparmor name=seccomp,profile=builtin] does not contain name=seccomp,profile=default
--- FAIL: TestDockerSuite/TestInfoSecurityOptions (0.01s)

Ah, dang. Missed on

thaJeztah · 2021-08-06T19:21:01Z

Updated! (hope it's green again now)

thaJeztah · 2021-08-07T08:10:50Z

TestCreateServiceConfigFileMode also continues to be a pain; quite flaky

=== RUN   TestCreateServiceConfigFileMode
    create_test.go:355: assertion failed: 2 (int) != 1 (int)
--- FAIL: TestCreateServiceConfigFileMode (8.55s)

otherwise green, but let me kick CI again

thaJeztah · 2021-08-07T08:26:58Z

Actually, I don't like that I'm now changing "default" to "builtin" in the first commit; that makes it hard to discover; let me move that to a separate commit; I'll ping when done

thaJeztah · 2021-08-07T13:24:09Z

Not sure what changed, but definitely seeing more of these recently:

#59 [shfmt 1/1] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     --mount=type=bind,src=hack/dockerfile/install,target=/tmp/install         PREFIX=/build /tmp/install/install.sh shfmt
#59 sha256:5c49c7cdc1f31702c618c3d616ce7295fad095d629951a20eb8dcd3e334fe8c7
#59 618.6 error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
#59 618.6 fatal: the remote end hung up unexpectedly
#59 618.6 fatal: protocol error: bad pack header
#59 ERROR: executor failed running [/bin/sh -c PREFIX=/build /tmp/install/install.sh shfmt]: exit code: 128

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Using "default" as a name is a bit ambiguous, because the _daemon_ default can be changed using the '--seccomp-profile' daemon flag. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Commit b237189 implemented an option to set the default seccomp profile in the daemon configuration. When that PR was reviewed, it was discussed to have the option accept the path to a custom profile JSON file; moby#26276 (comment) However, in the implementation, the special "unconfined" value was not taken into account. The "unconfined" value is meant to disable seccomp (more factually: run with an empty profile). While it's likely possible to achieve this by creating a file with an an empty (`{}`) profile, and passing the path to that file, it's inconsistent with the `--security-opt seccomp=unconfined` option on `docker run` and `docker create`, which is both confusing, and makes it harder to use (especially on Docker Desktop, where there's no direct access to the VM's filesystem). This patch adds the missing check for the special "unconfined" value. Co-authored-by: Tianon Gravi <admwiggin@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This allows containers to use the embedded default profile if a different default is set (e.g. "unconfined") in the daemon configuration. Without this option, users would have to copy the default profile to a file in order to use the default. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This makes sure that the value set in the daemon can be used as-is, without having to replicate the normalization logic elsewhere. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2021-08-07T13:43:04Z

Updated; moved renaming the default profile to a separate commit 👍 PTAL

cpuguy83

LGTM

thaJeztah added status/2-code-review area/security/seccomp area/daemon labels Jun 7, 2021

thaJeztah force-pushed the seccomp_unconfined_daemon branch from 010dede to c0933e6 Compare June 8, 2021 08:25

thaJeztah force-pushed the seccomp_unconfined_daemon branch 4 times, most recently from 33a0d93 to 5682060 Compare June 24, 2021 17:58

thaJeztah marked this pull request as ready for review June 24, 2021 17:58

tianon reviewed Jun 24, 2021

View reviewed changes

daemon/info.go Outdated Show resolved Hide resolved

tianon reviewed Jun 24, 2021

View reviewed changes

integration/daemon/daemon_test.go Outdated Show resolved Hide resolved

thaJeztah force-pushed the seccomp_unconfined_daemon branch from ffadfdc to 5eb2ab7 Compare July 7, 2021 11:11

thaJeztah added the impact/changelog label Jul 7, 2021

thaJeztah force-pushed the seccomp_unconfined_daemon branch 3 times, most recently from 4c7277f to dd548f8 Compare July 19, 2021 17:16

thaJeztah mentioned this pull request Aug 3, 2021

info: skip client-side warning about seccomp profile on API >= 1.42 docker/cli#3230

Merged

cpuguy83 reviewed Aug 3, 2021

View reviewed changes

thaJeztah force-pushed the seccomp_unconfined_daemon branch from dd548f8 to e99434c Compare August 4, 2021 16:15

tianon approved these changes Aug 4, 2021

View reviewed changes

thaJeztah force-pushed the seccomp_unconfined_daemon branch from e99434c to f28dde8 Compare August 5, 2021 19:59

thaJeztah force-pushed the seccomp_unconfined_daemon branch from f28dde8 to 6957572 Compare August 6, 2021 19:20

thaJeztah and others added 6 commits August 7, 2021 15:36

Add const for "unconfined" and default seccomp profiles

ee02257

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/config: rename the default seccomp profile to "builtin"

ac449d6

Using "default" as a name is a bit ambiguous, because the _daemon_ default can be changed using the '--seccomp-profile' daemon flag. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: move custom seccomp profile warning from CLI to daemon side

04f932a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon: normalize seccomp profile as part of setupSeccompProfile()

27aaadb

This makes sure that the value set in the daemon can be used as-is, without having to replicate the normalization logic elsewhere. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the seccomp_unconfined_daemon branch from 6957572 to 27aaadb Compare August 7, 2021 13:42

thaJeztah changed the title ~~Fix daemon.json and daemon --seccomp-profile not accepting "unconfined"~~ Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" Aug 7, 2021

thaJeztah added this to the 21.xx milestone Aug 9, 2021

thaJeztah mentioned this pull request Aug 10, 2021

Docker engine does not support the parameter "--security-opt seccomp=" when executing command "docker build" #34454

Closed

justincormack approved these changes Aug 11, 2021

View reviewed changes

cpuguy83 approved these changes Aug 11, 2021

View reviewed changes

cpuguy83 merged commit 7681a3e into moby:master Aug 11, 2021

thaJeztah deleted the seccomp_unconfined_daemon branch August 11, 2021 18:03

thaJeztah mentioned this pull request Oct 11, 2021

[20.10 backport] info: skip client-side warning about seccomp profile on API >= 1.42 docker/cli#3335

Merged

This was referenced Apr 12, 2023

docs/reference: update dockerd docs for changes in 23.0 docker/cli#4199

Merged

[23.0 backport] docs/reference: update dockerd docs for changes in 23.0 docker/cli#4202

Merged

thaJeztah mentioned this pull request Jan 5, 2024

cli/command/container: parseSecurityOpts: fix --security-opt seccomp=builtin docker/cli#4754

Merged

thaJeztah mentioned this pull request Jan 20, 2024

info: remove printSecurityOptionsWarnings, printServerWarnings docker/cli#4809

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" #42481

Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" #42481

thaJeztah commented Jun 7, 2021 •

edited

tianon commented Jun 24, 2021

tianon Jun 24, 2021

tianon Jun 24, 2021

thaJeztah Jun 24, 2021

thaJeztah commented Jul 7, 2021

tianon commented Jul 30, 2021

cpuguy83 left a comment

thaJeztah commented Aug 3, 2021

thaJeztah commented Aug 4, 2021

tianon left a comment

cpuguy83 commented Aug 6, 2021

thaJeztah commented Aug 6, 2021

thaJeztah commented Aug 6, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

cpuguy83 left a comment

Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" #42481

Fix daemon.json and daemon --seccomp-profile not accepting "unconfined", and rename default profile to "builtin" #42481

Conversation

thaJeztah commented Jun 7, 2021 • edited

tianon commented Jun 24, 2021

tianon Jun 24, 2021

Choose a reason for hiding this comment

tianon Jun 24, 2021

Choose a reason for hiding this comment

thaJeztah Jun 24, 2021

Choose a reason for hiding this comment

thaJeztah commented Jul 7, 2021

tianon commented Jul 30, 2021

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah commented Aug 3, 2021

thaJeztah commented Aug 4, 2021

tianon left a comment

Choose a reason for hiding this comment

cpuguy83 commented Aug 6, 2021

thaJeztah commented Aug 6, 2021

thaJeztah commented Aug 6, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

thaJeztah commented Aug 7, 2021

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah commented Jun 7, 2021 •

edited