Repository navigation
Tighten worker permissions with runtime-specific defaults - #2428
Tom O'Rourke (tjorourke) wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
b92da06 to
2f54f7d
Compare
2f54f7d to
716de59
Compare
716de59 to
9d6cd9c
Compare
15fed69 to
5fb14a9
Compare
|
Dmitry Berkovich (@dberkov) Zoe Zhao (@zoez7) could one of you review this WorkerPool security change, or point me to the right reviewer? The branch is now updated with main and conflicts are resolved. Controller/API race tests and code-generation checks pass, including the updated SandboxConfig fixture. Capability and seccomp behaviour has also been validated on EKS, as detailed above. I’d appreciate feedback on the opt-in API first; the secure-by-default questions can be handled separately. |
Could you please wait will Wed and I will find someone form security team to take a look on it. |
|
Of course no problem at all. |
|
SYS_CHROOT ... IIRC virtiofsd is using this to sandbox itself. Are you sure this is the right capability set, or it just managed to run? |
|
These all need close scrutiny, the chroot permission is a good example, we should not trade weakening the interior sandboxes to attempt to harden the normal runc/crun outer containers the workers run in. The hard isolation boundary is the actor sandbox or the node. Making them configurable may actually weaken the assumptions around them, I'm pretty sure we should just stick to a correctly hardened default and iterate on that. |
Thanks for the feedback Benjamin Elder (@BenTheElder) I will run some more tests and share test output. Ta! |
|
Benjamin Elder (@BenTheElder) thanks for the review. I've pushed a9653bd as a new commit, without squashing the earlier commits. TL;DR compared with the first proposal: With all optional drops selected:
On Fresh images of the integrated revision passed the permanent filesystem test with default and restricted workers on gVisor VM and micro-VM metal. This checked ownership, modes and contents through pause/resume and suspend/resume, plus micro-VM non-root access and device-node creation. The API, CRD and documentation are updated, and the race tests and repository verification checks passed. I agree the inner sandbox takes priority. The worker still retains |
Yes, I think substrate is responsible for getting this right. We should use the minimum capabilities needed to implement the sandboxes. Also, leaving it non-configurable for now makes it easier for us to reason about refining the set in the future. Seccomp needs more thought, IIRC this is tricky to distribute, but I'd like to consider that as separate change vs tightening the clearly portably pod settings. |
a9653bd to
f3f952f
Compare
Thanks Ben, agreed! I’ve reworked the PR to use fixed runtime-specific defaults and removed the per-pool security API. The reduced sets are now applied automatically: 9 capabilities for gVisor and 11 for micro-VM, retaining the permissions needed for filesystem ownership and virtiofsd’s guest operations. I’ve also removed the worker seccomp profiles and generator, leaving worker seccomp hardening and distribution for a separate change. Shall i raise a new issue/PR for this ? The PR is rebased and squashed. Local tests and verification pass. The reduced capability sets passed the earlier live filesystem and lifecycle tests. |
that would be great, thanks! I have this PR on my personal list, just lots of PRs in flight |
Use fixed capability sets for gVisor and micro-VM workers and disable privilege escalation. Preserve virtiofsd filesystem permissions and namespace isolation while removing unnecessary outer-worker capabilities. Cover the defaults and reconciliation with controller tests, and exercise filesystem ownership and guest operations across actor lifecycle transitions. Keep worker seccomp profile distribution out of this change. AI-assisted.
f3f952f to
774cee8
Compare
|
Thanks Ben. I've raised #2749 for worker seccomp, with the profiles and generator from this PR in draft #2750. The issue sets out the distribution options. I've suggested having atelet install the profile, but I'm happy to go another way. I've also rebased this PR onto main ( |
|
DAC_READ_SEARCH, DAC_OVERRIDE and SYS_ADMIN are all relatively high privilege capabilities, and we might be able to remove those as well, or limit the blast radius further (via user namespaces) I think we can relatively easily remove the need for DAC_READ_SEARCH by adding "--modcaps=-dac_read_search" to the virtiofsd flags (and then dropping dac_read_search from the capabilities given to the process). removing DAC_OVERRIDE and SYS_ADMIN seems more complicated, but maybe we mitigate by setting up user namespaces and then UID map to a low privilege user that still has access to the necessary ate-related host files? Doesn't necessarily need to be in this PR though. Just an idea for the future. |
Summary
Use fixed runtime-specific worker capability sets, following maintainer feedback that Substrate should own the minimum permissions needed to implement its sandboxes. Every worker drops ALL container-runtime default capabilities, adds back its runtime-specific set, and sets
allowPrivilegeEscalation: false.MKNOD,SETPCAP,SYS_CHROOT,SYS_PTRACENET_RAW,SETPCAP,SYS_CHROOT,SYS_PTRACEBoth workers still run as root and retain
SYS_ADMINandNET_ADMIN. This is incremental worker hardening; the existing deployment-permissions requirements still apply.Fixes #2422. That issue asked for these settings to be configurable per pool; following review, the workers get fixed, reviewed defaults instead. Worker seccomp is split out to #2749, with the profiles in draft #2750.
Changes
TestWorkerSecurityFilesystemand the probe fixture covering non-root-owned files through pause/resume and suspend/resume. The micro-VM lane also checks non-root access, cross-UID denial and device-node creation. The test checks the fixed defaults directly.CAP_MKNOD. Creating the 0:0 overlay whiteouts has not neededCAP_MKNODsince Linux 5.8.mainatc4518a13, including thego.ate.dev/substrateimport path and the upstream worker renames. Squash the PR to one commit.Actor container capabilities remain configured separately through
ActorTemplate.containers[].securityContext.capabilities. There is no new WorkerPool API or opt-in configuration.Validation
On the current revision (
774cee82, on upstreammainatc4518a13):go test -race -p 2 ./...andhack/verify-all.sh: passed.m6i.xlargeand micro-VM oni4i.metal: golden creation, andTestWorkerSecurityFilesystempassed. That covers pause/resume and suspend/resume with UID/GID 1000 ownership, setgid mode and contents on the rootfs and a durable directory, plus micro-VM non-root access, cross-UID denial and device-node creation./procon the nodes: the worker's permitted, effective and bounding sets are exactly the 9 gVisor (0x802030cb) or 11 micro-VM (0x882010df) capabilities, with no ambient capabilities,NoNewPrivs: 1and seccomp mode 0. runsc's gofer and boot processes run in their own mount, PID, network and user namespaces with their own root and seccomp filters. The virtiofsd serving child has its own mount, PID and network namespaces, its root at the shared directory, its normal nine capabilities and its own filter. cloud-hypervisor's VMM, vCPU and device threads run with their own filters.EPERMfromuserfaultfd, which needsCAP_SYS_PTRACEwhilevm.unprivileged_userfaultfdis 0. It reached the kernel with the previous capability set.pr-workflowcover both sandbox classes; they need a maintainer to approve the run on this fork PR.Test plan and findings for the earlier opt-in revision (PDF)
AI-assisted.