-
Notifications
You must be signed in to change notification settings - Fork 173
Persist DCR client credentials for OAuth session restoration #3418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Persist DCR client credentials for OAuth session restoration #3418
Conversation
Extend OAuth token persistence to also store Dynamic Client Registration (DCR) client credentials. This enables workloads that use DCR (like Datadog and Glean) to restore OAuth sessions across restarts without requiring a new browser-based authentication flow. Changes: - Add CachedClientIDRef and CachedClientSecretRef fields to remote.Config - Add HasCachedClientCredentials() and ClearCachedClientCredentials() helpers - Add TokenTypeOAuthClientID to secrets for secure storage - Expose ClientID and ClientSecret in OAuthFlowResult from discovery - Add ClientCredentialsPersister type for DCR credential persistence - Update handler to restore and persist DCR credentials - Update runner to wire up the client credentials persister For PKCE flows (like Datadog), only the client_id is persisted since no client_secret is issued. Closes stacklokgh-3335 Signed-off-by: Frederic Le Feurmou <flfeurmou@indeed.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3418 +/- ##
==========================================
+ Coverage 64.79% 65.10% +0.30%
==========================================
Files 381 404 +23
Lines 37149 39376 +2227
==========================================
+ Hits 24071 25634 +1563
- Misses 11191 11756 +565
- Partials 1887 1986 +99 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Frederic Le Feurmou <flfeurmou@indeed.com>
|
Addressed the review feedback: ✅ ClientID as plain text: Changed ✅ Added DCR metadata fields:
⏸️ Expiration handling logic: The fields are in place, but I haven't implemented the actual expiration check and re-registration flow yet. Let me know if that's needed in this PR or if it can be a follow-up. |
- Store ClientID as plain text instead of secret reference (it's public) - Add CachedClientSecretExpiresAt field for expiration tracking - Add CachedRegistrationAccessTokenRef field for registration updates - Update handler to use plain text ClientID - Simplify runner.go to not store ClientID in secret manager
a4098ec to
0127cb4
Compare
|
@flfeurmou-indeed that makes sense. I’ll create a separate issue to handle client secret expiry. I’ve added a couple of comments; once those are addressed, we can merge this PR and handle client secret expiry in a follow-up PR. |
- Remove TokenTypeOAuthClientID from secrets.go (client_id stored as plain text) - Use lowercase test values for CachedClientID to reflect plain text storage Signed-off-by: Frederic Le Feurmou <flfeurmou@indeed.com>
|
@flfeurmou-indeed Thanks for addressing the feedback 🚀 I’ve approved the PR. |
Summary
Extend OAuth token persistence to also store Dynamic Client Registration (DCR) client credentials. This enables workloads that use DCR (like Datadog and Glean) to restore OAuth sessions across restarts without requiring a new browser-based authentication flow.
This PR builds on #3382 which added refresh token persistence, addressing the remaining issue where DCR servers still required re-authentication because the dynamically registered
client_id(andclient_secretwhere applicable) were lost on restart.Problem
Remote MCP servers that use Dynamic Client Registration (e.g., Datadog, Glean) would lose their OAuth sessions after workload restarts because:
client_idwas not persistedclient_id, the token refresh request fails with "Client ID is required" or similar errorsSolution
Store DCR client credentials in the secret manager alongside refresh tokens:
CachedClientIDRef: Reference to the stored client IDCachedClientSecretRef: Reference to the stored client secret (when issued)For PKCE flows (like Datadog), only the
client_idis persisted since noclient_secretis issued.Changes
CachedClientIDRefandCachedClientSecretReffields toremote.ConfigHasCachedClientCredentials()andClearCachedClientCredentials()helper methodsTokenTypeOAuthClientIDto secrets for secure storageClientIDandClientSecretinOAuthFlowResultfrom discoveryClientCredentialsPersistertype for DCR credential persistenceTesting
HasCachedClientCredentialsandClearCachedClientCredentialsRelated
Closes #3335