Skip to content

Commit 92bbd15

Browse files
committed
fix(admin): forward session cookie on KeyViz fan-out so peers do not 401
PR #686 shipped fan-out with the design assumption that peers would accept anonymous calls on a private network (#685 3). But the receiving side runs the same SessionAuth middleware as the browser-facing path, so anonymous fan-out calls are rejected with 401 missing-session-cookie -- the cluster heatmap collapsed to "1 of N nodes responded" in any production-like deploy. Fix: forward the inbound user's cookies on every peer call so the peer's SessionAuth middleware sees a valid principal. Cluster nodes already share --adminSessionSigningKey for HA, so a cookie minted on node A is verifiable on node B without any new infrastructure. Behaviour delta: - KeyVizFanout.Run gains a cookies []*http.Cookie parameter; nil preserves the legacy unauthenticated behaviour for tests / future non-browser callers. - The handler passes r.Cookies() so a SPA poll forwards admin_session and admin_csrf transparently. - New TestKeyVizFanoutRunForwardsCookies pins that both cookies reach the peer verbatim. Design doc 3 rewritten: the "anonymous on a private network" claim was wrong (the earlier draft assumed the peer side would also be anonymous, which is not how the admin path is built). Phase 2-C MVP now documents cookie-forwarding plus the operator-trust assumption that --keyvizFanoutNodes points at trusted hosts only. Phase 2-C+ will replace cookie forwarding with a dedicated inter-node token so browser-session expiry and inter-node call validity are decoupled. Tested against a 6-node cluster: with this fix the heatmap shows all 6 nodes (was 1 of 6 with anonymous calls).
1 parent edaaa64 commit 92bbd15

4 files changed

Lines changed: 113 additions & 30 deletions

File tree

docs/design/2026_04_27_proposed_keyviz_cluster_fanout.md

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -132,22 +132,35 @@ elastickv \
132132
network); the HTTP client uses `http://`. A follow-up will
133133
introduce `--keyvizFanoutTLS` once the rest of the admin path
134134
has TLS too.
135-
- **Auth (Phase 2-C MVP)**: the fan-out path is anonymous — the
136-
aggregator issues unauthenticated GETs to peers. This is only
137-
acceptable on a fully-private intra-cluster network, which is
138-
the contract `--keyvizFanoutNodes` documents. **Do NOT enable
139-
fan-out across an untrusted network until Phase 2-C+ ships
140-
proper auth.** Per-call timeout: 2 s default, override via
141-
`--keyvizFanoutTimeout`.
142-
- **Auth (Phase 2-C+)**: a follow-up extends the fan-out path with
143-
a short-lived signed token derived from the existing admin
144-
session-signing key (`ELASTICKV_ADMIN_SESSION_SIGNING_KEY`).
145-
Pre-shared inter-node token, NOT a replay of the browser's
146-
session cookie — re-using the cookie would couple browser session
147-
TTL to inter-node call validity, and a compromised browser
148-
session would gain peer-call authority. The two paths are
149-
intentionally distinct. The earlier draft of this section
150-
conflated them; this paragraph supersedes it.
135+
- **Auth (Phase 2-C MVP)**: the aggregator forwards the inbound
136+
user's `admin_session` cookie (and any other cookies the request
137+
carries) on every peer call. The peer's `SessionAuth` middleware
138+
verifies the cookie against its own `--adminSessionSigningKey`;
139+
cluster nodes already share that key for HA, so a cookie minted
140+
on node A is verifiable on node B without any new infrastructure.
141+
- The earlier draft of this paragraph said "anonymous on a
142+
private network" — that was wrong: the receiving side enforces
143+
session auth, so anonymous calls are rejected with 401 and the
144+
cluster heatmap collapses to "1 of N nodes responded". The
145+
cookie-forwarding scheme above is what actually works.
146+
- **Trust model**: forwarding a session cookie to peers is safe
147+
when (a) every peer is operator-configured and trusted, and
148+
(b) the network is private (cookies are HttpOnly but ride
149+
plaintext HTTP for now). `--keyvizFanoutNodes` is the
150+
operator's explicit trust list. **Do NOT point
151+
`--keyvizFanoutNodes` at an untrusted host** — the user's
152+
admin session would be replayed there.
153+
- Per-call timeout: 2 s default, override via
154+
`--keyvizFanoutTimeout`.
155+
- **Auth (Phase 2-C+)**: a follow-up replaces cookie forwarding
156+
with a short-lived **inter-node** token derived from
157+
`ELASTICKV_ADMIN_SESSION_SIGNING_KEY`. Pre-shared, NOT a replay
158+
of the browser cookie — re-using the cookie couples browser
159+
session TTL to inter-node call validity, and a compromised
160+
browser session gains peer-call authority. The follow-up
161+
decouples the two paths so the inter-node call survives a
162+
browser-session expiry and a compromised cookie doesn't extend
163+
laterally.
151164

