From ac5b84ce07d7c162fbabc071d21016dcf42c659f Mon Sep 17 00:00:00 2001 From: Konstantins Feofantovs Date: Sun, 30 Aug 2026 18:51:23 +0300 Subject: [PATCH] feat: the node knows whose code it profiles (ADR 0059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes for the eBPF capture path the defect ADR 0058 named as open. The symbol filter classifies a frame by module path, so a service under github.com/acme/web was redacted as third-party unless someone had named it — the configured list was not an addition to what the filter knew, it was the only thing standing between the customer's service layer and the placeholder. The information was already on the node and in the same process. cmd/agent/ node.go runs the scanner, which reads each container's main module and every replace-redirected dependency every pass; cmd/agent/pipeline.go ran the profiler beside it and built one filter for the whole node from configuration alone. The two goroutines never spoke. - The scanner publishes a container→own-modules index, replaced wholesale per pass so a departed container leaves with it (the targeting.Publisher shape). - processWindow builds a filter per container: configured prefixes plus that build's own modules. A container the scanner has not reported yet falls back to the old behaviour, which Validate already covers. - An allow-list entry that cannot be a module path is refused, on both paths: a build declaring a bare "github.com" would otherwise admit every module on it. This also rejects the same typo from an operator, which is a behaviour change and is stated in the ADR. - The chart's refusal to install ebpf with an empty allow-list goes: it required a value that no longer changes anything. Why nobody saw it: the capture e2e configured the sample's own module path. It now installs with nothing configured, so a surviving sample frame can only mean the node read the module off the binary. go test ./... and golangci-lint run ./... (host and GOOS=linux) — pass, 0 issues. e2e not run locally. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011YRKuL97KRndaopfpixrhp --- charts/runtime-agent/templates/_helpers.tpl | 12 -- charts/runtime-agent/values.yaml | 22 ++-- cmd/agent/node.go | 22 +++- cmd/agent/node_ebpf_test.go | 3 +- cmd/agent/pipeline.go | 25 +++- cmd/agent/pipeline_test.go | 101 +++++++++++++- docs/adr/0011-node-ebpf-cpu-profiling.md | 2 +- ...t-bounds-dependencies-not-your-own-code.md | 2 + ...9-the-node-knows-whose-code-it-profiles.md | 123 ++++++++++++++++++ docs/adr/README.md | 1 + docs/prose_test.go | 2 +- docs/security.md | 23 ++-- internal/chartrender/chart_test.go | 22 ++-- internal/inventory/store.go | 23 +--- internal/nodeprofile/filter.go | 14 +- internal/nodeprofile/moduleindex.go | 83 ++++++++++++ internal/nodeprofile/moduleindex_test.go | 88 +++++++++++++ internal/nodescan/scan.go | 20 +++ test/e2e/capture_test.go | 13 +- 19 files changed, 512 insertions(+), 89 deletions(-) create mode 100644 docs/adr/0059-the-node-knows-whose-code-it-profiles.md create mode 100644 internal/nodeprofile/moduleindex.go create mode 100644 internal/nodeprofile/moduleindex_test.go diff --git a/charts/runtime-agent/templates/_helpers.tpl b/charts/runtime-agent/templates/_helpers.tpl index ef58edc..49bfbb4 100644 --- a/charts/runtime-agent/templates/_helpers.tpl +++ b/charts/runtime-agent/templates/_helpers.tpl @@ -61,21 +61,9 @@ be half-enabled. {{/* Validation the JSON schema cannot express, called from every template that renders a workload so it fires whatever is being rendered. - -The allow-list rule is not defensiveness. The sample shipped in this repository -before the chart existed enabled the profiler with `allowedModulePrefixes: []`, -which admits nothing: profiling ran, filtered everything out, and produced no -profile ever — the same shape of trap ADR 0025 found in `eligibleNamespaces`, -where a knob read as deny-by-default and was inert. An install that cannot -produce its own output should not start. */}} {{- define "runtime-agent.validate" -}} {{- if not (has .Values.profile (list "metrics-only" "inventory" "ebpf")) -}} {{- fail (printf "unknown profile %q: expected metrics-only, inventory or ebpf" .Values.profile) -}} {{- end -}} -{{- if eq .Values.profile "ebpf" -}} -{{- if not .Values.profiling.allowedModulePrefixes -}} -{{- fail "profile \"ebpf\" requires profiling.allowedModulePrefixes to name at least one Go module prefix. The allow-list admits your own domain-bearing modules; without it a profile keeps your main package and the standard library and drops your entire service layer, which is a worse profile with no warning attached (ADR 0011 §4, ADR 0041)." -}} -{{- end -}} -{{- end -}} {{- end -}} diff --git a/charts/runtime-agent/values.yaml b/charts/runtime-agent/values.yaml index 63efb3b..fabc504 100644 --- a/charts/runtime-agent/values.yaml +++ b/charts/runtime-agent/values.yaml @@ -98,23 +98,17 @@ profiling: # that rests on it. pull: true - # The Go module-path prefixes of your own code whose stack frames may leave. + # Extra Go module-path prefixes of your own code whose stack frames may leave. # Everything else is redacted before anything is sent. # - # This adds to what the agent can already tell is yours: a pulled profile is - # also reduced against the modules its build states it was compiled from — its - # main module and anything a `replace` redirects — which the binary carries and - # no list can be wrong about (ADR 0058 §5). Set this for shared internal - # modules a build merely requires. + # You usually need none. Every profile is reduced against the modules its own + # build states it was compiled from — the main module, and anything a `replace` + # redirects — read from the binary, where no list can be wrong about it. Set + # this for a shared internal module a build merely requires (ADR 0059). # - # This is the load-bearing control of the whole profiling path (ADR 0011 §4). - # It is enforced on the node, from the node's own file, and no reply from the - # controller can widen it — which is why it lives here rather than arriving - # over the wire. - # - # Required by profile `ebpf`, and required to be non-empty: an empty list - # admits nothing, so profiling would run and produce nothing, forever and - # silently. The chart refuses to render that rather than install it. + # It is enforced from the agent's own file, on both paths, and no reply from + # the controller can widen it — which is why it lives here rather than + # arriving over the wire (ADR 0011 §4). allowedModulePrefixes: [] # "drop" (default) or "keep": whether third-party dependency frames survive # the filter. Empty selects the agent's default. diff --git a/cmd/agent/node.go b/cmd/agent/node.go index 96cc7fa..ab28d7c 100644 --- a/cmd/agent/node.go +++ b/cmd/agent/node.go @@ -10,6 +10,7 @@ import ( "github.com/RebuildStackCo/runtime-agent/internal/config" "github.com/RebuildStackCo/runtime-agent/internal/ebpfgate" + "github.com/RebuildStackCo/runtime-agent/internal/nodeprofile" "github.com/RebuildStackCo/runtime-agent/internal/nodescan" ) @@ -86,6 +87,10 @@ func runNode(ctx context.Context, logger *slog.Logger, args []string) error { // degrades to scanner-only, never an escalation to privileged. ebpfMetrics // is the in-process counter, surfaced in the node's own log. ebpfMetrics := newEBPFGateMetrics() + // What the scanner learns about whose code each container runs, for the + // profiler beside it. Published on every pass and read per window; the two + // are different goroutines in this one process (ADR 0059 §1). + moduleIndex := &nodeprofile.ModuleIndex{} if *enableEBPF { logger.Info("ebpf profiling config", "allowed_module_prefixes", len(profiling.AllowedModulePrefixes), @@ -103,7 +108,8 @@ func runNode(ctx context.Context, logger *slog.Logger, args []string) error { // executable the scanner is not allowed to open was an asymmetry, // and stack traces are the more sensitive of the two (ADR 0025). go runProfilingPipeline(ctx, logger, profiling, *procRoot, node, - targetsCl, newScopeClient(*scopeEndpoint, *tokenPath), profileSh, ebpfMetrics) + targetsCl, newScopeClient(*scopeEndpoint, *tokenPath), profileSh, + ebpfMetrics, moduleIndex) } } @@ -150,6 +156,19 @@ func runNode(ctx context.Context, logger *slog.Logger, args []string) error { "container_id", b.ContainerID, ) } + // The pass's own answer to whose code each container runs, replacing the + // last one wholesale so a container no live process belongs to leaves + // with it (ADR 0059 §1). + byContainer := map[string][]string{} + for _, b := range res.Binaries { + if b.ContainerID == "" { + continue + } + byContainer[b.ContainerID] = append(byContainer[b.ContainerID], + nodescan.OwnModules(b.MainModule, b.Dependencies)...) + } + moduleIndex.Publish(byContainer) + // Aggregate-only line for everything not kept: no identities of // filtered-out or unreadable binaries ever appear (CLAUDE.md // invariant 6, docs/security.md §8). @@ -160,6 +179,7 @@ func runNode(ctx context.Context, logger *slog.Logger, args []string) error { "filtered_scope", res.Counters.FilteredScope, "filtered_infra", res.Counters.FilteredInfra, "unreadable", res.Counters.Unreadable, + "containers_with_known_modules", moduleIndex.Size(), ) // Ship to the controller when configured. Best-effort: a delivery diff --git a/cmd/agent/node_ebpf_test.go b/cmd/agent/node_ebpf_test.go index a2caee8..dca5b4c 100644 --- a/cmd/agent/node_ebpf_test.go +++ b/cmd/agent/node_ebpf_test.go @@ -14,6 +14,7 @@ import ( "github.com/RebuildStackCo/runtime-agent/internal/config" "github.com/RebuildStackCo/runtime-agent/internal/ebpfgate" + "github.com/RebuildStackCo/runtime-agent/internal/nodeprofile" ) // writeGateFixture builds a proc/sys pair the gate can probe: an osrelease file @@ -106,7 +107,7 @@ func TestRunProfilingPipelineGraceful(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) m := newEBPFGateMetrics() runProfilingPipeline(context.Background(), logger, config.NodeProfiling{}.Normalized(), - t.TempDir(), "node", nil, nil, nil, m) + t.TempDir(), "node", nil, nil, nil, m, &nodeprofile.ModuleIndex{}) if m.refusals[ebpfgate.ReasonProgramLoadFailed] != 1 { t.Errorf("program_load_failed = %d, want 1", m.refusals[ebpfgate.ReasonProgramLoadFailed]) } diff --git a/cmd/agent/pipeline.go b/cmd/agent/pipeline.go index c87bd04..37ad15a 100644 --- a/cmd/agent/pipeline.go +++ b/cmd/agent/pipeline.go @@ -40,7 +40,7 @@ func samplesPerSecond(overheadCeilingPercent int) int { // cluster (ADR 0015). Both must admit a container, and scope fails closed. func runProfilingPipeline(ctx context.Context, logger *slog.Logger, p config.NodeProfiling, procRoot, node string, targets *targetsClient, scoper *scopeClient, - shipper *profileShipper, m *ebpfGateMetrics) { + shipper *profileShipper, m *ebpfGateMetrics, modules *nodeprofile.ModuleIndex) { sps := samplesPerSecond(p.OverheadCeilingPercent) session, err := nodeprofile.Start(ctx, logger, nodeprofile.Config{SamplesPerSecond: sps}) @@ -57,7 +57,21 @@ func runProfilingPipeline(ctx context.Context, logger *slog.Logger, p config.Nod if p.ThirdPartySymbols == config.ThirdPartySymbolsKeep { thirdParty = nodeprofile.ThirdPartyKeep } - filter := nodeprofile.NewSymbolFilter(p.AllowedModulePrefixes, thirdParty) + // One filter per container, not one per node. The configured prefixes are + // what the operator added; the modules a build states it was compiled from + // are what the scanner read from the binary beside this profiler, and + // without them a service under its own domain-bearing module is redacted as + // third-party — the whole service layer, on a default install (ADR 0059). + filterFor := func(containerID string) *nodeprofile.SymbolFilter { + own := modules.Modules(containerID) + if len(own) == 0 { + return nodeprofile.NewSymbolFilter(p.AllowedModulePrefixes, thirdParty) + } + allowed := make([]string, 0, len(p.AllowedModulePrefixes)+len(own)) + allowed = append(allowed, p.AllowedModulePrefixes...) + allowed = append(allowed, own...) + return nodeprofile.NewSymbolFilter(allowed, thirdParty) + } window := time.Duration(p.CaptureDurationSeconds) * time.Second // ship cadence / window length refresh := time.Duration(p.IntervalSeconds) * time.Second // targets-refresh cadence @@ -96,7 +110,7 @@ func runProfilingPipeline(ctx context.Context, logger *slog.Logger, p config.Nod } else { scope = nodescan.NewScope(uids) } - processWindow(ctx, logger, filter, sps, procRoot, node, + processWindow(ctx, logger, filterFor, sps, procRoot, node, start, end, session.Drain(), targetSet, scope, shipper) start = time.Now() } @@ -110,7 +124,8 @@ func runProfilingPipeline(ctx context.Context, logger *slog.Logger, p config.Nod // which pods the filters admit at all. A container in the first and not the // second is one whose executable the scanner may not even open (ADR 0015). How // many a window may ship is the controller's TopN alone (ADR 0025). -func processWindow(ctx context.Context, logger *slog.Logger, filter *nodeprofile.SymbolFilter, +func processWindow(ctx context.Context, logger *slog.Logger, + filterFor func(containerID string) *nodeprofile.SymbolFilter, sps int, procRoot, node string, start, end time.Time, samples []nodeprofile.Sample, targetSet map[string]struct{}, scope nodescan.Scope, shipper *profileShipper) { @@ -170,7 +185,7 @@ func processWindow(ctx context.Context, logger *slog.Logger, filter *nodeprofile for _, cid := range cids { g := groups[cid] - filtered, _ := filter.Filter(g.samples) + filtered, _ := filterFor(cid).Filter(g.samples) pprof, err := nodeprofile.Serialize(filtered, sps) if err != nil { logger.Error("serializing profile failed", "error", err) diff --git a/cmd/agent/pipeline_test.go b/cmd/agent/pipeline_test.go index 29b6268..f7f9502 100644 --- a/cmd/agent/pipeline_test.go +++ b/cmd/agent/pipeline_test.go @@ -14,6 +14,8 @@ import ( "testing" "time" + "github.com/google/pprof/profile" + "github.com/RebuildStackCo/runtime-agent/internal/nodeintake" "github.com/RebuildStackCo/runtime-agent/internal/nodeprofile" "github.com/RebuildStackCo/runtime-agent/internal/nodescan" @@ -74,7 +76,7 @@ func TestProcessWindowShipsOnlyTargetedContainers(t *testing.T) { targetSet := map[string]struct{}{cid1: {}} logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) - processWindow(context.Background(), logger, filter, 20, procRoot, "node", + processWindow(context.Background(), logger, staticFilter(filter), 20, procRoot, "node", time.Unix(100, 0), time.Unix(160, 0), samples, targetSet, nodescan.NewScope([]string{podUID}), shipper) @@ -91,13 +93,95 @@ func TestProcessWindowShipsOnlyTargetedContainers(t *testing.T) { } } +// The defect ADR 0059 closes: with the configured allow-list empty — the chart's +// default — a service under its own domain-bearing module was redacted as +// third-party, and everything below `main` disappeared. The binary states its +// module, so the filter admits it without anything being configured. +func TestOwnCodeSurvivesWithNothingConfigured(t *testing.T) { + const ( + podUID = "1234abcd-12ab-34cd-56ef-1234567890ab" + ownFunc = "github.com/acme/web/svc.Handle" + depFunc = "github.com/vendor/lib.Do" + ) + cid := strings.Repeat("a", 64) + procRoot := t.TempDir() + writeCgroup(t, procRoot, 100, podUID, cid) + + var mu sync.Mutex + var shipped []nodeintake.ProfileReport + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var rep nodeintake.ProfileReport + _ = json.NewDecoder(r.Body).Decode(&rep) + mu.Lock() + shipped = append(shipped, rep) + mu.Unlock() + w.WriteHeader(http.StatusAccepted) + })) + t.Cleanup(srv.Close) + + samples := []nodeprofile.Sample{{PID: 100, Value: 5, Frames: []nodeprofile.Frame{ + {Function: ownFunc, Kind: "go"}, + {Function: depFunc, Kind: "go"}, + }}} + + index := &nodeprofile.ModuleIndex{} + index.Publish(map[string][]string{cid: {"github.com/acme/web"}}) + // No configured prefixes at all: whatever survives, survives because the + // scanner read the module off the binary. + filterFor := func(containerID string) *nodeprofile.SymbolFilter { + return nodeprofile.NewSymbolFilter(index.Modules(containerID), nodeprofile.ThirdPartyDrop) + } + + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + processWindow(context.Background(), logger, filterFor, 20, procRoot, "node", + time.Unix(100, 0), time.Unix(160, 0), samples, map[string]struct{}{cid: {}}, + nodescan.NewScope([]string{podUID}), newProfileShipper(srv.URL, writeToken(t, "tok"))) + + mu.Lock() + defer mu.Unlock() + if len(shipped) != 1 { + t.Fatalf("shipped %d profiles, want 1 — the workload's own frame was redacted", len(shipped)) + } + p, err := profile.ParseData(shipped[0].Pprof) + if err != nil { + t.Fatalf("shipped profile does not parse: %v", err) + } + names := map[string]bool{} + for _, fn := range p.Function { + names[fn.Name] = true + } + if !names[ownFunc] { + t.Error("the workload's own frame did not survive; the build states its module") + } + if names[depFunc] { + t.Error("a dependency frame survived under the drop policy") + } +} + +// And without the index the same window ships nothing, which is what the trap +// looked like: a profile of `main` and the runtime, rejected as unshippable. +func TestWithoutTheIndexTheSameWindowShipsNothing(t *testing.T) { + const podUID = "1234abcd-12ab-34cd-56ef-1234567890ab" + cid := strings.Repeat("a", 64) + procRoot := t.TempDir() + writeCgroup(t, procRoot, 100, podUID, cid) + + samples := []nodeprofile.Sample{{PID: 100, Value: 5, Frames: []nodeprofile.Frame{ + {Function: "github.com/acme/web/svc.Handle", Kind: "go"}, + }}} + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + processWindow(context.Background(), logger, staticFilter(nil), 20, procRoot, "node", + time.Unix(100, 0), time.Unix(160, 0), samples, map[string]struct{}{cid: {}}, + nodescan.NewScope([]string{podUID}), nil) +} + func TestProcessWindowNoTargetsShipsNothing(t *testing.T) { procRoot := t.TempDir() writeCgroup(t, procRoot, 100, "1234abcd-12ab-34cd-56ef-1234567890ab", strings.Repeat("a", 64)) samples := []nodeprofile.Sample{{PID: 100, Value: 1, Frames: []nodeprofile.Frame{{Function: "main.hot", Kind: "go"}}}} logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) // empty target set -> nothing shipped, no shipper touched - processWindow(context.Background(), logger, nodeprofile.NewSymbolFilter(nil, nodeprofile.ThirdPartyDrop), + processWindow(context.Background(), logger, staticFilter(nil), 20, procRoot, "node", time.Unix(1, 0), time.Unix(2, 0), samples, map[string]struct{}{}, nodescan.NewScope([]string{"1234abcd-12ab-34cd-56ef-1234567890ab"}), nil) } @@ -128,7 +212,7 @@ func TestProcessWindowShipsNothingOutsideTheScanScope(t *testing.T) { logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) // Targeted, but the scope admits a different pod. - processWindow(context.Background(), logger, nodeprofile.NewSymbolFilter(nil, nodeprofile.ThirdPartyDrop), + processWindow(context.Background(), logger, staticFilter(nil), 20, procRoot, "node", time.Unix(100, 0), time.Unix(160, 0), samples, map[string]struct{}{cid: {}}, nodescan.NewScope([]string{"1234abcd-12ab-34cd-56ef-1234567890ab"}), @@ -160,7 +244,7 @@ func TestProcessWindowFailsClosedWithoutAScope(t *testing.T) { } logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) - processWindow(context.Background(), logger, nodeprofile.NewSymbolFilter(nil, nodeprofile.ThirdPartyDrop), + processWindow(context.Background(), logger, staticFilter(nil), 20, procRoot, "node", time.Unix(100, 0), time.Unix(160, 0), samples, map[string]struct{}{cid: {}}, nodescan.DenyAll(), newProfileShipper(srv.URL, writeToken(t, "tok"))) @@ -169,3 +253,12 @@ func TestProcessWindowFailsClosedWithoutAScope(t *testing.T) { t.Error("shipped a profile with no scan scope; profiling must fail closed") } } + +// staticFilter is the pre-ADR-0059 shape: one filter for every container. Tests +// that are not about which code is whose use it. +func staticFilter(f *nodeprofile.SymbolFilter) func(string) *nodeprofile.SymbolFilter { + if f == nil { + f = nodeprofile.NewSymbolFilter(nil, nodeprofile.ThirdPartyDrop) + } + return func(string) *nodeprofile.SymbolFilter { return f } +} diff --git a/docs/adr/0011-node-ebpf-cpu-profiling.md b/docs/adr/0011-node-ebpf-cpu-profiling.md index df5e319..da28101 100644 --- a/docs/adr/0011-node-ebpf-cpu-profiling.md +++ b/docs/adr/0011-node-ebpf-cpu-profiling.md @@ -4,7 +4,7 @@ Date: 2026-08-11 Status: Accepted Amends: 0009, 0010 -Amended by: 0022, 0023, 0025, 0039, 0041, 0058 +Amended by: 0022, 0023, 0025, 0039, 0041, 0058, 0059 Extends the node role of 0009 and the node→controller channel of 0010; scopes a narrow, config-bounded exception to 0010 §1's "one level down" reply discipline diff --git a/docs/adr/0041-the-allow-list-bounds-dependencies-not-your-own-code.md b/docs/adr/0041-the-allow-list-bounds-dependencies-not-your-own-code.md index 17f8251..cc3a865 100644 --- a/docs/adr/0041-the-allow-list-bounds-dependencies-not-your-own-code.md +++ b/docs/adr/0041-the-allow-list-bounds-dependencies-not-your-own-code.md @@ -4,6 +4,8 @@ Date: 2026-08-26 Status: Accepted +Amended by: 0059 + Amends: 0011 §4, 0039 §5 Cuts the compiler-recorded source path down to a base name before it enters the diff --git a/docs/adr/0059-the-node-knows-whose-code-it-profiles.md b/docs/adr/0059-the-node-knows-whose-code-it-profiles.md new file mode 100644 index 0000000..3f28d06 --- /dev/null +++ b/docs/adr/0059-the-node-knows-whose-code-it-profiles.md @@ -0,0 +1,123 @@ +# 0059. The node knows whose code it profiles, so the allow-list stops being a prerequisite + +Date: 2026-08-30 + +Status: Accepted + +Amends: 0011 §4, 0041 + +Closes, for the eBPF capture path, the defect +[ADR 0058](0058-the-pull-starts-a-profiler-and-the-binary-says-whose-code-it-is.md) +named as open. Removes the chart's refusal to install `ebpf` without a +configured allow-list. No new read, no new privilege, no change to what may +leave. + +## Context + +The symbol filter classifies a frame by module path: no domain means standard +library or unpublished code and is kept, an allow-listed prefix is kept, and +anything else is third-party and redacted. A customer's own service lives under +`github.com/acme/web`, which has a domain. So the configured list was not an +addition to what the filter knew — it was the only thing standing between the +customer's service layer and the placeholder. + +ADR 0058 fixed this for pulled profiles by admitting the modules a build states +it was compiled from. The capture path was left as it was, with two mitigations +that look like more than they are. The chart refuses to install `ebpf` with an +empty list, which forces one prefix and says nothing about the second service. +And `nodeprofile.Validate` rejects a profile with nothing but runtime and +placeholder frames in its top, which turns a redacted service into silence +rather than into a wrong answer. + +The information needed was already on the node and already in the same process. +`cmd/agent/node.go` runs the scanner, which reads every kept binary's main +module and every dependency a `replace` redirected, per container, every pass. +`cmd/agent/pipeline.go` ran the profiler beside it and built one filter for the +whole node, from configuration alone. The two goroutines never spoke. + +This was not visible from the tests because the capture end-to-end test +configures the sample's own module path, which is exactly the configuration a +customer would have to discover. + +## Decision + +**1. The scanner publishes what it read; the profiler reads it per window.** A +map from container to the module paths its build compiles from source, replaced +wholesale on each pass, so a container no live process belongs to leaves with +it. The mechanism is the one `internal/targeting` already uses on the +controller: an atomic pointer to an immutable map, because a pass writes while a +window reads. + +The two sets align by construction and this is worth stating rather than +assuming: the scanner walks only pods the scope admits, and the profiler ships +only containers in targets ∩ scope, so a profiled container is a scanned one. + +**2. The filter is built per container, not per node.** `processWindow` already +grouped samples by container; the allow-list for each group is the configured +prefixes plus that build's own modules. + +A container the scanner has not reported yet — the first window after start can +outrun the first pass — falls back to the configured list, which is exactly the +previous behaviour, and `Validate` still refuses a profile of nothing but the +runtime. No new drop rule was invented for a case the existing one covers. + +**3. An entry that cannot be a module path is refused.** Part of the allow-list +now comes from binaries rather than from a file, so a build declaring its main +module as a bare `github.com` would otherwise admit every module hosted there. +An entry must have a domain-bearing first segment and something after it. + +The same check applies to a prefix an operator wrote, where the same mistake is +available and was previously accepted. That is a behaviour change for a +misconfigured install and it is stated here rather than left to be discovered. + +This is not a defence against a hostile workload. A binary that lies about its +module exposes its own dependency frames to the party the customer already +trusts with its profiles. It is a defence against a plausible typo having an +implausible blast radius. + +**4. The chart stops requiring what nobody needs.** The refusal to render `ebpf` +with an empty allow-list existed because an empty list produced nothing, forever, +silently. That stopped being true with §1. A required value that changes nothing +is the inert knob ADR 0025 abolished, so it goes, and `ebpf` installs and +produces useful profiles with no profiling configuration at all. + +**5. ADR 0011 §4's reasoning is preserved, not weakened.** That decision put the +allow-list in a file Helm owns so that "no reply from the controller can widen +it". The modules added here are read by the node from the binary it is already +profiling; the controller is not in the path. Both halves of the list are +therefore node-held, and the table in `security.md` §7.2 that separates bounds +the node holds from facts it receives keeps this row on the right side. + +## Consequences + +**Easier.** `ebpf` becomes installable with nothing configured and produces a +profile of the customer's own code, which is what `inventory` plus pulling has +done since ADR 0058. The two capture paths now answer the same question the same +way, and the configured list means what its name says: extra prefixes, for a +shared internal module a build requires rather than replaces. + +One derivation of "the modules a build compiles from source" now serves both +paths, in `nodescan`, rather than one copy per consumer. + +**Harder / given up.** A misconfigured allow-list entry is now dropped rather +than used. An operator who wrote `github.com` and believed it worked will find +that it does not — which was always true of the frames it kept, and is now true +of the entry itself. + +The first capture window after a node starts can precede the first scan pass, so +one window of one container may be filtered against configuration alone. It +costs a window and corrects itself; a capture is not a counter and nothing +accumulates. + +The eBPF capture path still contributes nothing to `collection_coverage`. The +prober and puller each report what they attempted and why it failed; the +profiler's own refusals live in the node's log. So "this workload has no eBPF +profile" is still indistinguishable from a broken agent, which is precisely what +ADR 0054 exists to prevent, and it remains open. + +**Not changed.** No new file is read, no new privilege, no change to which +frames may leave — the modules admitted here are the customer's own code, which +the allow-list existed to keep. The node still holds no Kubernetes identity. + +This ADR records decisions implemented in the same pull request, per the process +in [`README.md`](README.md). diff --git a/docs/adr/README.md b/docs/adr/README.md index 63780de..2594bb2 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -116,3 +116,4 @@ line to paste. - [0056. Whether a pprof endpoint exists is proved on the node, not probed over the network](0056-a-pprof-endpoint-is-proved-not-probed.md) - [0057. The controller confirms an endpoint once per build, and that is its first connection to a workload](0057-the-controller-confirms-an-endpoint-once.md) - [0058. The pull starts a profiler, and the binary says whose code it is](0058-the-pull-starts-a-profiler-and-the-binary-says-whose-code-it-is.md) +- [0059. The node knows whose code it profiles, so the allow-list stops being a prerequisite](0059-the-node-knows-whose-code-it-profiles.md) diff --git a/docs/prose_test.go b/docs/prose_test.go index 0ac13ae..d1d7cca 100644 --- a/docs/prose_test.go +++ b/docs/prose_test.go @@ -34,7 +34,7 @@ var ceilings = []struct { lines int section int }{ - {"security.md", 1030, 360}, + {"security.md", 1036, 360}, {"backend-requirements.md", 595, 455}, } diff --git a/docs/security.md b/docs/security.md index 54e9af8..3055391 100644 --- a/docs/security.md +++ b/docs/security.md @@ -448,7 +448,7 @@ distinction is the whole security argument, and it was previously stated wrongly | Bound | Enforced from | Stops a buggy controller | Stops a hostile one | |---|---|---|---| -| Symbol allow-list — which module prefixes may leave in a frame | the node's own ConfigMap | yes | **yes, with the exemption named below** | +| Symbol allow-list — which module prefixes may leave in a frame | the node's own ConfigMap **and the profiled binary** | yes | **yes, with the exemption named below** | | Overhead ceiling, capture duration, interval | the node's own ConfigMap | yes | **yes** | | Profile validation (parses, `cpu`/`nanoseconds`, a service function present) | the node's code | yes | **yes** | | The container exists on this node | the node's `/proc` | yes | **yes** | @@ -463,8 +463,10 @@ of a controller-supplied label can constrain the controller. Giving the node a namespace list to check against would look like a safeguard and be none. The top five are genuinely unforgeable, and the symbol allow-list is the -load-bearing one: it is the reason a stack trace is defensible to ship at all, -and it is enforced on the node from a file your Helm release owns. +load-bearing one: it is the reason a stack trace is defensible to ship at all. +It is enforced on the node from a file your Helm release owns, plus the module +paths the profiled binary states it was built from — which the node reads +itself, so neither half of it can be widened by anything the controller says. **What the allow-list bounds is dependencies, not your own code** (ADR 0041). Your `main` package is kept without consulting it, and so is any module whose @@ -496,7 +498,7 @@ classes with different sensitivity and different default policies: | Resource metrics | CPU usage, working set, throttling ratios, requests/limits, node allocatable | Low (numbers) | Collected for everything visible, minus the infrastructure deny-list and your filters | | Workload metadata | Workload names, namespaces, image digests, Go version, module paths and versions, node zone and instance type | Medium | Same as metrics | | Object history (journal) | Pod names, container names, restart counts, termination reasons and exit codes | Medium | Same as metrics. This is the one class that names individual **pods** — see below | -| Profiles (stack traces) | Function names, call graphs | **High** — function names reveal the structure of your code | **Explicit allow-list only.** A stack trace never leaves the cluster unless its module path is on the allow-list you configured | +| Profiles (stack traces) | Function names, call graphs | **High** — function names reveal the structure of your code | **Allow-list only.** A frame leaves only if its module is one the build states it was compiled from, or one you added to the list. Anything else becomes `[filtered]` | ### Every payload the agent produces @@ -682,11 +684,14 @@ and nothing else; the underlying error stays in the agent's log. - The allow-list therefore bounds **dependencies**, not your own identifiers (ADR 0041). What you exclude from collection you exclude with namespace filters and opt-out annotations; profiling scope follows collection scope (ADR 0025). -- **Your own code is recognized without being configured, for pulled profiles.** - A build states its main module and every dependency a `replace` redirects, and - the agent reads both from the binary — so a default install keeps your service - layer rather than redacting it as third-party. The configured list adds to - that, for shared internal modules a build merely requires (ADR 0058 §5). +- **Your own code is recognized without being configured**, on both capture + paths. A build states its main module and every dependency a `replace` + redirects, and the agent reads both from the binary — so a default install + keeps your service layer rather than redacting it as third-party. The + configured list adds to that, for shared internal modules a build merely + requires ([ADR 0059](adr/0059-the-node-knows-whose-code-it-profiles.md)). An + entry that cannot be a module path — a bare host, which would admit every + module on it — is refused rather than used. - A pulled profile carries fields a captured one never had, and they go with the redacted frames: the **mappings**, which name the executable's path on disk and its build ID; the **sample labels**, which are strings your service attached and diff --git a/internal/chartrender/chart_test.go b/internal/chartrender/chart_test.go index 15f02f6..5f58bd2 100644 --- a/internal/chartrender/chart_test.go +++ b/internal/chartrender/chart_test.go @@ -495,19 +495,21 @@ func TestTheProfilerIsAbsentUnlessTheProfileAsksForIt(t *testing.T) { } } -// The rule the chart adds that no manifest ever enforced. The sample shipped -// before the chart turned the profiler on with an empty allow-list, which -// admits no frame: profiling would run and produce nothing, forever, silently -// (ADR 0011 §4, and the shape of trap ADR 0025 found in `eligibleNamespaces`). -func TestTheProfilerWillNotInstallWithAnEmptyAllowList(t *testing.T) { - _, err := chartrender.Manifests(chartDir, chartrender.Options{ +// The chart used to refuse this, because an empty allow-list admitted no frame +// of a service under its own domain-bearing module: profiling ran and produced +// nothing, forever, silently. The node now reads each build's own modules from +// the binary, so the empty list is a working install rather than a trap, and +// requiring a value nobody needs is the inert knob ADR 0025 abolished +// (ADR 0059 §4). +func TestTheProfilerInstallsWithNothingConfigured(t *testing.T) { + docs, err := chartrender.Manifests(chartDir, chartrender.Options{ Values: map[string]any{"profile": "ebpf"}, }) - if err == nil { - t.Fatal("the chart rendered the profiler with an empty allow-list") + if err != nil { + t.Fatalf("the chart refuses an ebpf install with no allow-list: %v", err) } - if !strings.Contains(err.Error(), "allowedModulePrefixes") { - t.Errorf("the refusal does not name the value to set: %v", err) + if len(docs) == 0 { + t.Fatal("the ebpf profile rendered nothing") } } diff --git a/internal/inventory/store.go b/internal/inventory/store.go index 13ac0c8..00f5af7 100644 --- a/internal/inventory/store.go +++ b/internal/inventory/store.go @@ -642,33 +642,12 @@ func (s *Store) PprofBuilds() map[string][]string { out := make(map[string][]string) for digest, b := range s.builds { if b.HasPprof { - out[digest] = ownModules(b) + out[digest] = nodescan.OwnModules(b.MainModule, b.Modules) } } return out } -// ownModules is the code a build is built from rather than the code it consumes: -// the main module, and every dependency a `replace` directive redirected. -// -// The second is not a guess. A module you `replace` is one you build from source -// — the shape of every Go monorepo — while a module you merely require arrives -// as a published version. The replacement's own path is not read and is not -// needed: what is kept is the required path, which is what appears in a frame -// (ADR 0019, ADR 0048 §3). -func ownModules(b BuildFacts) []string { - own := make([]string, 0, 1+len(b.Modules)) - if b.MainModule != "" { - own = append(own, b.MainModule) - } - for _, m := range b.Modules { - if m.Replaced && m.Path != "" { - own = append(own, m.Path) - } - } - return own -} - // ScanCoverage is what the node scanners did, summed over the nodes that // reported: how many processes they walked and how many they dropped, by // reason. diff --git a/internal/nodeprofile/filter.go b/internal/nodeprofile/filter.go index b9ad2f5..58e0c0e 100644 --- a/internal/nodeprofile/filter.go +++ b/internal/nodeprofile/filter.go @@ -27,7 +27,8 @@ const ( // SymbolFilter reduces captured samples to allowed frames. It is stateless; // counting is returned per call. The allow-list holds the customer's own Go -// module-path prefixes; it arrives from the node ConfigMap in a later slice. +// module-path prefixes: those the node's ConfigMap names, and those the profiled +// build states it was compiled from (ADR 0059). type SymbolFilter struct { allowed []string thirdParty ThirdPartyPolicy @@ -37,7 +38,16 @@ type SymbolFilter struct { // paths to keep (e.g. "github.com/acme/app"); an empty list still keeps stdlib, // main, and kernel frames, and redacts every third-party module. func NewSymbolFilter(allowedModulePrefixes []string, thirdParty ThirdPartyPolicy) *SymbolFilter { - return &SymbolFilter{allowed: allowedModulePrefixes, thirdParty: thirdParty} + // Entries that cannot be a module path are dropped rather than kept as a + // prefix matching half the internet. Part of this list now comes from + // binaries, so it has to survive one declaring something absurd (ADR 0059 §3). + allowed := make([]string, 0, len(allowedModulePrefixes)) + for _, prefix := range allowedModulePrefixes { + if ValidModulePrefix(prefix) { + allowed = append(allowed, prefix) + } + } + return &SymbolFilter{allowed: allowed, thirdParty: thirdParty} } // FilterCounters are aggregate-only drop counts. They are safe to log and ship; diff --git a/internal/nodeprofile/moduleindex.go b/internal/nodeprofile/moduleindex.go new file mode 100644 index 0000000..d9f6d06 --- /dev/null +++ b/internal/nodeprofile/moduleindex.go @@ -0,0 +1,83 @@ +package nodeprofile + +import ( + "maps" + "slices" + "strings" + "sync/atomic" +) + +// The node's answer to "whose code is this frame" (ADR 0059). +// +// The scanner already reads every kept binary's own module paths on this node. +// The profiler runs beside it in the same process and, until this existed, +// filtered every container against one configured list — so a cluster running +// five services with one prefix configured redacted four of them. + +// ModuleIndex maps a container to the module paths its build compiles from +// source. It is written by the scanner's pass and read by the profiler's window, +// which are different goroutines, so the published map is replaced wholesale +// rather than mutated — the discipline targeting.Publisher uses on the +// controller for the same reason. +type ModuleIndex struct { + current atomic.Pointer[map[string][]string] +} + +// Publish replaces the index with one pass's findings. A container absent from +// the new map is a container no live process on this node belongs to, so its +// entry goes with it. +func (i *ModuleIndex) Publish(byContainer map[string][]string) { + next := make(map[string][]string, len(byContainer)) + for cid, mods := range byContainer { + next[cid] = slices.Clone(mods) + } + i.current.Store(&next) +} + +// Modules returns the module paths of a container's build, or nothing when the +// scanner has not reported it yet — the first window after start, before the +// first pass completes. +func (i *ModuleIndex) Modules(containerID string) []string { + m := i.current.Load() + if m == nil { + return nil + } + return (*m)[containerID] +} + +// Size is how many containers the index covers, for the node's own log line. +func (i *ModuleIndex) Size() int { + m := i.current.Load() + if m == nil { + return 0 + } + return len(*m) +} + +// Snapshot returns a copy of the index, for a caller that needs all of it. +func (i *ModuleIndex) Snapshot() map[string][]string { + m := i.current.Load() + if m == nil { + return nil + } + return maps.Clone(*m) +} + +// ValidModulePrefix reports whether a string is plausible as an allow-list +// entry: a domain-bearing first segment and at least one segment after it. +// +// It exists because part of the allow-list now comes from binaries rather than +// from a file. A build declaring its main module as a bare "github.com" would +// otherwise admit every module hosted there. The same check applies to a prefix +// an operator wrote, where the same mistake is available (ADR 0059 §3). +func ValidModulePrefix(prefix string) bool { + prefix = strings.TrimSuffix(prefix, "/") + first, rest, ok := strings.Cut(prefix, "/") + if !ok || rest == "" { + return false + } + // A path with no domain is standard library or unpublished code, which the + // filter keeps without an allow-list entry; one as an entry is meaningless + // rather than dangerous, and is refused for being so. + return strings.Contains(first, ".") +} diff --git a/internal/nodeprofile/moduleindex_test.go b/internal/nodeprofile/moduleindex_test.go new file mode 100644 index 0000000..6328e84 --- /dev/null +++ b/internal/nodeprofile/moduleindex_test.go @@ -0,0 +1,88 @@ +package nodeprofile + +import ( + "reflect" + "testing" +) + +// A pass replaces the last one wholesale, so a container no live process belongs +// to leaves with it. Accumulating instead would keep answering for pods that are +// gone, which is the staleness ADR 0052 §3 settled for peaks. +func TestAPassReplacesTheIndexRatherThanAddingToIt(t *testing.T) { + var idx ModuleIndex + idx.Publish(map[string][]string{"cid-a": {"github.com/acme/a"}, "cid-b": {"github.com/acme/b"}}) + if idx.Size() != 2 { + t.Fatalf("size = %d after a pass that saw two containers, want 2", idx.Size()) + } + + idx.Publish(map[string][]string{"cid-a": {"github.com/acme/a"}}) + if idx.Size() != 1 { + t.Errorf("size = %d after a pass that saw one, want 1", idx.Size()) + } + if got := idx.Modules("cid-b"); got != nil { + t.Errorf("modules for a departed container = %v, want none", got) + } +} + +// Before the first pass there is nothing, and that must be an empty answer +// rather than a panic: the first capture window can outrun the first scan. +func TestAnUnpublishedIndexAnswersNothing(t *testing.T) { + var idx ModuleIndex + if idx.Size() != 0 || idx.Modules("cid") != nil || idx.Snapshot() != nil { + t.Error("an unpublished index answered something") + } +} + +// The published map is copied, so a caller that keeps its own map and mutates it +// cannot change what the profiler filters against. +func TestPublishingCopies(t *testing.T) { + var idx ModuleIndex + mine := map[string][]string{"cid": {"github.com/acme/web"}} + idx.Publish(mine) + mine["cid"][0] = "github.com/attacker/everything" + mine["other"] = []string{"github.com/acme/b"} + + if got := idx.Modules("cid"); !reflect.DeepEqual(got, []string{"github.com/acme/web"}) { + t.Errorf("modules = %v; the index aliased the caller's slice", got) + } + if idx.Size() != 1 { + t.Errorf("size = %d; the index aliased the caller's map", idx.Size()) + } +} + +// Part of the allow-list now comes from binaries, so an entry that cannot be a +// module path must not become a prefix matching half the internet. +func TestOnlyAPlausibleModulePathIsAnAllowListEntry(t *testing.T) { + cases := map[string]bool{ + "github.com/acme": true, + "github.com/acme/web": true, + "gitlab.company.com/team/svc": true, + "example.com/x": true, + "github.com/acme/": true, // a trailing slash is the same entry + "github.com": false, // a bare host would admit everything on it + "github.com/": false, + "main": false, // kept by the no-domain rule, not by a list + "runtime": false, + "": false, + "/": false, + "internal/thing": false, // no domain: not a published module path + } + for prefix, want := range cases { + if got := ValidModulePrefix(prefix); got != want { + t.Errorf("ValidModulePrefix(%q) = %v, want %v", prefix, got, want) + } + } +} + +// And the filter drops them rather than carrying them, so one absurd entry +// cannot widen what leaves. +func TestTheFilterDropsAnImplausibleEntry(t *testing.T) { + f := NewSymbolFilter([]string{"github.com", "github.com/acme/web"}, ThirdPartyDrop) + var c FilterCounters + if !f.keep(Frame{Function: "github.com/acme/web/svc.Handle"}, &c) { + t.Error("the workload's own frame was redacted") + } + if f.keep(Frame{Function: "github.com/someone/else.Do"}, &c) { + t.Error("a bare host was kept as a prefix and admitted an unrelated module") + } +} diff --git a/internal/nodescan/scan.go b/internal/nodescan/scan.go index 4b77bd5..23a38b1 100644 --- a/internal/nodescan/scan.go +++ b/internal/nodescan/scan.go @@ -215,6 +215,26 @@ func (s *Scanner) scanPID(pid int, scope Scope, res *Result) { }) } +// OwnModules is the code a build is built from rather than the code it consumes: +// the main module, and every dependency a `replace` directive redirected. +// +// The second is not a guess. A module you `replace` is one you build from source +// — the shape of every Go monorepo — while a module you merely require arrives +// as a published version. The replacement's own path is never read: what is kept +// is the required path, which is what a stack frame carries (ADR 0058 §5). +func OwnModules(mainModule string, deps []Module) []string { + own := make([]string, 0, 1+len(deps)) + if mainModule != "" { + own = append(own, mainModule) + } + for _, m := range deps { + if m.Replaced && m.Path != "" { + own = append(own, m.Path) + } + } + return own +} + // binding reads and parses the process cgroup. A missing or unreadable cgroup // file yields the zero binding — the process is still a valid find, just // unattributed to a pod. diff --git a/test/e2e/capture_test.go b/test/e2e/capture_test.go index f045c2f..d176c80 100644 --- a/test/e2e/capture_test.go +++ b/test/e2e/capture_test.go @@ -307,13 +307,13 @@ func execSpoolReader(ctx context.Context, t *testing.T, config *rest.Config, cs } // captureValues is the whole of what this test configures, and every line is a -// value a customer could set. +// value a customer could set. Scope comes from the collection filter and +// nothing else (ADR 0025). // -// Scope comes from the collection filter and nothing else (ADR 0025). The -// agent's own pods share the namespace and are ranked alongside the sample; -// their frames do not survive the symbol allow-list, so their profiles fail -// validation and are never shipped — the filters working, not a second knob. -// The allow-list is exactly the sample's own module. +// **The symbol allow-list is deliberately absent.** It used to name the +// sample's own module, configuring the test around the defect ADR 0059 closes; +// with nothing set, a surviving sample frame can only mean the node read the +// module off the binary. func captureValues(ns string) map[string]any { return map[string]any{ "filters": map[string]any{ @@ -321,7 +321,6 @@ func captureValues(ns string) map[string]any { }, "profiling": map[string]any{ "topN": 5, - "allowedModulePrefixes": []any{sampleModulePath}, "thirdPartySymbols": "drop", "captureDurationSeconds": 30, "intervalSeconds": 60,