From edfe66dad2795fb15f3439bdacf54e06d3f01861 Mon Sep 17 00:00:00 2001 From: mashkovd Date: Tue, 13 Oct 2026 01:27:16 +0200 Subject: [PATCH 1/5] feat(api): sign in a ZITADEL-only principal (#377 PR-B 2b) In principal mode a ZITADEL identity with no single GitHub login now signs in as its own principal (provisioned on first sign-in) instead of being sent to the link page: the token's sub and pid are the principal id and it carries no groups. Tenant access then comes only from a GitOps member entry naming that principalId (proposal Q5, acceptance criterion 4). Outside principal mode nothing changes. The link store and the principal resolver must agree on the principal; when they do not, the sign-in is a 503 rather than a guess. The end-to-end test covers B6 on the API side: sign-in, token, middleware and role check give a ZITADEL-only member developer on its tenant, removing the entry removes it on the same token, and an unlinked identity with the same email gets nothing. Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE --- internal/api/oauth_upstream.go | 48 +++++++-- internal/api/oauth_upstream_test.go | 145 ++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 7 deletions(-) diff --git a/internal/api/oauth_upstream.go b/internal/api/oauth_upstream.go index 71eebf4d..ea98aaca 100644 --- a/internal/api/oauth_upstream.go +++ b/internal/api/oauth_upstream.go @@ -319,20 +319,39 @@ func (h *Handlers) handleOAuthZitadelCallback(w http.ResponseWriter, r *http.Req } id := auth.Identity{Provider: z.opts.ProviderName, Issuer: idt.Issuer, Subject: idt.Subject, Display: display, Kind: auth.KindHuman} p, gh, err := z.opts.Store.LinkedGitHub(r.Context(), id) - if err != nil { + var login, principalID string + switch { + case err == nil: + // The GitHub callback's tail, for the linked principal's GitHub + // login: same groups, same admin decision, same code. The principal + // is the one the store just found the link on. + login, principalID = gh.Display, p.ID + case h.opts.TenantAuthzMode == TenantAuthzPrincipal && zitadelOnly(p, err): + // Principal mode needs no GitHub login (mctl-api#377, Q5): the + // identity signs in as its principal, provisioned on first sight, + // and gets exactly the GitOps membership keyed by that principal. + var stopped string + if principalID, stopped = h.signinPrincipal(w, r, id); stopped != "" { + result = stopped + return + } + if p != nil && principalID != p.ID { + // The resolver and the store disagree on whose identity this + // is; minting for either would be a guess. + slog.Error("oauth: ZITADEL identity resolved to another principal", "store", p.ID, "resolver", principalID) + result = zitadelStoreUnavailable(w, errors.New("principal resolution disagrees with the link store")) + return + } + default: result = h.refuseZitadelSignin(w, display, p, err) return } - // From here on this is the GitHub callback's tail, for the linked - // principal's GitHub login: same groups, same admin decision, same code. - // The principal is the one the store just found the link on. - login := gh.Display var groups []string if h.opts.TenantAuthzMode != TenantAuthzPrincipal { groups = o.ResolveGroups(login) } - mctlCode, err := o.IssueCode(r.Context(), login, p.ID, pending.ClientID, pending.RedirectURI, pending.CodeChallenge, groups) + mctlCode, err := o.IssueCode(r.Context(), login, principalID, pending.ClientID, pending.RedirectURI, pending.CodeChallenge, groups) if err != nil { slog.Error("failed to issue auth code", "error", err) result = "internal_error" @@ -352,10 +371,25 @@ func (h *Handlers) handleOAuthZitadelCallback(w http.ResponseWriter, r *http.Req } target.RawQuery = cq.Encode() result = "issued" - slog.Info("oauth: ZITADEL sign-in", "principal_id", p.ID, "login", login, "zitadel_subject", idt.Subject) + slog.Info("oauth: ZITADEL sign-in", "principal_id", principalID, "login", login, "zitadel_subject", idt.Subject) http.Redirect(w, r, target.String(), http.StatusFound) } +// zitadelOnly reports whether a LinkedGitHub refusal only says the +// identity has no single GitHub login to sign in as: not linked to any +// principal yet, or a principal without exactly one live GitHub login. +// Principal mode signs those in as the principal. A refusal that names no +// principal where it must is a failed read, never this. +func zitadelOnly(p *principals.Principal, err error) bool { + switch { + case errors.Is(err, principals.ErrNotFound): + return true + case errors.Is(err, principals.ErrNoGitHubIdentity), errors.Is(err, principals.ErrGitHubLoginUnknown), errors.Is(err, principals.ErrAmbiguousGitHubIdentity): + return p != nil + } + return false +} + // refuseZitadelSignin answers a ZITADEL identity that cannot sign in and // returns the metric result. Only the store's documented refusals mean // "not linked"; any other error is a failed read and answers 503. diff --git a/internal/api/oauth_upstream_test.go b/internal/api/oauth_upstream_test.go index d2673315..05584be2 100644 --- a/internal/api/oauth_upstream_test.go +++ b/internal/api/oauth_upstream_test.go @@ -34,6 +34,7 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" "github.com/mctlhq/mctl-api/internal/auth" + "github.com/mctlhq/mctl-api/internal/operations" "github.com/mctlhq/mctl-api/internal/principals" ) @@ -581,3 +582,147 @@ func TestFlowStoreFailureIsAServerErrorNotABadState(t *testing.T) { } } } + +// ─── principal mode (mctl-api#377, Q5) ────────────────────────────────── + +// newPrincipalModeHarness is newUpstreamHarness in principal mode, with +// pr resolving (and provisioning) the ZITADEL identity's principal. +func newPrincipalModeHarness(t *testing.T, store *fakeLinkedStore, pr auth.PrincipalResolver) *upstreamHarness { + t.Helper() + h := newUpstreamHarness(t, OAuthUpstreamZitadel, store) + h.oauth.PrincipalMode = true + zopts := &OAuthZitadelOptions{ProviderName: "zitadel", Issuer: h.idp.srv.URL, ClientID: testOAuthZClientID, ClientSecret: testOAuthZClientSecret, Store: store} + h.router = NewRouter(Options{OAuthServer: h.oauth, OAuthUpstream: OAuthUpstreamZitadel, OAuthZitadel: zopts, + TenantAuthzMode: TenantAuthzPrincipal, Principals: pr}) + return h +} + +// A ZITADEL identity with no single GitHub login signs in as its own +// principal in principal mode: the token's sub and pid are the principal +// id, and no groups are minted (they are decided per request). +func TestZitadelSigninPrincipalMode(t *testing.T) { + const ( + linked = "prn_01JTESTAAAAAAAAAAAAAAAAAAA" + own = "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ" + other = "prn_01JBBBBBBBBBBBBBBBBBBBBBBB" + ) + ownP := &principals.Principal{ID: own, Kind: auth.KindHuman, Status: principals.StatusActive} + cases := map[string]struct { + store *fakeLinkedStore + resolver auth.PrincipalResolver + code int // 0: a code is issued + sub, pid string // the issued token's claims + result string // oauth_upstream_signins_total result + }{ + "linked to a GitHub login": {linkedTo("mashkovd"), signinResolver{prn: linked}, 0, "mashkovd", linked, "issued"}, + "never seen: provisioned": {&fakeLinkedStore{err: principals.ErrNotFound}, signinResolver{prn: own}, 0, own, own, "issued"}, + "own principal, no GitHub": {&fakeLinkedStore{p: ownP, err: principals.ErrNoGitHubIdentity}, signinResolver{prn: own}, 0, own, own, "issued"}, + "GitHub login unknown": {&fakeLinkedStore{p: ownP, err: principals.ErrGitHubLoginUnknown}, signinResolver{prn: own}, 0, own, own, "issued"}, + "two GitHub identities": {&fakeLinkedStore{p: ownP, err: principals.ErrAmbiguousGitHubIdentity}, signinResolver{prn: own}, 0, own, own, "issued"}, + "resolver disagrees": {&fakeLinkedStore{p: ownP, err: principals.ErrNoGitHubIdentity}, signinResolver{prn: other}, http.StatusServiceUnavailable, "", "", "store_unavailable"}, + "no resolver": {&fakeLinkedStore{err: principals.ErrNotFound}, nil, http.StatusServiceUnavailable, "", "", "principal_unavailable"}, + "resolver down": {&fakeLinkedStore{err: principals.ErrNotFound}, signinResolver{err: errors.New("store down")}, http.StatusServiceUnavailable, "", "", "principal_unavailable"}, + "principal disabled": {&fakeLinkedStore{err: principals.ErrNotFound}, signinResolver{err: auth.ErrPrincipalDisabled}, http.StatusForbidden, "", "", "principal_refused"}, + "identity revoked": {&fakeLinkedStore{err: auth.ErrIdentityRefused}, signinResolver{prn: own}, http.StatusForbidden, "", "", "refused"}, + "store down": {&fakeLinkedStore{err: errors.New("db down")}, signinResolver{prn: own}, http.StatusServiceUnavailable, "", "", "store_unavailable"}, + // A refusal that must name a principal and does not is a failed read. + "refusal without a principal": {&fakeLinkedStore{err: principals.ErrNoGitHubIdentity}, signinResolver{prn: own}, http.StatusServiceUnavailable, "", "", "store_unavailable"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + before := testutil.ToFloat64(oauthUpstreamSignins.WithLabelValues(auth.UpstreamZitadel, tc.result)) + h := newPrincipalModeHarness(t, tc.store, tc.resolver) + rec := h.callback(h.toZitadel(h.get(authorizeQuery("")))) + if n := testutil.ToFloat64(oauthUpstreamSignins.WithLabelValues(auth.UpstreamZitadel, tc.result)) - before; n != 1 { + t.Errorf("%s +%v, want +1", tc.result, n) + } + if tc.code != 0 { + if rec.Code != tc.code || rec.Header().Get("Location") != "" { + t.Fatalf("callback = %d Location=%q, want %d and no code", rec.Code, rec.Header().Get("Location"), tc.code) + } + return + } + loc, _ := url.Parse(rec.Header().Get("Location")) + if rec.Code != http.StatusFound || loc.Query().Get("code") == "" { + t.Fatalf("callback = %d %q, want a code: %s", rec.Code, loc, rec.Body.String()) + } + access, _, err := h.oauth.ExchangeCode(context.Background(), loc.Query().Get("code"), testVerifier, "client", testClientRedirect) + if err != nil { + t.Fatal(err) + } + claims := jwtClaims(t, access) + if claims["sub"] != tc.sub || claims["pid"] != tc.pid { + t.Fatalf("sub=%v pid=%v, want %s / %s", claims["sub"], claims["pid"], tc.sub, tc.pid) + } + if g, _ := claims["groups"].([]any); len(g) != 0 { + t.Fatalf("groups %v minted in principal mode", g) + } + }) + } +} + +// activePrincipals resolves nothing and reports every principal active: the +// middleware's per-request check of a token's principal. +type activePrincipals struct{} + +func (activePrincipals) ResolvePrincipal(context.Context, auth.Identity) (string, error) { + return "", errors.New("not a resolver") +} + +func (activePrincipals) CheckPrincipal(context.Context, string) error { return nil } + +// B6 end to end: a ZITADEL-only human signs in, and the token it gets is a +// developer of the tenant whose GitOps entry names its principal and no +// login. Removing the entry removes the access on the same token; an +// unlinked identity with the same email is another principal and gets +// nothing. +func TestZitadelOnlyPrincipal_SigninToTenantRole(t *testing.T) { + t.Setenv("AUTH_REQUIRED", "true") + const unlinked = "prn_01JQQQQQQQQQQQQQQQQQQQQQQQ" + signIn := func(prn string) (*upstreamHarness, string) { + h := newPrincipalModeHarness(t, &fakeLinkedStore{err: principals.ErrNotFound}, signinResolver{prn: prn}) + loc, _ := url.Parse(h.callback(h.toZitadel(h.get(authorizeQuery("")))).Header().Get("Location")) + access, _, err := h.oauth.ExchangeCode(context.Background(), loc.Query().Get("code"), testVerifier, "client", testClientRedirect) + if err != nil { + t.Fatal(err) + } + return h, access + } + listed := tenantsByPrincipal{zitadelOnlyPRN: {"commerce-proof"}} + roles := &principalRoles{byPRN: map[string]map[string][][]string{zitadelOnlyPRN: {"commerce-proof": {{"developer"}}}}} + handlers := &Handlers{opts: Options{TenantAuthzMode: TenantAuthzPrincipal, PrincipalMembership: roles}} + // developer reports whether token is a developer of commerce-proof, as + // the middleware and the role check decide it in principal mode. + developer := func(h *upstreamHarness, token string) bool { + t.Helper() + var user *auth.User + mw := auth.Middleware(nil, nil, nil, h.oauth, auth.WithPrincipalResolver(activePrincipals{}), + auth.WithPrincipalAuthz(&auth.PrincipalAuthz{Members: listed, Admins: map[string]bool{}})) + req := httptest.NewRequest(http.MethodGet, "/api/v1/whoami", nil) + req.Header.Set("Authorization", "Bearer "+token) + rec := httptest.NewRecorder() + mw(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { user = auth.UserFromContext(r.Context()) })).ServeHTTP(rec, req) + if user == nil { + t.Fatalf("token refused: %d %s", rec.Code, rec.Body.String()) + } + access := user.HasTenantAccess("commerce-proof") + role := !handlers.checkTenantRole(user, "commerce-proof", operations.RoleDeveloper, "test").refused() + if access != role { + t.Fatalf("tenant access %v but developer role %v", access, role) + } + return access + } + + h, member := signIn(zitadelOnlyPRN) + if !developer(h, member) { + t.Fatal("ZITADEL-only member was refused") + } + if hu, other := signIn(unlinked); developer(hu, other) { + t.Fatal("unlinked same-email identity was let in") + } + delete(listed, zitadelOnlyPRN) + delete(roles.byPRN, zitadelOnlyPRN) + if developer(h, member) { + t.Fatal("access survived removal of the GitOps entry") + } +} From 69a77dd926087b352b0a3324d21db2e061383cc5 Mon Sep 17 00:00:00 2001 From: mashkovd Date: Tue, 13 Oct 2026 01:27:16 +0200 Subject: [PATCH 2/5] docs: document ZITADEL-only sign-in and the whoami admin contract The sign-in table gets the principal-mode answers. /whoami reports admins in namespaces but never in groups for a principal-mode admin, so clients must read isAdmin (deferred P3 from #595). Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE --- README.md | 2 +- docs/principals.md | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index d0f4c891..a74c0114 100644 --- a/README.md +++ b/README.md @@ -134,7 +134,7 @@ docker run -p 8080:8080 mctl-api | `OAUTH_GITHUB_CLIENT_ID` | GitHub OAuth app client ID | — | No | | `OAUTH_GITHUB_CLIENT_SECRET` | GitHub OAuth app client secret | — | No | | `OAUTH_JWT_SECRET` | JWT signing secret for OAuth tokens | — | No | -| `OAUTH_UPSTREAM` | Where `/oauth/authorize` signs people in (mctl-api#467, see [docs/principals.md](docs/principals.md)): `github`, `zitadel` (as the linked principal's GitHub login), or `both` (a chooser, ZITADEL first). `zitadel`/`both` refuse startup without `OAUTH_JWT_SECRET`, both `OAUTH_ZITADEL_CLIENT_*` and the provider entry, and `both` also without both `OAUTH_GITHUB_CLIENT_*`. An unknown value refuses startup | `github` | No | +| `OAUTH_UPSTREAM` | Where `/oauth/authorize` signs people in (mctl-api#467, see [docs/principals.md](docs/principals.md)): `github`, `zitadel` (as the linked principal's GitHub login; under `TENANT_AUTHZ_MODE=principal` an identity with no GitHub login signs in as its own principal), or `both` (a chooser, ZITADEL first). `zitadel`/`both` refuse startup without `OAUTH_JWT_SECRET`, both `OAUTH_ZITADEL_CLIENT_*` and the provider entry, and `both` also without both `OAUTH_GITHUB_CLIENT_*`. An unknown value refuses startup | `github` | No | | `OAUTH_ZITADEL_CLIENT_ID` / `OAUTH_ZITADEL_CLIENT_SECRET` | Confidential ZITADEL client of the ZITADEL upstream (redirect `/oauth/zitadel/callback`); not the link client. Helm: `oauthZitadelSecret` | — | No | | `OAUTH_ZITADEL_PROVIDER` | `MCTL_OIDC_PROVIDERS` entry whose issuer the ZITADEL upstream verifies against; must be the one linking writes (`ZITADEL_LINK_PROVIDER`) | `zitadel` | No | | `OAUTH_ALLOWED_REDIRECT_URIS` | Comma-separated redirect URIs accepted at `/oauth/authorize` and by Dynamic Client Registration (`POST /oauth/register`). An entry is an **exact** string compare unless it ends in `/*`, which makes it a prefix match; prefer exact entries (the Cloudflare MCP portal's `https://mcp.mctl.ai/servers-callback` and its dashboard `…/mcp-server/oauth-callback/api` callback are listed exactly, never as a `dash.cloudflare.com/*` prefix). Loopback `http://` callbacks are always accepted (RFC 8252). A registration naming any other URI gets `400 invalid_redirect_uri` | glama, smithery, `chatgpt.com/connector/oauth/*` | No | diff --git a/docs/principals.md b/docs/principals.md index 75d3e6b3..d13fd4aa 100644 --- a/docs/principals.md +++ b/docs/principals.md @@ -182,10 +182,11 @@ read-only (`Store.LinkedGitHub`): | Lookup | Answer | |---|---| | linked to P, P has one live GitHub identity with a login | code for that login and P (`pid`): the same code, groups and admin decision (`ADMIN_USERS`) a GitHub sign-in of P yields; in `principal` mode no groups are minted | -| no row, or a principal of its own with no GitHub identity | 403 page linking to `/identity/link/zitadel`; nothing issued or provisioned | +| no row, or a principal of its own with no GitHub identity | 403 page linking to `/identity/link/zitadel`; nothing issued or provisioned. In `principal` mode: a code for the identity's own principal (provisioned on first sign-in), `sub` = `pid` = its principal id, with no groups; it reaches a tenant only once a GitOps member entry names that `principalId` | | identity revoked, or P disabled | 403 | -| P's GitHub login unknown (taken by another account) or two live GitHub identities | 409, nothing guessed | +| P's GitHub login unknown (taken by another account) or two live GitHub identities | 409, nothing guessed. In `principal` mode: a code for P with no login, as in the row above | | store error, or no principal store | 503, never "not linked" | +| (`principal` mode) the principal resolver answers a different principal than the link store | 503 `store_unavailable`; nothing issued | A pending authorization records its upstream: a GitHub callback cannot complete a ZITADEL authorization or the reverse, and an authorization whose @@ -193,6 +194,7 @@ upstream the current mode does not allow (one pending across a rollback) gets a 400. Sign-ins are counted in `oauth_upstream_signins_total{upstream,result}`. For `upstream="zitadel"`, `result` is `issued`, `unlinked`, `refused`, `store_unavailable`, +`principal_refused`, `principal_unavailable` (`principal` mode, as for GitHub), `invalid_token`, `upstream_error` (ZITADEL redirected back with an error), `upstream_unreachable` (discovery failed), `exchange_failed`, `bad_request` or `internal_error`, so "ZITADEL is down" and "mctl is @@ -348,6 +350,9 @@ caller's authorization; it never merges with the legacy one: an unresolved principal is 503. - a platform-skill role binding on `admins` means platform admin, never a tenant named `admins`. +- `/whoami` reports `admins` in `namespaces` for a platform admin, but + never in `groups`: admin is decided by principal, not carried as a + group. Clients must read `isAdmin`, not `groups.includes("admins")`. ### Local tokens carry the principal (`pid`) From c90fe4f1f7aa09dd33d256bab10f05724d722e47 Mon Sep 17 00:00:00 2001 From: mashkovd Date: Tue, 13 Oct 2026 01:36:05 +0200 Subject: [PATCH 3/5] fix(auth): confine the token-principal claim, finish switch docs Review P3s from #598 and #600. checkProviderContract now refuses a token-principal claim from any provider but the local OAuth one, symmetric with the reserved principal namespace. The switch criterion gains the zero-mismatch condition and the pid-coverage check, and the Metrics table lists the new counter and sign-in results. Two stale or misplaced comments are corrected. Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE --- docs/principals.md | 10 +++++++++ internal/api/oauth_upstream.go | 5 ++++- internal/auth/federation.go | 5 +++++ internal/auth/oauth_server.go | 6 +++--- internal/auth/token_principal_test.go | 31 ++++++++++++++++----------- 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/docs/principals.md b/docs/principals.md index d13fd4aa..8f7233de 100644 --- a/docs/principals.md +++ b/docs/principals.md @@ -409,6 +409,8 @@ the switch. | `tenant_member_entry_unkeyed{tenant}` | member entries with no `principalId` | | `tenant_principal_keys_invalid{tenant}` | 1 when a tenant's keys are invalid or the tenant cannot be read | | `tenant_principal_keys_scan_ok` | 1 when the last scan could list the tenants; the two gauges above are cleared when it is 0 | +| `oauth_token_principal_mismatch_total` | local tokens whose `pid` names a different principal than the one resolved from the login (outside `principal` mode, where it is still resolved) | +| `oauth_upstream_signins_total{result}` | also `principal_refused` (disabled or refused principal, 403) and `principal_unavailable` (could not be resolved, 503 in `principal` mode), for both upstreams | An `unresolved` comparison is never counted as a `match` or a `diff`: a decision that could not be computed is not evidence either way. The one @@ -431,6 +433,14 @@ increasing for human callers; `sum(tenant_member_entry_unkeyed) == 0`, ZITADEL MCP OAuth, ZITADEL bearer). A window in which a path saw no traffic does not count for that path. +Over the same window, `oauth_token_principal_mismatch_total` must stay at +zero. Before the flip, check pid coverage too: a live session minted +before the release that records `pid` is refused at the switch and its +holder must sign in again. A refresh carries an empty `principal_id` +forward, so such sessions end only by expiry or a new sign-in; count the +live refresh rows with an empty `principal_id` and decide whether that +many re-sign-ins are acceptable on switch day. + ### Backfill plan `GET /api/v1/admin/tenant-members/principal-plan` (platform admin acting diff --git a/internal/api/oauth_upstream.go b/internal/api/oauth_upstream.go index ea98aaca..07f0352e 100644 --- a/internal/api/oauth_upstream.go +++ b/internal/api/oauth_upstream.go @@ -37,7 +37,10 @@ import ( // the original upstream. ZITADEL signs the person in as the GitHub login of // the principal their ZITADEL identity is linked to (#435), so the code, // the tokens and the groups behind them are exactly what a GitHub sign-in of -// the same person yields. Nothing is linked or provisioned here. +// the same person yields. Nothing is linked here, and outside principal +// mode nothing is provisioned either. In principal mode (mctl-api#377) an +// identity with no single GitHub login signs in as its own principal, +// which the resolver provisions on first sight. // OAuthUpstreamMode is OAUTH_UPSTREAM. type OAuthUpstreamMode string diff --git a/internal/auth/federation.go b/internal/auth/federation.go index 8849df3f..8d74e951 100644 --- a/internal/auth/federation.go +++ b/internal/auth/federation.go @@ -364,6 +364,11 @@ func checkProviderContract(p Provider, v *Verified) error { if v.Identity.Provider != p.Name() && !principalTokenOf(p, v) { return fmt.Errorf("%w: provider %q returned identity in namespace %q", ErrProviderContractViolation, p.Name(), v.Identity.Provider) } + // The token-principal claim is trusted as checked-at-mint, so only the + // provider that mints it may carry it, as with the namespace above. + if _, local := p.(*localOAuthProvider); v.Claims.TokenPrincipal != "" && !local { + return fmt.Errorf("%w: provider %q returned a token principal", ErrProviderContractViolation, p.Name()) + } if v.Identity.Subject == "" && !v.Identity.GitHubLoginOnly() { return fmt.Errorf("%w: provider %q returned an empty subject", ErrProviderContractViolation, p.Name()) } diff --git a/internal/auth/oauth_server.go b/internal/auth/oauth_server.go index 7b24fe90..44f063f5 100644 --- a/internal/auth/oauth_server.go +++ b/internal/auth/oauth_server.go @@ -1529,6 +1529,9 @@ func (s *OAuthServer) RefreshAccessToken(refreshToken, clientID string) (string, slog.Error("oauth: refresh store unexpected error", "error", err) return "", "", fmt.Errorf("oauth: %w", ErrServerError) } + if err := s.refreshablePrincipal(sess.PrincipalID); err != nil { + return "", "", err + } // The row rotate() just wrote still carries storedGroups verbatim -- // refreshstore.Store.Rotate has no parameter for new groups, and // adding one would change the interface, the PostgresStore schema and @@ -1536,9 +1539,6 @@ func (s *OAuthServer) RefreshAccessToken(refreshToken, clientID string) (string, // That snapshot's staleness is no longer load-bearing because // groupsForSession re-resolves it here, bounded by the degraded-grace // fallback when resolution fails. - if err := s.refreshablePrincipal(sess.PrincipalID); err != nil { - return "", "", err - } var groups []string if !s.PrincipalMode { groups = s.groupsForSession(sess.Login, sess.Groups) diff --git a/internal/auth/token_principal_test.go b/internal/auth/token_principal_test.go index 2609bd52..9f8f8073 100644 --- a/internal/auth/token_principal_test.go +++ b/internal/auth/token_principal_test.go @@ -215,18 +215,25 @@ func TestRegistryRefusesThePrincipalNamespace(t *testing.T) { // Only the local-OAuth provider may return the principal namespace; any // other provider doing so could name any principal it liked. func TestOtherProviderCannotReturnThePrincipalNamespace(t *testing.T) { - bad := &fakeProvider{ - name: "acme", issuer: "https://issuer", claims: matchIssuer("https://issuer"), - verify: func(context.Context, string) (*Verified, error) { - return &Verified{Identity: Identity{Provider: ProviderPrincipal, Subject: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ", Kind: KindHuman}}, nil - }, - } - r, err := NewRegistry(nil, []Provider{bad}, nil) - if err != nil { - t.Fatal(err) - } - if _, err := r.Verify(context.Background(), fakeJWT("https://issuer")); !errors.Is(err, ErrProviderContractViolation) { - t.Fatalf("expected ErrProviderContractViolation, got %v", err) + for name, v := range map[string]*Verified{ + "principal namespace": {Identity: Identity{Provider: ProviderPrincipal, Subject: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ", Kind: KindHuman}}, + // Nor may it carry the token-principal claim in its own namespace. + "token principal claim": {Identity: Identity{Provider: "acme", Subject: "s-1", Kind: KindHuman}, + Claims: Claims{TokenPrincipal: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ"}}, + } { + t.Run(name, func(t *testing.T) { + bad := &fakeProvider{ + name: "acme", issuer: "https://issuer", claims: matchIssuer("https://issuer"), + verify: func(context.Context, string) (*Verified, error) { return v, nil }, + } + r, err := NewRegistry(nil, []Provider{bad}, nil) + if err != nil { + t.Fatal(err) + } + if _, err := r.Verify(context.Background(), fakeJWT("https://issuer")); !errors.Is(err, ErrProviderContractViolation) { + t.Fatalf("expected ErrProviderContractViolation, got %v", err) + } + }) } } From d58da5531899cb42dc036688cd65b804d785d597 Mon Sep 17 00:00:00 2001 From: mashkovd Date: Tue, 13 Oct 2026 01:47:57 +0200 Subject: [PATCH 4/5] fix(auth): refuse login-less refresh outside principal mode Review round on #600: - refreshablePrincipal also refuses a session with no login when not in principal mode. ValidateJWT refuses its access tokens there, so a refresh after a rollback from principal mode would only mint tokens that answer 401 and leave the client looping instead of signing in. - signinPrincipal treats a resolver answering no error but no valid principal id as a failed lookup (503 principal_unavailable in principal mode) rather than going on with an empty principal. - docs: /whoami id is the prn for a ZITADEL-only principal. - lint: mark the test fixture principal id as not a credential (G101). Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE --- docs/principals.md | 4 ++++ internal/api/oauth_signin_principal.go | 6 ++++++ internal/api/oauth_signin_principal_test.go | 3 +++ internal/auth/oauth_server.go | 21 ++++++++++++--------- internal/auth/token_principal_test.go | 11 ++++++++++- 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/docs/principals.md b/docs/principals.md index 8f7233de..d5409a97 100644 --- a/docs/principals.md +++ b/docs/principals.md @@ -353,6 +353,10 @@ caller's authorization; it never merges with the legacy one: - `/whoami` reports `admins` in `namespaces` for a platform admin, but never in `groups`: admin is decided by principal, not carried as a group. Clients must read `isAdmin`, not `groups.includes("admins")`. +- `/whoami` `id` is the GitHub login for a GitHub-linked caller, but the + principal id (`prn_…`) for a ZITADEL-only principal, which has no login. + Clients must not treat `id` as a GitHub login (link to a profile, look it + up in a repo's collaborators). ### Local tokens carry the principal (`pid`) diff --git a/internal/api/oauth_signin_principal.go b/internal/api/oauth_signin_principal.go index cb63d213..919bf7ab 100644 --- a/internal/api/oauth_signin_principal.go +++ b/internal/api/oauth_signin_principal.go @@ -16,6 +16,7 @@ package api import ( "errors" + "fmt" "log/slog" "net/http" "strconv" @@ -55,6 +56,11 @@ func (h *Handlers) signinPrincipal(w http.ResponseWriter, r *http.Request, id au return "", "" } prn, err := h.opts.Principals.ResolvePrincipal(r.Context(), id) + if err == nil && !auth.ValidPrincipalID(prn) { + // A resolver answering with no usable principal and no error has + // not resolved one: treat it like any other failed lookup. + err = fmt.Errorf("resolver returned %q, not a principal id", prn) + } switch { case err == nil: return prn, "" diff --git a/internal/api/oauth_signin_principal_test.go b/internal/api/oauth_signin_principal_test.go index a3876ee5..8db4edeb 100644 --- a/internal/api/oauth_signin_principal_test.go +++ b/internal/api/oauth_signin_principal_test.go @@ -58,6 +58,9 @@ func TestSigninPrincipal(t *testing.T) { {"refused, principal mode", signinResolver{err: fmt.Errorf("x: %w", auth.ErrIdentityRefused)}, true, "", http.StatusForbidden}, {"unresolved", signinResolver{err: unresolved}, false, "", 0}, {"unresolved, principal mode", signinResolver{err: unresolved}, true, "", http.StatusServiceUnavailable}, + {"empty answer", signinResolver{}, false, "", 0}, + {"empty answer, principal mode", signinResolver{}, true, "", http.StatusServiceUnavailable}, + {"malformed answer, principal mode", signinResolver{prn: "alice"}, true, "", http.StatusServiceUnavailable}, {"no resolver", nil, false, "", 0}, {"no resolver, principal mode", nil, true, "", http.StatusServiceUnavailable}, } diff --git a/internal/auth/oauth_server.go b/internal/auth/oauth_server.go index 44f063f5..6b12e3cb 100644 --- a/internal/auth/oauth_server.go +++ b/internal/auth/oauth_server.go @@ -1529,7 +1529,7 @@ func (s *OAuthServer) RefreshAccessToken(refreshToken, clientID string) (string, slog.Error("oauth: refresh store unexpected error", "error", err) return "", "", fmt.Errorf("oauth: %w", ErrServerError) } - if err := s.refreshablePrincipal(sess.PrincipalID); err != nil { + if err := s.refreshablePrincipal(sess.Login, sess.PrincipalID); err != nil { return "", "", err } // The row rotate() just wrote still carries storedGroups verbatim -- @@ -1562,7 +1562,7 @@ func (s *OAuthServer) RefreshAccessToken(refreshToken, clientID string) (string, // resolution. A degraded or fail-closed fallback must not overwrite the // snapshot: that would leave the next outage nothing to fall back on // (the store path never overwrites it, since Rotate carries it forward). - if err := s.refreshablePrincipal(entry.PrincipalID); err != nil { + if err := s.refreshablePrincipal(entry.Login, entry.PrincipalID); err != nil { return "", "", err } var groups []string @@ -1586,13 +1586,16 @@ func (s *OAuthServer) RefreshAccessToken(refreshToken, clientID string) (string, return accessToken, newRefreshToken, nil } -// refreshablePrincipal refuses, in principal mode, to refresh a session -// with no recorded principal (minted before mctl-api#377 recorded one): its -// access tokens would be refused anyway, and re-deriving the principal from -// the login is what principal mode removes. The holder signs in again. The -// error reads as an invalid grant, which is what tells a client to do that. -func (s *OAuthServer) refreshablePrincipal(principalID string) error { - if s.PrincipalMode && principalID == "" { +// refreshablePrincipal refuses to refresh a session whose access tokens +// ValidateJWT would refuse anyway: in principal mode, one with no recorded +// principal (minted before mctl-api#377 recorded one), since re-deriving the +// principal from the login is what principal mode removes; outside it, one +// with no login (a ZITADEL-only principal, after a rollback from principal +// mode). Refreshing either would only mint tokens that answer 401, which a +// client retries in a loop. The error reads as an invalid grant, which is +// what tells a client to sign in again. +func (s *OAuthServer) refreshablePrincipal(login, principalID string) error { + if (s.PrincipalMode && principalID == "") || (!s.PrincipalMode && login == "") { return errors.New("invalid or expired refresh token") } return nil diff --git a/internal/auth/token_principal_test.go b/internal/auth/token_principal_test.go index 9f8f8073..aa9487fb 100644 --- a/internal/auth/token_principal_test.go +++ b/internal/auth/token_principal_test.go @@ -219,7 +219,7 @@ func TestOtherProviderCannotReturnThePrincipalNamespace(t *testing.T) { "principal namespace": {Identity: Identity{Provider: ProviderPrincipal, Subject: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ", Kind: KindHuman}}, // Nor may it carry the token-principal claim in its own namespace. "token principal claim": {Identity: Identity{Provider: "acme", Subject: "s-1", Kind: KindHuman}, - Claims: Claims{TokenPrincipal: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ"}}, + Claims: Claims{TokenPrincipal: "prn_01JZZZZZZZZZZZZZZZZZZZZZZZ"}}, //nolint:gosec // G101: a principal id, not a credential } { t.Run(name, func(t *testing.T) { bad := &fakeProvider{ @@ -253,6 +253,15 @@ func TestRefresh_CarriesThePrincipal(t *testing.T) { if err != nil { t.Fatal(err) } + if tc.login == "" && !principalMode { + // Outside principal mode its access tokens would be + // refused, so the session is not refreshed either: the + // client signs in again rather than looping on 401s. + if _, _, err := o.RefreshAccessToken(rt, "c"); err == nil { + t.Fatal("login-less session refreshed outside principal mode") + } + continue + } // Twice: the successor must carry the principal too. for hop := range 2 { var at string From 901fa0859d13546760a2c26d801113f596ebc1e0 Mon Sep 17 00:00:00 2001 From: mashkovd Date: Tue, 13 Oct 2026 01:54:59 +0200 Subject: [PATCH 5/5] docs: fix the pid-coverage wording, add the rollback refresh rule Claude-Session: https://claude.ai/code/session_0151SmHa5Uwfv9sm6G2XwDnE --- docs/principals.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/docs/principals.md b/docs/principals.md index d5409a97..b6f33b95 100644 --- a/docs/principals.md +++ b/docs/principals.md @@ -384,7 +384,8 @@ rotation carries it forward. - In `principal` mode a token without a `pid` is refused (401), a principal-only token must have `sub == pid`, and a refresh of a session with no recorded principal answers `invalid_grant`. Outside it, a - principal-only token is refused. + principal-only token is refused (401), and a refresh of a session with + no login (a ZITADEL-only principal) answers `invalid_grant`. At the switch, sessions minted before this release carry no `pid`, so their holders sign in again. With the persistent refresh store the refresh @@ -392,7 +393,10 @@ token is rotated before its missing `pid` is refused, so a client that retries it gets `invalid_grant` and, once the rotation grace window has passed, trips reuse detection: the family is revoked and `oauth refresh token reuse detected; family revoked` is logged as a warning. On switch day -those warnings are pre-#377 sessions, not stolen tokens. +those warnings are pre-#377 sessions, not stolen tokens. A rollback out +of `principal` mode does the same to ZITADEL-only principals: their +sessions have no login, so their refresh is refused, and the same +reuse-detection warnings follow. A disabled principal, merged-away ones included (a merge disables `from`), is refused on every request. An **unlink is not**: the principal stays @@ -439,7 +443,7 @@ does not count for that path. Over the same window, `oauth_token_principal_mismatch_total` must stay at zero. Before the flip, check pid coverage too: a live session minted -before the release that records `pid` is refused at the switch and its +before the release that records no `pid` is refused at the switch and its holder must sign in again. A refresh carries an empty `principal_id` forward, so such sessions end only by expiry or a new sign-in; count the live refresh rows with an empty `principal_id` and decide whether that