Skip to content

Commit 53a38ea

Browse files
rustyconoverclaude
andcommitted
fix(serve): make every process in a deployment share one signing key
The signing key seals HTTP state tokens and catalog opaque data. When VGI_SIGNING_KEY was unset this minted a per-process key with os.urandom(32), which is correct for one process and silently wrong for more than one -- and nothing distinguished the two cases. The failure is load-dependent, which is what makes it expensive. A client whose connection stays pinned to one process never notices. One that reconnects mid-stream -- seek_to_token, a load balancer, a respawned worker -- hits a token sealed under a key the receiving process does not have, and gets an intermittent 400 that reads as flakiness. Demonstrated against four worker processes: 15 of 16 cross-connection resumes succeeded, one failed with "State token signature verification failed". resolve_shared_signing_key() replaces the bare urandom: - Configured: use it. The only correct setting for a load-balanced or multi-instance deployment, since nothing here can reach a peer we did not start. - Unset, single process: mint, and say so at WARNING -- naming the env var and the symptom, rather than staying silent as before. - Unset, pre-fork parent: mint *and export to the environment*, so children inherit one key instead of each minting its own. With that, the same four-worker probe passes 16 of 16. Needed regardless of which server we run: the hazard belongs to any pre-fork model, not to granian specifically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2b1cf28 commit 53a38ea

2 files changed

Lines changed: 177 additions & 4 deletions

File tree

‎tests/test_signing_key.py‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
# © Copyright 2025-2026, Query.Farm LLC - https://query.farm
2+
# SPDX-License-Identifier: Apache-2.0
3+
4+
"""Tests for shared signing-key resolution (``vgi.serve``).
5+
6+
The signing key seals HTTP state tokens. Every process that might serve a
7+
continuation has to hold the same one, and when they do not the failure is
8+
load-dependent: a pinned connection works, a reconnect mid-stream does not.
9+
These tests pin the resolution rules so that cannot regress into silence.
10+
"""
11+
12+
from __future__ import annotations
13+
14+
import os
15+
16+
import pytest
17+
18+
from vgi.serve import (
19+
SIGNING_KEY_ENV,
20+
_warn_if_ephemeral_signing_key,
21+
resolve_shared_signing_key,
22+
)
23+
24+
25+
def test_configured_key_is_used_verbatim(monkeypatch: pytest.MonkeyPatch) -> None:
26+
"""An operator-set key wins and is reported as not ephemeral."""
27+
monkeypatch.setenv(SIGNING_KEY_ENV, "a-shared-secret")
28+
key, ephemeral = resolve_shared_signing_key(propagate_to_children=False)
29+
assert key == b"a-shared-secret"
30+
assert ephemeral is False
31+
32+
33+
def test_unset_key_is_minted_and_flagged(monkeypatch: pytest.MonkeyPatch) -> None:
34+
"""With nothing configured we mint one and say so."""
35+
monkeypatch.delenv(SIGNING_KEY_ENV, raising=False)
36+
key, ephemeral = resolve_shared_signing_key(propagate_to_children=False)
37+
assert len(key) >= 32
38+
assert ephemeral is True
39+
# Not exported: this process starts no children that would need it.
40+
assert SIGNING_KEY_ENV not in os.environ
41+
42+
43+
def test_minted_key_is_exported_for_children(monkeypatch: pytest.MonkeyPatch) -> None:
44+
"""A pre-fork parent exports the key so its workers inherit one value.
45+
46+
This is the whole point: without it each forked worker imports the app
47+
and mints its own, and tokens stop being portable between them.
48+
"""
49+
monkeypatch.delenv(SIGNING_KEY_ENV, raising=False)
50+
key, ephemeral = resolve_shared_signing_key(propagate_to_children=True)
51+
assert ephemeral is True
52+
exported = os.environ.get(SIGNING_KEY_ENV)
53+
assert exported is not None
54+
assert exported.encode() == key
55+
56+
# A child re-resolving from the inherited environment gets the same key.
57+
child_key, child_ephemeral = resolve_shared_signing_key(propagate_to_children=True)
58+
assert child_key == key
59+
assert child_ephemeral is False
60+
61+
62+
def test_independent_processes_disagree_without_configuration(monkeypatch: pytest.MonkeyPatch) -> None:
63+
"""Two unrelated processes mint different keys -- the bug being prevented.
64+
65+
Documents *why* propagation exists: absent a configured key and absent
66+
export, two resolutions produce keys that cannot open each other's
67+
tokens.
68+
"""
69+
from vgi_rpc import crypto
70+
71+
monkeypatch.delenv(SIGNING_KEY_ENV, raising=False)
72+
key_a, _ = resolve_shared_signing_key(propagate_to_children=False)
73+
key_b, _ = resolve_shared_signing_key(propagate_to_children=False)
74+
assert key_a != key_b
75+
76+
sealed = crypto.seal_bytes(b"stream-cursor", key_a, aad=b"aad", version=5)
77+
assert crypto.open_bytes(sealed, key_a, aad=b"aad", version=5) == b"stream-cursor"
78+
with pytest.raises(crypto.SealError):
79+
crypto.open_bytes(sealed, key_b, aad=b"aad", version=5)
80+
81+
82+
def test_ephemeral_warning_is_loud_for_single_process(
83+
monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
84+
) -> None:
85+
"""A per-process key warns, naming the env var and the symptom."""
86+
with caplog.at_level("INFO", logger="vgi.serve"):
87+
_warn_if_ephemeral_signing_key(is_ephemeral=True, multiprocess=False)
88+
assert any(r.levelname == "WARNING" for r in caplog.records)
89+
assert SIGNING_KEY_ENV in caplog.text
90+
91+
92+
def test_configured_key_does_not_warn(caplog: pytest.LogCaptureFixture) -> None:
93+
"""Nothing to say when the operator configured a key."""
94+
with caplog.at_level("INFO", logger="vgi.serve"):
95+
_warn_if_ephemeral_signing_key(is_ephemeral=False, multiprocess=False)
96+
assert not caplog.records

