feat: add On-Behalf-Of flow for browser-based MCP client support - #370
Conversation
|
@microsoft-github-policy-service agree |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
=======================================
Coverage ? 41.73%
=======================================
Files ? 73
Lines ? 6886
Branches ? 0
=======================================
Hits ? 2874
Misses ? 3819
Partials ? 193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
141353a to
174300c
Compare
gossion
left a comment
There was a problem hiding this comment.
Thanks for this PR! The OBO approach is the right solution for browser-based clients. A few correctness and safety issues worth addressing before merge.
| } | ||
|
|
||
| se.azureToken = armToken | ||
| se.oboExpiresAt = time.Now().Add(55 * time.Minute) |
There was a problem hiding this comment.
Race condition on sessionEntry fields
refreshSessionOBO mutates se.azureToken, se.oboExpiresAt, and se.clusterToken in-place while concurrent requests for the same session can be reading those same fields. sync.Map only protects map-level operations, not struct field access.
Suggest adding a sync.Mutex to sessionEntry:
type sessionEntry struct {
mu sync.Mutex // protects azureToken, clusterToken, oboExpiresAt
tokenInfo *auth.TokenInfo
bearerToken string
azureToken string
clusterToken string
oboExpiresAt time.Time
expiresAt time.Time
}Alternatively, make sessionEntry immutable and replace the whole entry atomically with sync.Map.Store after refresh — this avoids the mutex entirely and makes the concurrent-read path lock-free.
| type AuthMiddleware struct { | ||
| provider *AzureOAuthProvider | ||
| serverURL string | ||
| sessions sync.Map // Mcp-Session-Id → *sessionEntry |
There was a problem hiding this comment.
No session eviction — potential memory leak
Expired sessions are only removed lazily when a request happens to arrive after expiry. Browser sessions that close without reconnecting accumulate forever in the sync.Map.
Consider a background cleanup goroutine started in NewAuthMiddleware:
go func() {
ticker := time.NewTicker(30 * time.Minute)
defer ticker.Stop()
for range ticker.C {
now := time.Now()
m.sessions.Range(func(k, v any) bool {
if now.After(v.(*sessionEntry).expiresAt) {
m.sessions.Delete(k)
}
return true
})
}
}()A 30–60 minute sweep interval is sufficient given the 24 h max session lifetime.
| next.ServeHTTP(w, r) | ||
| // Wrap the ResponseWriter to capture the Mcp-Session-Id mcp-go assigns on the | ||
| // response, then cache the full auth result (including bearer token for OBO refresh). | ||
| oboExpiry := time.Now().Add(55 * time.Minute) // slightly under the 1h ARM token lifetime |
There was a problem hiding this comment.
OBO token expiry is hardcoded to 55 minutes
The OBO token response includes an expires_in field. If tenant policy issues shorter-lived tokens, this hardcoded estimate may cause the server to serve an already-expired token until the proactive refresh kicks in.
Consider returning the actual expires_in from ExchangeOBO and using it to compute oboExpiresAt, rather than assuming a fixed 55-minute window.
| // Extract aks_resource_id from params, falling back to the configured default | ||
| aksResourceID, _ := params["aks_resource_id"].(string) | ||
| if aksResourceID == "" { | ||
| aksResourceID = os.Getenv("AZURE_AKS_RESOURCE_ID") |
There was a problem hiding this comment.
extractRequestContext reads AZURE_AKS_RESOURCE_ID directly — bypasses the config layer
config.go already reads this env var and stores it in cfg.DefaultAKSResourceID, which is then passed through RegisterKubectlTools → createCallKubectlTool. Having extractRequestContext also call os.Getenv directly creates a second, independent code path for the same setting.
This means any future changes to validation or override precedence would need to be applied in two places. Suggest injecting the default into RunCommandExecutor at construction time:
type RunCommandExecutor struct {
defaultAKSResourceID string
}
func NewRunCommandExecutor(defaultAKSResourceID string) *RunCommandExecutor {
return &RunCommandExecutor{defaultAKSResourceID: defaultAKSResourceID}
}Then use e.defaultAKSResourceID as the fallback here instead of os.Getenv.
|
README.md: new flags and env var are not documented The CLI flags reference block in README.md is missing the three new flags added by this PR. Suggest adding them to the flags list: And to the Environment variables section: |
…re#370) * feat: add On-Behalf-Of flow for browser-based MCP client support * fix: suppress false positives
Summary
Browser-based MCP clients such as Claude Web authenticate via OAuth but cannot supply the
X-Azure-Tokenheader required bytokenAuthOnlykubectl tools, causing allcall_kubectlinvocations to fail withX-Azure-Token not found in context. This PR adds an On-Behalf-Of (OBO) token exchange flow that resolves this entirely server-side with no changes required from the client.Changes
On-Behalf-Of token exchange (
--oauth-obo-enabled)https://management.azure.com/user_impersonation) used to authenticate the RunCommand API call, and one for an AKS cluster token (6dae42f8-4368-4678-94ff-3960e28e3630) required by AAD-enabled clusters using Kubernetes RBACcall_kubectlfinds them without any client-side changesSession caching
Mcp-Session-Idon follow-up requests with noAuthorizationheader. The middleware now caches OBO tokens per session so these continuation requests succeedDefault cluster resource ID (
--default-aks-resource-id/AZURE_AKS_RESOURCE_ID)aks_resource_idbecomes optional in thecall_kubectltool schema — MCP clients no longer ask the user to supply the cluster resource ID on every conversationHelm chart
One new value:
Azure AD requirements
The app registration requires the Azure Service Management → user_impersonation delegated permission and a client secret (passed via AZURE_CLIENT_SECRET / azure.clientSecret).
Backwards compatibility
All new behaviour is gated behind --oauth-obo-enabled (default: false). Existing deployments using workload identity, local az login, or manual X-Azure-Token headers are completely unaffected.