Skip to content

Commit c7a78b0

Browse files
feat(broker): CredentialResolver — per-user-only ordering + policy seam (spec 074, MCP-1039) (#688)
* feat(broker): CredentialResolver — per-user-only ordering + policy seam (spec 074, MCP-1039) Add the per-user credential resolver (T6) that selects which brokered credential to inject on a proxied request. Strict per-user-only ordering (FR-013/FR-014), with no shared or static fallback: 1. valid cached per-user credential (refreshed if near-expiry); 2. else token-exchange / Entra OBO from the stored IdP subject token; 3. else, for oauth_connect upstreams the user has not connected, an actionable NotConnectedError carrying the connect URL; 4. else ErrNoCredential. - Single-flight coalescing per (user, server) so concurrent acquisitions do not trigger duplicate upstream token flows (golang.org/x/sync). - PolicyHook seam (FR-015) evaluated per call before a credential is returned; ships with an allow-all default (no policy engine yet). - Unauthenticated callers and a disabled store are rejected up front. - Exchanger/Connector/ConnectorProvider interfaces decouple the resolver from the concrete TokenExchanger (T4) and OAuthConnector (T5); both satisfy the interfaces via compile-time assertions. TDD: each ordering branch, near-expiry refresh (token-exchange + connect), unconnected -> connect-URL error, unauthenticated reject, store-disabled, policy-denied, no-static-fallback, and single-flight under -race. Related: spec 074 (MCP-1033). Builds on T3 (#601), T4 (#600), T5 (#602), all merged to main. Co-Authored-By: Paperclip <noreply@paperclip.ing> * fix(broker): address review on CredentialResolver — singleflight ctx, double-exchange, reconnect error Review (MCP-2490 / Critic) on PR #688: - must-fix: detach the caller's context inside the single-flight closure with context.WithoutCancel. The flight runs the acquisition once for all co-pending callers; inheriting the first caller's cancellation let a single client disconnect/timeout broadcast ctx.Err() to every waiter for the same (user, server). Per-caller cancellation still applies at the policy/return layer (caller's original ctx). - advisory: collapse the per-mode acquire/refresh paths so a near-expiry token-exchange miss no longer calls Exchange twice (refresh-then-fallthrough); a single Exchange now covers both cache-miss and near-expiry. - advisory: an already-connected oauth_connect user whose refresh fails now gets an actionable reconnect error (NotConnectedError.Reason) instead of a misleading "never connected" message; the connect URL is still surfaced. - advisory: document that the Exchanger (T4) and Connector (T5) persist their results themselves, so the resolver never calls store.Put. Tests: add single-flight caller-cancellation detach, no-double-exchange on near-expiry failure, and connect-flow refresh-fail -> reconnect. Full broker suite green under -tags server -race; golangci-lint v2.5.0 clean. Co-Authored-By: Paperclip <noreply@paperclip.ing> * docs(auth-broker): document per-user credential resolution ordering (spec 074, MCP-1039) Add a "Credential resolution" section describing the resolver's strict per-user-only ordering (cached/refresh -> token-exchange/OBO -> actionable connect-URL error -> no-credential), the no-shared/static-fallback guarantee, single-flight coalescing, and the policy-decision seam. Keeps the feature doc consistent with the CredentialResolver added in this PR (review follow-up). Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
1 parent 0aad8d5 commit c7a78b0

4 files changed

Lines changed: 846 additions & 1 deletion

File tree

docs/features/auth-broker.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,17 @@ Config validation fails with `auth_broker.authorization_endpoint is required for
7979

8080
A denied consent (`error=access_denied`) clears the pending flow and stores nothing.
8181

82+
## Credential resolution
83+
84+
On each proxied request the broker resolves the per-user credential to inject, in a strict **per-user-only** order. There is **no shared or static fallback** — a request that cannot produce a per-user credential fails rather than borrowing another identity:
85+
86+
1. A valid cached per-user credential is injected directly; if it is within the near-expiry window it is refreshed first (re-minted for `token_exchange`/`entra_obo`, or renewed from the stored refresh token for `oauth_connect`).
87+
2. Otherwise, for `token_exchange`/`entra_obo`, a credential is minted from the user's stored IdP subject token.
88+
3. Otherwise, for `oauth_connect` upstreams the user has not connected — or whose stored credential expired and could not be refreshed — the request fails with an **actionable error carrying the connect URL**, so the user is told to (re)connect rather than being silently denied.
89+
4. Otherwise the request fails with "no per-user credential available".
90+
91+
Concurrent requests for the same `(user, upstream)` are coalesced (single-flight) so a burst does not trigger duplicate upstream token flows. A policy-decision hook is evaluated per call immediately before the credential is returned; no policy engine ships yet, so it permits every injection by default.
92+
8293
## See also
8394

8495
- [OAuth Authentication](./oauth-authentication.md) — upstream OAuth for the personal edition.

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ require (
3535
go.opentelemetry.io/otel/trace v1.44.0
3636
go.uber.org/zap v1.28.0
3737
golang.org/x/mod v0.37.0
38+
golang.org/x/sync v0.20.0
3839
golang.org/x/sys v0.46.0
3940
golang.org/x/term v0.44.0
4041
gopkg.in/natefinch/lumberjack.v2 v2.2.1
@@ -139,7 +140,6 @@ require (
139140
go.yaml.in/yaml/v2 v2.4.2 // indirect
140141
go.yaml.in/yaml/v3 v3.0.4 // indirect
141142
golang.org/x/net v0.55.0 // indirect
142-
golang.org/x/sync v0.20.0 // indirect
143143
golang.org/x/text v0.37.0 // indirect
144144
golang.org/x/tools v0.45.0 // indirect
145145
google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect
Lines changed: 349 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,349 @@
1+
//go:build server
2+
3+
package broker
4+
5+
import (
6+
"context"
7+
"errors"
8+
"fmt"
9+
"time"
10+
11+
"go.uber.org/zap"
12+
"golang.org/x/sync/singleflight"
13+
14+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
15+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth"
16+
)
17+
18+
// defaultRefreshThreshold is how close to expiry a cached credential may be
19+
// before the resolver proactively refreshes it. A credential expiring within
20+
// this window is treated as stale (FR-013).
21+
const defaultRefreshThreshold = 60 * time.Second
22+
23+
// Sentinel errors returned by the resolver. They are deliberately coarse and
24+
// secret-free so they can be surfaced to callers and audited (FR-014/FR-019).
25+
var (
26+
// ErrUnauthenticated is returned when Resolve is called without a user
27+
// identity. Brokering is strictly per-user; an anonymous caller is rejected
28+
// before any store or upstream access (FR-014).
29+
ErrUnauthenticated = errors.New("credential resolver: unauthenticated caller")
30+
31+
// ErrNoCredential is returned when no per-user credential can be produced and
32+
// no actionable connect flow is available. There is deliberately no shared or
33+
// static fallback (FR-014).
34+
ErrNoCredential = errors.New("credential resolver: no per-user credential available")
35+
36+
// ErrBrokerNotConfigured is returned when the target server has no auth_broker
37+
// block. Such upstreams are not brokered and behave exactly as today.
38+
ErrBrokerNotConfigured = errors.New("credential resolver: server has no auth_broker configuration")
39+
)
40+
41+
// Exchanger mints an upstream credential by exchanging the user's stored IdP
42+
// subject token (token_exchange / entra_obo). *TokenExchanger satisfies it.
43+
type Exchanger interface {
44+
Exchange(ctx context.Context, userID, serverKey string, cfg *config.AuthBrokerConfig) (*UpstreamCredential, error)
45+
}
46+
47+
// Connector drives the per-user OAuth connect flow (Path B). *OAuthConnector
48+
// satisfies it. The resolver uses Refresh to renew a near-expiry connect-flow
49+
// credential and BuildAuthorizationURL to produce an actionable connect URL
50+
// when the user has not yet connected the upstream.
51+
type Connector interface {
52+
ServerKey() string
53+
BuildAuthorizationURL(userID string) (authURL, state string, err error)
54+
Refresh(ctx context.Context, userID string) (*UpstreamCredential, error)
55+
}
56+
57+
// ConnectorProvider resolves the per-upstream OAuthConnector for a server. The
58+
// REST layer (T8) supplies an implementation that assembles a ConnectorConfig
59+
// from the server's auth_broker block plus the gateway's callback URL. It is
60+
// only consulted for oauth_connect-mode upstreams.
61+
type ConnectorProvider interface {
62+
ConnectorFor(server *config.ServerConfig) (Connector, error)
63+
}
64+
65+
// NotConnectedError is returned when an oauth_connect upstream cannot produce a
66+
// usable per-user credential and the user must (re)consent. It carries the
67+
// authorize URL the caller redirects the user to (FR-013, actionable error) and
68+
// a Reason that distinguishes a first-time connect from an expired credential
69+
// whose refresh failed (so callers do not tell an already-connected user they
70+
// have "never connected").
71+
type NotConnectedError struct {
72+
ServerName string
73+
ConnectURL string
74+
Reason string
75+
}
76+
77+
func (e *NotConnectedError) Error() string {
78+
if e.Reason != "" {
79+
return fmt.Sprintf("credential resolver: upstream %q requires connection (%s); connect at: %s",
80+
e.ServerName, e.Reason, e.ConnectURL)
81+
}
82+
return fmt.Sprintf("credential resolver: upstream %q is not connected for this user; connect at: %s",
83+
e.ServerName, e.ConnectURL)
84+
}
85+
86+
// PolicyDecision is the verdict of the policy-decision seam evaluated before a
87+
// resolved credential is returned. Allow=false blocks the injection.
88+
type PolicyDecision struct {
89+
Allow bool
90+
Reason string
91+
}
92+
93+
// PolicyInput is the context handed to the policy seam.
94+
type PolicyInput struct {
95+
UserID string
96+
ServerName string
97+
ServerKey string
98+
Credential *UpstreamCredential
99+
}
100+
101+
// PolicyHook is the policy-decision seam (FR-015). No policy engine ships now;
102+
// the resolver defaults to an allow-all hook. A future engine implements this
103+
// interface without changing the resolver.
104+
type PolicyHook interface {
105+
Evaluate(ctx context.Context, in PolicyInput) (PolicyDecision, error)
106+
}
107+
108+
// PolicyHookFunc adapts a function to the PolicyHook interface.
109+
type PolicyHookFunc func(ctx context.Context, in PolicyInput) (PolicyDecision, error)
110+
111+
// Evaluate implements PolicyHook.
112+
func (f PolicyHookFunc) Evaluate(ctx context.Context, in PolicyInput) (PolicyDecision, error) {
113+
return f(ctx, in)
114+
}
115+
116+
// allowAllPolicy is the default seam implementation: it permits every
117+
// injection. It exists so the resolver always has a non-nil hook (FR-015).
118+
type allowAllPolicy struct{}
119+
120+
func (allowAllPolicy) Evaluate(_ context.Context, _ PolicyInput) (PolicyDecision, error) {
121+
return PolicyDecision{Allow: true}, nil
122+
}
123+
124+
// PolicyDeniedError is returned when the policy seam blocks a resolved
125+
// credential from being injected.
126+
type PolicyDeniedError struct {
127+
ServerName string
128+
Reason string
129+
}
130+
131+
func (e *PolicyDeniedError) Error() string {
132+
if e.Reason != "" {
133+
return fmt.Sprintf("credential resolver: policy denied credential for %q: %s", e.ServerName, e.Reason)
134+
}
135+
return fmt.Sprintf("credential resolver: policy denied credential for %q", e.ServerName)
136+
}
137+
138+
// ResolverDeps are the collaborators a CredentialResolver needs. Store and
139+
// Exchanger are required for token-exchange upstreams; Connectors is required
140+
// only for oauth_connect upstreams. Policy and Logger are optional.
141+
type ResolverDeps struct {
142+
Store CredentialStore
143+
Exchanger Exchanger
144+
Connectors ConnectorProvider
145+
Policy PolicyHook
146+
Logger *zap.Logger
147+
RefreshThreshold time.Duration
148+
}
149+
150+
// CredentialResolver produces the per-user upstream credential to inject on a
151+
// proxied request. It applies a strict per-user-only ordering (FR-013/FR-014):
152+
//
153+
// 1. a valid cached per-user credential (refreshed if near-expiry);
154+
// 2. else a freshly token-exchanged / OBO credential from the stored IdP
155+
// subject token;
156+
// 3. else, for oauth_connect upstreams the user has not connected, an
157+
// actionable NotConnectedError carrying the connect URL;
158+
// 4. else ErrNoCredential.
159+
//
160+
// There is no shared or static fallback. Concurrent acquisitions for the same
161+
// (user, server) are coalesced via single-flight so the upstream authorization
162+
// server is not hit with duplicate flows.
163+
type CredentialResolver struct {
164+
store CredentialStore
165+
exchanger Exchanger
166+
conns ConnectorProvider
167+
policy PolicyHook
168+
logger *zap.Logger
169+
170+
refreshThreshold time.Duration
171+
group singleflight.Group
172+
}
173+
174+
// NewCredentialResolver constructs a resolver from its dependencies, applying
175+
// defaults for the optional fields.
176+
func NewCredentialResolver(deps ResolverDeps) *CredentialResolver {
177+
logger := deps.Logger
178+
if logger == nil {
179+
logger = zap.NewNop()
180+
}
181+
policy := deps.Policy
182+
if policy == nil {
183+
policy = allowAllPolicy{}
184+
}
185+
threshold := deps.RefreshThreshold
186+
if threshold <= 0 {
187+
threshold = defaultRefreshThreshold
188+
}
189+
return &CredentialResolver{
190+
store: deps.Store,
191+
exchanger: deps.Exchanger,
192+
conns: deps.Connectors,
193+
policy: policy,
194+
logger: logger.Named("credential-resolver"),
195+
refreshThreshold: threshold,
196+
}
197+
}
198+
199+
// Resolve returns the per-user credential to inject for (userID, server),
200+
// applying the ordering described on CredentialResolver. The policy seam is
201+
// evaluated per call after acquisition; credential acquisition itself is
202+
// coalesced per (user, server) via single-flight.
203+
func (r *CredentialResolver) Resolve(ctx context.Context, userID string, server *config.ServerConfig) (*UpstreamCredential, error) {
204+
if userID == "" {
205+
return nil, ErrUnauthenticated
206+
}
207+
if server == nil || server.AuthBroker == nil {
208+
return nil, ErrBrokerNotConfigured
209+
}
210+
if r.store == nil || !r.store.Enabled() {
211+
return nil, ErrStoreDisabled
212+
}
213+
214+
serverKey := oauth.GenerateServerKey(server.Name, server.URL)
215+
216+
// Coalesce concurrent acquisitions for the same (user, server) so duplicate
217+
// upstream token flows are not triggered (reuse the single-flight pattern).
218+
//
219+
// The flight runs the acquisition once for every co-pending caller. Detach
220+
// the caller's cancellation with context.WithoutCancel so the in-flight
221+
// acquisition is not aborted — and its error broadcast to all waiters — just
222+
// because whichever caller happened to start the flight cancelled (client
223+
// disconnect, timeout). Per-caller cancellation still applies below at the
224+
// policy/return layer, which uses the caller's original ctx.
225+
flightKey := userID + "\x00" + serverKey
226+
v, err, _ := r.group.Do(flightKey, func() (interface{}, error) {
227+
return r.acquire(context.WithoutCancel(ctx), userID, serverKey, server)
228+
})
229+
if err != nil {
230+
return nil, err
231+
}
232+
cred, ok := v.(*UpstreamCredential)
233+
if !ok || cred == nil {
234+
return nil, ErrNoCredential
235+
}
236+
237+
// Policy-decision seam: evaluated per call, before the credential is handed
238+
// to the caller (FR-015). Default hook allows everything.
239+
decision, perr := r.policy.Evaluate(ctx, PolicyInput{
240+
UserID: userID,
241+
ServerName: server.Name,
242+
ServerKey: serverKey,
243+
Credential: cred,
244+
})
245+
if perr != nil {
246+
return nil, fmt.Errorf("credential resolver: policy evaluation failed: %w", perr)
247+
}
248+
if !decision.Allow {
249+
return nil, &PolicyDeniedError{ServerName: server.Name, Reason: decision.Reason}
250+
}
251+
return cred, nil
252+
}
253+
254+
// acquire runs the per-user-only ordering for a single (user, server). It is
255+
// invoked inside the single-flight group.
256+
//
257+
// Acquisition and refresh share a path per mode so a near-expiry cache miss does
258+
// not trigger a redundant double acquisition. The Exchanger (T4) and Connector
259+
// (T5) persist their results into the store themselves, so the resolver never
260+
// calls store.Put — it only reads the cache via store.Get.
261+
func (r *CredentialResolver) acquire(ctx context.Context, userID, serverKey string, server *config.ServerConfig) (*UpstreamCredential, error) {
262+
cfg := server.AuthBroker
263+
264+
// 1. Serve a still-valid, not-near-expiry cached credential directly.
265+
cached, err := r.store.Get(userID, serverKey)
266+
hasCache := err == nil && cached != nil
267+
switch {
268+
case hasCache:
269+
if cached.IsValid() && !cached.ExpiresWithin(r.refreshThreshold) {
270+
return cached, nil
271+
}
272+
// Stale / near-expiry: renewed by the per-mode path below.
273+
case errors.Is(err, ErrNotFound):
274+
// No cache: acquired by the per-mode path below.
275+
default:
276+
// Unexpected store error (not "missing"): surface it.
277+
return nil, fmt.Errorf("credential resolver: load cached credential: %w", err)
278+
}
279+
280+
switch cfg.Mode {
281+
case config.AuthBrokerModeTokenExchange, config.AuthBrokerModeEntraOBO:
282+
// 2. Token-exchange / OBO: the first-acquisition and refresh paths are
283+
// identical (re-mint from the stored IdP subject token), so a single
284+
// Exchange call covers both the cache-miss and near-expiry cases.
285+
if r.exchanger == nil {
286+
return nil, fmt.Errorf("credential resolver: no token exchanger configured for mode %q", cfg.Mode)
287+
}
288+
return r.exchanger.Exchange(ctx, userID, serverKey, cfg)
289+
290+
case config.AuthBrokerModeOAuthConnect:
291+
conn, cerr := r.connectorFor(server)
292+
if cerr != nil {
293+
return nil, cerr
294+
}
295+
// A cached connect-flow credential means the user already connected:
296+
// renew transparently via the stored refresh token. Only when that
297+
// refresh fails do we ask the (already-connected) user to reconnect.
298+
if hasCache && cached.RefreshToken != "" {
299+
refreshed, rerr := conn.Refresh(ctx, userID)
300+
if rerr == nil {
301+
return refreshed, nil
302+
}
303+
r.logger.Warn("connect-flow credential refresh failed; user must reconnect",
304+
zap.String("server", server.Name), zap.Error(rerr))
305+
return nil, r.notConnected(conn, server, userID, "stored credential expired and refresh failed; reconnect required")
306+
}
307+
// 3. Never connected, or connected without a usable refresh token and now
308+
// expired — both require (re)consent through the connect flow.
309+
reason := "not connected"
310+
if hasCache {
311+
reason = "stored credential expired; reconnect required"
312+
}
313+
return nil, r.notConnected(conn, server, userID, reason)
314+
315+
default:
316+
// 4. No recognised acquisition strategy and no per-user credential.
317+
return nil, ErrNoCredential
318+
}
319+
}
320+
321+
// notConnected builds the actionable NotConnectedError carrying the upstream
322+
// authorize URL the caller must redirect the user to, tagged with reason.
323+
func (r *CredentialResolver) notConnected(conn Connector, server *config.ServerConfig, userID, reason string) error {
324+
authURL, _, aerr := conn.BuildAuthorizationURL(userID)
325+
if aerr != nil {
326+
return fmt.Errorf("credential resolver: build connect URL: %w", aerr)
327+
}
328+
return &NotConnectedError{ServerName: server.Name, ConnectURL: authURL, Reason: reason}
329+
}
330+
331+
// connectorFor resolves the per-upstream connector, guarding against a missing
332+
// provider (only oauth_connect upstreams need one).
333+
func (r *CredentialResolver) connectorFor(server *config.ServerConfig) (Connector, error) {
334+
if r.conns == nil {
335+
return nil, fmt.Errorf("credential resolver: no connector provider configured for oauth_connect upstream %q", server.Name)
336+
}
337+
conn, err := r.conns.ConnectorFor(server)
338+
if err != nil {
339+
return nil, fmt.Errorf("credential resolver: resolve connector: %w", err)
340+
}
341+
return conn, nil
342+
}
343+
344+
// Compile-time assertions that the concrete broker types satisfy the resolver's
345+
// collaborator interfaces.
346+
var (
347+
_ Exchanger = (*TokenExchanger)(nil)
348+
_ Connector = (*OAuthConnector)(nil)
349+
)

0 commit comments

Comments
 (0)