From c7fd5418308434d010cbcad7324c21c0afb80c0b Mon Sep 17 00:00:00 2001 From: Konstantins Feofantovs Date: Thu, 3 Sep 2026 23:54:40 +0300 Subject: [PATCH] fix: bound the JWKS refresh rate and reject an unknown symbol policy (ADR 0066) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two places where input the agent does not trust decided something quietly. The JWKS refetch. jwt.ParseWithClaims asks for the key before it checks the signature, and the handler verifies before it reads the body — so the kid comes from an unverified token and an unauthenticated caller chose how often the controller queried its own API server, two GETs at a time, serialized behind one mutex with a 10s ceiling each. Refreshes now happen at most once per 30s, and the stamp is taken before the attempt so a failing refresh is bounded too. The cost is a rotation picked up up to one node scan round late, on a payload that is re-sent by the next pass. The symbol policy. profiling.thirdPartySymbols was compared to "keep" and everything else took the other branch, so "Keep" and every typo silently meant "drop". It governs which frames may leave the node, so an unrecognized value now fails the start, the same shape as ADR 0040's rule for an unset expectedSubject. Also carries the .gitignore fix that PR #103 was meant to land and did not: an unanchored `coverage.*` still matches a source file in any directory. An existing test needed the clock advanced to keep proving its own claim: with the floor in place, "the refresh failed" and "no refresh was attempted" look alike from outside, so it now counts attempts on the source. go test -race ./... green; golangci-lint 0 issues; chart-lint and vulncheck clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011YRKuL97KRndaopfpixrhp --- .gitignore | 8 +- ...10-node-to-controller-inventory-channel.md | 2 +- ...ed-caller-does-not-set-the-refresh-rate.md | 85 +++++++++++++++++ docs/adr/README.md | 1 + internal/config/config.go | 37 +++++++- internal/config/config_test.go | 42 ++++++++ internal/nodeauth/jwks.go | 55 +++++++++-- internal/nodeauth/jwks_test.go | 95 ++++++++++++++++++- 8 files changed, 307 insertions(+), 18 deletions(-) create mode 100644 docs/adr/0066-an-unverified-caller-does-not-set-the-refresh-rate.md diff --git a/.gitignore b/.gitignore index 2d8b40b..590e1f9 100644 --- a/.gitignore +++ b/.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/ diff --git a/docs/adr/0010-node-to-controller-inventory-channel.md b/docs/adr/0010-node-to-controller-inventory-channel.md index 32864d4..e87388f 100644 --- a/docs/adr/0010-node-to-controller-inventory-channel.md +++ b/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 diff --git a/docs/adr/0066-an-unverified-caller-does-not-set-the-refresh-rate.md b/docs/adr/0066-an-unverified-caller-does-not-set-the-refresh-rate.md new file mode 100644 index 0000000..2429932 --- /dev/null +++ b/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. diff --git a/docs/adr/README.md b/docs/adr/README.md index 73903c5..acb0821 100644 --- a/docs/adr/README.md +++ b/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) diff --git a/internal/config/config.go b/internal/config/config.go index a93ef25..a90d872 100644 --- a/internal/config/config.go +++ b/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) + } +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 2c6b95c..5cbd459 100644 --- a/internal/config/config_test.go +++ b/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) + } + } +} diff --git a/internal/nodeauth/jwks.go b/internal/nodeauth/jwks.go index 930631b..46d90e6 100644 --- a/internal/nodeauth/jwks.go +++ b/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() +} diff --git a/internal/nodeauth/jwks_test.go b/internal/nodeauth/jwks_test.go index d933fb8..dd35fc9 100644 --- a/internal/nodeauth/jwks_test.go +++ b/internal/nodeauth/jwks_test.go @@ -111,7 +111,11 @@ func TestCachingKeySourceCachesThenRefreshesOnUnknownKid(t *testing.T) { t.Fatal(err) } srv := newJWKSTestServer(t, rsaJWKS("rsa-1", &key1.PublicKey)) - c := &CachingKeySource{Source: &HTTPKeySource{IssuerBaseURL: srv.URL, Client: srv.Client()}} + at := time.Unix(1_700_000_000, 0) + c := &CachingKeySource{ + Source: &HTTPKeySource{IssuerBaseURL: srv.URL, Client: srv.Client()}, + now: func() time.Time { return at }, + } ctx := context.Background() if _, err := c.Key(ctx, "rsa-1"); err != nil { @@ -124,14 +128,16 @@ func TestCachingKeySourceCachesThenRefreshesOnUnknownKid(t *testing.T) { t.Fatalf("JWKS fetched %d times for a cached kid, want 1", got) } - // Rotate the cluster's signing key: a new kid appears. The next lookup for - // the unknown kid must trigger exactly one refetch and then resolve. + // Rotate the cluster's signing key: a new kid appears. Once the refresh + // floor has passed, the lookup for the unknown kid must trigger exactly one + // refetch and then resolve. key2, err := rsa.GenerateKey(rand.Reader, 2048) if err != nil { t.Fatal(err) } rotated := rsaJWKS("rsa-2", &key2.PublicKey) srv.body.Store(&rotated) + at = at.Add(defaultRefetchInterval) if _, err := c.Key(ctx, "rsa-2"); err != nil { t.Fatalf("lookup after rotation: %v", err) @@ -214,7 +220,19 @@ func TestAFailedRefreshKeepsTheKeysAlreadyInHand(t *testing.T) { t.Fatal(err) } srv := newJWKSTestServer(t, rsaJWKS("rsa-1", &key.PublicKey)) - c := &CachingKeySource{Source: &HTTPKeySource{IssuerBaseURL: srv.URL, Client: srv.Client()}} + at := time.Unix(1_700_000_000, 0) + // Attempts are counted on the source itself: after the change of ADR 0066, + // "the refresh failed" and "no refresh was made" both leave the server's own + // hit count where it was, and only the first is what this test is about. + var attempts atomic.Int64 + source := &HTTPKeySource{IssuerBaseURL: srv.URL, Client: srv.Client()} + c := &CachingKeySource{ + Source: keySourceFunc(func(ctx context.Context) (*KeySet, error) { + attempts.Add(1) + return source.Fetch(ctx) + }), + now: func() time.Time { return at }, + } ctx := context.Background() if _, err := c.Key(ctx, "rsa-1"); err != nil { @@ -222,13 +240,82 @@ func TestAFailedRefreshKeepsTheKeysAlreadyInHand(t *testing.T) { } // The source goes away, and a lookup for an unknown kid fails against it. + // The clock moves past the refresh floor first, so this is a refresh that + // was attempted and failed rather than one the floor declined to make — + // which is the case this test is about. srv.Close() + at = at.Add(defaultRefetchInterval) if _, err := c.Key(ctx, "rsa-2"); err == nil { t.Fatal("a refresh against a dead source should error") } + if got := attempts.Load(); got != 2 { + t.Fatalf("the source was asked %d times, want 2: the failing refresh must have been attempted", got) + } // The key that was already held still verifies. if _, err := c.Key(ctx, "rsa-1"); err != nil { t.Errorf("the cached key was lost when a refresh failed: %v", err) } } + +// The defense of ADR 0066: the kid comes from an unverified token, so a caller +// that can reach the port picks it. Without the floor, each invented kid is two +// GETs to the API server. +func TestUnknownKidsWithinTheFloorCostNoFetch(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + srv := newJWKSTestServer(t, rsaJWKS("rsa-1", &key.PublicKey)) + at := time.Unix(1_700_000_000, 0) + c := &CachingKeySource{ + Source: &HTTPKeySource{IssuerBaseURL: srv.URL, Client: srv.Client()}, + now: func() time.Time { return at }, + } + + for i := range 50 { + if _, err := c.Key(context.Background(), fmt.Sprintf("forged-%d", i)); err == nil { + t.Fatal("a kid the cluster does not advertise should never resolve") + } + } + if got := srv.jwksHits.Load(); got != 1 { + t.Fatalf("JWKS fetched %d times for 50 invented kids, want 1", got) + } + + // The floor is a delay, not a latch: the next rotation is still picked up. + at = at.Add(defaultRefetchInterval) + if _, err := c.Key(context.Background(), "forged-51"); err == nil { + t.Fatal("a kid the cluster does not advertise should never resolve") + } + if got := srv.jwksHits.Load(); got != 2 { + t.Fatalf("JWKS fetched %d times, want 2 (one more once the floor passed)", got) + } +} + +// A refresh that failed is rate-limited on the same floor. An API server that is +// down is where retrying once per request costs the most and helps least. +func TestAFailedRefreshIsRateLimitedToo(t *testing.T) { + var attempts atomic.Int64 + at := time.Unix(1_700_000_000, 0) + c := &CachingKeySource{ + Source: keySourceFunc(func(context.Context) (*KeySet, error) { + attempts.Add(1) + return nil, errors.New("api server unreachable") + }), + now: func() time.Time { return at }, + } + + for range 20 { + if _, err := c.Key(context.Background(), "rsa-1"); err == nil { + t.Fatal("a lookup with no key set should error") + } + } + if got := attempts.Load(); got != 1 { + t.Fatalf("fetch attempted %d times behind an unreachable server, want 1", got) + } +} + +// keySourceFunc adapts a function to KeySource. +type keySourceFunc func(context.Context) (*KeySet, error) + +func (f keySourceFunc) Fetch(ctx context.Context) (*KeySet, error) { return f(ctx) }