fix(pkg): refresh the IRMA JWT verification key on signature mismatch#241
Conversation
…artup
The Jwt auth middleware fetched {IRMA_SERVER}/publickey during worker
initialization and failed the whole server when the IRMA server was
unreachable at that moment ("can not start server service"), creating a hard
restart-order dependency. The unbounded reqwest::get could also stall worker
startup on a hanging connect.
The key now lives in a JwtKeyCache that fetches it (with a 5s timeout) on the
first JWT request: startup does no network I/O, and while the IRMA server is
unreachable JWT requests degrade to 503 UpstreamError instead of the PKG never
coming up.
Fixes #233
The key was fetched once and cached for the life of the process, so rotating the IRMA server's JWT signing key broke every Yivi-authenticated request (401 "JWT decoding error") until each PKG instance was manually restarted. On an InvalidSignature decode failure the JwtKeyCache now re-fetches the key once and retries the JWT, rate-limited to one rotation-triggered fetch per 30s per worker so a flood of forged tokens cannot become a fetch-flood on the IRMA server. Other decode failures (malformed, expired) never trigger a refresh. Fixes #234
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix itself is solid — the InvalidSignature-only refresh, the 30s per-worker debounce, and the borrow-scoping around the await all look correct, and the regression test covers heal-on-rotation, debounced mismatch, and no-refresh-on-garbage. But the PR can't build as committed.
Blocking: the four include_str! fixtures the test depends on (priv_a.pem, pub_a.pem, priv_b.pem, pub_b.pem under pg-pkg/testdata/jwt_rotation/) were never committed — only README.md is on the branch. Root cause: .gitignore has *.pem, so a plain git add skipped them silently. Result: cargo test -p pg-pkg fails to compile with 4 "couldn't read … .pem" errors, breaking the Build workspace job. The PR body's claim that the keypairs are committed is therefore inaccurate.
Fix: git add -f pg-pkg/testdata/jwt_rotation/*.pem and push.
After generating equivalent throwaway RSA keypairs locally, test_jwt_key_refresh_on_rotation and all pg-pkg tests pass, and cargo fmt --check + cargo clippy are clean — so this is the only thing standing between the PR and green CI.
Note: this is stacked on #240 (base fix/jwt-key-lazy-fetch); merge that first, then retarget.
|
I reviewed this and the fix itself looks solid — the Fix is simple: Full review: #241 |
While the IRMA server is unreachable, every JWT request re-ran the key fetch (up to the full 5s timeout), so a burst of JWT traffic during an outage could tie each worker up in back-to-back fetch timeouts. Failed fetches are now negatively cached for 3s: requests inside the window fail fast with 503 and fetching resumes automatically afterwards.
refresh() now goes through fetch_and_store, so rotation-triggered refreshes share the failure negative-cache with the first-use fetch.
|
Fixed the blocker, /dobby — good catch: the repo's Also merged |
|
Thanks for the update! I'm on it — I'll open/update a draft PR with the force-added fixtures and the |
There was a problem hiding this comment.
Sign-off — the cycle-1 blocker is resolved and the PR is clean.
Blocker fixed: all four jwt_rotation PEM fixtures (priv_a, pub_a, priv_b, pub_b) are now committed to the tree (force-added past the *.pem gitignore in 9f65acb). Verified on a fresh checkout of HEAD: cargo test -p pg-pkg --bins test_jwt_key_refresh_on_rotation compiles and passes, so a clean clone builds. cargo fmt --check on pg-pkg is clean.
Fix review: the InvalidSignature-only refresh, the 30s per-worker debounce (MIN_REFRESH_INTERVAL), and the borrow-scoped decode-then-refresh-then-retry all look correct. refresh() now routes through fetch_and_store, so the rotation refresh shares the failure negative-cache merged forward from #240. The regression test covers heal-on-rotation, debounced mismatch, and no-refresh-on-garbage, asserting upstream fetch counts at each step — no findings.
Note: this is stacked on #240 (base fix/jwt-key-lazy-fetch); the two-dot diff against that base is a real 220-line change, not a no-op. Merge #240 first, then retarget/merge this.
|
Nice catch on the gitignore — good call resolving it that way. I re-checked a fresh clone of HEAD: the four One thing before you merge: this is stacked on #240, so merge that first, then retarget/merge #241. Merging itself is on you per policy. |
Negative caching only kicks in after a failed fetch completes, so requests arriving while the very first fetch was still in flight each started their own — a small thundering herd of fetch timeouts. ensure_key now serializes fetches on an async lock with a double-check: one request does the network I/O, the rest wait and then observe either the fresh key or the fresh negative cache.
ensure_key (first use, early-returns when a waiter finds the key already fetched) and refresh (rotation, always re-fetches) both funnel through a locked fetch_and_record, so concurrent bursts share one upstream fetch in both paths.
1 participant
Fixes #234. Stacked on #240 (base branch =
fix/jwt-key-lazy-fetch) — merge that first, then retarget/merge this.Before: the IRMA JWT verification key was fetched once and cached forever. Rotating the IRMA server's signing key — routine hygiene or emergency rotation — silently broke every Yivi-authenticated request (
401 JWT decoding error) until each PKG instance was restarted. Reproduced live with the e2e stack.After: on an
InvalidSignaturedecode failure,JwtKeyCachere-fetches the key once and retries the JWT — a rotation heals on the next request.Abuse guard: rotation-triggered refreshes are rate-limited (30s per worker), and only
InvalidSignaturetriggers one — malformed/expired tokens never do. A flood of forged tokens can't become a fetch-flood on the IRMA server.Regression test
test_jwt_key_refresh_on_rotationcovers all three behaviors against a local rotating/publickeyserver (committed throwaway keypairs inpg-pkg/testdata/jwt_rotation/): heal-on-rotation, debounced mismatch, and no-refresh-on-garbage.