Skip to content
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

dockerd: add --print-default-{seccomp,apparmor} profile #39923

Closed
wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Sep 13, 2019

Signed-off-by: Akihiro Suda akihiro.suda.cz@hco.ntt.co.jp

- What I did
added --print-default-{seccomp,apparmor} profile to dockerd, so that the user can inspect the default configuration

Fix #33060

- How to verify it

$ dockerd --print-default-seccomp-profile
{
        "defaultAction": "SCMP_ACT_ERRNO",
        "archMap": [
                {
                        "architecture": "SCMP_ARCH_X86_64",
                        "subArchitectures": [
                                "SCMP_ARCH_X86",
                                "SCMP_ARCH_X32"
                        ]
                },
...
                {
                        "names": [
                                "syslog"
                        ],
                        "action": "SCMP_ACT_ALLOW",
                        "args": [],
                        "comment": "",
                        "includes": {
                                "caps": [
                                        "CAP_SYSLOG"
                                ]
                        },
                        "excludes": {}
                }
        ]
}
$ dockerd --print-default-apparmor-profile


#include <tunables/global>


profile docker-default flags=(attach_disconnected,mediate_deleted) {

  #include <abstractions/base>


  network,
  capability,
  file,
  umount,


  signal (receive) peer=unconfined
,

  signal (send,receive) peer=docker-default,


  deny @{PROC}/* w,   # deny write for all files directly in /proc (not in a subdir)
  # deny write to files not in /proc/<number>/** or /proc/sys/**
  deny @{PROC}/{[^1-9],[^1-9][^0-9],[^1-9s][^0-9y][^0-9s],[^1-9][^0-9][^0-9][^0-9]*}/** w,
  deny @{PROC}/sys/[^k]** w,  # deny /proc/sys except /proc/sys/k* (effectively /proc/sys/kernel)
  deny @{PROC}/sys/kernel/{?,??,[^s][^h][^m]**} w,  # deny everything except shm* in /proc/sys/kernel/
  deny @{PROC}/sysrq-trigger rwklx,
  deny @{PROC}/kcore rwklx,

  deny mount,

  deny /sys/[^f]*/** wklx,
  deny /sys/f[^s]*/** wklx,
  deny /sys/fs/[^c]*/** wklx,
  deny /sys/fs/c[^g]*/** wklx,
  deny /sys/fs/cg[^r]*/** wklx,
  deny /sys/firmware/** rwklx,
  deny /sys/kernel/security/** rwklx,


  # suppress ptrace denials when using 'docker ps' or using 'ps' inside a container
  ptrace (trace,read,tracedby,readby) peer=docker-default,

}

- Description for the changelog
dockerd: add --dump-default-{seccomp,apparmor} profile

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

🐧

@AkihiroSuda
Copy link
Member Author

@justincormack @thaJeztah PTAL?

@AkihiroSuda
Copy link
Member Author

@thaJeztah @justincormack @cpuguy83 PTAL?

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, thought I left a comment already, but apparently forgot 😞

Wondering if we should add flags for these, or if we should log the profiles when (re)loading the configuration; same as I did in #36019 (8378dcf), that way both systemctl service reload docker (or (I think)) sending a SIGHUP would print the active configuration and profiles in the logs

cmd/dockerd/docker.go Outdated Show resolved Hide resolved
cmd/dockerd/docker.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Wondering if we should add flags for these, or if we should log the profiles when (re)loading the configuration

Guess it's slightly more complicate for the AppArmor profile to log it (the seccomp profile is JSON so easier to log) 🤔

@AkihiroSuda
Copy link
Member Author

Wondering if we should add flags for these, or if we should log the profiles when (re)loading the configuration; same as I did in #36019 (8378dcf), that way both systemctl service reload docker (or (I think)) sending a SIGHUP would print the active configuration and profiles in the logs

The latter approach almost doesn't work for scripting

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda changed the title dockerd: add --dump-default-{seccomp,apparmor} profile dockerd: add --print-default-{seccomp,apparmor} profile Sep 26, 2019
@thaJeztah
Copy link
Member

The latter approach almost doesn't work for scripting

True; basically trying to prevent adding a lot of flags if there's no real need; reading the related ticket; the main reason is to have the possibility to audit what's loaded. I'm not sure if scripting is involved in that, or if that's manually reading the profile (having the profile logged on startup, reload / SIGHUP / SIGUSR1) would still work for that.

If a file is needed with the profiles, we could consider doing the same as we do for the stack dumps, and write it to a file

for range c {
path, err := stackdump.DumpStacks(root)
if err != nil {
logrus.WithError(err).Error("failed to write goroutines dump")
} else {
logrus.Infof("goroutine stacks written to %s", path)
}
}

@AkihiroSuda
Copy link
Member Author

the main reason is to have the possibility to audit what's loaded

No, the main reason (to me) is to facilitate people to write custom profiles by modifying the default template: docker/for-linux#788

@cpuguy83
Copy link
Member

This seems fine, but an API endpoint (e.g. perhaps something like GET /debug/profiles/apparmor may be more helpful, especially for cases like docker desktop.

@AkihiroSuda
Copy link
Member Author

@thaJeztah API endpoint SGTY?

@AkihiroSuda
Copy link
Member Author

ping @thaJeztah

@AkihiroSuda
Copy link
Member Author

@thaJeztah WDYT about #39923 (comment) ?

Also curious whether this should be under /system or /debug. I think this feature is not only for debugging, and can be a path like /system/default_profiles/{apparmor,seccomp}

@mrueg
Copy link
Contributor

mrueg commented Sep 4, 2020

@cpuguy83 this would be nice to have in v21.x

Comment on lines +40 to +48
if printDefaultSeccompProfile && printDefaultApparmorProfile {
return errors.New("conflicting flag: --print-default-seccomp-profile and --print-default-apparmor-profile")
}
if printDefaultSeccompProfile {
return daemon.PrintDefaultSeccompProfile(cmd.OutOrStdout())
}
if printDefaultApparmorProfile {
return daemon.PrintDefaultAppArmorProfile(cmd.OutOrStdout())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that these cannot be combined, should we have a single flag that takes an option? Something like;

--print-default-profile=<seccomp|apparmor>

or (removing -default from the flag name)

--print-profile=<seccomp|apparmor>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also be the reverse, and make it --print-defaults (so that we could, e.g. use it to print other defaults that are configured, such as dns, ulimit, etc)

@thaJeztah
Copy link
Member

As to

API endpoint SGTY?

I think that would be useful to have (but will be a bigger change obviously); not sure if that endpoint should always print everything, or have options to provide more granular information (to avoid getting in the same situation as we have with the /info endpoint returning far too much information for most uses)

@AkihiroSuda
Copy link
Member Author

Should we implement --print-profile=<seccomp|apparmor> first in this PR, and then revisit the REST API in a follow-up PR?

@thaJeztah
Copy link
Member

@AkihiroSuda works for me; I just left another comment, thinking about naming it --print-default (which could then be used more-widely perhaps); WDYT? (suggestions/input welcome)

@AkihiroSuda
Copy link
Member Author

--print-default SGTM

@thaJeztah
Copy link
Member

@AkihiroSuda I just recalled this PR was still pending; looks like it needs a rebase 😬 - could you have a look?

@AkihiroSuda
Copy link
Member Author

We may have multiple built-in profiles (#42441), so this PR will probably needs to be redesigned

@AkihiroSuda AkihiroSuda closed this Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to see contents of AppArmor Profile "docker-default"
4 participants