fix: repoint the workload key source at workload_issuers - #6282
Merged
Conversation
Contributor
🦋 Changeset detectedLatest commit: c24f73e 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 |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 3 files
Linked issue analysis
Linked issue: AIM-265: fix: repoint the workload key source at workload_issuers
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | workloadIssuerKeySource takes a workload_issuers row and builds a jwks.Source from its jwks_uri | The parameter now uses workloadidentity_repo.WorkloadIssuer, jwks_uri is passed to NewRemoteSource, and the test verifies the resulting cache key. |
| ✅ | No reference to remote_session_issuers remains on the workload verification path | The workload key-source implementation removes the remotesessions repo import and now uses the workloadidentity repo type. |
| ✅ | Operator-facing JWKS URI errors identify the issuer by name | Both empty-URI and source-construction errors use issuer.Name; the empty-URI test verifies the issuer name appears in the error. |
| ✅ | Fetches remain charged to the workload budget keyed by authorization server | The existing workloadFetchScope(endpoint) is retained unchanged when constructing the source. |
Base automatically changed from
waaronwong/aim-252-feat-resolve-a-workload-issuer-by-url
to
main
September 10, 2026 19:55
aa-wong
force-pushed
the
waaronwong/aim-265-fix-repoint-the-workload-key-source-at-workload_issuers
branch
from
September 10, 2026 19:56
444a72d to
8b03559
Compare
This comment has been minimized.
This comment has been minimized.
workloadIssuerKeySource still read remote_session_issuers, the table this project moved off, which left the last step of the verification path bound to a schema the design abandoned. It now takes the row workloadidentity.ResolveIssuerByURL returns. Two things fall out of the new table rather than being chosen here. jwks_uri is NOT NULL, so the missing-URI branch stops being an operator error to explain and becomes a guard against a row that cannot exist; the "re-run discovery" advice went with it, because there is no longer a discovery gap for it to describe. And a workload issuer has no slug, so the errors name it by name, which is the identifier an operator works with anyway. Fetch scoping, the per-authorization-server budget and the reasoning behind it are untouched. Claude-Session: https://claude.ai/code/session_01KYwFta55KqHStFGvWUHiJc
aa-wong
force-pushed
the
waaronwong/aim-265-fix-repoint-the-workload-key-source-at-workload_issuers
branch
from
September 10, 2026 21:01
8b03559 to
e103b12
Compare
aa-wong
changed the base branch from
main
to
waaronwong/aim-149-feat-admitted-workload-identity-lookup-behind-an-injected
September 10, 2026 21:01
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
daviddanialy
approved these changes
Sep 11, 2026
NOT NULL does not exclude the empty string, and workload_issuers carries no non-empty CHECK on jwks_uri the way it does on name. Describe the guard as handling invalid persisted data rather than an unstorable row. Claude-Session: https://claude.ai/code/session_01FPKfHcF2LkCeMT8DG4nojx
aa-wong
added a commit
that referenced
this pull request
Sep 11, 2026
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. -->
[AIM-148](https://linear.app/speakeasy/issue/AIM-148/feat-bound-workload-issuer-lookups-on-the-admission-path) ## Summary `admit` resolves an assertion's `iss` to the workload issuer row in the caller's tenancy, or reports `errWorkloadIssuerUntrusted`. Two things bound it: - **A fleet-wide rate limit**, `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 `workloadIssuerKeySource` reads `jwks_uri` off 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 unrecognised `iss` cannot 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 `workloadIssuerMissReason` taxonomy went with it: wrapping `errWorkloadIssuerURLInvalid` inside `errWorkloadIssuerUntrusted` carries the same distinction. `main` is merged in rather than rebased onto, because intermediate commits here edit `remotesessions` files they later revert. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
aa-wong
merged commit Sep 11, 2026
c0423af
into
waaronwong/aim-149-feat-admitted-workload-identity-lookup-behind-an-injected
9 checks passed
aa-wong
deleted the
waaronwong/aim-265-fix-repoint-the-workload-key-source-at-workload_issuers
branch
September 11, 2026 18:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AIM-265
Summary
workloadIssuerKeySourcenow takes theworkload_issuersrow thatworkloadidentity.ResolveIssuerByURLreturns, instead of aremote_session_issuersrow. No reference toremote_session_issuersremains on the workload verification path.Two changes follow from the new table rather than being decisions made here:
jwks_uribranch stops being an operator error.jwks_uriis stored at discovery time and isNOT NULL, so there is no discovery gap for the old "re-run discovery" advice to describe. The check stays as a guard over invalid persisted data —NOT NULLdoes not exclude"", and unlikenamethe column carries no non-emptyCHECK.name. A workload issuer has no slug: its issuer URL is already its canonical machine-readable identity.Fetch scoping and the per-authorization-server budget are untouched.
Motivation
This shipped in #5840 before the schema reversal and was the last thing on the verification path still bound to the table the project moved off. The key source decides which keys an assertion is verified against, so leaving it pointed at the abandoned table is what stopped the verification milestone closing honestly.
No production caller yet — AIM-259 is the grant that will call it — so this repoints the seam before anything depends on the wrong shape of it.