feat: bound workload issuer lookups on the admission path - #5878
Conversation
🦋 Changeset detectedLatest commit: 1832a90 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Running ultrareview automatically — This PR rewrites issuer resolution into a shared security boundary and adds a negative cache with subtle keying/eviction semantics; a bug could admit untrusted issuers or cross-tenant leaks, and it changes live API error behavior.. I'll post findings when complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 10m 48s
All reported issues were addressed across 4 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Reviewed (multi-agent: security / concurrency / refactor / tests). Solid overall — refactor is behavior-preserving, cache internals race-free, key design correct. Three things before merge: Design question — admission trust breadth. Blocker: no Flaky test + prod window. In Minor:
Nits: cached malformed repeats lose the Verified clean: poisoning surface (key = exact lookup inputs, tenancy server-side), memory bounds (fixed-size digest keys, FIFO matches claims), error taxonomy (outages never cached), no egress from rejection, no conflict with meta-MCP on main. Tests pass |
|
Thanks — this was a good catch list. All of it except the trust-breadth question is in The flaky test and the prod window were one bug, and your fix was the right one. Re-checking Changeset added — Singleflight context: detached rather than documented. You're right that Spelling-independence is now tested at Both nits were real and are fixed. The malformed/cached one more so than it first looks: losing
Doubled The trust-breadth question is the one I'd like your read on rather than a change. My position: org- and global-tier rows are operator-provisioned through the management API, so tier inheritance is the trust model, and Verified: |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I'd reach for internal/ratelimit before creating more semaphore handling.
clientAssertionKeyRefreshRate in client_assertion_verifier.go already declares a ratelimit.Rate for the sibling client assertion path on this same surface.
Deferring the limiter to a follow-up and landing workloadIssuerLookupSlots in its place inverts the order, I think. A limiter keyed on the endpoint would make the negative cache an optimization rather than a defense, and would remove the need for the semaphore entirely.
The semaphore is my specific concern. It is a single package-level bound shared by every tenant on the replica, and because positive results are not cached it gates legitimate lookups too. Four concurrent connections carrying distinct issuer spellings fill all four slots, and unrelated organizations then queue and time out into "acquire workload issuer lookup slot", which is not errWorkloadIssuerUntrusted and so surfaces as a 5xx rather than a 401. That is a cross-tenant denial surface the code does not have today, introduced by the mitigation.
Would it make sense to pull the limiter forward, so it lands before admit is wired in, and let the miss cache ride on top of it once the rate ceiling is the actual bound?
|
You are right on all four points, and I checked each against the code before changing anything rather than taking the shape of the argument for granted. Reworked in 2e21f35. The semaphore is gone. Confirming the specific concern, because it was worse than I had it in my head:
And the part I had not thought through: before the semaphore the pool was the bound, and waiting on a pool connection is backpressure that eventually serves the query. I converted that into a hard failure, and made it global. That is a cross-tenant denial surface the code did not have, introduced by the mitigation. Agreed. The limiter is pulled forward. Your framing of the ordering is the part that actually changed my mind: with a real ceiling the negative cache is an optimization, not a defense. The comments now say that, since the previous ones asserted the opposite. Two answers, not one. Following the jwks resolver: One deliberate divergence worth flagging: Five tests cover it, each mutation-checked — ignoring the refusal verdict, failing open on a store outage, and collapsing the scope to one global bucket each make a specific test fail. Left alone deliberately: positive results are still uncached. Caching them would make legitimate steady-state traffic free, but it needs an invalidation story for a revoked issuer, and that belongs with the configuration work rather than here. |
|
Before going further on the implementation, I'd like to settle two things about the shape of this step. First, is this ticket still live? Its stated motivation is that an unrecognized Second, if an issuer level pre-check does survive that question, should it be reading Answering these two changes what the miss cache, the tier precedence reuse, and the limiter are for, so I'd rather check now. |
|
Both worth settling, and the first one changes this PR. In order. The egress motivation is dead, and the description is wrong. Confirmed rather than conceded — So "if an unrecognised AIM-149 consumes this stage rather than replacing it. This is where I don't think the conclusion follows from the premise. The lookup is #5952, and its entry point is: func admitWorkloadIdentity(
ctx context.Context,
lookup workloadIdentityLookup,
endpoint *ResolvedMcpEndpoint,
issuer *remotesessions_repo.RemoteSessionIssuer, // already resolved
externalSubject string,
) errorand the key it builds holds If what you mean is that AIM-149 should key on the issuer URL directly and skip the row resolution entirely, that's a real alternative — but it's a change to #5952's shape and to the AIM-143 index, not a reason this ticket is dead. Worth saying which you meant, because the two readings lead to quite different work. Trust breadth: the observation is right, the narrowing is one stage later. Everything factual here holds. Where I'd push back is on it being this stage's job. Issuer admission establishes that an assertion is genuine; it was never meant to establish that the machine is ours. #5952 is where that happens, and its doc raises your concern in your own terms:
The triple is exact — no pattern, prefix, or wildcard field — because "wildcarding a CI subject is the misconfiguration that hands production credentials to anyone able to push a branch." Or more bluntly, from the same file: "Without this step, trusting GitHub Actions would admit anybody's GitHub Actions." It went up a few hours before your comment, so you may not have had it in view. Where each of these actually lands. Since the milestone splits this across several PRs, and reviewing any one of them in isolation makes the predicate look wider than it ends up:
That last row is the one I don't think is closed, only misfiled. It applies to #5952 exactly as much as to this PR — that lookup carries a Independent of all of the above: What I think is actually left. Strip out the two claims above and something real remains, and it's the part I'd rather answer than argue with. The cache, the limiter, the singleflight, the miss taxonomy — all of it was argued for across three review rounds in which the thing being defended was Gram's egress. It isn't any more. It defends one indexed That may still be worth defending, but on those terms rather than by inheriting a threat model that's been designed out. So, concretely: with the ceiling in place and the egress argument gone, is the miss cache still carrying its weight, or should this land as the limiter alone and pick the cache back up when there's a live caller to measure it against? I'd take the second if you read it that way — smaller PR, and the cache is easy to restore. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/internal/remotesessions/issuerhandlers.go">
<violation number="1">
P2: When the caller supplies `https://idp.example.com:0443`, this URL lookup treats it as distinct from a row stored as `https://idp.example.com` or `https://idp.example.com:443`, so automatic setup can miss an existing issuer and create a duplicate. Normalize the parsed port numerically before constructing the match candidates.
(Based on your team's feedback about numeric port normalization in canonical issuer comparison.)</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K77tkbxAUGVNG4SRx3fqhW
…okups Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K77tkbxAUGVNG4SRx3fqhW
Review follow-ups on the workload issuer admission helpers. Re-check the miss cache inside the singleflight function. The outer read and inflight.Do are not one step, so a caller preempted between them arrives after the leader's flight has ended and starts a second lookup for a key already known to be a miss. That cost a redundant query and made the concurrent-burst test's strict assertion racy. Same double-check botFrameworkAuthenticator carries, for the same reason. Run the lookup detached from the caller's context. The flight runs under whichever caller opened it, so a leader that went away handed context.Canceled to everyone sharing its lookup — on a grant reachable without credentials, an abandoned request could fail a legitimate one resolving the same issuer. Detaching makes the timeout the only bound, so the lookup carries an explicit one rather than leaning on the pool's 60s statement timeout. Record why an issuer was rejected, not just that it was. A malformed iss and an unknown one are different rejections, and serving one under the other made the same input answer two ways depending only on whether an entry was live. The reason is stored; the parse error's detail is not, since that text derives from an unbounded unauthenticated value and would put an attacker-sized string back inside the entry cap. Treat a lapsed entry as absent in remember(). It early-returned on any present entry, so a fresh miss colliding with a corpse was dropped. admit reaches remember only through seen, which cleans up on the way past, so this was unreachable there — but it is a footgun for the next consumer. Drop the duplicated wrap on the resolve path: ResolveIssuerByURL already describes the query the handler was describing again. Claude-Session: https://claude.ai/code/session_01DRsSfRrF9g7aLbvUcmEwXZ
…llable Detaching the admission flight from its caller stopped an abandoned request failing a legitimate one sharing the same lookup, but it also meant a client that disconnects no longer takes its work back out of the database pool. singleflight collapses concurrent resolutions of one issuer and does nothing for distinct ones, so a flood of different issuer spellings — free to produce on a grant reachable without credentials — could hold one query per spelling until the lookup timeout, in a pool whose size is pgx's unconfigured default of max(4, NumCPU). Bound concurrent lookups with a small semaphore, and switch to DoChan so each caller keeps its own cancellation while the flight it opened survives it and still records its miss. A caller that gives up now returns its own context error rather than a trust decision, so an abandoned request is never reported as a rejected issuer. Claude-Session: https://claude.ai/code/session_01DRsSfRrF9g7aLbvUcmEwXZ
The slot bound and the detached lookup context were both explained against what the code did before them, which reads as history rather than as a description of the code under the comment. State the properties directly: the flight is detached, the caller keeps its own cancellation through the select, and the timeout plus the slot bound are what hold the flight. Claude-Session: https://claude.ai/code/session_01DRsSfRrF9g7aLbvUcmEwXZ
A process-wide concurrency bound was the wrong instrument. It was shared by every tenant on the replica, and because only misses are cached it gated legitimate lookups too: a handful of concurrent connections carrying distinct issuer spellings could hold every slot while unrelated organizations queued and timed out. That timeout was not a trust decision either, so it surfaced as a 5xx rather than anything a caller could map — a cross-tenant denial surface introduced by the mitigation. Charge each lookup against a ratelimit.Limiter keyed on the endpoint's authorization server instead, the same tenant boundary workloadFetchScope uses next door and for the same reason: no endpoint can spend another's budget, and nothing derived from the request can mint a fresh one. The buckets live in Redis, so the ceiling is fleet-wide rather than multiplied by replica count. With a real ceiling in place the negative cache becomes an optimization rather than the defense — it keeps the common repeat cheap, and the rate limit is what makes a flood of distinct spellings finite. A refusal and a store outage are separate answers, matching the jwks resolver: one is a decision the budget made and carries a retry hint, the other is the store failing to make one and fails closed. Neither is remembered as a miss, since neither says anything about the issuer. Claude-Session: https://claude.ai/code/session_01DRsSfRrF9g7aLbvUcmEwXZ
The motivation this file was written against no longer exists. AIM-146 shipped a key source that reads a jwks_uri already stored on the issuer row, so an unrecognised iss cannot become an outbound request whatever admission does. The comments here still claimed it could, and that claim was doing the work of justifying the miss cache. What a miss actually costs is one indexed SELECT, bounded by the per-endpoint rate limiter, with the cache sitting on top of it. Also drops the claim that this stage decides what "this endpoint trusts". The lookup resolves against the project, organization and platform tiers, so a platform-curated row resolves for every tenant in the fleet. That breadth is correct for a stage that only establishes that Gram knows the issuer; the subject-level admission that follows is the security boundary, and the comment now says so rather than implying this is it. Raised by bflad and, on the trust-breadth wording, by daviddanialy in review. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
The miss cache was per-replica, which multiplies the cost of an unknown issuer by the replica count: every replica has to miss once before any of them holds the answer. The limiter it sits on top of is already fleet-wide for exactly that reason, so the cache was reintroducing the multiplier the limiter exists to avoid. Moves it onto internal/cache, the same TypedCacheObject path sessions and chatsessions use. Set-if-absent preserves the property that repeating a miss cannot extend it, now applied with the TTL in one Redis command rather than under a mutex. A read the store cannot answer falls through to the lookup rather than rejecting, since answering a rejection from a failed read would deny a legitimate workload. The explicit entry cap is gone, since the store caps nothing on its own. The bound moves upstream instead: a miss is only recorded after a lookup a budget charge admitted, so the limiter bounding queries bounds writes by the same amount. Two tests cover what the cap used to, one for the size of a stored entry and one for the charge gating the write. Claude-Session: https://claude.ai/code/session_011bpf9Ufpo7wAWYhUy8E2wP
The comments ran to 54% of the file and restated each point two or three ways. Cut to the decision plus the one reason that is not evident from the code: why the cache key is the supplied spelling, why tenancy is the organization and project, why a nil budget refuses rather than admits, and why an outage is never remembered as a miss. Also drops the claim that the lookup draws on a platform tier so a curated row resolves fleet-wide. Workload issuers move to their own tenant-scoped table (AIM-143, 2026-09-08), and that sentence describes a design being removed. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
…uer row Workload issuer trust moves to its own workload_session_issuers table (AIM-143, 2026-09-08), so this path must not name remote_session_issuers. The lookup already sat behind an injected function type, which is what makes the change small: it now answers with the issuer's id rather than the row, and admission returns that id. An id is all this file ever needed. Everything downstream keys on it, and returning a row tied admission to whichever table happened to hold it. Drops newWorkloadIssuerLookup and the remotesessions files with it. The concrete binding belongs with the workload issuer resolver in AIM-252, and exporting ResolveIssuerByURL was only justified here by the workload path sharing it — which it no longer does. ResolveIssuerByURL therefore stays unexported and the management API keeps resolving as it does today. The malformed-URL case now matches a sentinel this package owns, so admission depends on the lookup's contract rather than on a particular store's errors. TestNewWorkloadIssuerLookup_MalformedIssuerNeverReachesTheDatabase moves to AIM-252 with the binding it covers. Every other test is unchanged in substance. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
The issuer resolution stage was written when workload issuer trust lived in the tri-tier remote_session_issuers catalog. It now resolves against workload_issuers, which is tenant-scoped with no platform tier, so the endpoint-framed wording no longer describes what the code does. errWorkloadIssuerUntrusted said "issuer is not trusted by this endpoint" while its own doc comment explicitly disclaimed that phrasing as too strong. It now says what the doc says: no workload issuer in this tenancy describes that issuer url. Trust for a particular workload is the subject admission that follows, not this. The lookup contract now states why the endpoint is the input — it names the tenancy the resolution runs against, its project and the organization above it — rather than implying the endpoint is itself the trust scope. workloadIssuerLookupScope keeps the authorization server as its key and now says why that is deliberately not the resolution tenancy: it is a denial-of-service bound, sized per MCP server so a flood against one cannot starve the others. No behaviour change. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
8bc51b4 to
40584c6
Compare
The miss cache remembered a rejected issuer for 30s so a repeat cost a cache read rather than a query. It does not earn that. It never defended the attack it was justified by. This file already said so: a flood of distinct issuer spellings misses the cache every time and shares no flight, so the rate limiter was always the bound. And that limiter caps lookups at 120/min per authorization server, which means the cache was optimizing a path already capped at two indexed SELECTs per second. It also traded the wrong way round. The cache is shared Redis, so it replaced a Postgres index probe with a network round trip of the same order — and admit read it first on every request, so a successful admission paid a lookup that always missed. Latency moved onto the happy path to leave the failure path. The no-egress property was never the cache's doing: jwks_uri is stored, so nothing here fetches regardless. Against that, a 30-second window where a newly registered issuer stays rejected on replicas holding a miss, and a whole reason taxonomy existing only so a cached rejection could answer with the same status a fresh one would. That taxonomy is gone — wrapping errWorkloadIssuerURLInvalid in errWorkloadIssuerUntrusted carries the distinction natively. Kept: the rate limiter, which is the real bound on a credential-less path, and singleflight, which collapses a simultaneous burst of one spelling in-process for no dependency. The former miss key is now the singleflight key, which is what it always was underneath. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
aa-wong
changed the title
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Two referred to the miss cache that no longer exists, and one narrated its removal rather than explaining the code. The rest were long enough to obscure the file they document. Claude-Session: https://claude.ai/code/session_01BkWwZUepfhVUssCuUCbW3s
…-caching-for-issuers-not-on-the-allowlist
Returning only the id was meant to keep this file free of whichever table holds an issuer, but that decoupling no longer exists to protect: workloadIssuerKeySource in authnchallenge_workloadauth.go now takes a workload_issuers row, so the package is tied to the type either way. The id-only seam just meant the one caller that needs jwks_uri would re-read the row this lookup had already read. main is merged in rather than rebased onto: intermediate commits on this branch edit remotesessions files they later revert, and replaying them over the schema pivot conflicts for no net change. Claude-Session: https://claude.ai/code/session_01KYwFta55KqHStFGvWUHiJc
…he-workload-key-source-at-workload_issuers' into AIM-148/feat-negative-caching-for-issuers-not-on-the-allowlist
This comment has been minimized.
This comment has been minimized.
AIM-255 ## Summary The database side of the admission interface #5952 defines and wires empty. One query and the function over it, in `server/internal/workloadidentity` beside `ResolveIssuerByURL`, so both reads on the workload path share one `repo.DBTX`. `WorkloadIdentityIsAdmitted` returns `EXISTS`, not the row — the boolean is the whole of what admission needs, and returning a row invites a caller to read something else off it. The tenancy predicate is deliberately identical to the issuer resolver's: `organization_id` unconditionally, so a project-tier row can never answer outside its organization, and a project arm reading the caller's own project or the organization tier. The subject is matched by exact equality, with no expression around the column that would make the lookup index unusable. ## The two nulls `project_id` is nullable on the table and on the query, and they mean different things — an **unset row** is the organization tier and answers every project, an **unset query** is an organization-scoped caller a project-tier row must not answer. SQL gives this for free: `project_id = @project_id` is not true when the parameter is NULL. Both directions are tested. ## Failing closed before the store An empty organization, nil issuer or empty subject returns `(false, nil)` without querying, for the same reason `ResolveIssuerByURL` checks its organization: a tenancy hole should not depend on a data property a seed could break. A store error propagates as an error and never as non-admission. ## Motivation #5952 ships the admission logic with no storage so the security logic stays independently testable. This supplies the store behind it. No production caller yet — AIM-259 is the grant that will call it. ## Stacking Based on #5952, which defines the types. Deliberately not stacked behind #6282 or #5878: this is the last Urgent item in the verification milestone and only needs those types. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds a database-backed admission lookup for workload identities, scoped to the caller's project or organization tier. The new `IsAdmitted` function queries `workload_identity_admissions` for an active row matching organization, issuer, and subject, returning EXISTS rather than the row itself. Store errors propagate as errors; missing key parameters fail closed without querying. Not yet wired to any request path. **Key behaviors** - Tenancy matches the issuer resolver: `organization_id` always applies, and `project_id` either equals the caller's own project or is NULL (organization tier). - An organization-scoped caller (NULL project) sees only organization-tier rows; a project-tier admission never answers it. - Soft-deleted rows are ignored; failure surfaces as an error, never as a denial. - Tests cover both tiers, sibling projects, withdrawn rows, and every key component mismatch. <sup>Written for commit e86178d. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/speakeasy-api/gram/pull/6294?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
c24f73e
into
waaronwong/aim-265-fix-repoint-the-workload-key-source-at-workload_issuers
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
3 participants
AIM-148
Summary
admitresolves an assertion'sissto the workload issuer row in the caller's tenancy, or reportserrWorkloadIssuerUntrusted. Two things bound it:PerMinute(120).WithBurst(30), keyed on the authorization server's identifier and charged before the query, so a refusal costs only the bucket read.singleflight, collapsing concurrent resolutions of one spelling. In-process, no dependency.A spent budget, an unreachable bucket and an absent limiter are each reported distinctly from a rejection — none is a statement about the issuer, and a caller mapping "untrusted" onto a 401 must not answer one for an outage. An absent limiter refuses rather than running unbounded: this path is reachable without credentials, so no ceiling means no grant.
The lookup returns the issuer row rather than its id, because
workloadIssuerKeySourcereadsjwks_urioff it (#6282).This resolves an issuer; it does not authorize a workload. Whether a particular machine may authenticate is the subject admission in #5952, and that is the security boundary.
Motivation
The grant is reachable without credentials by design, so the cheapest request anyone can produce would otherwise buy a database query.
The limiter is keyed per authorization server rather than per replica: a process-wide budget would let one tenant exhaust every other tenant's. It is deliberately not the tenancy the lookup resolves against.
The flight key is
(organization, project, raw spelling), not a canonical form. Lookup matches a closed set of spellings, so two inputs sharing a canonical form do not necessarily share a result, and collapsing them would serve one caller's answer to another that would have matched.Nothing here fetches — the key source reads a stored
jwks_uri— so an unrecognisedisscannot become an outbound request.For anyone who reviewed the earlier version
This was "negative caching for issuers not on the allowlist"; the cache is removed, −509 lines. It never defended the attack it was justified by — a flood of distinct spellings misses every time and shares no flight — and it read first on every request, so a successful admission paid a Redis lookup that always missed. Its
workloadIssuerMissReasontaxonomy went with it: wrappingerrWorkloadIssuerURLInvalidinsideerrWorkloadIssuerUntrustedcarries the same distinction.mainis merged in rather than rebased onto, because intermediate commits here editremotesessionsfiles they later revert.