From 69042899a8ba3dbdaab0ac7a12fdebb11ff6b194 Mon Sep 17 00:00:00 2001 From: defangdevs Date: Fri, 6 Nov 2026 03:48:40 +0000 Subject: [PATCH] fix(web): bound stale portal signing keys and serialize JWKS refreshes (#634) Every return path of portal_jwks_keys now enforces JWKS_MAX_STALE (two hours since the last successful fetch), including the refresh cooldown and failed-fetch paths. The refresh attempt is reserved under a file lock before the network call, so concurrent requests cause at most one fetch. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01RVYuG58Xp5uVuw6PdS6GHW --- docs/portal-handoff.md | 6 +- flake.nix | 21 ++++ modules/agent-box.nix | 87 ++++++++++----- modules/src/settings-daemon.py | 87 ++++++++++----- .../agent-box-settings/bin/agent-box-settings | 87 ++++++++++----- tests/test-portal-jwks.py | 104 ++++++++++++++++++ 6 files changed, 300 insertions(+), 92 deletions(-) create mode 100644 tests/test-portal-jwks.py diff --git a/docs/portal-handoff.md b/docs/portal-handoff.md index 88ebce5b..3a7b2132 100644 --- a/docs/portal-handoff.md +++ b/docs/portal-handoff.md @@ -156,8 +156,10 @@ skew allowance — so a minute after you stop using it). **An outage is survivable, but not indefinitely.** If the endpoint is unreachable the box keeps using the keys it already cached, so a brief blip -does not lock anyone out. The cost is that a **withdrawn** key stays usable -for up to the cache hour. If you need a key dead sooner than that, the +does not lock anyone out, but never for more than two hours after the last +successful fetch: past that the box rejects handovers until the endpoint +answers (Basic authentication still works). The cost is that a **withdrawn** +key stays usable for up to those two hours. If you need a key dead sooner than that, the revocation to reach for is per-account (§6), not the key set. ## 3. Delivery: POST, never a URL diff --git a/flake.nix b/flake.nix index 7f06cbe2..69c911a0 100644 --- a/flake.nix +++ b/flake.nix @@ -2949,6 +2949,27 @@ open(sys.argv[3], "w").write(header + yaml.safe_dump(data, sort_keys=True))' \ cp log "$out" ''; + # Stale-key bound and refresh serialization for the portal JWKS + # cache (issue #634), against the assembled daemon. + portal-jwks = + pkgs.runCommand "agent-box-portal-jwks" + { + nativeBuildInputs = [ pkgs.python3 ]; + daemon = ./tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings; + tests = ./tests/test-portal-jwks.py; + } '' + install -d repo/tests/golden/web/payloads/agent-box-settings/bin + cp "$daemon" \ + repo/tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings + cp "$tests" repo/tests/test-portal-jwks.py + python3 repo/tests/test-portal-jwks.py > log 2>&1 || { + cat log + exit 1 + } + cat log + cp log "$out" + ''; + # Browser-independent protocol coverage for the resumable receiver # (issue #898), against the same assembled daemon that ships. This # includes a deliberately severed PATCH request and continuation diff --git a/modules/agent-box.nix b/modules/agent-box.nix index b68ae187..bba44dd1 100644 --- a/modules/agent-box.nix +++ b/modules/agent-box.nix @@ -21866,6 +21866,9 @@ PORTAL_JWKS_URL = os.environ.get("AGENT_BOX_PORTAL_JWKS_URL", "") or ( # expires anyway, so this is the ceiling on how long a WITHDRAWN key stays # usable rather than how quickly a new one is picked up. JWKS_TTL = 3600 +# Hard ceiling on trusting a cached key set, outage or not (issue #634): +# the TTL plus one hour of grace for a portal that is briefly unreachable. +JWKS_MAX_STALE = 2 * JWKS_TTL # Floor between refetches triggered by an unknown `kid`. Without it the # unauthenticated handover route is an outbound-request amplifier: a caller # minting tokens with random `kid`s would have the box hammer the portal. @@ -31397,39 +31400,63 @@ def portal_jwks_keys(kid, force=False): lever on the portal this floor exists to deny. """ now = int(time.time()) - cached = portal_jwks_read_cache() or {} - try: - fetched = int(cached.get("fetched", 0)) - except (TypeError, ValueError): - fetched = 0 - try: - attempted = int(cached.get("attempted", 0)) - except (TypeError, ValueError): - attempted = 0 - keys = cached.get("keys") or [] - if not isinstance(keys, list): - keys = [] - fresh = keys and now - fetched < JWKS_TTL - if fresh and not force: - return keys - if now - attempted < JWKS_REFETCH_FLOOR: - # An attempt happened too recently to try again, whether that was - # the ordinary refresh or a forced retry. Answer from what we have - # rather than making either path a lever on the portal. - return keys + with portal_jwks_lock(): + cached = portal_jwks_read_cache() or {} + fetched, attempted, keys = portal_jwks_unpack(cached) + # Every return below passes through this bound: a key set older + # than JWKS_MAX_STALE is no key set, however the refresh went. + usable = keys if now - fetched < JWKS_MAX_STALE else [] + fresh = usable and now - fetched < JWKS_TTL + if fresh and not force: + return usable + if now - attempted < JWKS_REFETCH_FLOOR: + # An attempt happened too recently to try again, whether that + # was the ordinary refresh or a forced retry. Answer from what + # we have rather than making either path a lever on the portal. + return usable + # Reserve the attempt BEFORE the network call, under the lock, so + # concurrent requests see it and do not each start a fetch. + portal_jwks_write_cache( + {"fetched": fetched, "attempted": now, "keys": keys}) document = portal_jwks_fetch() if document is None: - # Fetch failed. Stale keys still beat none: the portal being briefly - # unreachable should not lock out a handover signed by a key we - # already hold. A withdrawn key outliving its withdrawal by up to - # JWKS_TTL is the cost, and it is why that value is not days. - # `attempted` still moves, so a persistent outage stays capped at - # one fetch per JWKS_REFETCH_FLOOR rather than one per request. + # Fetch failed. Stale keys still beat none, but only until + # JWKS_MAX_STALE: the portal being briefly unreachable should not + # lock out a handover signed by a key we already hold, while a + # withdrawn key must not outlive that bound however long the outage. + return usable + with portal_jwks_lock(): return portal_jwks_write_cache( - {"fetched": fetched, "attempted": now, "keys": keys})["keys"] - return portal_jwks_write_cache( - {"fetched": now, "attempted": now, - "keys": document.get("keys", [])})["keys"] + {"fetched": now, "attempted": now, + "keys": document.get("keys", [])})["keys"] + + +def portal_jwks_unpack(cached): + """(fetched, attempted, keys) from a cache record, tolerating junk.""" + def seconds(name): + try: + return int(cached.get(name, 0)) + except (TypeError, ValueError): + return 0 + keys = cached.get("keys") or [] + return seconds("fetched"), seconds("attempted"), ( + keys if isinstance(keys, list) else []) + + +@contextlib.contextmanager +def portal_jwks_lock(): + """Serialize cache read-decide-reserve across threads and processes.""" + with _PORTAL_JWKS_THREAD_LOCK: + fd = os.open(portal_jwks_cache_path() + ".lock", + os.O_RDWR | os.O_CREAT, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + os.close(fd) + + +_PORTAL_JWKS_THREAD_LOCK = threading.Lock() # SubjectPublicKeyInfo prefix for an Ed25519 key: SEQUENCE { SEQUENCE { diff --git a/modules/src/settings-daemon.py b/modules/src/settings-daemon.py index 088fd423..914c7650 100644 --- a/modules/src/settings-daemon.py +++ b/modules/src/settings-daemon.py @@ -284,6 +284,9 @@ # expires anyway, so this is the ceiling on how long a WITHDRAWN key stays # usable rather than how quickly a new one is picked up. JWKS_TTL = 3600 +# Hard ceiling on trusting a cached key set, outage or not (issue #634): +# the TTL plus one hour of grace for a portal that is briefly unreachable. +JWKS_MAX_STALE = 2 * JWKS_TTL # Floor between refetches triggered by an unknown `kid`. Without it the # unauthenticated handover route is an outbound-request amplifier: a caller # minting tokens with random `kid`s would have the box hammer the portal. @@ -7218,39 +7221,63 @@ def portal_jwks_keys(kid, force=False): lever on the portal this floor exists to deny. """ now = int(time.time()) - cached = portal_jwks_read_cache() or {} - try: - fetched = int(cached.get("fetched", 0)) - except (TypeError, ValueError): - fetched = 0 - try: - attempted = int(cached.get("attempted", 0)) - except (TypeError, ValueError): - attempted = 0 - keys = cached.get("keys") or [] - if not isinstance(keys, list): - keys = [] - fresh = keys and now - fetched < JWKS_TTL - if fresh and not force: - return keys - if now - attempted < JWKS_REFETCH_FLOOR: - # An attempt happened too recently to try again, whether that was - # the ordinary refresh or a forced retry. Answer from what we have - # rather than making either path a lever on the portal. - return keys + with portal_jwks_lock(): + cached = portal_jwks_read_cache() or {} + fetched, attempted, keys = portal_jwks_unpack(cached) + # Every return below passes through this bound: a key set older + # than JWKS_MAX_STALE is no key set, however the refresh went. + usable = keys if now - fetched < JWKS_MAX_STALE else [] + fresh = usable and now - fetched < JWKS_TTL + if fresh and not force: + return usable + if now - attempted < JWKS_REFETCH_FLOOR: + # An attempt happened too recently to try again, whether that + # was the ordinary refresh or a forced retry. Answer from what + # we have rather than making either path a lever on the portal. + return usable + # Reserve the attempt BEFORE the network call, under the lock, so + # concurrent requests see it and do not each start a fetch. + portal_jwks_write_cache( + {"fetched": fetched, "attempted": now, "keys": keys}) document = portal_jwks_fetch() if document is None: - # Fetch failed. Stale keys still beat none: the portal being briefly - # unreachable should not lock out a handover signed by a key we - # already hold. A withdrawn key outliving its withdrawal by up to - # JWKS_TTL is the cost, and it is why that value is not days. - # `attempted` still moves, so a persistent outage stays capped at - # one fetch per JWKS_REFETCH_FLOOR rather than one per request. + # Fetch failed. Stale keys still beat none, but only until + # JWKS_MAX_STALE: the portal being briefly unreachable should not + # lock out a handover signed by a key we already hold, while a + # withdrawn key must not outlive that bound however long the outage. + return usable + with portal_jwks_lock(): return portal_jwks_write_cache( - {"fetched": fetched, "attempted": now, "keys": keys})["keys"] - return portal_jwks_write_cache( - {"fetched": now, "attempted": now, - "keys": document.get("keys", [])})["keys"] + {"fetched": now, "attempted": now, + "keys": document.get("keys", [])})["keys"] + + +def portal_jwks_unpack(cached): + """(fetched, attempted, keys) from a cache record, tolerating junk.""" + def seconds(name): + try: + return int(cached.get(name, 0)) + except (TypeError, ValueError): + return 0 + keys = cached.get("keys") or [] + return seconds("fetched"), seconds("attempted"), ( + keys if isinstance(keys, list) else []) + + +@contextlib.contextmanager +def portal_jwks_lock(): + """Serialize cache read-decide-reserve across threads and processes.""" + with _PORTAL_JWKS_THREAD_LOCK: + fd = os.open(portal_jwks_cache_path() + ".lock", + os.O_RDWR | os.O_CREAT, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + os.close(fd) + + +_PORTAL_JWKS_THREAD_LOCK = threading.Lock() # SubjectPublicKeyInfo prefix for an Ed25519 key: SEQUENCE { SEQUENCE { diff --git a/tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings b/tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings index 5cba88d3..1d998ed9 100644 --- a/tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings +++ b/tests/golden/web/payloads/agent-box-settings/bin/agent-box-settings @@ -782,6 +782,9 @@ PORTAL_JWKS_URL = os.environ.get("AGENT_BOX_PORTAL_JWKS_URL", "") or ( # expires anyway, so this is the ceiling on how long a WITHDRAWN key stays # usable rather than how quickly a new one is picked up. JWKS_TTL = 3600 +# Hard ceiling on trusting a cached key set, outage or not (issue #634): +# the TTL plus one hour of grace for a portal that is briefly unreachable. +JWKS_MAX_STALE = 2 * JWKS_TTL # Floor between refetches triggered by an unknown `kid`. Without it the # unauthenticated handover route is an outbound-request amplifier: a caller # minting tokens with random `kid`s would have the box hammer the portal. @@ -10313,39 +10316,63 @@ def portal_jwks_keys(kid, force=False): lever on the portal this floor exists to deny. """ now = int(time.time()) - cached = portal_jwks_read_cache() or {} - try: - fetched = int(cached.get("fetched", 0)) - except (TypeError, ValueError): - fetched = 0 - try: - attempted = int(cached.get("attempted", 0)) - except (TypeError, ValueError): - attempted = 0 - keys = cached.get("keys") or [] - if not isinstance(keys, list): - keys = [] - fresh = keys and now - fetched < JWKS_TTL - if fresh and not force: - return keys - if now - attempted < JWKS_REFETCH_FLOOR: - # An attempt happened too recently to try again, whether that was - # the ordinary refresh or a forced retry. Answer from what we have - # rather than making either path a lever on the portal. - return keys + with portal_jwks_lock(): + cached = portal_jwks_read_cache() or {} + fetched, attempted, keys = portal_jwks_unpack(cached) + # Every return below passes through this bound: a key set older + # than JWKS_MAX_STALE is no key set, however the refresh went. + usable = keys if now - fetched < JWKS_MAX_STALE else [] + fresh = usable and now - fetched < JWKS_TTL + if fresh and not force: + return usable + if now - attempted < JWKS_REFETCH_FLOOR: + # An attempt happened too recently to try again, whether that + # was the ordinary refresh or a forced retry. Answer from what + # we have rather than making either path a lever on the portal. + return usable + # Reserve the attempt BEFORE the network call, under the lock, so + # concurrent requests see it and do not each start a fetch. + portal_jwks_write_cache( + {"fetched": fetched, "attempted": now, "keys": keys}) document = portal_jwks_fetch() if document is None: - # Fetch failed. Stale keys still beat none: the portal being briefly - # unreachable should not lock out a handover signed by a key we - # already hold. A withdrawn key outliving its withdrawal by up to - # JWKS_TTL is the cost, and it is why that value is not days. - # `attempted` still moves, so a persistent outage stays capped at - # one fetch per JWKS_REFETCH_FLOOR rather than one per request. + # Fetch failed. Stale keys still beat none, but only until + # JWKS_MAX_STALE: the portal being briefly unreachable should not + # lock out a handover signed by a key we already hold, while a + # withdrawn key must not outlive that bound however long the outage. + return usable + with portal_jwks_lock(): return portal_jwks_write_cache( - {"fetched": fetched, "attempted": now, "keys": keys})["keys"] - return portal_jwks_write_cache( - {"fetched": now, "attempted": now, - "keys": document.get("keys", [])})["keys"] + {"fetched": now, "attempted": now, + "keys": document.get("keys", [])})["keys"] + + +def portal_jwks_unpack(cached): + """(fetched, attempted, keys) from a cache record, tolerating junk.""" + def seconds(name): + try: + return int(cached.get(name, 0)) + except (TypeError, ValueError): + return 0 + keys = cached.get("keys") or [] + return seconds("fetched"), seconds("attempted"), ( + keys if isinstance(keys, list) else []) + + +@contextlib.contextmanager +def portal_jwks_lock(): + """Serialize cache read-decide-reserve across threads and processes.""" + with _PORTAL_JWKS_THREAD_LOCK: + fd = os.open(portal_jwks_cache_path() + ".lock", + os.O_RDWR | os.O_CREAT, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + os.close(fd) + + +_PORTAL_JWKS_THREAD_LOCK = threading.Lock() # SubjectPublicKeyInfo prefix for an Ed25519 key: SEQUENCE { SEQUENCE { diff --git a/tests/test-portal-jwks.py b/tests/test-portal-jwks.py new file mode 100644 index 00000000..5ebc0844 --- /dev/null +++ b/tests/test-portal-jwks.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +r"""Stale-key bound and refresh serialization for the portal JWKS cache (#634). + +Subject is the assembled golden daemon (see test-downloads.py for why). +No network: portal_jwks_fetch is stubbed. Run: python3 tests/test-portal-jwks.py +""" +import importlib.machinery +import importlib.util +import os +import pathlib +import shutil +import tempfile +import threading +import time +import unittest + +REPO = pathlib.Path(__file__).resolve().parent.parent +DAEMON = (REPO / "tests" / "golden" / "web" / "payloads" + / "agent-box-settings" / "bin" / "agent-box-settings") +KEYS = [{"kid": "k1", "kty": "OKP"}] + + +def load(work): + saved = dict(os.environ) + os.environ.clear() + os.environ["PATH"] = saved.get("PATH", "/usr/bin:/bin") + os.environ["HOME"] = work + os.environ["AGENT_BOX_SETTINGS_USER"] = "alice" + os.environ["AGENT_BOX_SETTINGS_ENV_FILE"] = os.path.join(work, "env") + os.environ["AGENT_BOX_SESSIONS_FILE"] = os.path.join(work, "s.json") + os.environ["AGENT_BOX_SETTINGS_BASE"] = "/alice/settings" + try: + loader = importlib.machinery.SourceFileLoader("jwks_under_test", + str(DAEMON)) + spec = importlib.util.spec_from_loader(loader.name, loader) + mod = importlib.util.module_from_spec(spec) + loader.exec_module(mod) + finally: + os.environ.clear() + os.environ.update(saved) + return mod + + +class Jwks(unittest.TestCase): + def setUp(self): + self.work = tempfile.mkdtemp() + self.d = load(self.work) + self.calls = 0 + self.result = None + + def fetch(): + self.calls += 1 + time.sleep(0.05) + return self.result + self.d.portal_jwks_fetch = fetch + + def tearDown(self): + shutil.rmtree(self.work, ignore_errors=True) + + def seed(self, age, attempted_age): + now = int(time.time()) + self.d.portal_jwks_write_cache( + {"fetched": now - age, "attempted": now - attempted_age, + "keys": KEYS}) + + def test_expired_keys_rejected_on_failed_fetch(self): + self.seed(365 * 86400, 365 * 86400) + self.assertEqual(self.d.portal_jwks_keys("k1"), []) + + def test_expired_keys_rejected_during_cooldown(self): + self.seed(365 * 86400, 0) + self.assertEqual(self.d.portal_jwks_keys("k1"), []) + self.assertEqual(self.calls, 0) + + def test_stale_within_grace_still_served_on_outage(self): + self.seed(self.d.JWKS_TTL + 60, self.d.JWKS_TTL + 60) + self.assertEqual(self.d.portal_jwks_keys("k1"), KEYS) + + def test_expiry_survives_repeated_failures(self): + self.seed(self.d.JWKS_MAX_STALE - 1, 1000) + self.assertEqual(self.d.portal_jwks_keys("k1"), KEYS) + cache = self.d.portal_jwks_read_cache() + cache["fetched"] -= 10 + cache["attempted"] -= 1000 + self.d.portal_jwks_write_cache(cache) + self.assertEqual(self.d.portal_jwks_keys("k1"), []) + + def test_concurrent_requests_fetch_once(self): + self.seed(self.d.JWKS_TTL + 60, self.d.JWKS_TTL + 60) + self.result = {"keys": KEYS} + out = [] + threads = [threading.Thread( + target=lambda: out.append(self.d.portal_jwks_keys("k1"))) + for _ in range(6)] + for t in threads: + t.start() + for t in threads: + t.join() + self.assertEqual(self.calls, 1) + self.assertEqual(len(out), 6) + + +if __name__ == "__main__": + unittest.main(verbosity=2)