‎vgi/serve.py‎

Lines changed: 81 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import importlib.util
3030
import logging
3131
import os
32+
import secrets
3233
import sys
3334
from collections.abc import Callable
3435
from types import ModuleType
@@ -262,10 +263,11 @@ def create_app(
262263
# Resolve the signing key once, here, so the worker (which seals catalog
263264
# opaque-data envelopes) and the HTTP state-token machinery share the same
264265
# key. When the operator did not configure VGI_SIGNING_KEY, generate an
265-
# ephemeral per-process key: sealed values are then valid for the life of
266-
# this process and clients re-ATTACH after a restart.
266+
# key. See resolve_shared_signing_key for why every process serving this
267+
# deployment has to agree on it.
267268
if signing_key is None:
268-
signing_key = os.urandom(32)
269+
signing_key, is_ephemeral = resolve_shared_signing_key(propagate_to_children=False)
270+
_warn_if_ephemeral_signing_key(is_ephemeral=is_ephemeral, multiprocess=False)
269271

270272
if proxy_proof_required is None:
271273
proxy_proof_required = (os.environ.get("VGI_PROXY_PROOF_MODE") or "").strip().lower() == "require"
@@ -412,14 +414,89 @@ def serve(
412414
app()
413415

414416

417+
SIGNING_KEY_ENV = "VGI_SIGNING_KEY"
418+
419+
415420
def _resolve_signing_key() -> bytes | None:
416421
"""Read ``VGI_SIGNING_KEY`` from the environment."""
417-
raw = os.environ.get("VGI_SIGNING_KEY")
422+
raw = os.environ.get(SIGNING_KEY_ENV)
418423
if raw:
419424
return raw.encode()
420425
return None
421426

422427

428+
def resolve_shared_signing_key(*, propagate_to_children: bool) -> tuple[bytes, bool]:
429+
"""Resolve the signing key every process in this deployment must agree on.
430+
431+
The key seals HTTP state tokens and catalog opaque data. Every process
432+
that might serve a continuation for a stream has to hold the *same* one:
433+
a token sealed by one key fails the AEAD check under another, and the
434+
failure is load-dependent rather than deterministic. A client whose
435+
connection stays pinned to one process never notices; one that reconnects
436+
mid-stream -- ``seek_to_token``, a load balancer, a respawned worker --
437+
hits an intermittent 400 that looks like flakiness.
438+
439+
Resolution:
440+
441+
- ``VGI_SIGNING_KEY`` set: use it. Tokens survive restarts and are valid
442+
across every process configured with the same value. This is the only
443+
correct setting for a load-balanced or multi-instance deployment,
444+
because nothing here can reach a peer we did not start.
445+
- Unset: mint a random key for this deployment. Tokens are then valid for
446+
the life of these processes and clients re-ATTACH after a restart.
447+
448+
Args:
449+
propagate_to_children: True when this process will start worker
450+
processes that import the app themselves (a pre-fork server).
451+
A minted key is then exported to the environment so those
452+
children inherit it instead of each minting its own -- which is
453+
the bug this function exists to prevent.
454+
455+
Returns:
456+
``(key, is_ephemeral)``. ``is_ephemeral`` is True when the key was
457+
minted here rather than configured, so callers can say so out loud.
458+
459+
"""
460+
configured = _resolve_signing_key()
461+
if configured is not None:
462+
return configured, False
463+
464+
minted = secrets.token_urlsafe(32)
465+
if propagate_to_children:
466+
# Children inherit os.environ, so exporting before we start them is
467+
# what makes them agree. Same channel the operator would use, so this
468+
# adds no exposure they did not already have by setting it.
469+
os.environ[SIGNING_KEY_ENV] = minted
470+
return minted.encode(), True
471+
472+
473+
def _warn_if_ephemeral_signing_key(*, is_ephemeral: bool, multiprocess: bool) -> None:
474+
"""Say plainly when state tokens will not be portable.
475+
476+
Silence here is what makes the failure mode expensive: an operator who
477+
scales to two instances gets a server that works until a request lands
478+
on the wrong one.
479+
"""
480+
if not is_ephemeral:
481+
return
482+
if multiprocess:
483+
_logger.info(
484+
"No %s configured; minted one for this deployment and exported it to worker processes. "
485+
"State tokens are valid until restart. Set %s to keep them valid across restarts.",
486+
SIGNING_KEY_ENV,
487+
SIGNING_KEY_ENV,
488+
)
489+
return
490+
_logger.warning(
491+
"No %s configured; using a per-process key. State tokens and catalog handles are valid only "
492+
"for this process. If you run more than one instance behind a load balancer, set %s to the "
493+
"same value in every one -- otherwise a client that reconnects mid-stream gets an "
494+
"intermittent 400.",
495+
SIGNING_KEY_ENV,
496+
SIGNING_KEY_ENV,
497+
)
498+
499+
423500
def _resolve_describe(cli_value: bool) -> bool:
424501
"""Apply ``VGI_ENABLE_DESCRIBE`` env var override.
425502

0 commit comments

Comments
 (0)