152165
## 4. Merge rules
153166

internal/admin/keyviz_fanout.go

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -205,9 +205,17 @@ type peerResult struct {
205205
// cluster (peers empty) Run returns local with a Fanout block that
206206
// reports Expected=1, Responded=1.
207207
//
208+
// cookies are attached to every peer request so the receiving node's
209+
// SessionAuth middleware sees a valid admin session. Production
210+
// passes the inbound request's cookies; nil disables cookie
211+
// forwarding (peers will 401 unless they have their own bypass).
212+
// All cluster nodes must share the same --adminSessionSigningKey for
213+
// the cookie minted by node A to be verifiable on node B; the
214+
// existing HA setup already requires this.
215+
//
208216
// Run never returns an error: peer-level failures surface in the
209217
// FanoutResult; aggregation is best-effort.
210-
func (f *KeyVizFanout) Run(ctx context.Context, params keyVizParams, local KeyVizMatrix) KeyVizMatrix {
218+
func (f *KeyVizFanout) Run(ctx context.Context, params keyVizParams, local KeyVizMatrix, cookies []*http.Cookie) KeyVizMatrix {
211219
if f == nil || len(f.peers) == 0 {
212220
merged := local
213221
merged.Fanout = &FanoutResult{
@@ -218,7 +226,7 @@ func (f *KeyVizFanout) Run(ctx context.Context, params keyVizParams, local KeyVi
218226
return merged
219227
}
220228

221-
results := f.fetchPeersParallel(ctx, params)
229+
results := f.fetchPeersParallel(ctx, params, cookies)
222230

223231
matrices := []KeyVizMatrix{local}
224232
statuses := []FanoutNodeStatus{{Node: f.selfName(), OK: true}}
@@ -260,7 +268,7 @@ func countOK(statuses []FanoutNodeStatus) int {
260268
return n
261269
}
262270

263-
func (f *KeyVizFanout) fetchPeersParallel(ctx context.Context, params keyVizParams) []peerResult {
271+
func (f *KeyVizFanout) fetchPeersParallel(ctx context.Context, params keyVizParams, cookies []*http.Cookie) []peerResult {
264272
// Cap per-peer wall time so a single slow node cannot hold the
265273
// SPA poll open beyond the configured timeout. The parent
266274
// context is preserved as the cancellation root so an early
@@ -274,15 +282,15 @@ func (f *KeyVizFanout) fetchPeersParallel(ctx context.Context, params keyVizPara
274282
wg.Add(1)
275283
go func(i int, peer string) {
276284
defer wg.Done()
277-
matrix, err := f.fetchPeer(callCtx, peer, params)
285+
matrix, err := f.fetchPeer(callCtx, peer, params, cookies)
278286
results[i] = peerResult{node: peer, matrix: matrix, err: err}
279287
}(i, peer)
280288
}
281289
wg.Wait()
282290
return results
283291
}
284292

285-
func (f *KeyVizFanout) fetchPeer(ctx context.Context, peer string, params keyVizParams) (*KeyVizMatrix, error) {
293+
func (f *KeyVizFanout) fetchPeer(ctx context.Context, peer string, params keyVizParams, cookies []*http.Cookie) (*KeyVizMatrix, error) {
286294
target, err := buildKeyVizPeerURL(peer, params)
287295
if err != nil {
288296
return nil, pkgerrors.Wrap(err, "build peer url")
@@ -292,6 +300,18 @@ func (f *KeyVizFanout) fetchPeer(ctx context.Context, peer string, params keyViz
292300
return nil, pkgerrors.Wrap(err, "new request")
293301
}
294302
req.Header.Set("Accept", "application/json")
303+
// Forward the inbound user's admin session cookie so the peer's
304+
// SessionAuth middleware sees a valid principal. Without this
305+
// the peer rejects every fan-out request with 401 — the missing
306+
// piece in the Phase 2-C MVP design (#685 §3 said "anonymous on
307+
// a private network" but the receiving side still enforces
308+
// session auth, so anonymous calls don't actually go through).
309+
// Forwarding the cookie works because all admin nodes share
310+
// --adminSessionSigningKey for HA; a cookie minted by node A is
311+
// verifiable on node B.
312+
for _, c := range cookies {
313+
req.AddCookie(c)
314+
}
295315
resp, err := f.client.Do(req)
296316
if err != nil {
297317
return nil, pkgerrors.Wrap(err, "peer request")

internal/admin/keyviz_fanout_test.go

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"net/http"
77
"net/http/httptest"
88
"strings"
9+
"sync"
910
"testing"
1011
"time"
1112

@@ -152,6 +153,50 @@ func TestMergeKeyVizMatricesDistinctRowsPreserveOrder(t *testing.T) {
152153
require.Equal(t, "route:9", merged.Rows[2].BucketID)
153154
}
154155

156+
// TestKeyVizFanoutRunForwardsCookies pins the auth bug fix that
157+
// brought all peers up from 401 to 200: the inbound user's session
158+
// cookies are forwarded on every peer request so the receiving
159+
// node's SessionAuth middleware sees a valid principal. Without
160+
// this, peers reject every fan-out call with 401 missing-session-
161+
// cookie and the cluster heatmap collapses to "1 of N nodes
162+
// responded".
163+
func TestKeyVizFanoutRunForwardsCookies(t *testing.T) {
164+
t.Parallel()
165+
var seenCookies [][]*http.Cookie
166+
var seenMu sync.Mutex
167+
peer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
168+
if !strings.HasSuffix(r.URL.Path, "/admin/api/v1/keyviz/matrix") {
169+
http.NotFound(w, r)
170+
return
171+
}
172+
seenMu.Lock()
173+
seenCookies = append(seenCookies, r.Cookies())
174+
seenMu.Unlock()
175+
w.Header().Set("Content-Type", "application/json")
176+
w.WriteHeader(http.StatusOK)
177+
_ = json.NewEncoder(w).Encode(KeyVizMatrix{Series: keyVizSeriesReads})
178+
}))
179+
defer peer.Close()
180+
181+
f := NewKeyVizFanout("self:8080", []string{peer.URL}).WithHTTPClient(peer.Client())
182+
cookies := []*http.Cookie{
183+
{Name: "admin_session", Value: "session-token-abc"},
184+
{Name: "admin_csrf", Value: "csrf-token-def"},
185+
}
186+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, cookies)
187+
require.True(t, merged.Fanout.Nodes[1].OK)
188+
seenMu.Lock()
189+
defer seenMu.Unlock()
190+
require.Len(t, seenCookies, 1)
191+
got := seenCookies[0]
192+
names := make(map[string]string, len(got))
193+
for _, c := range got {
194+
names[c.Name] = c.Value
195+
}
196+
require.Equal(t, "session-token-abc", names["admin_session"], "session cookie must be forwarded verbatim")
197+
require.Equal(t, "csrf-token-def", names["admin_csrf"], "all inbound cookies forwarded; CSRF is innocuous on a GET")
198+
}
199+
155200
// TestKeyVizFanoutRunSinglePeerOK exercises the end-to-end happy
156201
// path: one peer responds with a parseable matrix; the aggregator
157202
// merges it with the local view and reports both nodes ok.
@@ -176,7 +221,7 @@ func TestKeyVizFanoutRunSinglePeerOK(t *testing.T) {
176221
},
177222
}
178223

179-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, local)
224+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, local, nil)
180225
require.Equal(t, []uint64{10}, merged.Rows[0].Values, "reads must sum across local + peer")
181226
require.NotNil(t, merged.Fanout)
182227
require.Equal(t, 2, merged.Fanout.Expected)
@@ -209,7 +254,7 @@ func TestKeyVizFanoutRunPeerHTTPError(t *testing.T) {
209254
},
210255
}
211256

212-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, local)
257+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, local, nil)
213258
require.Equal(t, []uint64{3}, merged.Rows[0].Values, "5xx peer must not perturb local counts")
214259
require.NotNil(t, merged.Fanout)
215260
require.Equal(t, 2, merged.Fanout.Expected)
@@ -238,7 +283,7 @@ func TestKeyVizFanoutRunPeerTimeout(t *testing.T) {
238283
WithTimeout(50 * time.Millisecond)
239284

240285
start := time.Now()
241-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads})
286+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, nil)
242287
require.Less(t, time.Since(start), 1*time.Second, "fan-out must not wait beyond its per-peer timeout")
243288
require.NotNil(t, merged.Fanout)
244289
require.Equal(t, 1, merged.Fanout.Responded)
@@ -258,7 +303,7 @@ func TestKeyVizFanoutRunNoPeers(t *testing.T) {
258303
{BucketID: "route:1", Values: []uint64{99}},
259304
},
260305
}
261-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesWrites, rows: 1024}, local)
306+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesWrites, rows: 1024}, local, nil)
262307
require.Equal(t, []uint64{99}, merged.Rows[0].Values)
263308
require.Equal(t, 1, merged.Fanout.Expected)
264309
require.Equal(t, 1, merged.Fanout.Responded)
@@ -322,7 +367,7 @@ func TestKeyVizFanoutRunPeerOrder(t *testing.T) {
322367
defer second.Close()
323368

324369
f := NewKeyVizFanout("self:8080", []string{first.URL, second.URL}).WithHTTPClient(first.Client())
325-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, matrix)
370+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, matrix, nil)
326371
require.Equal(t, []FanoutNodeStatus{
327372
{Node: "self:8080", OK: true},
328373
{Node: first.URL, OK: true},
@@ -369,7 +414,7 @@ func TestKeyVizFanoutRunPeerExceedsBodyLimit(t *testing.T) {
369414
WithResponseBodyLimit(testCap)
370415

371416
start := time.Now()
372-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads})
417+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, nil)
373418
require.Less(t, time.Since(start), 5*time.Second, "decode must respect the size cap and complete promptly")
374419
require.NotNil(t, merged.Fanout)
375420
require.Equal(t, 2, merged.Fanout.Expected)
@@ -416,7 +461,7 @@ func TestKeyVizFanoutRunPeerNearCapSucceedsWithWarning(t *testing.T) {
416461
WithHTTPClient(peer.Client()).
417462
WithResponseBodyLimit(testCap)
418463

419-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads})
464+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, nil)
420465
require.NotNil(t, merged.Fanout)
421466
require.Len(t, merged.Fanout.Nodes, 2)
422467
require.True(t, merged.Fanout.Nodes[0].OK, "self always reports ok")
@@ -458,7 +503,7 @@ func TestKeyVizFanoutRunPeerOverlargeBody(t *testing.T) {
458503
f := NewKeyVizFanout("self:8080", []string{peer.URL}).WithHTTPClient(peer.Client())
459504

460505
start := time.Now()
461-
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads})
506+
merged := f.Run(context.Background(), keyVizParams{series: keyVizSeriesReads, rows: 1024}, KeyVizMatrix{Series: keyVizSeriesReads}, nil)
462507
require.Less(t, time.Since(start), 5*time.Second, "decode must respect the size cap and complete promptly")
463508
require.NotNil(t, merged.Fanout)
464509
require.True(t, merged.Fanout.Nodes[1].OK, "in-cap response must succeed; cap is 64 MiB and the synthetic body is well under that")

internal/admin/keyviz_handler.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,12 @@ func (h *KeyVizHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
178178
matrix := pivotKeyVizColumns(cols, params.series, params.rows)
179179
matrix.GeneratedAt = h.now()
180180
if h.fanout != nil {
181-
matrix = h.fanout.Run(r.Context(), params, matrix)
181+
// Forward the inbound user's cookies so the peer's SessionAuth
182+
// middleware sees a valid principal. Cluster nodes share
183+
// --adminSessionSigningKey for HA, so a cookie minted on this
184+
// node verifies on any peer. nil cookies make peers 401,
185+
// which surfaces as ok=false in the per-node status.
186+
matrix = h.fanout.Run(r.Context(), params, matrix, r.Cookies())
182187
}
183188
w.Header().Set("Content-Type", "application/json; charset=utf-8")
184189
w.Header().Set("Cache-Control", "no-store")

0 commit comments

Comments
 (0)