From 7a0757430429440dd7efe351b60f4ffb99612c67 Mon Sep 17 00:00:00 2001 From: Dave Sargent Date: Sun, 17 May 2026 09:25:59 -0700 Subject: [PATCH] feat(rotation): Phase 5 drift detection in agent.Client (#72) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the dashboard signs an outbound request with kid X but the agent matches kid Y instead (because rotation propagated on one side but not the other), drift detection logs the disagreement so an operator can resync. Closes the observability loop on the install→use→remove cycle: Phase 2 added the request header, the agent's auth middleware already echoes the matched kid, and this commit wires the dashboard-side comparison. What's here ----------- `agent.HeaderKID = "X-Gearbox-Kid"` is the shared constant the dashboard sends and the agent echoes back. The agent's middleware sets it on every authenticated response (see Phase 1). `agent.Client` - `SetDriftHandler(DriftHandler)` installs an optional callback invoked when `resp.Header.Get(HeaderKID)` differs from the kid the client was built with. Reads c.onDrift at RoundTrip time, so the handler can be installed AFTER construction (typical for long- lived per-box clients held by the dashboard). - Transport wrap: `kidObservingTransport` sits between the http client and the underlying TLS transport, calling `c.checkDrift` on every successful response. One central point of inspection — no invasive edits to every `doRequest*` method. - `LogDriftHandler(logger, boxID)` builds a ready-to-use DriftHandler that emits a structured warn-level log. Lowest-friction wiring for the long-lived clients held in the WebSocketManager. Test fix -------- `TestClientTimeout` was a flaky pre-existing test whose substring check was case-sensitive — Go's net/http error message capitalises "Timeout" sometimes and emits "context deadline exceeded" other times. Fixed by lower-casing the error message before substring matching. Verified stable across 5 runs. Tests ----- `client_drift_test.go` exercises the four corners of the matrix: - Drift handler fires when kid mismatches. - Doesn't fire when kid matches. - Doesn't fire when the agent omits the header (older agents). - Doesn't fire when the dashboard client has no kid. Not yet wired into production code ---------------------------------- Adding `agent.LogDriftHandler` is the small API surface; deciding *where* to call SetDriftHandler is a separate design choice (WebSocketManager? capability poller? every short-lived handler.agentClient()?). Deferring that integration so this PR stays focused on the observability primitive itself. A follow-up can install LogDriftHandler at every site that constructs a kid- bearing client. Refs: Phase 5 of the implementation plan posted to #72. Co-Authored-By: Claude Opus 4.7 (1M context) --- gearbox/internal/framework/agent/client.go | 85 ++++++++++++++-- .../framework/agent/client_drift_test.go | 98 +++++++++++++++++++ .../internal/framework/agent/client_test.go | 6 +- 3 files changed, 182 insertions(+), 7 deletions(-) create mode 100644 gearbox/internal/framework/agent/client_drift_test.go diff --git a/gearbox/internal/framework/agent/client.go b/gearbox/internal/framework/agent/client.go index beb81c0..30d7bec 100644 --- a/gearbox/internal/framework/agent/client.go +++ b/gearbox/internal/framework/agent/client.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "log" + "log/slog" "net" "net/http" "net/url" @@ -33,6 +34,7 @@ type Client struct { baseURL string apiKey string kid string // optional; when set, sent as X-Gearbox-Kid header + onDrift DriftHandler httpClient *http.Client } @@ -40,10 +42,35 @@ type Client struct { // entry id. The agent's middleware echoes the matched kid on every // authenticated response (see middleware.ResponseHeaderKID); the // dashboard sends this kid on outbound requests so the agent + audit -// log can correlate keys. Phase 5 (drift detection) compares the +// log can correlate keys. Drift detection (Phase 5) compares the // request-time kid with the echoed response kid. const HeaderKID = "X-Gearbox-Kid" +// DriftHandler is invoked when the agent's echoed kid differs from +// the kid the client sent. Implementations typically log + surface a +// "rotation drift" banner; the default is to do nothing (set via +// SetDriftHandler). +// +// expected = what the client sent on the request +// actual = what the agent echoed back on the response +// +// The handler is called on the request goroutine; cheap operations +// only (logging is fine, network calls are not). +type DriftHandler func(expected, actual string) + +// LogDriftHandler returns a DriftHandler that emits a structured +// warn-level log line — the lowest-friction wiring for a long-lived +// agent.Client. The "box_id" tag lets the operator filter the journal +// to a single box. +func LogDriftHandler(logger *slog.Logger, boxID int64) DriftHandler { + return func(expected, actual string) { + logger.Warn("rotation drift: agent matched a different kid than expected", + "box_id", boxID, + "expected_kid", expected, + "actual_kid", actual) + } +} + // NewClient creates a new HAProxy Agent API client. func NewClient(baseURL, apiKey string) *Client { return NewClientWithTimeout(baseURL, apiKey, DefaultTimeout) @@ -74,6 +101,29 @@ func (c *Client) WithKID(kid string) *Client { // paths. func (c *Client) KID() string { return c.kid } +// SetDriftHandler installs a callback invoked when the agent's +// echoed X-Gearbox-Kid response header differs from the kid the +// client sent. Nil disables drift detection (the default). +// +// Use this on long-lived Client instances cached per box; the rotator +// builds short-lived clients per call and wouldn't benefit from +// installing a handler. +func (c *Client) SetDriftHandler(h DriftHandler) { c.onDrift = h } + +// checkDrift inspects resp's X-Gearbox-Kid header against the kid the +// client sent. Called from each doRequest* path on success. No-op when +// the client has no kid or no handler is installed. +func (c *Client) checkDrift(resp *http.Response) { + if resp == nil || c.kid == "" || c.onDrift == nil { + return + } + actual := resp.Header.Get(HeaderKID) + if actual == "" || actual == c.kid { + return + } + c.onDrift(c.kid, actual) +} + // setAuthHeaders applies the standard auth + Accept headers and, when // the client was built with a kid, the X-Gearbox-Kid request header. // Callers must call this BEFORE setting Content-Type so their override @@ -135,14 +185,37 @@ func NewClientWithTimeout(baseURL, apiKey string, timeout time.Duration) *Client }, } - return &Client{ + c := &Client{ baseURL: baseURL, apiKey: apiKey, - httpClient: &http.Client{ - Timeout: timeout, - Transport: transport, - }, } + c.httpClient = &http.Client{ + Timeout: timeout, + // Wrap the base transport so every response is inspected for the + // X-Gearbox-Kid header against c.kid. The transport reads c.kid + // and c.onDrift at RoundTrip time, so SetDriftHandler is reflected + // immediately without rebuilding the client. + Transport: &kidObservingTransport{base: transport, c: c}, + } + return c +} + +// kidObservingTransport wraps an http.RoundTripper and invokes the +// owning Client's checkDrift on every successful response. Implements +// the drift-detection half of Phase 5 (issue #72) — the dashboard +// learns immediately when an agent's matched kid disagrees with the +// kid the dashboard sent, signalling a partial rotation. +type kidObservingTransport struct { + base http.RoundTripper + c *Client +} + +func (t *kidObservingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err == nil { + t.c.checkDrift(resp) + } + return resp, err } // BuildTLSConfig is the exported alias for createTLSConfig — used by diff --git a/gearbox/internal/framework/agent/client_drift_test.go b/gearbox/internal/framework/agent/client_drift_test.go new file mode 100644 index 0000000..3ab5a86 --- /dev/null +++ b/gearbox/internal/framework/agent/client_drift_test.go @@ -0,0 +1,98 @@ +package agent + +import ( + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" +) + +func TestDriftHandler_FiresOnKIDMismatch(t *testing.T) { + // Mock agent that always echoes a hard-coded kid in the response. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(HeaderKID, "actual-kid") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer srv.Close() + + c := NewClientWithKID(srv.URL, "ignored", "expected-kid") + var fired atomic.Bool + var seenExpected, seenActual string + c.SetDriftHandler(func(expected, actual string) { + fired.Store(true) + seenExpected, seenActual = expected, actual + }) + + if _, err := c.doRequest("GET", "/anything", url.Values{}); err != nil { + t.Fatalf("doRequest: %v", err) + } + if !fired.Load() { + t.Errorf("drift handler not invoked") + } + if seenExpected != "expected-kid" || seenActual != "actual-kid" { + t.Errorf("drift handler args: expected=%q actual=%q", seenExpected, seenActual) + } +} + +func TestDriftHandler_DoesNotFireOnMatch(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(HeaderKID, "matched") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer srv.Close() + + c := NewClientWithKID(srv.URL, "ignored", "matched") + var fired atomic.Bool + c.SetDriftHandler(func(_, _ string) { fired.Store(true) }) + + if _, err := c.doRequest("GET", "/anything", url.Values{}); err != nil { + t.Fatalf("doRequest: %v", err) + } + if fired.Load() { + t.Errorf("drift handler incorrectly invoked on matching kid") + } +} + +func TestDriftHandler_DoesNotFireWhenAgentDoesNotEchoKID(t *testing.T) { + // Older agents won't set X-Gearbox-Kid. The drift handler should + // stay silent rather than flagging every response as "drift". + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer srv.Close() + + c := NewClientWithKID(srv.URL, "ignored", "expected-kid") + var fired atomic.Bool + c.SetDriftHandler(func(_, _ string) { fired.Store(true) }) + + if _, err := c.doRequest("GET", "/anything", url.Values{}); err != nil { + t.Fatalf("doRequest: %v", err) + } + if fired.Load() { + t.Errorf("drift handler incorrectly invoked when agent sent no kid header") + } +} + +func TestDriftHandler_DoesNotFireWhenClientHasNoKID(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(HeaderKID, "something") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + })) + defer srv.Close() + + c := NewClient(srv.URL, "ignored") // no kid + var fired atomic.Bool + c.SetDriftHandler(func(_, _ string) { fired.Store(true) }) + + if _, err := c.doRequest("GET", "/anything", url.Values{}); err != nil { + t.Fatalf("doRequest: %v", err) + } + if fired.Load() { + t.Errorf("drift handler invoked when client has no kid; should no-op") + } +} diff --git a/gearbox/internal/framework/agent/client_test.go b/gearbox/internal/framework/agent/client_test.go index e58a408..35a3ef1 100644 --- a/gearbox/internal/framework/agent/client_test.go +++ b/gearbox/internal/framework/agent/client_test.go @@ -579,7 +579,11 @@ func TestClientTimeout(t *testing.T) { t.Fatal("expected timeout error") } - if !strings.Contains(err.Error(), "context deadline exceeded") && !strings.Contains(err.Error(), "timeout") { + // The error message format varies by Go version + timing: sometimes + // "context deadline exceeded", sometimes "Client.Timeout exceeded + // while awaiting headers" (capital T). Match either by lower-casing. + msg := strings.ToLower(err.Error()) + if !strings.Contains(msg, "context deadline exceeded") && !strings.Contains(msg, "timeout") { t.Errorf("expected timeout error, got %v", err) } }