| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Radoslav Dimitrov
|
9e762b03c7 |
docs(auth): document fine-grained PAT permission for org publishing (+ pagination-cap test) (#1435)
Follow-up to #1383, surfaced while validating that change end-to-end against staging. ## Why #1383 hardened org-namespace publishing to require Owner (`admin`) role, read via `GET /user/memberships/orgs`. Validating on staging revealed a gap: the docs only mention the classic `read:org` scope, but a **fine-grained PAT** (GitHub's recommended token type) needs a *different* permission to expose org membership. Without it, GitHub returns no org memberships for the token, so the exchange silently grants only the personal namespace — with no hint why org publishing "doesn't work." This is **not** a code bug (the behavior is fail-closed and correct given GitHub's response); it's a documentation/UX gap. ## What - **Docs** (`authentication.mdx`, `github-actions.mdx`): spell out both token types — - Classic PAT → `read:org` scope - Fine-grained PAT → **Organization permissions → Members → Read-only** (the equivalent), noting a fine-grained PAT is bound to a single resource owner. - Also clarified `read:user` is not required. - **Test** (`github_at_test.go`): cover the `maxOrgMembershipPages` pagination cap — when every page is full, the exchange **fails closed** (no token) rather than returning a possibly-truncated org grant. This path was previously untested. - **Comment** (`github_at.go`): qualify the `X-GitHub-SSO` 403 check as best-effort — we have not confirmed the memberships *list* endpoint emits that header on SSO enforcement (it may just omit the org). The check is safe either way, but the comment no longer overstates it as a guarantee. ## Verification (live, against staging) Confirmed by exchanging real tokens for `rdimitrov` on `staging.registry.modelcontextprotocol.io`: - **Classic `read:org` PAT** across 16 real org memberships → the JWT granted exactly the 8 **admin** orgs + personal namespace, and **zero** of the 8 **member** orgs (incl. `stacklok`, `modelcontextprotocol`) — the core #1383 behavior, working. - **Fine-grained PAT**: without Members:read → personal namespace only (fail-closed); after adding Members:read → the org (Owner) namespace is granted. This is what the new docs describe. No runtime behavior change here beyond the added test; the docs and comment are the substance. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
Tadas Antanavicius
|
2bc37b1999 |
fix(auth): grant org namespace only to org Owners, not all members (#1383)
## 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 from [`GET
/users/{username}/orgs`](https://docs.github.com/en/rest/orgs/orgs#list-organizations-for-a-user),
which:
1. returns only **public** org memberships, and
2. carries **no role** — an org Owner and a brand-new member look
identical.
So membership was silently treated as publishing authority. Anyone who
is (or whose public membership lists them as) a member of `acme` could
publish — and, because publish is a pure namespace-glob with no
per-server ownership check, **overwrite** — any server under
`io.github.acme/*`.
## The fix
Switch to [`GET
/user/memberships/orgs`](https://docs.github.com/en/rest/orgs/members#list-organization-memberships-for-the-authenticated-user),
which returns the **authenticated 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 unchanged** — you always get
`io.github.<your-username>/*`.
- **Org namespaces now require Owner.** GitHub has no org-level
"maintainer" role; org membership is either `admin` (Owner) or `member`.
We grant only on `admin`.
- **`read:org` is the only scope needed.** A minimal token without it
still authenticates and still publishes to your personal namespace; a
genuine missing-scope `403` from 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. A `403`/`429` that 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-powerless `read:org`
token.
The authorization decision stays stateless and delegated to GitHub — no
new storage, no new endpoints, no schema changes — consistent with
[design principle #2](docs/design/design-principles.md) (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.mdx` documents how to
contain that blast radius:
- store the token as a **GitHub Actions Environment secret** (the PAT
example uses `environment:`),
- restrict that environment to the default branch / release tags and
require PR reviews on it, so only reviewed, maintainer-approved code can
reach the token,
- optionally add a **required environment reviewer** for explicit
per-publish approval,
- and avoid the two footguns (`pull_request_target` checkout,
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](https://github.com/modelcontextprotocol/registry/issues/982#issuecomment-3982959559)
([Notion analysis
doc](https://www.notion.so/MCP-Registry-Authorization-Model-Analysis-Issue-982-31736f79f74980bc90c6e53de1c564c5))
is the clearest framing of the problem and the option space, laying out
three broad solutions:
- **Approach A — GitHub App as the authority layer.** Strong model, but
its natural unit of authority is a *repo*, so it needs a repo→server
mapping to authorize a namespace. The safe, flexible version of that
mapping is registry-managed state — which collapses into Approach B.
It's also GitHub-only.
- **Approach B — a registry-managed grants table** (OIDC bootstrap, or
device-flow admin check). The most capable and provider-agnostic option,
and probably where v1 wants to go. But it introduces exactly the
"registry manages an authorization database" layer we wanted to avoid
taking on right now: revocation, sync, audit, migration.
- **Approach C — scoped bearer tokens (npm-style).** Good ergonomics,
but again implies issuance/storage the registry would own.
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:
- **Owners are always in the loop.** Only org Owners can publish org
servers; there's no delegated "this team can publish this server" — that
needs Approach B's state. Reduced flexibility in exchange for zero new
infrastructure.
- **Blast radius within an org is unchanged.** An Owner (or Owner-scoped
CI token) can still overwrite *any* server under the org namespace,
because there's still no per-server publisher ownership. That
hijack-an-existing-server problem is real but **orthogonal** to this fix
and belongs in its own change (record publisher identity per server;
enforce it on update) — a follow-up to take alongside the OIDC hardening
below.
- **OIDC is untouched and remains coarser.** The `github-oidc` path
still grants the whole `io.github.<owner>/*` namespace to any workflow
with `id-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:
- member-role org is **not** granted; only admin orgs are
- a missing-scope `403` (no `read:org`) still yields the personal
namespace and no error
- an admin org on a **later page** of memberships is still granted
(pagination correctness)
- a `5xx` from the memberships call **fails closed** (no token issued)
rather than degrading to personal-only
- a rate-limit `403` (`X-RateLimit-Remaining: 0` / `Retry-After`)
**fails closed** — not mistaken for a missing-scope `403`
- existing org/name-validation tests updated to the new endpoint + role
shape
`gofmt`/`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's `ExchangeToken` and decoding the `permissions`
claim 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 are `active`, verified via the same
endpoint the fix uses (`GET /user/memberships/orgs/{org}`):
| Token (role) | `io.github.Tadasant-Test-Org/*` granted? | Other
patterns granted |
|---|---|---|
| Owner (`admin`) | ✅ yes | personal `io.github.<owner>/*` |
| member | ❌ no | personal `io.github.<member>/*` + the orgs that
account actually Owns |
The member case is the meaningful one: that membership is **active and
returned** by `GET /user/memberships/orgs`, and is excluded purely
because its role is `member`, not `admin`. Under the previous `GET
/users/{username}/orgs` behavior (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.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Radoslav Dimitrov <radoslav@stacklok.com>
|
||
|
Radoslav Dimitrov
|
099a2d702e |
Deduplicate server JSON structs between publisher and registry (#318)
<!-- Provide a brief summary of your changes --> ## Motivation and Context <!-- Why is this change needed? What problem does it solve? --> The following PR eliminates duplicate struct definitions by migrating the publisher tool to use canonical types from pkg/model, creating a single source of truth across the codebase. The publisher tool previously had its own copies of structs that were duplicated, ie. ServerJSON, Package, Repository, VersionDetail, EnvironmentVariable, RuntimeArgument and a few others which was not optimal **Changes in the PR:** - Refactored the structs to single canonical types located at pkg/model - Refactor the implementation to use/build canonical structures directly - Remove all duplicate definitions (5 structs eliminated) - Publisher now uses model.ServerJSON, model.Package, model.Repository, etc. - Renamed some of the structs to better reflect their use case, i.e. ServerJSON instead of ServerDetail - Fixes an issue where the Remote property was missing from the previous publisher copy of the Server **Benefits:** - Single source of truth - Schema changes in one place - Type safety - Compiler catches mismatches - Enhanced validation - Field constraints and bson tags - Zero breaking changes - Same CLI interface and JSON output - Future-proof - New canonical fields automatically available Supersedes: #217 and #240 ## How Has This Been Tested? <!-- Have you tested this in a real application? Which scenarios were tested? --> Locally ## Breaking Changes <!-- Will users need to update their code or configurations? --> No ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [ ] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [ ] My code follows the repository's style guidelines - [ ] New and existing tests pass locally - [ ] I have added appropriate error handling - [ ] I have added or updated documentation as needed ## Additional context <!-- Add any other context, implementation notes, or design decisions --> --------- Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> |
||
adam jones
|
12ab632d32 |
auth: Add GitHub OIDC auth (#278)
Adds support for auth'ing to the registry via [GitHub Actions OIDC](https://docs.github.com/en/actions/concepts/security/openid-connect). Fixes #271 ## Motivation and Context People will probably want to publish to the registry from CI environments. DNS auth can work here fine (https://github.com/modelcontextprotocol/registry/issues/270). But for people who haven't set up DNS, the standard GitHub device flow won't really work here. We could make this nice by supporting GitHub Actions OIDC. ## How Has This Been Tested? Tested locally with a real GitHub OIDC token from a personal repo. ## Breaking Changes None ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply. --> - [x] I have read the [MCP Documentation](https://modelcontextprotocol.io) - [x] My code follows the repository's style guidelines - [x] New and existing tests pass locally - [x] I have added appropriate error handling - [x] I have added or updated documentation as needed |
Renamed from internal/api/handlers/v0/auth/github_test.go (Previous history)