Skip to content

feat(mctl-api): sign in through ZITADEL only (OAUTH_UPSTREAM=zitadel) - #1694

Merged
mashkovd merged 2 commits into
mainfrom
feat/mctl-api-oauth-upstream-zitadel
Oct 9, 2026
Merged

mashkovd merged 2 commits into
mainfrom
feat/mctl-api-oauth-upstream-zitadel

Conversation

@mashkovd

@mashkovd mashkovd commented Oct 6, 2026

Copy link
Copy Markdown
Member

Why

Phase 2 of #1500. OAUTH_UPSTREAM has been both since #1681 as a canary. The canary was proven end to end on 2026-10-06 18:57Z: the owner reconnected the claude.ai connector to https://api.mctl.ai/mcp through ZITADEL and whoami returned the same principal, admin flag and teams as on the GitHub path. The owner asked to drop both.

What

One value in bootstrap/templates/mctl-platform/mctl-api.yaml: OAUTH_UPSTREAM: "both" becomes "zitadel", and the comment above it describes the new mode.

What changes for people

  • New sign-ins through /oauth/authorize go to auth.mctl.ai directly. The chooser page and the GitHub option are gone.
  • A person whose ZITADEL identity is not linked gets a 403 page with a "Link your ZITADEL identity" button. The link flow (/identity/link/zitadel) is not gated by this setting: its GitHub leg is dispatched before the upstream check in handleOAuthGitHubCallback. They sign in with GitHub and ZITADEL once, then start the connection again.
  • Existing sessions keep working. The refresh grant is served from mctl-api's own store and never goes upstream.
  • Bearer callers are unaffected (the CLI with a GitHub token, service principals): the setting gates /oauth/authorize only.

Who is not linked yet

obukhov23 and greirk (tenant erpact) have not confirmed linking. Linking them without their participation is not possible today: a ZITADEL sign-in by an unlinked person creates no principal, so the admin merge endpoint has nothing to merge, and there is no admin "attach identity" endpoint. They link themselves on first use through the page above.

In the hours since the current pod started (2026-10-06 06:52Z) oauth_upstream_signins_total shows one completed sign-in, through ZITADEL; none through GitHub.

Rollback

Set the value back to both. No data is changed by either direction.

After merge

  • GET https://api.mctl.ai/oauth/authorize?... redirects to auth.mctl.ai, with no chooser.
  • ...&upstream=github no longer reaches github.com.
  • /identity/link/zitadel still starts the link flow.
  • The owner's existing connector session keeps working.

Draft until the owner confirms the merge.

Refs #1500

🤖 Generated with Claude Code

The canary on `both` was proven end to end on 2026-10-06: the owner
reconnected an MCP connector through ZITADEL and got the same principal,
admin flag and teams as on the GitHub path.

Keeping GitHub as a second way in means two sign-in paths to reason
about and keeps the GitHub OAuth app in the trust boundary. Nobody is
locked out by removing it: an unlinked person is sent to the link flow,
which proves the old account with one GitHub sign-in in any mode, and
existing sessions refresh without going upstream.

Refs #1500
@mashkovd

mashkovd commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Holding this as a draft. Two checks have to pass before it merges (raised by the session running the #1500 migration):

  1. The Cloudflare MCP portal path is untested. Each server on mcp.mctl.ai runs its own OAuth to api.mctl.ai through the Cloudflare server callback. The canary on 2026-10-06 exercised the direct claude.ai connector only. Sign the portal api server in through ZITADEL while both is still set.
  2. Nobody has listed who is linked. Phase 1 step 2 of feat(unified-identity): move Argo CD, portal and Cloudflare Access sign-in to ZITADEL #1500 is not done. Only the owner is known to be linked. After the switch every other member of tenants mctl, ovk, labs and erpact, and any GitHub-only connector user, gets the 403 link page at their next re-authorization. Existing sessions survive on refresh, so the failures show up later, not at the switch.

Also for whoever links: sign in to ZITADEL with the login name, not the e-mail (zitadel/zitadel#12852).

The description above says nobody is locked out, which is still accurate, but it understates the second point.

@mashkovd
mashkovd marked this pull request as ready for review October 9, 2026 20:27
Copilot AI balanced review requested due to automatic review settings October 9, 2026 20:27
@mashkovd mashkovd added the steward:escalated pr-steward escalated to a human label Oct 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit df9a6c3.

No significant issues found.

@mashkovd

mashkovd commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

Taken over by the zitadel-github-migration session with the owner's OK (2026-10-09). Merged main (c087579), marked ready, labeled steward:escalated so it merges only through the manual gate. Please don't push to this branch.

@mashkovd

mashkovd commented Oct 9, 2026

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 2m 17s —— View job


Review complete ✅

  • Fetch pinned commits and compute diff (git diff 7d7862c5...c087579)
  • Review diff for correctness/security/operational safety
  • Post verdict

Diff: one file, +12/-8, all in platform-gitops/bootstrap/templates/mctl-platform/mctl-api.yaml — the OAUTH_UPSTREAM value flips from "both" to "zitadel", with the comment block above it rewritten to describe the new mode. No template logic, RBAC, or resource changes.

Verification done:

  • Confirmed oauthZitadelSecret: mctl-api-oidc-zitadel (line 330) is already wired, so OAUTH_ZITADEL_CLIENT_ID/SECRET — which the new comment says zitadel mode requires to start — are already present. The comment's claim is accurate, not aspirational.
  • The surrounding comment at lines 319–330 independently documents the same mode-switch behavior (mctl-api refuses to start in those modes without them), consistent with the new comment added here.
  • No other references to OAUTH_UPSTREAM or "both" left stale elsewhere in this file.

Per this repo's review policy, a single config/env-var value change with no logic changes is trivial. 0 P1/P2 findings.

Submitted: APPROVE on commit c087579 — review.

Note for the human gate: this verdict is about the diff only. The operational blockers raised in-thread by @mashkovd (Cloudflare MCP portal path untested, incomplete linking roster for tenants mctl/ovk/labs/erpact) are rollout-readiness concerns, not defects in this change, and aren't resolved by this approval — steward:escalated still gates the actual merge.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial config change: single env var flip (OAUTH_UPSTREAM both -> zitadel) plus comment update. No logic/template changes. Prerequisite OAUTH_ZITADEL_CLIENT_ID/SECRET wiring (oauthZitadelSecret) already present and confirmed consistent. 0 P1/P2.

@mashkovd
mashkovd merged commit ae0487c into main Oct 9, 2026
29 checks passed
@mashkovd
mashkovd deleted the feat/mctl-api-oauth-upstream-zitadel branch October 9, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

steward:escalated pr-steward escalated to a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants