From a474a266701975902db5cb6257cf84880322c43c Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 18 Aug 2026 18:38:45 -0700 Subject: [PATCH] sec(dind): gate --security-opt seccomp/apparmor=unconfined behind allow_privileged HostConfig.SecurityOpt was applied by a loop that ran regardless of dind.allow_privileged, so a sibling container could strip its own seccomp filter with `docker run --security-opt seccomp=unconfined` on a host configured to deny the elevation stack. Same for apparmor=unconfined. The gate checked Privileged and CapAdd only; the later loop applied a field the gate never looked at. Extend the existing gate rather than adding a second one. elevatingSecurityOpt is now the single answer to "does this --security-opt ask for elevation?" and both checkPrivilegedGate and the applier (securityOptSpecOpts) read it, so refusing and applying cannot drift apart again. Both the key=value and the legacy key:value spellings are covered because the applier accepted both. Behaviour with allow_privileged = true is unchanged: the options still reach the OCI spec, which is what setup-buildx's container driver needs. Every other --security-opt value is still ignored rather than refused. The 403 names the option, the sandbox it removes, and the config knob that governs it, and it still runs after request-shape validation so a malformed create keeps its 400. Fixes #172 --- pkg/config/config.go | 7 +- pkg/dind/containers.go | 86 ++++++++--- pkg/dind/dind.go | 9 +- pkg/dind/privileged_gate_test.go | 253 +++++++++++++++++++++++++++++++ 4 files changed, 331 insertions(+), 24 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index ae0ae709..19da3434 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -441,9 +441,10 @@ type DindConfig struct { CacheMaxAge time.Duration `toml:"cache_max_age"` // AllowPrivileged controls whether `docker run --privileged` (or - // HostConfig.Privileged=true / HostConfig.CapAdd) from inside a job - // is honored. When true, a sibling container can request the full - // elevation stack (all caps, all devices, seccomp/apparmor off, + // HostConfig.Privileged=true / HostConfig.CapAdd, or + // `--security-opt seccomp=unconfined` / `apparmor=unconfined`) from + // inside a job is honored. When true, a sibling container can request + // the full elevation stack (all caps, all devices, seccomp/apparmor off, // writable sysfs/cgroupfs) — needed for KIND clusters, nested // containerd, /dev/fuse-style mounts, etc. When false, such requests // are rejected with HTTP 403. diff --git a/pkg/dind/containers.go b/pkg/dind/containers.go index fc6570d7..7b1b73d7 100644 --- a/pkg/dind/containers.go +++ b/pkg/dind/containers.go @@ -210,9 +210,67 @@ func (s *Server) resolveContainerID(nameOrID string) string { return nameOrID } +// elevatingSecurityOpt reports whether a single --security-opt value asks this +// shim to strip a sandbox layer, and names the mechanism it strips ("seccomp" +// or "apparmor"). +// +// This is the single answer to "does this --security-opt ask for elevation?": +// both checkPrivilegedGate and securityOptSpecOpts call it, so the gate cannot +// decide one thing while the applier does another. That divergence was the bug +// — the gate looked at Privileged/CapAdd while a separate, ungated loop applied +// oci.WithSeccompUnconfined for `--security-opt seccomp=unconfined`, letting a +// sibling container drop its own seccomp filter on a host configured with +// dind.allow_privileged = false. +// +// Docker accepts both the `key=value` and the legacy `key:value` spelling, so +// both are recognised here. Anything else (`label=...`, `no-new-privileges=...`, +// a custom seccomp profile path) does not reach the OCI spec at all — see +// securityOptSpecOpts — so it is not elevation and is not refused. +func elevatingSecurityOpt(opt string) (mechanism string, elevating bool) { + key, value, ok := strings.Cut(opt, "=") + if !ok { + key, value, ok = strings.Cut(opt, ":") + } + if !ok || value != "unconfined" { + return "", false + } + switch key { + case "seccomp", "apparmor": + return key, true + } + return "", false +} + +// securityOptSpecOpts turns HostConfig.SecurityOpt into the OCI options that +// implement it. Only the elevating values do anything; every other +// --security-opt is ignored, exactly as it was before this was a function. +// +// Reachable only with the gate open — checkPrivilegedGate refuses these same +// values with 403 when dind.allow_privileged = false, off the same +// elevatingSecurityOpt predicate, so refusing and applying cannot drift apart. +func securityOptSpecOpts(securityOpt []string) []oci.SpecOpts { + var out []oci.SpecOpts + for _, opt := range securityOpt { + switch mechanism, _ := elevatingSecurityOpt(opt); mechanism { + case "seccomp": + out = append(out, oci.WithSeccompUnconfined) + case "apparmor": + out = append(out, oci.WithApparmorProfile("")) + } + } + return out +} + // checkPrivilegedGate returns a user-facing rejection message and blocked=true -// when the request asks for elevation (Privileged=true or CapAdd) but the gate -// is closed (allowPrivileged=false). Otherwise blocked=false and msg is empty. +// when the request asks for elevation but the gate is closed +// (allowPrivileged=false). Otherwise blocked=false and msg is empty. +// +// "Elevation" is every request field this handler turns into an OCI option that +// loosens the sandbox: Privileged, CapAdd, and the --security-opt values that +// disable seccomp or AppArmor. Keep new ones here rather than checking them at +// the point of use — one place has to answer "did this request ask for the +// elevation stack?", or a field gets applied on a path the gate never sees. +// // Pure function so the handler stays simple and tests don't need a containerd // client to exercise the gate logic. func checkPrivilegedGate(allowPrivileged bool, hc *hostConfig) (msg string, blocked bool) { @@ -225,6 +283,12 @@ func checkPrivilegedGate(allowPrivileged bool, hc *hostConfig) (msg string, bloc if len(hc.CapAdd) > 0 { return fmt.Sprintf("--cap-add (%v) is disabled on this host (set dind.allow_privileged = true in ephemerd config to enable)", hc.CapAdd), true } + for _, opt := range hc.SecurityOpt { + if mechanism, elevating := elevatingSecurityOpt(opt); elevating { + return fmt.Sprintf("--security-opt %s=unconfined is disabled on this host: it removes the %s sandbox that keeps an unprivileged sibling container unprivileged (set dind.allow_privileged = true in ephemerd config to enable)", + mechanism, mechanism), true + } + } return "", false } @@ -445,21 +509,9 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { opts = append(opts, oci.WithAddedCapabilities(req.HostConfig.CapAdd)) } - // Security options (seccomp=unconfined, apparmor=unconfined). - for _, opt := range req.HostConfig.SecurityOpt { - switch { - case opt == "seccomp=unconfined" || opt == "seccomp:unconfined": - opts = append(opts, oci.WithSeccompUnconfined) - case strings.HasPrefix(opt, "apparmor=") || strings.HasPrefix(opt, "apparmor:"): - profile := strings.SplitN(opt, "=", 2) - if len(profile) == 1 { - profile = strings.SplitN(opt, ":", 2) - } - if len(profile) == 2 && profile[1] == "unconfined" { - opts = append(opts, oci.WithApparmorProfile("")) - } - } - } + // Security options (seccomp=unconfined, apparmor=unconfined), gated + // by checkPrivilegedGate above. + opts = append(opts, securityOptSpecOpts(req.HostConfig.SecurityOpt)...) // Private cgroup namespace (--cgroupns=private). if req.HostConfig.CgroupnsMode == "private" { diff --git a/pkg/dind/dind.go b/pkg/dind/dind.go index a3af56cc..70322c89 100644 --- a/pkg/dind/dind.go +++ b/pkg/dind/dind.go @@ -170,10 +170,11 @@ type Config struct { RunnerNetNS string // AllowPrivileged controls whether sibling containers may opt into - // the full elevation stack via HostConfig.Privileged or via - // HostConfig.CapAdd. When false, requests carrying either are - // rejected with HTTP 403. See config.DindConfig.AllowPrivileged for - // the threat model. + // the full elevation stack via HostConfig.Privileged, via + // HostConfig.CapAdd, or via the HostConfig.SecurityOpt values that + // switch off seccomp or AppArmor. When false, requests carrying any of + // them are rejected with HTTP 403. See config.DindConfig.AllowPrivileged + // for the threat model. AllowPrivileged bool // RegistryMirror routes this job's image pulls through a LAN diff --git a/pkg/dind/privileged_gate_test.go b/pkg/dind/privileged_gate_test.go index c6d034fa..b29adf37 100644 --- a/pkg/dind/privileged_gate_test.go +++ b/pkg/dind/privileged_gate_test.go @@ -2,6 +2,7 @@ package dind import ( "bytes" + "context" "encoding/json" "io" "log/slog" @@ -9,6 +10,9 @@ import ( "net/http/httptest" "strings" "testing" + + "github.com/containerd/containerd/v2/pkg/oci" + ocispec "github.com/opencontainers/runtime-spec/specs-go" ) // gateTestServer returns a Server with only the fields handleContainerCreate @@ -133,3 +137,252 @@ func TestCheckPrivilegedGate_ClosedAllowsZeroHostConfig(t *testing.T) { t.Errorf("non-elevated HostConfig blocked: msg=%q", msg) } } + +// --security-opt gate. Before this was gated, HostConfig.SecurityOpt was +// applied by a loop that ran regardless of allowPrivileged, so +// `docker run --security-opt seccomp=unconfined` stripped the sibling's seccomp +// filter on a host configured with dind.allow_privileged = false. + +func TestHandleContainerCreate_SeccompUnconfinedDeniedWhenGateClosed(t *testing.T) { + s := gateTestServer(false) + w := postCreate(t, s, createRequest{ + Image: "alpine:3.20", + HostConfig: &hostConfig{SecurityOpt: []string{"seccomp=unconfined"}}, + }) + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", w.Code, w.Body.String()) + } + if !bytes.Contains(w.Body.Bytes(), []byte("seccomp=unconfined")) { + t.Errorf("body should echo the refused option: %s", w.Body.String()) + } + if !bytes.Contains(w.Body.Bytes(), []byte("dind.allow_privileged")) { + t.Errorf("body should name the config knob that governs it: %s", w.Body.String()) + } +} + +func TestHandleContainerCreate_ApparmorUnconfinedDeniedWhenGateClosed(t *testing.T) { + s := gateTestServer(false) + w := postCreate(t, s, createRequest{ + Image: "alpine:3.20", + HostConfig: &hostConfig{SecurityOpt: []string{"apparmor=unconfined"}}, + }) + if w.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403; body=%s", w.Code, w.Body.String()) + } + if !bytes.Contains(w.Body.Bytes(), []byte("apparmor=unconfined")) { + t.Errorf("body should echo the refused option: %s", w.Body.String()) + } +} + +func TestCheckPrivilegedGate_ClosedRejectsSecurityOpt(t *testing.T) { + // The colon spellings are here because the apply loop accepts them too: + // gate and applier read the same predicate, and this is what stops the + // two from drifting apart again. + cases := []struct { + name string + opt string + want string + }{ + {"seccomp equals", "seccomp=unconfined", "seccomp=unconfined"}, + {"seccomp colon", "seccomp:unconfined", "seccomp=unconfined"}, + {"apparmor equals", "apparmor=unconfined", "apparmor=unconfined"}, + {"apparmor colon", "apparmor:unconfined", "apparmor=unconfined"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + msg, blocked := checkPrivilegedGate(false, &hostConfig{SecurityOpt: []string{tc.opt}}) + if !blocked { + t.Fatalf("blocked = false for %q, want true", tc.opt) + } + if !strings.Contains(msg, tc.want) { + t.Errorf("msg = %q, want it to name %q", msg, tc.want) + } + if !strings.Contains(msg, "dind.allow_privileged") { + t.Errorf("msg = %q, want it to name the governing config knob", msg) + } + }) + } +} + +func TestCheckPrivilegedGate_ClosedRejectsSecurityOptAmongOthers(t *testing.T) { + // The elevating value is not the first element: the gate has to scan the + // whole slice, not just SecurityOpt[0]. + msg, blocked := checkPrivilegedGate(false, &hostConfig{ + SecurityOpt: []string{"label=disable", "no-new-privileges=true", "seccomp=unconfined"}, + }) + if !blocked { + t.Fatal("blocked = false, want true") + } + if !strings.Contains(msg, "seccomp=unconfined") { + t.Errorf("msg = %q, want it to name the offending option", msg) + } +} + +func TestCheckPrivilegedGate_ClosedAllowsUnrelatedSecurityOpt(t *testing.T) { + // Only the values this shim turns into a sandbox-loosening OCI option are + // elevation. Everything else is ignored by the applier, so refusing it + // here would be a refusal with no security value — and would break jobs + // that pass a harmless --security-opt. + for _, opt := range []string{ + "label=disable", + "label:user:someuser", + "no-new-privileges=true", + "no-new-privileges", + "seccomp=/path/to/profile.json", + "apparmor=docker-default", + "apparmor:docker-default", + "seccomp=unconfined-ish", + "unconfined", + "", + } { + t.Run(opt, func(t *testing.T) { + msg, blocked := checkPrivilegedGate(false, &hostConfig{SecurityOpt: []string{opt}}) + if blocked { + t.Errorf("--security-opt %q blocked: msg=%q", opt, msg) + } + }) + } +} + +func TestCheckPrivilegedGate_OpenAllowsSecurityOpt(t *testing.T) { + // dind.allow_privileged = true has to keep working: it is what lets + // setup-buildx's container driver run. + msg, blocked := checkPrivilegedGate(true, &hostConfig{ + SecurityOpt: []string{"seccomp=unconfined", "apparmor=unconfined"}, + }) + if blocked { + t.Errorf("blocked = true with gate open; msg=%q", msg) + } +} + +func TestHandleContainerCreate_SecurityOptPassesGateWhenOpen(t *testing.T) { + // With the gate open the request must reach the runtime path. The test + // server has no containerd client, so "got past the gate" shows up as the + // 500 from the nil-client check — the point is that it is not a 403. + setPlatformGOOS(t, "linux") // otherwise the Windows sibling gate answers 501 first + s := gateTestServer(true) + for _, opt := range []string{"seccomp=unconfined", "seccomp:unconfined", "apparmor=unconfined", "apparmor:unconfined"} { + t.Run(opt, func(t *testing.T) { + w := postCreate(t, s, createRequest{ + Image: "alpine:3.20", + HostConfig: &hostConfig{SecurityOpt: []string{opt}}, + }) + if w.Code == http.StatusForbidden { + t.Fatalf("--security-opt %s refused with the gate open: %s", opt, w.Body.String()) + } + if w.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 (nil containerd client); body=%s", w.Code, w.Body.String()) + } + }) + } +} + +func TestHandleContainerCreate_MalformedRequestStillGets400(t *testing.T) { + // Request-shape validation runs before the gate, so a broken request gets + // the accurate 400 rather than a misleading 403 about host policy. + s := gateTestServer(false) + + t.Run("undecodable body", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/containers/create", strings.NewReader("{not json")) + w := httptest.NewRecorder() + s.handleContainerCreate(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", w.Code, w.Body.String()) + } + }) + + t.Run("missing image", func(t *testing.T) { + w := postCreate(t, s, createRequest{ + HostConfig: &hostConfig{SecurityOpt: []string{"seccomp=unconfined"}}, + }) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", w.Code, w.Body.String()) + } + }) +} + +func TestSecurityOptSpecOpts_AppliesElevationWhenAsked(t *testing.T) { + // The other half of "preserve legitimate behaviour": with the gate open the + // options must still reach the OCI spec, because that is what setup-buildx's + // container driver relies on. Both opts are pure spec edits, so they can be + // applied with a nil client and container. + for _, opt := range []string{"seccomp=unconfined", "seccomp:unconfined"} { + t.Run(opt, func(t *testing.T) { + spec := &oci.Spec{Linux: &ocispec.Linux{Seccomp: &ocispec.LinuxSeccomp{DefaultAction: ocispec.ActErrno}}} + specOpts := securityOptSpecOpts([]string{opt}) + if len(specOpts) != 1 { + t.Fatalf("securityOptSpecOpts(%q) produced %d opts, want 1", opt, len(specOpts)) + } + applySpecOpts(t, spec, specOpts) + if spec.Linux.Seccomp != nil { + t.Errorf("seccomp profile still set after %s", opt) + } + }) + } + for _, opt := range []string{"apparmor=unconfined", "apparmor:unconfined"} { + t.Run(opt, func(t *testing.T) { + spec := &oci.Spec{Process: &ocispec.Process{ApparmorProfile: "docker-default"}} + specOpts := securityOptSpecOpts([]string{opt}) + if len(specOpts) != 1 { + t.Fatalf("securityOptSpecOpts(%q) produced %d opts, want 1", opt, len(specOpts)) + } + applySpecOpts(t, spec, specOpts) + if spec.Process.ApparmorProfile != "" { + t.Errorf("apparmor profile = %q after %s, want cleared", spec.Process.ApparmorProfile, opt) + } + }) + } + t.Run("unrelated opts are no-ops", func(t *testing.T) { + spec := &oci.Spec{ + Process: &ocispec.Process{ApparmorProfile: "docker-default"}, + Linux: &ocispec.Linux{Seccomp: &ocispec.LinuxSeccomp{DefaultAction: ocispec.ActErrno}}, + } + applySpecOpts(t, spec, securityOptSpecOpts([]string{"label=disable", "no-new-privileges=true", "apparmor=docker-default"})) + if spec.Linux.Seccomp == nil { + t.Error("seccomp profile cleared by a non-elevating --security-opt") + } + if spec.Process.ApparmorProfile != "docker-default" { + t.Errorf("apparmor profile = %q, want it untouched", spec.Process.ApparmorProfile) + } + }) +} + +func applySpecOpts(t *testing.T, spec *oci.Spec, opts []oci.SpecOpts) { + t.Helper() + for _, o := range opts { + if err := o(context.Background(), nil, nil, spec); err != nil { + t.Fatalf("applying spec opt: %v", err) + } + } +} + +func TestElevatingSecurityOpt_NamesTheMechanism(t *testing.T) { + // securityOptSpecOpts switches on the returned mechanism and the 403 + // message quotes it, so both spellings have to normalise to the same word. + cases := []struct { + opt string + wantMech string + wantEleva bool + }{ + {"seccomp=unconfined", "seccomp", true}, + {"seccomp:unconfined", "seccomp", true}, + {"apparmor=unconfined", "apparmor", true}, + {"apparmor:unconfined", "apparmor", true}, + {"apparmor=docker-default", "", false}, + {"seccomp=./profile.json", "", false}, + {"label=disable", "", false}, + {"no-new-privileges=true", "", false}, + {"", "", false}, + } + for _, tc := range cases { + t.Run(tc.opt, func(t *testing.T) { + mechanism, elevating := elevatingSecurityOpt(tc.opt) + if elevating != tc.wantEleva { + t.Fatalf("elevating = %v, want %v", elevating, tc.wantEleva) + } + if mechanism != tc.wantMech { + t.Errorf("mechanism = %q, want %q", mechanism, tc.wantMech) + } + }) + } +}