Skip to content

Accept RS256 access tokens, bound the JWKS caches, and give an operator something to hold on to - #99

Open
tophcodes wants to merge 5 commits into
mainfrom
claude/quadpod-critical-feedback-2mkec1
Open

Accept RS256 access tokens, bound the JWKS caches, and give an operator something to hold on to#99
tophcodes wants to merge 5 commits into
mainfrom
claude/quadpod-critical-feedback-2mkec1

Conversation

@tophcodes

Copy link
Copy Markdown
Owner

Five independent findings from a review pass, one commit each, each commit compiling on its own.

2313221 An access token is verified under its key's algorithm, not ES256 alone

The verifier was pinned to ES256 whatever key the issuer published, so every RS256 issuer was refused, and refused as BadSignature, which reads as a forgery rather than as a capability this pod lacks. RS256 is the algorithm OIDC Core requires every provider to support.

The sharper half: the pod could not read its own signature. op::keys::signer_for signs RS256 for a key that declares it, alg_of publishes RS256 in the JWKS and signing_algs advertises it in discovery, so pointing --op-signing-keys at an RSA key produced a pod that rejected every token it issued. Nothing said so, because every test here used the generated ES256 key. op::mint's new test is that regression.

verifier_for picks the verifier from the resolved key instead. The safety argument is unchanged: the algorithm is a property of the key the issuer published, so a token can no more choose it than it can choose the key, and decode_with_verifier still refuses a header that disagrees with the verifier it was handed. The alg: none and HS256 forgery tests are untouched and still pass; two new tests check that a token signed by the wrong key type is refused in both directions.

A key type this pod cannot verify is now UnsupportedKeyType, not BadSignature, so a misconfigured issuer is distinguishable from an attack in the log.

ADR-3 settled the identical question one layer down for DPoP proofs, for the same reason and with the same conclusion.

8d90b43 The DPoP authorization scheme is matched case-insensitively

RFC 9110 §11.1. Bearer stays refused in every spelling, which is what the second test pins.

760cf4f The JWKS caches are bounded

HttpWebIdIssuers capped both of its caches and swept expired entries; HttpJwksResolver capped neither and removed an entry only on a successful re-fetch. Both key their caches by a string taken off an unverified credential, so the discipline belonged in both.

It was also the wrong half to leave open. A JWKS is resolved before the WebID-issuer binding runs, and the key is the token's iss, read by peek_untrusted_issuer before any signature is checked. With no --trusted-issuer allowlist configured — the documented default — nothing constrains that string: a distinct iss per request is one permanent negative-cache entry per request. The unbounded map was the one in front of the bounded one.

insert_bounded moves to auth::cache and both resolvers use it. The new test drives resolve itself rather than the helper, and stays hermetic because the production FetchPolicy refuses an http:// loopback URL before a socket is opened.

c94eb40 Drain on SIGTERM, serve liveness, answer --version

ADR-7 gives a store directory to exactly one process, so every deployment is a stop and a start with no second replica to absorb the gap. Without draining, SIGTERM killed in-flight requests wherever they were parked. ADR-2's sequence atomicity means nothing was corrupted, but a caller learned the outcome of its own write by having the connection dropped, and a POST is not safe to retry blind.

Liveness is GET /.well-known/health, and the placement is the argument. A route at /health would shadow the resource a user is entitled to store there: the router would answer, the graph would stay, and no write method would reach it — the hazard crate::space already refuses /.well-known/ for. A second reserved segment would cost a name in every pod's URI space forever. Here the segment is already reserved, already GET-only, and already the one exemption tests/route_coverage.rs carries, so this widens none of the three. The cost, stated in the code and in uri-space.md: health is not an IANA-registered well-known name; the shape follows draft-inadarei-api-health-check.

It is liveness, not readiness, and touches neither store. An unauthenticated probe that queried the store would be a store round trip anyone could ask for with no credential and no rate limit in front of it.

7ef1872 CI audits dependencies against the RustSec advisory database

497 crates reachable from an internet-facing binary and nothing checking any of them. cargo-audit goes in the dev shell so its version is pinned by flake.lock like every other tool. Separate job, so an advisory published on RustSec's timetable does not turn the clippy/test signal red on an unrelated PR.

It was red on arrival. Three advisory IDs, five hits, each --ignored with its path and reason recorded next to it:

  • RUSTSEC-2026-0194 / -0195 — quick-xml, both DoS, 7.5. Two paths. One is oxigraph's RDF/XML parser, which nothing constructs: rdf::Format::ALL is closed over five media types with no RDF/XML arm. The other is calaminedctaprudof_lib — a spreadsheet reader linked into the pod because rudof carries DCTAP.
  • RUSTSEC-2026-0235 — rkyv out-of-bounds read, via rust_decimalrudof_rdfrudof_lib.

None is reachable from a request. Listing them rather than silencing warnings wholesale keeps a new advisory red and makes adding one a diff someone reviews. Depending on the narrower shacl crate instead of rudof_lib would clear all three.

cargo fmt --check is deliberately not here: the tree is not rustfmt-clean, and reformatting it would rewrite the deliberately compact layout of the test modules.

Verification

  • cargo clippy --all-targets -- -D warnings clean
  • cargo test green: 676 tests, 14 new
  • All 34 check: lines in docs/constraints.md pass. One of them caught a real mistake in this branch: the new op::mint test wrote a key file directly, breaking "Only op::keys touches the key file". Fixed by adding write_test_key_file to keys.rs, mirroring the existing remove_test_key_file.
  • --version and the two /.well-known/ behaviours checked against a running binary, not only in tests: quadpod 0.1.0; GET /.well-known/health200 application/health+json with the OP off; PUT on it → 405; SIGTERM → drain log then exit 0.
  • Each of the five commits cargo check --all-targets clean on its own, so the history stays bisectable.

Not addressed here

From the same review, left alone deliberately: write serialisation (If-Match is read-then-write across three awaits, so two concurrent conditional PUTs both succeed — the real correctness hole), SHACL validation and up-to-64 MB RDF parsing running on tokio workers with no spawn_blocking, rudof_libshacl, and notify.rs having no production consumer.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ


Generated by Claude Code

…56 alone

The verifier was pinned to ES256 whatever key the issuer published, so every
RS256 issuer was refused, and refused as `BadSignature`, which reads as a
forgery rather than as a capability this pod lacks. RS256 is the algorithm
OIDC Core requires every provider to support.

Worse, the pod could not read its own signature. `op::keys::signer_for` signs
RS256 for a key that declares it, `alg_of` publishes `RS256` in the JWKS and
`signing_algs` advertises it in discovery, so an operator pointing
`--op-signing-keys` at an RSA key got a pod that rejected every token it
issued. Nothing said so: every test here used the generated ES256 key.

`verifier_for` picks the verifier from the resolved key instead, RS256 for
RSA and ES256 for EC P-256, the same two `signer_for` signs with. The safety
argument is unchanged and stated where it lives: the algorithm is a property
of the key the issuer published, so a token can no more choose it than it can
choose the key, and `decode_with_verifier` still refuses a header that
disagrees with the verifier it was handed. The `alg: none` and HS256 forgery
tests are untouched and still pass.

A key type this pod cannot verify is now `UnsupportedKeyType` rather than
`BadSignature`, so an operator can tell a misconfigured issuer from an attack.

ADR-3 settled the identical question one layer down for DPoP proofs, for the
same reason and with the same conclusion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ
RFC 9110 §11.1: "the authentication scheme is case-insensitive". The
comparison was `scheme != "DPoP"`, so a client spelling it `dpop` was
presenting the same credential and being refused for it.

Nothing that used to be accepted is now refused, and `Bearer` stays refused in
every spelling, which is the property the second test pins. The token itself
stays case-sensitive and is passed through untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ
…w freely

`HttpWebIdIssuers` capped both of its caches and swept expired entries;
`HttpJwksResolver` capped neither and removed an entry only on a successful
re-fetch. Both modules key their caches by a string taken off an unverified
credential, so the discipline belonged in both, and nothing recorded that only
one had it.

It was also the wrong half to leave open. A JWKS is resolved before the
WebID-issuer binding runs, and the key is the token's `iss`, read by
`peek_untrusted_issuer` before any signature is checked. With no
`--trusted-issuer` allowlist configured, the documented default, nothing
constrains that string at all: a distinct `iss` per request is one permanent
negative-cache entry per request, so the unbounded map was the one in front of
the bounded one.

`insert_bounded` moves to `auth::cache` and both resolvers use it, one
derivation rather than two that disagreed. The helper's own tests move with
it; the new one drives `resolve` itself, since that is the path an anonymous
caller actually has, and stays hermetic because the production `FetchPolicy`
refuses an `http://` loopback URL before a socket is opened.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ
Three things an operator needs and none of which existed.

`--version` reads `CARGO_PKG_VERSION`, so a binary someone did not build can
say which one it is.

`axum::serve` now takes a shutdown signal. ADR-7 gives a store directory to
exactly one process, so every deployment is a stop and a start with no second
replica to absorb the gap; without draining, the supervisor's `SIGTERM` killed
in-flight requests at whatever `.await` they were parked on. ADR-2's sequence
atomicity means nothing was corrupted, but a caller learned the outcome of its
own write by having the connection dropped, and a `POST` is not safe to retry
blind.

Liveness is `GET /.well-known/health`, and the placement is the point. A route
at `/health` would shadow the resource a user is entitled to store there: the
router would answer, the graph would stay, and no write method would reach it.
That is the hazard `crate::space` already refuses `/.well-known/` for, and a
second reserved segment would cost a name in every pod's URI space forever.
Here the segment is already reserved, already GET-only, and already the one
exemption `tests/route_coverage.rs` carries, so this widens none of the three.
The cost is that `health` is not an IANA-registered well-known name; the shape
follows `draft-inadarei-api-health-check` so it is at least the one an operator
would guess.

It is liveness, not readiness, and touches neither store: an unauthenticated
probe that queried the store would be a store round trip anyone could ask for
with no credential and no rate limit in front of it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ
497 crates reachable from an internet-facing binary and nothing checking any
of them. `cargo-audit` goes in the dev shell so its version is pinned by
flake.lock like every other tool and the local command is the one CI runs.

A separate job, because an advisory is published on RustSec's timetable rather
than this repository's and must not turn the clippy/test signal red on an
unrelated pull request.

It is red on arrival without the three `--ignore` entries, and the reason each
is there is recorded next to it: two quick-xml DoS advisories and one rkyv
out-of-bounds read, all in code no request reaches. One arrives through
oxigraph's RDF/XML parser, which nothing constructs, since `rdf::Format::ALL`
is closed over five media types with no RDF/XML arm. The other two arrive
through `rudof_lib`, which this pod uses for SHACL over Turtle and which drags
in a spreadsheet reader (calamine, via DCTAP) and rust_decimal behind it.
Depending on the narrower `shacl` crate would clear both.

Listing them rather than silencing warnings wholesale keeps a new advisory
red, and makes adding one a diff someone reviews.

`cargo fmt --check` is deliberately not here: the tree is not rustfmt-clean,
and reformatting it would rewrite the deliberately compact layout of the test
modules.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJQkf2dLQZtbYfGaf8QkWZ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants