From bd87c653ecc9bfd78bfa1cbb266f42dc5a10a0e1 Mon Sep 17 00:00:00 2001 From: Frederic Le Feurmou Date: Thu, 22 Jan 2026 22:15:57 -0600 Subject: [PATCH 1/4] Persist DCR client credentials for OAuth session restoration 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 gh-3335 Signed-off-by: Frederic Le Feurmou --- docs/server/docs.go | 7 +++ docs/server/swagger.json | 7 +++ docs/server/swagger.yaml | 7 +++ pkg/auth/discovery/discovery.go | 6 ++ pkg/auth/remote/config.go | 16 ++++++ pkg/auth/remote/config_test.go | 66 ++++++++++++++++++++++ pkg/auth/remote/handler.go | 61 ++++++++++++++++++-- pkg/auth/remote/persisting_token_source.go | 4 ++ pkg/auth/secrets/secrets.go | 7 +++ pkg/runner/runner.go | 44 +++++++++++++++ 10 files changed, 220 insertions(+), 5 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index e741f3b3e0..a3529d85e5 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -800,6 +800,13 @@ const docTemplate = `{ "bearer_token_file": { "type": "string" }, + "cached_client_id_ref": { + "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.", + "type": "string" + }, + "cached_client_secret_ref": { + "type": "string" + }, "cached_refresh_token_ref": { "description": "Cached OAuth token reference for persistence across restarts.\nThe refresh token is stored securely in the secret manager, and this field\ncontains the reference to retrieve it (e.g., \"OAUTH_REFRESH_TOKEN_workload\").\nThis enables session restoration without requiring a new browser-based login.", "type": "string" diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 562f16bba1..49504517db 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -793,6 +793,13 @@ "bearer_token_file": { "type": "string" }, + "cached_client_id_ref": { + "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.", + "type": "string" + }, + "cached_client_secret_ref": { + "type": "string" + }, "cached_refresh_token_ref": { "description": "Cached OAuth token reference for persistence across restarts.\nThe refresh token is stored securely in the secret manager, and this field\ncontains the reference to retrieve it (e.g., \"OAUTH_REFRESH_TOKEN_workload\").\nThis enables session restoration without requiring a new browser-based login.", "type": "string" diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 5eea6133ec..de3b384345 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -722,6 +722,13 @@ components: type: string bearer_token_file: type: string + cached_client_id_ref: + description: |- + Cached DCR client credentials for persistence across restarts. + These are obtained during Dynamic Client Registration and needed to refresh tokens. + type: string + cached_client_secret_ref: + type: string cached_refresh_token_ref: description: |- Cached OAuth token reference for persistence across restarts. diff --git a/pkg/auth/discovery/discovery.go b/pkg/auth/discovery/discovery.go index 8376394399..d918f17d4b 100644 --- a/pkg/auth/discovery/discovery.go +++ b/pkg/auth/discovery/discovery.go @@ -510,6 +510,10 @@ type OAuthFlowResult struct { AccessToken string RefreshToken string Expiry time.Time + + // DCR client credentials for persistence (obtained during Dynamic Client Registration) + ClientID string + ClientSecret string } func shouldDynamicallyRegisterClient(config *OAuthFlowConfig) bool { @@ -691,6 +695,8 @@ func newOAuthFlow(ctx context.Context, oauthConfig *oauth.Config, config *OAuthF AccessToken: tokenResult.AccessToken, RefreshToken: tokenResult.RefreshToken, Expiry: tokenResult.Expiry, + ClientID: oauthConfig.ClientID, + ClientSecret: oauthConfig.ClientSecret, }, nil } diff --git a/pkg/auth/remote/config.go b/pkg/auth/remote/config.go index 258ce74725..ef247f0495 100644 --- a/pkg/auth/remote/config.go +++ b/pkg/auth/remote/config.go @@ -54,6 +54,11 @@ type Config struct { // This enables session restoration without requiring a new browser-based login. CachedRefreshTokenRef string `json:"cached_refresh_token_ref,omitempty" yaml:"cached_refresh_token_ref,omitempty"` CachedTokenExpiry time.Time `json:"cached_token_expiry,omitempty" yaml:"cached_token_expiry,omitempty"` + + // Cached DCR client credentials for persistence across restarts. + // These are obtained during Dynamic Client Registration and needed to refresh tokens. + CachedClientIDRef string `json:"cached_client_id_ref,omitempty" yaml:"cached_client_id_ref,omitempty"` + CachedClientSecretRef string `json:"cached_client_secret_ref,omitempty" yaml:"cached_client_secret_ref,omitempty"` } // BearerTokenEnvVarName is the environment variable name used for bearer token authentication. @@ -142,6 +147,17 @@ func (c *Config) ClearCachedTokens() { c.CachedTokenExpiry = time.Time{} } +// HasCachedClientCredentials returns true if the config has cached DCR client credentials. +func (c *Config) HasCachedClientCredentials() bool { + return c.CachedClientIDRef != "" +} + +// ClearCachedClientCredentials removes any cached DCR client credential references from the config. +func (c *Config) ClearCachedClientCredentials() { + c.CachedClientIDRef = "" + c.CachedClientSecretRef = "" +} + // DefaultResourceIndicator derives the resource indicator (RFC 8707) from the remote server URL. // This function should only be called when the user has not explicitly provided a resource indicator. // If the resource indicator cannot be derived, it returns an empty string. diff --git a/pkg/auth/remote/config_test.go b/pkg/auth/remote/config_test.go index ea201478ae..78b127c115 100644 --- a/pkg/auth/remote/config_test.go +++ b/pkg/auth/remote/config_test.go @@ -158,3 +158,69 @@ func TestConfig_UnmarshalJSON_BearerTokenFields(t *testing.T) { }) } } + +func TestConfig_HasCachedClientCredentials(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config Config + expected bool + }{ + { + name: "no cached credentials", + config: Config{}, + expected: false, + }, + { + name: "has cached client ID only", + config: Config{ + CachedClientIDRef: "OAUTH_CLIENT_ID_test", + }, + expected: true, + }, + { + name: "has both cached credentials", + config: Config{ + CachedClientIDRef: "OAUTH_CLIENT_ID_test", + CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", + }, + expected: true, + }, + { + name: "has only cached client secret (invalid state)", + config: Config{ + CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := tt.config.HasCachedClientCredentials() + if result != tt.expected { + t.Errorf("HasCachedClientCredentials() = %v, want %v", result, tt.expected) + } + }) + } +} + +func TestConfig_ClearCachedClientCredentials(t *testing.T) { + t.Parallel() + + config := Config{ + CachedClientIDRef: "OAUTH_CLIENT_ID_test", + CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", + } + + config.ClearCachedClientCredentials() + + if config.CachedClientIDRef != "" { + t.Errorf("CachedClientIDRef should be empty, got %s", config.CachedClientIDRef) + } + if config.CachedClientSecretRef != "" { + t.Errorf("CachedClientSecretRef should be empty, got %s", config.CachedClientSecretRef) + } +} diff --git a/pkg/auth/remote/handler.go b/pkg/auth/remote/handler.go index bf09524510..fa6821c151 100644 --- a/pkg/auth/remote/handler.go +++ b/pkg/auth/remote/handler.go @@ -17,9 +17,10 @@ import ( // Handler handles authentication for remote MCP servers. // Supports OAuth/OIDC-based authentication with automatic discovery. type Handler struct { - config *Config - tokenPersister TokenPersister - secretProvider secrets.Provider + config *Config + tokenPersister TokenPersister + clientCredentialsPersister ClientCredentialsPersister + secretProvider secrets.Provider } // NewHandler creates a new remote authentication handler @@ -40,6 +41,12 @@ func (h *Handler) SetSecretProvider(provider secrets.Provider) { h.secretProvider = provider } +// SetClientCredentialsPersister sets a callback function that will be called +// when DCR client credentials are obtained and need to be persisted. +func (h *Handler) SetClientCredentialsPersister(persister ClientCredentialsPersister) { + h.clientCredentialsPersister = persister +} + // Authenticate is the main entry point for remote MCP server authentication func (h *Handler) Authenticate(ctx context.Context, remoteURL string) (oauth2.TokenSource, error) { // Priority 1: Bearer token authentication (if configured) @@ -175,6 +182,16 @@ func (h *Handler) wrapWithPersistence(result *discovery.OAuthFlowResult) oauth2. } } + // Persist DCR client credentials if available (for servers that use Dynamic Client Registration) + // Only persist if client_id exists - client_secret may be empty for PKCE flows + if h.clientCredentialsPersister != nil && result.ClientID != "" { + if err := h.clientCredentialsPersister(result.ClientID, result.ClientSecret); err != nil { + logger.Warnf("Failed to persist DCR client credentials: %v", err) + } else { + logger.Infof("Successfully persisted DCR client credentials for future restarts") + } + } + // Wrap the token source to persist refreshed tokens tokenSource := result.TokenSource if h.tokenPersister != nil { @@ -184,6 +201,37 @@ func (h *Handler) wrapWithPersistence(result *discovery.OAuthFlowResult) oauth2. return tokenSource } +// resolveClientCredentials returns the client ID and secret to use, preferring +// cached DCR credentials over statically configured ones. +func (h *Handler) resolveClientCredentials(ctx context.Context) (clientID, clientSecret string) { + // First try to use statically configured credentials + clientID = h.config.ClientID + clientSecret = h.config.ClientSecret + + // If we have cached DCR client credentials and a secret provider, use those instead + if h.config.HasCachedClientCredentials() && h.secretProvider != nil { + cachedClientID, err := h.secretProvider.GetSecret(ctx, h.config.CachedClientIDRef) + if err != nil { + logger.Warnf("Failed to retrieve cached client ID, falling back to config: %v", err) + } else { + clientID = cachedClientID + logger.Debugf("Using cached DCR client credentials (client_id: %s)", clientID) + } + + // Client secret may be empty for PKCE flows + if h.config.CachedClientSecretRef != "" { + cachedClientSecret, err := h.secretProvider.GetSecret(ctx, h.config.CachedClientSecretRef) + if err != nil { + logger.Warnf("Failed to retrieve cached client secret: %v", err) + } else { + clientSecret = cachedClientSecret + } + } + } + + return clientID, clientSecret +} + // tryRestoreFromCachedTokens attempts to create a TokenSource from cached tokens func (h *Handler) tryRestoreFromCachedTokens( ctx context.Context, @@ -201,10 +249,13 @@ func (h *Handler) tryRestoreFromCachedTokens( return nil, fmt.Errorf("failed to retrieve cached refresh token: %w", err) } + // Resolve client credentials - prefer cached DCR credentials over config + clientID, clientSecret := h.resolveClientCredentials(ctx) + // Build OAuth2 config for token refresh oauth2Config := &oauth2.Config{ - ClientID: h.config.ClientID, - ClientSecret: h.config.ClientSecret, + ClientID: clientID, + ClientSecret: clientSecret, Scopes: scopes, Endpoint: oauth2.Endpoint{ AuthURL: h.config.AuthorizeURL, diff --git a/pkg/auth/remote/persisting_token_source.go b/pkg/auth/remote/persisting_token_source.go index 3d229da076..a57cba3355 100644 --- a/pkg/auth/remote/persisting_token_source.go +++ b/pkg/auth/remote/persisting_token_source.go @@ -18,6 +18,10 @@ import ( // since the access token can be regenerated from it. type TokenPersister func(refreshToken string, expiry time.Time) error +// ClientCredentialsPersister is called when DCR client credentials need to be persisted. +// This is used to store client_id and client_secret obtained during Dynamic Client Registration. +type ClientCredentialsPersister func(clientID, clientSecret string) error + // PersistingTokenSource wraps an oauth2.TokenSource and persists tokens // whenever they are refreshed. This enables session restoration across // workload restarts without requiring a new browser-based OAuth flow. diff --git a/pkg/auth/secrets/secrets.go b/pkg/auth/secrets/secrets.go index 46121a3817..6f674ea4ec 100644 --- a/pkg/auth/secrets/secrets.go +++ b/pkg/auth/secrets/secrets.go @@ -28,6 +28,8 @@ const ( // TokenTypeOAuthRefreshToken represents a cached OAuth refresh token // #nosec G101 - this is a type identifier, not a credential TokenTypeOAuthRefreshToken TokenType = "oauth_refresh_token" + // TokenTypeOAuthClientID represents a cached OAuth client ID from DCR + TokenTypeOAuthClientID TokenType = "oauth_client_id" ) // tokenTypeConfig holds configuration for each token type @@ -53,6 +55,11 @@ var tokenTypeConfigs = map[TokenType]tokenTypeConfig{ target: "oauth_refresh", errorContext: "OAuth refresh token", }, + TokenTypeOAuthClientID: { + prefix: "OAUTH_CLIENT_ID_", + target: "oauth_client_id", + errorContext: "OAuth client ID", + }, } // ProcessSecret processes a secret, converting plain text to CLI format if needed. diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go index d34ba68315..0a94b96c39 100644 --- a/pkg/runner/runner.go +++ b/pkg/runner/runner.go @@ -605,6 +605,50 @@ func (r *Runner) handleRemoteAuthentication(ctx context.Context) (oauth2.TokenSo logger.Debugf("Stored OAuth refresh token in secret manager as %s", secretName) return nil }) + + // Set up client credentials persister for DCR (Dynamic Client Registration) + authHandler.SetClientCredentialsPersister(func(clientID, clientSecret string) error { + // Generate unique secret names for client credentials + clientIDSecretName, err := authsecrets.GenerateUniqueSecretNameWithPrefix( + r.Config.Name, + "OAUTH_CLIENT_ID_", + secretManager, + ) + if err != nil { + return fmt.Errorf("failed to generate client ID secret name: %w", err) + } + + // Store the client ID in the secret manager + if err := authsecrets.StoreSecretInManagerWithProvider(ctx, clientIDSecretName, clientID, secretManager); err != nil { + return fmt.Errorf("failed to store client ID: %w", err) + } + r.Config.RemoteAuthConfig.CachedClientIDRef = clientIDSecretName + + // Only store client secret if it's non-empty (PKCE flows may not have one) + if clientSecret != "" { + clientSecretSecretName, err := authsecrets.GenerateUniqueSecretNameWithPrefix( + r.Config.Name, + "OAUTH_CLIENT_SECRET_", + secretManager, + ) + if err != nil { + return fmt.Errorf("failed to generate client secret secret name: %w", err) + } + + if err := authsecrets.StoreSecretInManagerWithProvider(ctx, clientSecretSecretName, clientSecret, secretManager); err != nil { + return fmt.Errorf("failed to store client secret: %w", err) + } + r.Config.RemoteAuthConfig.CachedClientSecretRef = clientSecretSecretName + } + + // Save the updated config to persist the references + if err := r.Config.SaveState(ctx); err != nil { + return fmt.Errorf("failed to save config with client credential references: %w", err) + } + + logger.Debugf("Stored DCR client credentials in secret manager (client_id: %s)", clientIDSecretName) + return nil + }) } // Perform authentication From 4351e7e40ee285da37be90e7b2fb3972add7c108 Mon Sep 17 00:00:00 2001 From: Frederic Le Feurmou Date: Thu, 22 Jan 2026 23:04:46 -0600 Subject: [PATCH 2/4] fix(lint): move nosec comment to same line to satisfy gosec Signed-off-by: Frederic Le Feurmou --- pkg/auth/secrets/secrets.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/auth/secrets/secrets.go b/pkg/auth/secrets/secrets.go index 6f674ea4ec..18ca1af727 100644 --- a/pkg/auth/secrets/secrets.go +++ b/pkg/auth/secrets/secrets.go @@ -28,8 +28,8 @@ const ( // TokenTypeOAuthRefreshToken represents a cached OAuth refresh token // #nosec G101 - this is a type identifier, not a credential TokenTypeOAuthRefreshToken TokenType = "oauth_refresh_token" - // TokenTypeOAuthClientID represents a cached OAuth client ID from DCR - TokenTypeOAuthClientID TokenType = "oauth_client_id" + // TokenTypeOAuthClientID represents a cached OAuth client ID from Dynamic Client Registration + TokenTypeOAuthClientID TokenType = "oauth_client_id" // #nosec G101 ) // tokenTypeConfig holds configuration for each token type From 0127cb4a33e13fc990ec457da462d8098b43ee15 Mon Sep 17 00:00:00 2001 From: Frederic Le Feurmou Date: Tue, 27 Jan 2026 20:27:04 -0600 Subject: [PATCH 3/4] fix: address DCR review feedback - 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 --- docs/server/docs.go | 12 ++++++++++-- docs/server/swagger.json | 12 ++++++++++-- docs/server/swagger.yaml | 13 ++++++++++++- pkg/auth/remote/config.go | 15 ++++++++++++--- pkg/auth/remote/config_test.go | 10 +++++----- pkg/auth/remote/handler.go | 20 ++++++++------------ pkg/runner/runner.go | 23 +++++------------------ 7 files changed, 62 insertions(+), 43 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index a3529d85e5..d0eb38ec08 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -800,8 +800,8 @@ const docTemplate = `{ "bearer_token_file": { "type": "string" }, - "cached_client_id_ref": { - "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.", + "cached_client_id": { + "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.\nClientID is stored as plain text since it's public information.", "type": "string" }, "cached_client_secret_ref": { @@ -811,6 +811,14 @@ const docTemplate = `{ "description": "Cached OAuth token reference for persistence across restarts.\nThe refresh token is stored securely in the secret manager, and this field\ncontains the reference to retrieve it (e.g., \"OAUTH_REFRESH_TOKEN_workload\").\nThis enables session restoration without requiring a new browser-based login.", "type": "string" }, + "cached_reg_token_ref": { + "description": "RegistrationAccessToken is used to update/delete the client registration.\nStored as a secret reference since it's sensitive.", + "type": "string" + }, + "cached_secret_expiry": { + "description": "ClientSecretExpiresAt indicates when the client secret expires (if provided by the DCR server).\nA zero value means the secret does not expire.", + "type": "string" + }, "cached_token_expiry": { "type": "string" }, diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 49504517db..93f6ebd632 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -793,8 +793,8 @@ "bearer_token_file": { "type": "string" }, - "cached_client_id_ref": { - "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.", + "cached_client_id": { + "description": "Cached DCR client credentials for persistence across restarts.\nThese are obtained during Dynamic Client Registration and needed to refresh tokens.\nClientID is stored as plain text since it's public information.", "type": "string" }, "cached_client_secret_ref": { @@ -804,6 +804,14 @@ "description": "Cached OAuth token reference for persistence across restarts.\nThe refresh token is stored securely in the secret manager, and this field\ncontains the reference to retrieve it (e.g., \"OAUTH_REFRESH_TOKEN_workload\").\nThis enables session restoration without requiring a new browser-based login.", "type": "string" }, + "cached_reg_token_ref": { + "description": "RegistrationAccessToken is used to update/delete the client registration.\nStored as a secret reference since it's sensitive.", + "type": "string" + }, + "cached_secret_expiry": { + "description": "ClientSecretExpiresAt indicates when the client secret expires (if provided by the DCR server).\nA zero value means the secret does not expire.", + "type": "string" + }, "cached_token_expiry": { "type": "string" }, diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index de3b384345..71c21efcd6 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -722,10 +722,11 @@ components: type: string bearer_token_file: type: string - cached_client_id_ref: + cached_client_id: description: |- Cached DCR client credentials for persistence across restarts. These are obtained during Dynamic Client Registration and needed to refresh tokens. + ClientID is stored as plain text since it's public information. type: string cached_client_secret_ref: type: string @@ -736,6 +737,16 @@ components: contains the reference to retrieve it (e.g., "OAUTH_REFRESH_TOKEN_workload"). This enables session restoration without requiring a new browser-based login. type: string + cached_reg_token_ref: + description: |- + RegistrationAccessToken is used to update/delete the client registration. + Stored as a secret reference since it's sensitive. + type: string + cached_secret_expiry: + description: |- + ClientSecretExpiresAt indicates when the client secret expires (if provided by the DCR server). + A zero value means the secret does not expire. + type: string cached_token_expiry: type: string callback_port: diff --git a/pkg/auth/remote/config.go b/pkg/auth/remote/config.go index ef247f0495..1d885b31c8 100644 --- a/pkg/auth/remote/config.go +++ b/pkg/auth/remote/config.go @@ -57,8 +57,15 @@ type Config struct { // Cached DCR client credentials for persistence across restarts. // These are obtained during Dynamic Client Registration and needed to refresh tokens. - CachedClientIDRef string `json:"cached_client_id_ref,omitempty" yaml:"cached_client_id_ref,omitempty"` + // ClientID is stored as plain text since it's public information. + CachedClientID string `json:"cached_client_id,omitempty" yaml:"cached_client_id,omitempty"` CachedClientSecretRef string `json:"cached_client_secret_ref,omitempty" yaml:"cached_client_secret_ref,omitempty"` + // ClientSecretExpiresAt indicates when the client secret expires (if provided by the DCR server). + // A zero value means the secret does not expire. + CachedSecretExpiry time.Time `json:"cached_secret_expiry,omitempty" yaml:"cached_secret_expiry,omitempty"` + // RegistrationAccessToken is used to update/delete the client registration. + // Stored as a secret reference since it's sensitive. + CachedRegTokenRef string `json:"cached_reg_token_ref,omitempty" yaml:"cached_reg_token_ref,omitempty"` } // BearerTokenEnvVarName is the environment variable name used for bearer token authentication. @@ -149,13 +156,15 @@ func (c *Config) ClearCachedTokens() { // HasCachedClientCredentials returns true if the config has cached DCR client credentials. func (c *Config) HasCachedClientCredentials() bool { - return c.CachedClientIDRef != "" + return c.CachedClientID != "" } // ClearCachedClientCredentials removes any cached DCR client credential references from the config. func (c *Config) ClearCachedClientCredentials() { - c.CachedClientIDRef = "" + c.CachedClientID = "" c.CachedClientSecretRef = "" + c.CachedSecretExpiry = time.Time{} + c.CachedRegTokenRef = "" } // DefaultResourceIndicator derives the resource indicator (RFC 8707) from the remote server URL. diff --git a/pkg/auth/remote/config_test.go b/pkg/auth/remote/config_test.go index 78b127c115..1a231fbf3b 100644 --- a/pkg/auth/remote/config_test.go +++ b/pkg/auth/remote/config_test.go @@ -175,14 +175,14 @@ func TestConfig_HasCachedClientCredentials(t *testing.T) { { name: "has cached client ID only", config: Config{ - CachedClientIDRef: "OAUTH_CLIENT_ID_test", + CachedClientID: "OAUTH_CLIENT_ID_test", }, expected: true, }, { name: "has both cached credentials", config: Config{ - CachedClientIDRef: "OAUTH_CLIENT_ID_test", + CachedClientID: "OAUTH_CLIENT_ID_test", CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", }, expected: true, @@ -211,14 +211,14 @@ func TestConfig_ClearCachedClientCredentials(t *testing.T) { t.Parallel() config := Config{ - CachedClientIDRef: "OAUTH_CLIENT_ID_test", + CachedClientID: "OAUTH_CLIENT_ID_test", CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", } config.ClearCachedClientCredentials() - if config.CachedClientIDRef != "" { - t.Errorf("CachedClientIDRef should be empty, got %s", config.CachedClientIDRef) + if config.CachedClientID != "" { + t.Errorf("CachedClientID should be empty, got %s", config.CachedClientID) } if config.CachedClientSecretRef != "" { t.Errorf("CachedClientSecretRef should be empty, got %s", config.CachedClientSecretRef) diff --git a/pkg/auth/remote/handler.go b/pkg/auth/remote/handler.go index fa6821c151..83927d7ca9 100644 --- a/pkg/auth/remote/handler.go +++ b/pkg/auth/remote/handler.go @@ -208,18 +208,14 @@ func (h *Handler) resolveClientCredentials(ctx context.Context) (clientID, clien clientID = h.config.ClientID clientSecret = h.config.ClientSecret - // If we have cached DCR client credentials and a secret provider, use those instead - if h.config.HasCachedClientCredentials() && h.secretProvider != nil { - cachedClientID, err := h.secretProvider.GetSecret(ctx, h.config.CachedClientIDRef) - if err != nil { - logger.Warnf("Failed to retrieve cached client ID, falling back to config: %v", err) - } else { - clientID = cachedClientID - logger.Debugf("Using cached DCR client credentials (client_id: %s)", clientID) - } - - // Client secret may be empty for PKCE flows - if h.config.CachedClientSecretRef != "" { + // If we have cached DCR client credentials, use those instead + if h.config.HasCachedClientCredentials() { + // ClientID is stored as plain text (it's public information) + clientID = h.config.CachedClientID + logger.Debugf("Using cached DCR client credentials (client_id: %s)", clientID) + + // Client secret is stored securely and may be empty for PKCE flows + if h.config.CachedClientSecretRef != "" && h.secretProvider != nil { cachedClientSecret, err := h.secretProvider.GetSecret(ctx, h.config.CachedClientSecretRef) if err != nil { logger.Warnf("Failed to retrieve cached client secret: %v", err) diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go index 0a94b96c39..066ace5605 100644 --- a/pkg/runner/runner.go +++ b/pkg/runner/runner.go @@ -608,21 +608,8 @@ func (r *Runner) handleRemoteAuthentication(ctx context.Context) (oauth2.TokenSo // Set up client credentials persister for DCR (Dynamic Client Registration) authHandler.SetClientCredentialsPersister(func(clientID, clientSecret string) error { - // Generate unique secret names for client credentials - clientIDSecretName, err := authsecrets.GenerateUniqueSecretNameWithPrefix( - r.Config.Name, - "OAUTH_CLIENT_ID_", - secretManager, - ) - if err != nil { - return fmt.Errorf("failed to generate client ID secret name: %w", err) - } - - // Store the client ID in the secret manager - if err := authsecrets.StoreSecretInManagerWithProvider(ctx, clientIDSecretName, clientID, secretManager); err != nil { - return fmt.Errorf("failed to store client ID: %w", err) - } - r.Config.RemoteAuthConfig.CachedClientIDRef = clientIDSecretName + // Store client ID directly (it's public information) + r.Config.RemoteAuthConfig.CachedClientID = clientID // Only store client secret if it's non-empty (PKCE flows may not have one) if clientSecret != "" { @@ -641,12 +628,12 @@ func (r *Runner) handleRemoteAuthentication(ctx context.Context) (oauth2.TokenSo r.Config.RemoteAuthConfig.CachedClientSecretRef = clientSecretSecretName } - // Save the updated config to persist the references + // Save the updated config to persist the credentials if err := r.Config.SaveState(ctx); err != nil { - return fmt.Errorf("failed to save config with client credential references: %w", err) + return fmt.Errorf("failed to save config with client credentials: %w", err) } - logger.Debugf("Stored DCR client credentials in secret manager (client_id: %s)", clientIDSecretName) + logger.Debugf("Stored DCR client credentials (client_id: %s)", clientID) return nil }) } From 74868face1fb8a2d772bbe14a39411269ff97005 Mon Sep 17 00:00:00 2001 From: Frederic Le Feurmou Date: Fri, 30 Jan 2026 14:47:21 -0600 Subject: [PATCH 4/4] fix: address review feedback - remove unused TokenTypeOAuthClientID - 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 --- pkg/auth/remote/config_test.go | 6 +++--- pkg/auth/secrets/secrets.go | 7 ------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/pkg/auth/remote/config_test.go b/pkg/auth/remote/config_test.go index 1a231fbf3b..bbefc78315 100644 --- a/pkg/auth/remote/config_test.go +++ b/pkg/auth/remote/config_test.go @@ -175,14 +175,14 @@ func TestConfig_HasCachedClientCredentials(t *testing.T) { { name: "has cached client ID only", config: Config{ - CachedClientID: "OAUTH_CLIENT_ID_test", + CachedClientID: "test_client_id", }, expected: true, }, { name: "has both cached credentials", config: Config{ - CachedClientID: "OAUTH_CLIENT_ID_test", + CachedClientID: "test_client_id", CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", }, expected: true, @@ -211,7 +211,7 @@ func TestConfig_ClearCachedClientCredentials(t *testing.T) { t.Parallel() config := Config{ - CachedClientID: "OAUTH_CLIENT_ID_test", + CachedClientID: "test_client_id", CachedClientSecretRef: "OAUTH_CLIENT_SECRET_test", } diff --git a/pkg/auth/secrets/secrets.go b/pkg/auth/secrets/secrets.go index 18ca1af727..46121a3817 100644 --- a/pkg/auth/secrets/secrets.go +++ b/pkg/auth/secrets/secrets.go @@ -28,8 +28,6 @@ const ( // TokenTypeOAuthRefreshToken represents a cached OAuth refresh token // #nosec G101 - this is a type identifier, not a credential TokenTypeOAuthRefreshToken TokenType = "oauth_refresh_token" - // TokenTypeOAuthClientID represents a cached OAuth client ID from Dynamic Client Registration - TokenTypeOAuthClientID TokenType = "oauth_client_id" // #nosec G101 ) // tokenTypeConfig holds configuration for each token type @@ -55,11 +53,6 @@ var tokenTypeConfigs = map[TokenType]tokenTypeConfig{ target: "oauth_refresh", errorContext: "OAuth refresh token", }, - TokenTypeOAuthClientID: { - prefix: "OAUTH_CLIENT_ID_", - target: "oauth_client_id", - errorContext: "OAuth client ID", - }, } // ProcessSecret processes a secret, converting plain text to CLI format if needed.