fix: bound the JWKS refresh rate and reject an unknown symbol policy (ADR 0066)

.gitignore

@@ -17,11 +17,13 @@ bin/
 # Local run logs from e2e / manual agent runs
 test/e2e/logs/
 
-# Code coverage profiles and other test artifacts
+# Code coverage profiles and other test artifacts. The coverage patterns are
+# anchored to the root, where the tooling writes them: unanchored, they also
+# match a source file legitimately named coverage.go, and swallow it in silence.
 *.out
-coverage.*
+/coverage.*
 *.coverprofile
-profile.cov
+/profile.cov
 
 # Dependency directories (remove the comment below to include it)
 # vendor/

docs/adr/0010-node-to-controller-inventory-channel.md

@@ -4,7 +4,7 @@ Date: 2026-08-10
 
 Status: Accepted
 Amends: 0008, 0009
-Amended by: 0011, 0039, 0040
+Amended by: 0011, 0039, 0040, 0066
 
 Realizes the promise in 0008 §4 point 4; extends 0009, whose node role was
 log-only. The one-way boundary of 0001 and the identity model of 0005/0008 are

docs/adr/0066-an-unverified-caller-does-not-set-the-refresh-rate.md

@@ -0,0 +1,85 @@
+# 0066. An unverified caller does not set the refresh rate, and a value outside the enumeration does not load
+
+Date: 2026-09-03
+
+Status: Accepted
+
+Amends: 0010
+
+Two places where input the agent does not trust decided something quietly. A
+JWKS refresh is now rate-limited, because the key ID that triggers it comes out
+of a token nobody has verified yet; and a `thirdPartySymbols` outside its two
+values is a startup failure rather than a silent fall back to `drop`.
+
+## Context
+
+**The refresh.** `CachingKeySource` refetches the cluster JWKS whenever it is
+asked for a key ID it does not hold — the signal that the cluster rotated its
+signing keys (ADR 0010). Rotation is rare, so the refetch looked rare.
+
+It is not, because of where in the request the key ID is read. `jwt.ParseWithClaims`
+calls the key function *before* it checks the signature, and the handler calls
+`Verify` *before* it reads the body. So a caller that can reach the intake port
+supplies the `kid` at no cost to itself, and each distinct value it invents cost
+two GETs to the API server — discovery and JWKS — serialized behind this type's
+one mutex with a ten-second ceiling each. The refresh rate was a property of the
+caller.
+
+Reaching the port is not free: the chart's NetworkPolicy admits only the node
+DaemonSet, and the cluster's CNI has to enforce it (ADR 0039). That bounds who
+can do this; it does not make the controller's traffic to its own API server
+someone else's variable, and a policy that is not enforced is exactly the case
+worth surviving.
+
+**The value.** `profiling.thirdPartySymbols` is "drop" or "keep". It was
+compared to `"keep"` and everything else took the other branch, so `"Keep"`,
+`"true"` and a typo all meant `drop`. Failing closed is the right direction —
+but this field governs the allow-list that decides which frames may leave the
+node (ADR 0011 §4), and it is the one control whose effect an operator cannot
+check by reading their own values file. `UnmarshalStrict` already rejects a
+field that does not exist; nothing rejected a value that does not exist.
+
+## Decision
+
+A refresh happens at most once per 30 seconds. A miss inside that window
+returns an error without touching the network, and the stamp is taken before
+the attempt, so a refresh that failed is rate-limited on the same floor — an
+unreachable API server is where retrying per request costs the most and helps
+least.
+
+Thirty seconds against the node's one-minute scan interval: a rotation is picked
+up at most one round late, and a report that did not authenticate is re-sent by
+the next pass rather than lost (ADR 0003). The cost is paid in the rare case,
+the bound holds in the cheap one.
+
+`Config` and `NodeConfig` gained a `Validate`, called from the loader, which
+rejects a `thirdPartySymbols` outside the enumeration. Empty stays valid: it
+selects the default, and it is what a default install renders.
+
+Alternatives considered:
+
+- **Negative-caching the unknown key IDs** instead of a time floor. It bounds
+  repeats of one ID and nothing else — an attacker sends a new ID each time, and
+  the cache is then a memory cost with no bound of its own.
+- **Moving the fetch out from under the mutex** (single-flight). It is the fix
+  for a *slow* refresh stalling verification, which is a real second problem;
+  it does nothing about how often the fetch happens, which is this one.
+- **Rate-limiting the endpoint** rather than the refresh. It would have to be
+  set high enough for a large fleet's legitimate traffic, and the expensive
+  request is indistinguishable from the cheap one at that layer.
+- **Validating `thirdPartySymbols` at use** rather than at load, taking an
+  unknown value as `drop` and logging it. A log line is not read on a config
+  the customer set months ago; the point is that the file and the behavior
+  cannot disagree.
+
+## Consequences
+
+The controller's own query rate to its API server is bounded by the controller.
+A key rotation is picked up up to thirty seconds later than before, which costs
+at most one scan round of one loss-harmless payload.
+
+An install whose `thirdPartySymbols` is misspelled now fails to start where it
+used to run with a stricter policy than the file asked for. That is a behavior
+change for a configuration that was already not doing what it said, and it is
+the same shape as ADR 0040's rule for an unset `expectedSubject`: a control that
+cannot be honored stops the agent rather than silently becoming something else.

