Mounts and capabilities #507
Conversation
to avoid shadowing - popular open-sourced container runtimes like docker and containerD will sort mounts in alphabetical order to avoid this issue, but there's no guarantee that all container runtime would do so
internal/resource/statefulset.go
Outdated
| Drop: []corev1.Capability{"ALL"}, | ||
| Add: []corev1.Capability{"CHOWN", "FOWNER"}, | ||
| Drop: []corev1.Capability{ | ||
| "FSETID", | ||
| "KILL", | ||
| "SETGID", | ||
| "SETUID", | ||
| "SETPCAP", | ||
| "NET_BIND_SERVICE", | ||
| "NET_RAW", | ||
| "SYS_CHROOT", | ||
| "MKNOD", | ||
| "AUDIT_WRITE", | ||
| "SETFCAP", | ||
| }, |
There was a problem hiding this comment.
@coro proposed in #303 (comment) to drop all and only allow the needed capabilities.
Allow listing rather than deny listing is a better security standard.
IIRC the corresponding bug in the ESXi runtime will be fixed soon.
@Zerpet was discovering that issue and was in touch with that team.
There was a problem hiding this comment.
As I said in standup, I am aware of this fix: https://gitlab.eng.vmware.com/core-build/spherelet-agent/commit/31b0f133d2fca969b4e561d8f05a26b1f08dd498.
However, It's unclear to me when this fix can be released and how often does user upgrade while using these environments with spherelet. As long as we are dropping all known unneeded capabilities, I don't see much difference of security protection between the two different approaches. In addition to that, this is only the security context for our initContainer, which is ephemeral and not long running. IMO we should merge this update to make sure that our operator works with spherelet before GA. Thoughts? @mkuratczyk @coro
There was a problem hiding this comment.
Oh right, I thought the bug mentioned in standup was a different one. My bad.
In that case, let's merge this PR as is and revert this change in the future once the fix is rolled out?
There was a problem hiding this comment.
Feel free to add a comment like // Remove default capabilities allowed by Docker except for CHOWN and FOWNER. Otherwise, it's hard to read what capabilities the container will run with.
internal/resource/statefulset.go
Outdated
| Drop: []corev1.Capability{"ALL"}, | ||
| Add: []corev1.Capability{"CHOWN", "FOWNER"}, | ||
| Drop: []corev1.Capability{ | ||
| "FSETID", | ||
| "KILL", | ||
| "SETGID", | ||
| "SETUID", | ||
| "SETPCAP", | ||
| "NET_BIND_SERVICE", | ||
| "NET_RAW", | ||
| "SYS_CHROOT", | ||
| "MKNOD", | ||
| "AUDIT_WRITE", | ||
| "SETFCAP", | ||
| }, |
There was a problem hiding this comment.
Feel free to add a comment like // Remove default capabilities allowed by Docker except for CHOWN and FOWNER. Otherwise, it's hard to read what capabilities the container will run with.
- previous configurations didn't work with ESXi runtime - dropping all capabilities except CHOWN, DAC_OVERRIDE, and FOWNER - CHOWN is necessary to change a file's owner, the second is necessary to permit chown to traverse directories to which root doesn't otherwise have access. FOWNER bypass checks on operations that require the file system UID of the process to match the UID of the file.
ChunyiLyu
force-pushed
the
mounts-and-capabilities
branch
from
6c3e4e2 to
7549bfd
Compare
This fixes issue: https://github.com/rabbitmq/tanzu-cluster-operator/issues/20
Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Summary Of Changes
Strictly speaking DAC_OVERRIDE isn't necessary, only DAC_READ_SEARCH is, but as it's not included in the default set using it is slightly more complex. I don't think there's much practical benefit since by default the only process with any capabilities at all is the chown process (everything else runs as 472 and gets no capabilities at all).
Additional Context
Local Testing
unit, integration, and system tests have all passed