fix(auth): grant org namespace only to org Owners, not all members#1383
fix(auth): grant org namespace only to org Owners, not all members#1383tadasant wants to merge 4 commits into
Conversation
GitHub access-token exchange granted `io.github.<org>/*` publish
permission for *every* organization a user belonged to, derived from
`GET /users/{username}/orgs` — which returns only public memberships and
carries no role. Any member of an org (or anyone whose public membership
listed it) could publish, and silently overwrite, servers under the
org's namespace.
Switch to `GET /user/memberships/orgs`, which returns the caller's role
per org (and includes private memberships), and grant the org namespace
only when the role is `admin` (org Owner). Personal namespaces are
unaffected.
The memberships endpoint requires the `read:org` scope. A minimal token
without it authenticates fine and still publishes to the personal
namespace; the 403 from the memberships call is treated as "no admin
orgs" so we don't force users to over-scope a token. The token needs no
repository scopes — the registry never touches code.
Refs #982.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The token that authenticates to an org namespace can publish and overwrite any server under io.github.<org>/*, and by default it is reachable by every repository writer (not just org Owners) via branch or tag pushes. Document how to contain that blast radius: - store the token as an Environment secret (update the PAT example to use `environment:`) - restrict the environment to the default branch / release tags and protect that branch with required reviews - optionally require an environment reviewer for per-publish approval - avoid pull_request_target-with-checkout and self-hosted runners on public repos Also clarify that the PAT's read:org scope is what authorizes org publishing and that no repository scopes are needed. Refs #982. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The two added subtests pushed TestGitHubHandler_ExchangeToken over the cyclop max (complexity 23 > 20). Move the member-role and missing-read:org cases into TestGitHubHandler_ExchangeToken_OrgRoles. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
From an implementation point of view PR looks good but few points that I want to raise as small concerns here:
|
This is a good point - maybe we should get rid of the OIDC path altogether? It feels like basically the same problem as the member-only check. What do you think @BobDickinson @rdimitrov ?
Valid concerns, but I think as long as we are transparent about these limitations and our guidance (i.e. use those protections, don't grant Owner just share a PAT), I think it's better than the status quo while we work out the bigger change of creating a delegation table system. |
|
I think I'm good with landing this as a short-term fix for the PAT path (gating org publishing to Owners is clearly better than the current state) 👍 Regarding the other items in the discussion:
Overall I think we can land this for PAT, keep OIDC and if you agree track per-server ownership via the OIDC |
Address review feedback on the org-Owner-only namespace fix: - Disambiguate the 403 from GET /user/memberships/orgs. A 403 with X-RateLimit-Remaining: 0 (or Retry-After present) is a throttle, not a missing read:org scope, so fail closed rather than silently degrading a legitimate Owner to personal-only permissions. - Correct the githubOrgRoleAdmin godoc: GitHub returns admin/member/ billing_manager; only admin carries publish authority. - Add tests: admin org on a later page is still granted (pagination), 5xx memberships error fails closed, and rate-limit 403 fails closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bug (scoped to
github-at)When you exchange a GitHub OAuth/PAT token for a registry JWT, the handler granted
io.github.<org>/*publish permission for every organization you belong to. It derived that list fromGET /users/{username}/orgs, which:So membership was silently treated as publishing authority. Anyone who is (or whose public membership lists them as) a member of
acmecould publish — and, because publish is a pure namespace-glob with no per-server ownership check, overwrite — any server underio.github.acme/*.The fix
Switch to
GET /user/memberships/orgs, which returns the authenticated caller's role per org (and includes private memberships), and grant the org namespace only when the role isadmin(org Owner).io.github.<your-username>/*.admin(Owner) ormember. We grant only onadmin.read:orgis the only scope needed. A minimal token without it still authenticates and still publishes to your personal namespace; a genuine missing-scope403from the memberships call is treated as "no admin orgs" rather than a hard failure, so we don't push anyone to over-scope a token. A403/429that is actually a rate limit (X-RateLimit-Remaining: 0/Retry-After) fails closed instead, so a throttle can't silently strip a real Owner's org grant. Critically, the token needs no repository scopes — the registry never reads or writes code, so an org Owner can hand CI a near-powerlessread:orgtoken.The authorization decision stays stateless and delegated to GitHub — no new storage, no new endpoints, no schema changes — consistent with design principle #2 (minimal operational burden).
Containing the CI token
The registry-side change makes the org Owner the only one who can mint org-namespace authority. But if that Owner drops the resulting PAT into a plain repo secret, every repository writer can reach it (via branch/tag pushes that run repo-controlled build/test code in the privileged job) — quietly re-widening publish access beyond Owners. External fork contributors still cannot reach it by default (no secrets on fork PRs, no branch push), barring misconfigurations like
pull_request_target-with-checkout or self-hosted runners on public repos.docs/modelcontextprotocol-io/github-actions.mdxdocuments how to contain that blast radius:environment:),pull_request_targetcheckout, self-hosted runners on public repos).This shrinks who holds the org credential; it does not change the fact that an authorized publish can still overwrite a different server in the same namespace until per-server publisher ownership exists (the follow-up noted below).
Why this shape, given the alternatives
Pree's writeup (Notion analysis doc) is the clearest framing of the problem and the option space, laying out three broad solutions:
The two-property test (does the registry control the decision? is it granted through an act requiring real authority?) is the right lens. This change satisfies it without new state: the registry makes the decision (it filters on role), and the authority is real (live GitHub Owner role, re-checked on every short-lived token exchange).
What this deliberately gives up, so reviewers can weigh it honestly:
github-oidcpath still grants the wholeio.github.<owner>/*namespace to any workflow withid-token: write, i.e. effectively anyone with write access to any repo in the org, and it excludes non-Actions CI (Jenkins/GitLab can't mint GitHub Actions OIDC). The admin-PAT path here is the higher-assurance option for org publishing; OIDC stays as a lower-assurance Actions convenience to be tightened separately.Revocation
The registry JWT is 5-minute, no-refresh (per #264/#273), so loss of Owner status propagates within minutes on the next token exchange, with no revocation list to maintain.
Tests
go test ./internal/api/handlers/v0/auth/...passes, including:403(noread:org) still yields the personal namespace and no error5xxfrom the memberships call fails closed (no token issued) rather than degrading to personal-only403(X-RateLimit-Remaining: 0/Retry-After) fails closed — not mistaken for a missing-scope403gofmt/go vet/golangci-lint(CI-pinned v2.11.4) clean; CI green on all checks.Live verification
Confirmed end-to-end against real
api.github.com(not just mocks) by exercising this branch'sExchangeTokenand decoding thepermissionsclaim of the returned JWT.A dedicated org, Tadasant Test Org (login
Tadasant-Test-Org), is Owned by one account and has a second account joined as a plain member. Both memberships areactive, verified via the same endpoint the fix uses (GET /user/memberships/orgs/{org}):io.github.Tadasant-Test-Org/*granted?admin)io.github.<owner>/*io.github.<member>/*+ the orgs that account actually OwnsThe member case is the meaningful one: that membership is active and returned by
GET /user/memberships/orgs, and is excluded purely because its role ismember, notadmin. Under the previousGET /users/{username}/orgsbehavior (membership = authority, no role), the same active membership would have granted the org namespace. The role filter is also selective rather than blanket-deny — the member account still receives the namespaces of the orgs where it genuinely holds Owner.