diff --git a/cmd/mecated/mcplogin.go b/cmd/mecated/mcplogin.go index 352c694c1..f467f98cf 100644 --- a/cmd/mecated/mcplogin.go +++ b/cmd/mecated/mcplogin.go @@ -149,7 +149,19 @@ func runMCPLogin(args []string, stdout io.Writer) error { return err } - opts := oauthlogin.Options{NoBrowser: parsed.noBrowser} + // PinCallbackPath fixes the callback's path (not its port): selectMCPLoginServer + // only ever returns a server whose OAuth was populated by loadOAuthClient, which + // accepts exactly a preregistered confidential client or a CIMD client — never + // DCR (mcpprofile.go) — so every server reachable here already commits to a + // client identity that must be registered ahead of time, and a random callback + // path can never match a value fixed in advance. Both client kinds only ever + // target MCP-shaped, RFC 8252-aware authorization servers, which accept any port + // for a registered loopback redirect_uri as long as the path matches — so unlike + // oauthlogin.ExactRedirectURL (a fully fixed callback, port included, for a + // general-purpose OIDC target that cannot be assumed to implement RFC 8252 + // dynamic-port matching), this login still gets an unpredictable port every run, + // preserving the squatting resistance a foreseeable port would give up. + opts := oauthlogin.Options{NoBrowser: parsed.noBrowser, PinCallbackPath: true} if parsed.noBrowser { opts.URLWriter = stdout } diff --git a/cmd/mecated/mcplogin_test.go b/cmd/mecated/mcplogin_test.go index 851e4058b..60ec1d261 100644 --- a/cmd/mecated/mcplogin_test.go +++ b/cmd/mecated/mcplogin_test.go @@ -190,7 +190,7 @@ func TestMCPLoginUsesExplicitOperatorPrecedence(t *testing.T) { } } -func TestRunMCPLoginExecutionPathUsesRandomCallback(t *testing.T) { +func TestRunMCPLoginExecutionPathUsesFixedCallback(t *testing.T) { key := base64.StdEncoding.EncodeToString(make([]byte, 32)) t.Setenv("MECATL_LOGIN_KEY", key) t.Setenv("MECATL_LOGIN_CREDENTIAL", base64.StdEncoding.EncodeToString([]byte("opaque"))) @@ -207,8 +207,8 @@ func TestRunMCPLoginExecutionPathUsesRandomCallback(t *testing.T) { if server.Name != "GitHub" || server.OAuth == nil || server.OAuth.CredentialStore == nil || server.OAuth.CredentialReader != nil { t.Fatalf("selected server = %#v", server) } - if !opts.NoBrowser || opts.URLWriter == nil || opts.RedirectURL != "" { - t.Fatalf("runtime options = %#v; no-browser or random-path default was not forwarded", opts) + if !opts.NoBrowser || opts.URLWriter == nil || !opts.PinCallbackPath || opts.RedirectURL != "" { + t.Fatalf("runtime options = %#v; no-browser or pinned-callback-path was not forwarded", opts) } return nil } diff --git a/mcp/oauthlogin/runtime.go b/mcp/oauthlogin/runtime.go index 28d2d4628..dd05227ed 100644 --- a/mcp/oauthlogin/runtime.go +++ b/mcp/oauthlogin/runtime.go @@ -21,10 +21,18 @@ const ( callbackBytes = 32 shutdownTimeout = time.Second + // fixedCallbackPath is the well-known, pre-registerable callback path shared by + // ExactRedirectURL (fixed path + fixed port) and Options.PinCallbackPath (fixed + // path + ephemeral port) — see PinCallbackPath's doc comment for why a target's + // own capabilities decide which of the two a caller should use. + fixedCallbackPath = "/oauth/callback" + // ExactRedirectURL is the fixed callback URI used by clients that have a - // pre-registered redirect. It is deliberately IPv4-literal and must not be + // pre-registered redirect requiring an EXACT string match, port included (a + // general-purpose OIDC target with no obligation to implement RFC 8252 loopback + // dynamic-port matching). It is deliberately IPv4-literal and must not be // changed to localhost or a wildcard address. - ExactRedirectURL = "http://127.0.0.1:18473/oauth/callback" + ExactRedirectURL = "http://127.0.0.1:18473" + fixedCallbackPath ) var ( @@ -69,10 +77,25 @@ type Options struct { URLWriter io.Writer Launcher BrowserLauncher - // RedirectURL enables an explicitly configured callback. The only accepted - // value is ExactRedirectURL; empty preserves the random-path, ephemeral-port - // behavior used by existing callers. + // RedirectURL enables an explicitly configured callback with a FIXED PORT as + // well as a fixed path. The only accepted value is ExactRedirectURL. Use this + // only for a target that requires an exact redirect_uri string match (a + // general-purpose OIDC target with no obligation to implement RFC 8252 loopback + // dynamic-port matching) — it reintroduces local port-squatting exposure that + // PinCallbackPath does not (see its doc comment). Mutually exclusive with + // PinCallbackPath. Empty preserves the random-path, ephemeral-port default. RedirectURL string + + // PinCallbackPath fixes the callback's PATH to the same well-known value + // ExactRedirectURL uses, while still binding an EPHEMERAL port. Use this for a + // target whose authorization server implements RFC 8252 §7.3 loopback dynamic- + // port matching — the AS accepts any port for a registered loopback redirect_uri + // as long as the path matches — which lets a client register one fixed + // redirect_uri while every login still gets its own unpredictable port, + // preserving the squatting resistance ExactRedirectURL's fixed port gives up. + // Mutually exclusive with RedirectURL: New rejects setting both rather than + // silently picking one. + PinCallbackPath bool } // Result is the validated loopback authorization response. @@ -104,6 +127,9 @@ func New(opts Options) (*Runtime, error) { if opts.RedirectURL != "" && !isExactRedirectURL(opts.RedirectURL) { return nil, errors.New("OAuth redirect URL is invalid") } + if opts.RedirectURL != "" && opts.PinCallbackPath { + return nil, errors.New("OAuth redirect URL and pinned callback path are mutually exclusive") + } launcher := opts.Launcher if launcher == nil { launcher = systemBrowserLauncher{} @@ -142,22 +168,9 @@ func (r *Runtime) Authorize(ctx context.Context, expectedIssuer string, authoriz if err := ctx.Err(); err != nil { return err } - path := "" - address := "127.0.0.1:0" - callbackHost := "" - redirectURL := "" - attemptPolicy := attemptMatchingRoute - if r.opts.RedirectURL == "" { - path, err = randomCallbackPath(r.random) - if err != nil { - return errors.New("generate OAuth callback path: failed") - } - } else { - path = "/oauth/callback" - address = "127.0.0.1:18473" - callbackHost = "127.0.0.1:18473" - redirectURL = ExactRedirectURL - attemptPolicy = attemptFixedRoute + path, address, callbackHost, redirectURL, attemptPolicy, err := resolveCallbackMode(r.opts, r.random) + if err != nil { + return err } ln, err := r.listen(ctx, "tcp4", address) if err != nil { @@ -286,6 +299,41 @@ func randomCallbackPath(reader io.Reader) (string, error) { return callbackPrefix + base64.RawURLEncoding.EncodeToString(raw[:]), nil } +// resolveCallbackMode picks the callback path, bind address, Host-header match, any +// pre-computed redirect URL, and attempt policy for one Authorize call. Extracted from +// Authorize purely to keep that function's branch count under the gocyclo limit; the +// three cases mirror Options.RedirectURL/PinCallbackPath's doc comments exactly. +func resolveCallbackMode(opts Options, random io.Reader) ( + path, address, callbackHost, redirectURL string, attemptPolicy callbackAttemptPolicy, err error, +) { + address = "127.0.0.1:0" + switch { + case opts.RedirectURL != "": + // Fixed path AND fixed port: an exact redirect_uri string match, for a + // target with no obligation to implement RFC 8252 dynamic-port matching. + path = fixedCallbackPath + address = "127.0.0.1:18473" + callbackHost = "127.0.0.1:18473" + redirectURL = ExactRedirectURL + attemptPolicy = attemptFixedRoute + case opts.PinCallbackPath: + // Fixed path, ephemeral port: address/callbackHost/redirectURL stay at their + // random-port defaults — computed by the caller from the OS-assigned + // listener address, exactly like the random-path case below — so only the + // path is pinned; an RFC 8252-compliant AS's dynamic-port matching accepts + // whichever port this run happens to get. + path = fixedCallbackPath + attemptPolicy = attemptFixedRoute + default: + attemptPolicy = attemptMatchingRoute + path, err = randomCallbackPath(random) + if err != nil { + return "", "", "", "", 0, errors.New("generate OAuth callback path: failed") + } + } + return path, address, callbackHost, redirectURL, attemptPolicy, nil +} + func isExactRedirectURL(raw string) bool { return raw == ExactRedirectURL } diff --git a/mcp/oauthlogin/runtime_test.go b/mcp/oauthlogin/runtime_test.go index cfa250239..19eae45d9 100644 --- a/mcp/oauthlogin/runtime_test.go +++ b/mcp/oauthlogin/runtime_test.go @@ -719,6 +719,120 @@ func TestExactRedirectValidationIsStrict(t *testing.T) { } } +// TestPinCallbackPathUsesFixedPathEphemeralPort proves PinCallbackPath fixes only the +// callback PATH: two successive Authorize calls both land on fixedCallbackPath, but get +// different ports, since RFC 8252 dynamic-port matching (which the client this option is +// for already relies on) never checks the port. +func TestPinCallbackPathUsesFixedPathEphemeralPort(t *testing.T) { + var redirect string + runtime, err := New(Options{ + PinCallbackPath: true, + Launcher: launcherFunc(func(_ context.Context, _ string) error { + valid, _ := http.NewRequest(http.MethodGet, callbackURL(redirect, "good", "s", testIssuer), nil) + if got := request(t, valid).status; got != http.StatusOK { + t.Fatalf("valid status = %d", got) + } + return nil + }), + }) + if err != nil { + t.Fatal(err) + } + + authorizeOnce := func() string { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err := runtime.Authorize(ctx, testIssuer, func(ctx context.Context, got string, present func(context.Context, string) (Result, error)) error { + redirect = got + result, err := present(ctx, "https://as.example.test/authorize?state=s") + if err != nil { + return err + } + if result.Code != "good" { + t.Fatalf("result = %#v", result) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + return redirect + } + + first := authorizeOnce() + second := authorizeOnce() + + for _, redirect := range []string{first, second} { + if !strings.HasSuffix(redirect, fixedCallbackPath) { + t.Fatalf("redirect = %q, want suffix %q", redirect, fixedCallbackPath) + } + } + firstPort := strings.TrimSuffix(strings.TrimPrefix(first, "http://127.0.0.1:"), fixedCallbackPath) + secondPort := strings.TrimSuffix(strings.TrimPrefix(second, "http://127.0.0.1:"), fixedCallbackPath) + if firstPort == "" || secondPort == "" { + t.Fatalf("could not extract ports from %q, %q", first, second) + } + if firstPort == secondPort { + t.Fatalf("both authorizations bound the same port %q; PinCallbackPath must not fix the port", firstPort) + } +} + +// TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts mirrors +// TestExactRedirectUnauthenticatedFloodDoesNotSpendAttempts: a pinned path is public and +// pre-registered exactly like ExactRedirectURL, so it must get the same attemptFixedRoute +// policy (ambient probes never exhaust the attempt budget). +func TestPinCallbackPathUnauthenticatedFloodDoesNotSpendAttempts(t *testing.T) { + var redirect string + runtime, err := New(Options{ + PinCallbackPath: true, + Launcher: launcherFunc(func(_ context.Context, _ string) error { + for i := range 2 * maxRequestAttempts { + var req *http.Request + switch i % 3 { + case 0: + req, _ = http.NewRequest(http.MethodGet, callbackURL(redirect, "probe", "wrong-state", testIssuer), nil) + case 1: + req, _ = http.NewRequest(http.MethodPost, redirect+"?state=wrong-state", nil) + default: + req, _ = http.NewRequest(http.MethodGet, redirect+"?state=%zz", nil) + } + if got := request(t, req).status; got < 400 { + t.Fatalf("probe %d status = %d", i, got) + } + } + valid, _ := http.NewRequest(http.MethodGet, callbackURL(redirect, "good", "s", testIssuer), nil) + if got := request(t, valid).status; got != http.StatusOK { + t.Fatalf("valid status after pinned-path flood = %d", got) + } + return nil + }), + }) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + err = runtime.Authorize(ctx, testIssuer, func(ctx context.Context, got string, present func(context.Context, string) (Result, error)) error { + redirect = got + result, err := present(ctx, "https://as.example.test/authorize?state=s") + if err == nil && result.Code != "good" { + t.Fatalf("result = %#v", result) + } + return err + }) + if err != nil { + t.Fatal(err) + } +} + +// TestPinCallbackPathAndRedirectURLAreMutuallyExclusive proves New refuses the +// nonsensical combination rather than silently preferring one or the other. +func TestPinCallbackPathAndRedirectURLAreMutuallyExclusive(t *testing.T) { + if _, err := New(Options{RedirectURL: ExactRedirectURL, PinCallbackPath: true}); err == nil { + t.Fatal("accepted RedirectURL and PinCallbackPath set together") + } +} + func TestCancellationWhileWaitingForCallback(t *testing.T) { started := make(chan struct{}) var redirect string