feat: delegation token cache - #36
Conversation
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
…es apart. Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Signed-off-by: Teryl Taylor <terylt@ibm.com>
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Thorough, well-reasoned work. The cache-key derivation is carefully anchored on the credential rather than the identity, with clear documentation of why and references to real CVEs that got this wrong. The dual-clock strategy (monotonic for TTL, wall-clock for token expiry) handles the host-suspend edge case correctly. The coalescing guarantee via moka is well-motivated, the NotCacheable enum is a clean way to refuse caching with diagnostics, and the test suite is strong — particularly two_callers_are_never_handed_each_others_token and the concurrency test.
One medium finding on config validation below.
| self.staleness.floor_seconds, self.ttl_ceiling_seconds | ||
| )); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
[Medium] validate() checks floor_seconds >= ttl_ceiling_seconds but does not account for jitter_seconds pushing the margin past the ceiling for a fraction of entries.
Example: floor_seconds: 250, jitter_seconds: 100, ttl_ceiling_seconds: 300 passes validation because 250 < 300. But serve_window() computes margin = max(fraction * 300, 250) + (jitter_byte / 255) * 100 = 250 + 0..100, giving a window of 300 − (250..350) = −50..50. For roughly half the entries (those with jitter_byte > ~127) the window is zero or negative, so they are minted but never served from cache. The runtime warned_unservable latch catches this per-token, but only after traffic arrives — the same class of silent-zero-hit-rate configuration the existing floor >= ceiling check was designed to prevent at startup.
Add a check after the existing floor >= ceiling guard:
if self.staleness.floor_seconds.saturating_add(self.staleness.jitter_seconds)
>= self.ttl_ceiling_seconds
{
return Err(format!(
"cache.staleness.floor_seconds ({}) + cache.staleness.jitter_seconds ({}) \
is at or above cache.ttl_ceiling_seconds ({}), so most entries would be \
stale before they could be served",
self.staleness.floor_seconds,
self.staleness.jitter_seconds,
self.ttl_ceiling_seconds
));
}…taleness margin. Signed-off-by: Teryl Taylor <terylt@ibm.com>
…-cache Signed-off-by: Teryl Taylor <terylt@ibm.com> # Conflicts: # CHANGELOG.md # Cargo.lock # builtins/plugins/delegator-oauth/src/delegator.rs
araujof
left a comment
There was a problem hiding this comment.
Nice work! This will be a great performance improvement for delegation flows.
I found two issues to address before merging:
- In
cache/store.rs, the wall-clock check only considers the token's expiry. After a system suspend, it can serve a token beyondttl_ceiling_seconds, defeating the documented revocation limit. It should also checkminted_at + serve_for. - The identity warning is described as a config-load warning, but
RouteDispatchPlanis built on the first request. A bad route therefore loads without warning. This check should run during config loading.
The existing tests pass, but a focused test reproduces the expiry issue.
…n at config load. Signed-off-by: Teryl Taylor <terylt@ibm.com>
|
Updated based on @araujof comments. |
Signed-off-by: Teryl Taylor <terylt@ibm.com>
Brings in praxis-proxy#38 (one hook authority), praxis-proxy#41 (runtime snapshot writers), and the CI fixes. Conflicts, seven files: - CHANGELOG.md: additive in all three sections, kept both sides. This branch also carried older copies of two of praxis-proxy#38's entries; dropped those in favour of the merged versions. - cmf/view.rs: took praxis-proxy#38's `phase_is`, which reads the registry instead of the hook's name. - cmf/constants.rs: praxis-proxy#38's corrected response-hook wording plus this branch's routing paragraphs on both hooks. - config.rs, apl-runtime/visitor.rs, tests/global_http_authz.rs: one-sided additions, kept both. - engine.rs: praxis-proxy#41's factory resolution and praxis-proxy#38's fixture helpers alongside this branch's route-key validation. `resolve_identity_plugins_for_route` needed adapting rather than merging: this branch changed it to take the route already matched, and praxis-proxy#36 added a caller passing the old (type, name, scope) triple. That caller now resolves the route first. It runs at config load with no request line, so no `http:` route is reachable from it. Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Summary
Every
delegatestep runs an RFC 8693 exchange. A token valid for five minutes is used once and discarded, which puts anIdPround trip on every delegated call and makes theIdPa dependency of every request rather than of every few minutes.This lets the OAuth delegator reuse a token it already minted. Off by default; a
cache:block turns it on.Closes #30.
Cache Key
The key is an HMAC-SHA256 over 14 length-prefixed components under a per-process secret. The load-bearing component is the credential being exchanged, not
security.subject.id:fast-jwtshipped an identity-derived key and it became GHSA-rp9m-7r4c-75qg at CVSS 9.1. Its default key, the token itself, was never affected.Excluded deliberately:
security.subject.idfor the reasons above, andAgentExtension.agent_id, which is caller-settable session state rather than a credential.Refused outright, so the delegation falls back to minting: an empty credential on a subject that should carry one, a route with an unrendered
resource_template(a handler may one day render request arguments into it), and any#[non_exhaustive]variant this build does not recognise.Behavior
cache:block the delegator runs the code it ran before.this_workloadandclientonly. Their entry count is bounded by configuration;userandcaller_workloadgrow with the caller population and are opt-in throughcache.subjects.Instantdoes not advance across a host suspend.A cached token stays usable after an
IdP-side revocation until its entry retires.cache.ttl_ceiling_secondsis the bound.Also here: a config-load warning when a route has a
delegatestep that exchanges the caller's credential but resolves no identity for it, sinceidentity:is per-route and optional.Not in this PR
A mint rate limit. Reaching the delegator implies identity resolution has validated the credential, so the amplification concern is narrower than feat(delegator-oauth): cache delegated tokens until expiry #30 assumed. What remains is hardening rather than a fix.
A sweep for expired entries independent of eviction pressure, so an expired entry holds its slot until something touches it.
A weigher. The bound counts entries, not bytes.
Any way to clear the cache on demand. The revocation window above is bounded by
cache.ttl_ceiling_secondsbut cannot be ended early. Aflush()exists on the store and is tested, but nothing is wired to it. Restarting the process or reloading config drops the cache as a side effect, sinceload_configbuilds fresh plugin instances.Worth knowing before revocation support is designed: the key is an HMAC of the credential, so an event carrying the revoked token can be turned back into the exact keys to invalidate. An event naming only a subject cannot, because
subject.idis deliberately not in the key, and would need either a secondary index or a full flush. Checking revocation by introspection on the read path would put back the round trip the cache removes.Any cross-process tier. The cache and its coalescing are per process, so N proxies cold on one key produce N exchanges.
Testing
104 tests. The load-bearing one is
two_callers_are_never_handed_each_others_token, driven through the handler against a mockIdPwith both mocks matching onsubject_token, so it also checks the delegator sent the right credential for each caller. Removing the credential from the key derivation fails that test and only that test.concurrent_requests_for_one_key_mint_onceruns 16 tasks through a barrier on a multi-threaded runtime and asserts exactly one exchange.The handler's trait signature is unchanged, which is why the 23 pre-existing delegator e2e tests pass untouched.
Dependencies
hmacandsha2for the key (sha2was already vendored, sohmacadds one crate and nothing transitive; blake3 would have brought a new tree for a speed advantage that does not matter at one hash per delegation).mokafor the store, which adds five crates and supplies the coalescing and never-cache-an-error guarantees that would otherwise be hand-rolled here. All permissive licences.