docs/adr/README.md

@@ -123,3 +123,4 @@ line to paste.
 - [0063. A profile the collector dominates is still a profile](0063-a-profile-of-the-collector-is-still-a-profile.md)
 - [0064. The node is described, and its comings and goings recorded](0064-the-node-is-described-and-its-comings-recorded.md)
 - [0065. The vocabulary of a collected fact is a package, and it does not know Kubernetes](0065-the-vocabulary-is-a-package.md)
+- [0066. An unverified caller does not set the refresh rate, and a value outside the enumeration does not load](0066-an-unverified-caller-does-not-set-the-refresh-rate.md)

internal/config/config.go

@@ -228,7 +228,14 @@ func LoadNode(path string) (NodeConfig, error) {
 	return load[NodeConfig](path)
 }
 
-func load[T any](path string) (T, error) {
+// validated is the half of a schema that the type system does not carry.
+// UnmarshalStrict rejects a field that does not exist; this rejects a value that
+// does not exist for a field that does.
+type validated interface {
+	Validate() error
+}
+
+func load[T validated](path string) (T, error) {
 	var cfg T
 	if path == "" {
 		return cfg, nil
@@ -240,5 +247,33 @@ func load[T any](path string) (T, error) {
 	if err := yaml.UnmarshalStrict(raw, &cfg); err != nil {
 		return cfg, fmt.Errorf("parsing config %s: %w", path, err)
 	}
+	if err := cfg.Validate(); err != nil {
+		var zero T
+		return zero, fmt.Errorf("invalid config %s: %w", path, err)
+	}
 	return cfg, nil
 }
+
+// Validate checks the controller's enumerated values.
+func (c Config) Validate() error {
+	return validThirdPartySymbols(c.Profiling.ThirdPartySymbols)
+}
+
+// Validate checks the node's enumerated values.
+func (c NodeConfig) Validate() error {
+	return validThirdPartySymbols(c.Profiling.ThirdPartySymbols)
+}
+
+// validThirdPartySymbols accepts the two policies and the empty value that
+// selects the default (see the Normalized methods). An unrecognized value is a
+// startup failure rather than the silent "drop" it used to mean: this field
+// governs which frames may leave the node (ADR 0011 §4, ADR 0066).
+func validThirdPartySymbols(s string) error {
+	switch s {
+	case "", ThirdPartySymbolsDrop, ThirdPartySymbolsKeep:
+		return nil
+	default:
+		return fmt.Errorf("profiling.thirdPartySymbols is %q; it must be %q or %q",
+			s, ThirdPartySymbolsDrop, ThirdPartySymbolsKeep)
+	}
+}

internal/config/config_test.go

@@ -3,6 +3,7 @@ package config
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -210,3 +211,44 @@ func TestNodeProfilingNormalizedDefaults(t *testing.T) {
 		t.Errorf("set CaptureDurationSeconds overwritten: %d", got.CaptureDurationSeconds)
 	}
 }
+
+// A value outside the enumeration used to parse cleanly and then mean "drop",
+// because nothing compared it to anything (ADR 0066). It governs which frames
+// may leave the node, so it fails the start instead.
+func TestAnUnknownThirdPartySymbolsValueIsAStartupFailure(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		body string
+		load func(string) error
+	}{
+		{"controller", "profiling:\n  thirdPartySymbols: Keep\n",
+			func(p string) error { _, err := Load(p); return err }},
+		{"node", "profiling:\n  thirdPartySymbols: yes\n",
+			func(p string) error { _, err := LoadNode(p); return err }},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.load(write(t, tc.body))
+			if err == nil {
+				t.Fatal("an unrecognized policy was accepted; it would have silently meant drop")
+			}
+			if !strings.Contains(err.Error(), "thirdPartySymbols") {
+				t.Errorf("error = %v, want it to name the field the operator has to fix", err)
+			}
+		})
+	}
+}
+
+// The two policies and the empty value that selects the default all load. The
+// empty case is the one a default install ships (charts/runtime-agent renders
+// the key only when set), so a rule that rejected it would break every install.
+func TestTheThirdPartySymbolsEnumerationLoads(t *testing.T) {
+	for _, value := range []string{"", ThirdPartySymbolsDrop, ThirdPartySymbolsKeep} {
+		body := "profiling:\n  thirdPartySymbols: " + `"` + value + `"` + "\n"
+		if _, err := Load(write(t, body)); err != nil {
+			t.Errorf("controller rejected %q: %v", value, err)
+		}
+		if _, err := LoadNode(write(t, body)); err != nil {
+			t.Errorf("node rejected %q: %v", value, err)
+		}
+	}
+}

internal/nodeauth/jwks.go

@@ -241,26 +241,42 @@ func (s StaticKeys) Key(_ context.Context, kid string) (crypto.PublicKey, error)
 // milliseconds; this is a ceiling on a hang, not a budget.
 const defaultFetchTimeout = 10 * time.Second
 
+// defaultRefetchInterval is how often an unknown kid may cost a refresh
+// (ADR 0066).
+//
+// The node's scan interval is a minute and a lost report is re-sent by the next
+// pass (ADR 0003), so a rotation picked up this late costs at most one round of
+// one payload — against an unauthenticated caller otherwise choosing how often
+// the controller talks to the API server.
+const defaultRefetchInterval = 30 * time.Second
+
 // CachingKeySource is a KeyProvider that caches the fetched KeySet and refetches
-// once when asked for a kid it does not hold — the signal that the cluster
-// rotated its signing keys. Concurrent verifications share one refresh.
+// when asked for a kid it does not hold — the signal that the cluster rotated
+// its signing keys. Concurrent verifications share one refresh, and refreshes
+// are rate-limited: see Key.
 type CachingKeySource struct {
 	Source KeySource
 	// FetchTimeout bounds a single refresh; zero selects defaultFetchTimeout.
 	FetchTimeout time.Duration
+	// RefetchInterval is the floor between refreshes; zero selects
+	// defaultRefetchInterval.
+	RefetchInterval time.Duration
 
 	mu     sync.Mutex
 	cached *KeySet
+	// lastFetch is when a refresh was last attempted, successfully or not.
+	lastFetch time.Time
+	// now is the clock, a field so a test can hold time still.
+	now func() time.Time
 }
 
-// Key returns the key for kid, fetching the set on first use and refetching once
-// if the cached set lacks it. Still absent after that is an error: the token was
-// signed by a key the cluster does not advertise.
+// Key returns the key for kid, fetching the set on first use and refetching when
+// the cached set lacks it and the refresh floor has passed. Absent after that is
+// an error: the token was signed by a key the cluster does not advertise.
 //
-// The refresh runs under the mutex, which serializes it — a key rotation makes
-// every verification miss the cache at once, and one attempt at a time keeps
-// that off a control plane that is likely rolling. The cost is that a slow
-// refresh stalls verification, which is why the fetch carries its own deadline.
+// A miss is rate-limited because a miss is not a trusted event — the kid is read
+// before the signature is checked, so an unverified caller picks it (ADR 0066).
+// The refresh is serialized by the mutex and carries its own deadline.
 func (c *CachingKeySource) Key(ctx context.Context, kid string) (crypto.PublicKey, error) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
@@ -271,6 +287,19 @@ func (c *CachingKeySource) Key(ctx context.Context, kid string) (crypto.PublicKe
 		}
 	}
 
+	interval := c.RefetchInterval
+	if interval <= 0 {
+		interval = defaultRefetchInterval
+	}
+	at := c.clock()
+	if !c.lastFetch.IsZero() && at.Sub(c.lastFetch) < interval {
+		return nil, fmt.Errorf("nodeauth: no signing key for kid %q, and the key set was refreshed less than %s ago", kid, interval)
+	}
+	// Stamped before the attempt, so a failing refresh is rate-limited too: an
+	// API server that is down is the case where retrying per request costs the
+	// most and helps least.
+	c.lastFetch = at
+
 	timeout := c.FetchTimeout
 	if timeout <= 0 {
 		timeout = defaultFetchTimeout
@@ -289,3 +318,11 @@ func (c *CachingKeySource) Key(ctx context.Context, kid string) (crypto.PublicKe
 	c.cached = set
 	return set.Key(kid)
 }
+
+// clock is the refresh floor's time source, defaulting to the wall clock.
+func (c *CachingKeySource) clock() time.Time {
+	if c.now == nil {
+		return time.Now()
+	}
+	return c.now()
+}