fix(mcp/oauthlogin): pin mcp login's callback path, keep the port ephemeral - #1377
Open
reyortiz3 wants to merge 2 commits into
Open
fix(mcp/oauthlogin): pin mcp login's callback path, keep the port ephemeral#1377reyortiz3 wants to merge 2 commits into
reyortiz3 wants to merge 2 commits into
Conversation
Every server reachable through `mecated mcp login` already commits to a client identity that must be registered ahead of time: selectMCPLoginServer only returns servers whose OAuth was populated by loadOAuthClient, which accepts exactly a preregistered confidential client or a CIMD client, never DCR. Leaving oauthlogin.Options.RedirectURL empty put every such login on the runtime's random-path, ephemeral-port default (ADR 0112) instead, so the presented redirect_uri never matched what either client kind had registered -- the login could never actually succeed against a standards-compliant authorization server enforcing RFC 8252 loopback matching (which tolerates only a varying port, never a varying path). Set RedirectURL to the existing ExactRedirectURL constant instead, the same fixed callback cmd/mecatui/login.go's remote-login path already uses. Renamed and updated the one test that explicitly locked in the old random-path behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit reused ExactRedirectURL for mecated mcp login, which fixes BOTH the callback path and the port. That reintroduces exactly the local port-squatting exposure issue #522's original design required avoiding ("use an unguessable callback path"): a well-known port is predictable and can be pre-bound by another local process before the legitimate login starts, hijacking or denying the real callback. Only the PATH actually needs to be fixed for a preregistered/CIMD client's redirect_uri to be statically registerable. RFC 8252 SS7.3 loopback matching -- which every authorization server this login talks to already implements, since neither client kind is reachable through DCR -- ignores the port on both sides of the comparison. So the port can and should stay ephemeral, exactly as the random-path default already does; only the path needs to stop being randomly generated per invocation. Add Options.PinCallbackPath: fixes the callback to fixedCallbackPath while still binding "127.0.0.1:0" (OS-assigned port), extracted alongside ExactRedirectURL's fully-fixed mode into resolveCallbackMode to keep Authorize's branch count under the gocyclo limit. ExactRedirectURL itself is untouched and stays available for a target that genuinely needs an exact string match (e.g. cmd/mecatui/login.go's remote OIDC login, which cannot assume its target implements RFC 8252 dynamic-port matching). mecated mcp login now uses PinCallbackPath instead. This also removes the port-contention regression the previous commit introduced: concurrent `mecated mcp login` calls to different servers each get their own ephemeral port again, same as before either commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
mecated mcp login <server>could never actually complete against a standards-compliant authorization server for any profile it supports.selectMCPLoginServeronly ever returns a server whoseOAuthwas populated byloadOAuthClient— which accepts exactly a preregistered confidential client or a CIMD client, never DCR (internal/cliconfig/mcpprofile.go). Both of those client kinds commit to a redirect_uri that must be registered ahead of time. ButrunMCPLoginleft the login runtime on its random-callback-path, ephemeral-port default (ADR 0112) — a fresh path and port on every single invocation. RFC 8252 §7.3 loopback matching (which a standards-compliant AS implements) only tolerates a varying port; the path must match exactly. So the presented redirect_uri could never match anything registered in advance, for either supported client kind.This surfaced while wiring Connector Gateway's embedded auth server for CIMD interop with mecatl (stacklok/stacklok-enterprise-platform#3843): the connector-gateway side is ready, but a real
mecated mcp loginagainst it would still fail on this.Fix (revised after review discussion)
An earlier version of this PR reused
oauthlogin.ExactRedirectURL(mecatui's remote-login constant), which fixes both the callback path and the port (18473). On reflection that reintroduces exactly what issue #522's original design required avoiding: "use an unguessable callback path". A well-known, fixed port is squattable — another local process can pre-bind it before the legitimate login starts, hijacking or denying the real callback.Only the path actually needs to be fixed for a preregistered/CIMD client's redirect_uri to be statically registerable. RFC 8252 §7.3 loopback matching — which every AS this command talks to already implements, since neither reachable client kind goes through DCR — ignores the port on both sides of the comparison. So this PR instead adds
oauthlogin.Options.PinCallbackPath: it fixes the callback to a well-known path but keeps binding an ephemeral port (127.0.0.1:0), exactly like the existing random-path default already does.mecated mcp loginnow uses this instead ofExactRedirectURL.Net effect:
mecated mcp logincalls to different servers each get their own ephemeral port, same as before this PR.oauthlogin.ExactRedirectURLitself is untouched and still available —cmd/mecatui/login.go's remote-login flow keeps using it, since a general-purpose OIDC target can't be assumed to implement RFC 8252 dynamic-port matching the way an MCP-shaped AS does.Development stage
Optionsfield + a smallAuthorizerefactor + a new test matrix, no new subsystem).Contract linkage
oauthlogin.Options/ExactRedirectURL/the fixed-vs-random callback split already exist; this adds a third, narrower mode alongside them (PinCallbackPath) and one call site's choice of which mode to use. No new abstractions beyond that.ExactRedirectURL's fully-fixed mode (introduced later, for mecatui) is also untouched.Issue relationship
Relates to stacklok/stacklok-enterprise-platform#3843 (Connector Gateway ↔ mecatl CIMD interop). No mecatl-side issue was filed for this specific gap before this PR.
Type of change
Test plan
task lint) — 0 issues, all modulesgo test ./cmd/mecated/... ./mcp/oauthlogin/... ./internal/app/...all pass. Fulltask testrun also checked: its two failures (TestNoStaleMovedADRSlugs,TestToolHiveImportsStayBehindApprovedAdapterLeaves) are pre-existing artifacts of an unrelated nested worktree present on disk in this environment (.claude/worktrees/product-metrics-otel/), not caused by this change.go run ./cmd/mecademo) — not applicablemecated mcp login's docs mention the callback shapecmd/andmcp/oauthloginonly/panel-review— not run in this sessionChanges
mcp/oauthlogin/runtime.gofixedCallbackPath; addOptions.PinCallbackPath(fixed path, ephemeral port); extractresolveCallbackModeto keepAuthorize's branch count under the gocyclo limit; validateRedirectURL/PinCallbackPathare mutually exclusivemcp/oauthlogin/runtime_test.goTestPinCallbackPathUsesFixedPathEphemeralPort(proves path fixed, port varies across two runs),TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts(mirrors the equivalentExactRedirect*test),TestPinCallbackPathAndRedirectURLAreMutuallyExclusivecmd/mecated/mcplogin.goPinCallbackPath: trueinstead ofRedirectURL: ExactRedirectURLcmd/mecated/mcplogin_test.goPinCallbackPathis forwarded insteadUser-facing change
mecated mcp login <server>now binds its OAuth callback listener on a fixed, well-known path (/oauth/callback) instead of a random one, but the port is still chosen by the OS at bind time, same as before this change — no port pinning, no port-contention risk. Operators registering a preregistered client or publishing a CIMD document for a mecatl-facing server should usehttp://127.0.0.1/oauth/callback(no port, or any port — matching ignores it) as the redirect_uri.Special notes for reviewers
ExactRedirectURL(fixed path + fixed port), which a security-minded read flagged as reintroducing the port-squatting exposure issue Runtime-owned browser and loopback login for MCP OAuth #522 deliberately avoided. This version fixes only the path, keeping the port ephemeral, which should fully reconcile both concerns — please double check that reasoning holds.🤖 Generated with Claude Code