Implement Client Secret Expiry and Renewal for Dynamic Client Registration

pkg/auth/discovery/discovery.go

@@ -512,6 +512,14 @@ type OAuthFlowConfig struct {
 	Resource             string // RFC 8707 resource indicator (optional)
 	OAuthParams          map[string]string
 	ScopeParamName       string // Override scope query parameter name (e.g., "user_scope" for Slack)
+
+	// DCR renewal metadata — populated by handleDynamicRegistration and threaded
+	// into OAuthFlowResult so callers can persist the data for RFC 7592 operations.
+	SecretExpiry            time.Time // zero means the secret never expires
+	RegistrationAccessToken string    //nolint:gosec // G117: field legitimately holds sensitive data
+	RegistrationClientURI   string
+	TokenEndpointAuthMethod string
+	RegisteredCallbackPort  int
 }
 
 // OAuthFlowResult contains the result of an OAuth flow
@@ -527,6 +535,16 @@ type OAuthFlowResult struct {
 	// DCR client credentials for persistence (obtained during Dynamic Client Registration)
 	ClientID     string
 	ClientSecret string //nolint:gosec // G117: field legitimately holds sensitive data
+
+	// DCR renewal metadata (RFC 7591 §3.2.1 / RFC 7592).
+	// SecretExpiry is zero when the provider did not issue an expiring secret.
+	// RegistrationAccessToken and RegistrationClientURI are empty when the
+	// provider does not support RFC 7592 management operations.
+	SecretExpiry            time.Time
+	RegistrationAccessToken string //nolint:gosec // G117: field legitimately holds sensitive data
+	RegistrationClientURI   string
+	TokenEndpointAuthMethod string
+	RegisteredCallbackPort  int
 }
 
 func shouldDynamicallyRegisterClient(config *OAuthFlowConfig) bool {
@@ -599,20 +617,12 @@ func PerformOAuthFlow(ctx context.Context, issuer string, config *OAuthFlowConfi
 // exists.
 //
 // One consequence of option (b) is that the resolver's RFC 7591 §3.2.1
-// expiry-driven refetch does NOT participate in the CLI's cross-
-// invocation persistence loop: each PerformOAuthFlow call builds a fresh
-// in-memory store, so a "cached but expired" entry from the previous
-// invocation never reaches the resolver. Cross-invocation expiry is also
-// NOT enforced by the remote handler's gate today —
-// HasCachedClientCredentials only checks CachedClientID != "" and does
-// not consult CachedSecretExpiry, so an expired-but-still-cached client
-// gets reused on the next invocation and surfaces as a token-endpoint
-// failure rather than a clean DCR re-registration. Tightening the gate
-// to also check CachedSecretExpiry is open follow-up work; the
-// behaviour today is "cross-invocation expiry is unhandled". Within a
-// single invocation, the resolver's expiry check is still in the loop
-// and would fire if the same call site somehow registered, persisted,
-// and re-queried the in-memory store — but the CLI never does this today.
+// expiry-driven refetch does NOT participate in the CLI's cross-invocation
+// persistence loop: each PerformOAuthFlow call builds a fresh in-memory store,
+// so a cached entry from a previous invocation never reaches the resolver.
+// Cross-invocation client-secret expiry is handled instead by the remote
+// handler, which consults CachedSecretExpiry and renews through RFC 7592 before
+// cached credentials are used.
 //
 // Wrapping the remote handler's secretProvider into a dcr.CredentialStore
 // adapter (option (a)) would close that loop and is the natural follow-up;
@@ -660,6 +670,18 @@ func handleDynamicRegistration(ctx context.Context, issuer string, config *OAuth
 		config.TokenURL = resolution.TokenEndpoint
 	}
 
+	// Store DCR renewal metadata for RFC 7592 operations.
+	// A zero ClientSecretExpiresAt means the secret never expires (RFC 7591 §3.2.1).
+	config.SecretExpiry = resolution.ClientSecretExpiresAt
+	config.RegistrationAccessToken = resolution.RegistrationAccessToken
+	config.RegistrationClientURI = resolution.RegistrationClientURI
+	config.TokenEndpointAuthMethod = resolution.TokenEndpointAuthMethod
+	config.RegisteredCallbackPort = config.CallbackPort
+
+	if resolution.RegistrationAccessToken != "" {
+		slog.Debug("DCR response includes registration access token for RFC 7592 operations")
+	}
+
 	return nil
 }
 
@@ -892,6 +914,12 @@ func newOAuthFlow(ctx context.Context, oauthConfig *oauth.Config, config *OAuthF
 		Expiry:       tokenResult.Expiry,
 		ClientID:     oauthConfig.ClientID,
 		ClientSecret: oauthConfig.ClientSecret,
+		// DCR renewal metadata — populated only when dynamic registration was performed.
+		SecretExpiry:            config.SecretExpiry,
+		RegistrationAccessToken: config.RegistrationAccessToken,
+		RegistrationClientURI:   config.RegistrationClientURI,
+		TokenEndpointAuthMethod: config.TokenEndpointAuthMethod,
+		RegisteredCallbackPort:  config.RegisteredCallbackPort,
 	}, nil
 }
 

pkg/auth/remote/config.go

@@ -11,7 +11,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/stacklok/toolhive-core/registry/types"
+	registry "github.com/stacklok/toolhive-core/registry/types"
 	httpval "github.com/stacklok/toolhive-core/validation/http"
 )
 
@@ -68,7 +68,8 @@ type Config struct {
 	// 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.
+	// CachedRegTokenRef is a secret manager reference to the registration_access_token
+	// returned in the DCR response. Used for RFC 7592 client update operations.
 	// Stored as a secret reference since it's sensitive.
 	CachedRegTokenRef string `json:"cached_reg_token_ref,omitempty" yaml:"cached_reg_token_ref,omitempty"`
 
@@ -78,6 +79,17 @@ type Config struct {
 	// rotation clears CachedClientID without touching the stable CIMD URL.
 	// Read by resolveClientCredentials to send the correct client_id on token refresh.
 	CachedCIMDClientID string `json:"cached_cimd_client_id,omitempty" yaml:"cached_cimd_client_id,omitempty"`
+	// CachedRegClientURI is the registration_client_uri from the DCR response.
+	// This is the endpoint used for RFC 7592 client read/update/delete operations.
+	// Stored as plain text since it is not sensitive.
+	CachedRegClientURI string `json:"cached_reg_client_uri,omitempty" yaml:"cached_reg_client_uri,omitempty"`
+	// CachedTokenEndpointAuthMethod is the auth method used for the token endpoint
+	// (e.g., "client_secret_basic", "none"). Persisted for RFC 7592 updates.
+	CachedTokenEndpointAuthMethod string `json:"cached_token_auth_method,omitempty" yaml:"cached_token_auth_method,omitempty"`
+	// CachedDCRCallbackPort is the callback port that was actually registered
+	// during DCR. It may differ from CallbackPort when the requested port was
+	// unavailable and a fallback port was selected.
+	CachedDCRCallbackPort int `json:"cached_dcr_callback_port,omitempty" yaml:"cached_dcr_callback_port,omitempty"`
 }
 
 // BearerTokenEnvVarName is the environment variable name used for bearer token authentication.
@@ -184,6 +196,9 @@ func (c *Config) ClearCachedClientCredentials() {
 	c.CachedClientSecretRef = ""
 	c.CachedSecretExpiry = time.Time{}
 	c.CachedRegTokenRef = ""
+	c.CachedRegClientURI = ""
+	c.CachedTokenEndpointAuthMethod = ""
+	c.CachedDCRCallbackPort = 0
 }
 
 // LogContext returns the upstream issuer and resolved client_id for use as

pkg/auth/remote/handler.go

@@ -9,10 +9,12 @@ import (
 	"fmt"
 	"log/slog"
 	"strings"
+	"time"
 
 	"golang.org/x/oauth2"
 
 	"github.com/stacklok/toolhive/pkg/auth/discovery"
+	"github.com/stacklok/toolhive/pkg/networking"
 	"github.com/stacklok/toolhive/pkg/oauthproto"
 	"github.com/stacklok/toolhive/pkg/secrets"
 )
@@ -24,6 +26,7 @@ type Handler struct {
 	tokenPersister             TokenPersister
 	clientCredentialsPersister ClientCredentialsPersister
 	secretProvider             secrets.Provider
+	httpClient                 networking.HTTPClient
 }
 
 // NewHandler creates a new remote authentication handler
@@ -50,6 +53,11 @@ func (h *Handler) SetClientCredentialsPersister(persister ClientCredentialsPersi
 	h.clientCredentialsPersister = persister
 }
 
+// SetHTTPClient sets the HTTP client used for RFC 7592 registration management requests.
+func (h *Handler) SetHTTPClient(client networking.HTTPClient) {
+	h.httpClient = client
+}
+
 // 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)
@@ -207,7 +215,15 @@ func (h *Handler) wrapWithPersistence(result *discovery.OAuthFlowResult) oauth2.
 	// CIMD client IDs (HTTPS URLs) are stable constants and are stored separately below.
 	if h.clientCredentialsPersister != nil && result.ClientID != "" &&
 		!oauthproto.IsClientIDMetadataDocumentURL(result.ClientID) {
-		if err := h.clientCredentialsPersister(result.ClientID, result.ClientSecret); err != nil {
+		if err := h.clientCredentialsPersister(
+			result.ClientID,
+			result.ClientSecret,
+			result.SecretExpiry,
+			result.RegistrationAccessToken,
+			result.RegistrationClientURI,
+			result.TokenEndpointAuthMethod,
+			result.RegisteredCallbackPort,
+		); err != nil {
 			slog.Warn("Failed to persist DCR client credentials", "error", err)
 		} else {
 			slog.Debug("Successfully persisted DCR client credentials for future restarts")
@@ -232,6 +248,8 @@ func (h *Handler) wrapWithPersistence(result *discovery.OAuthFlowResult) oauth2.
 
 // resolveClientCredentials returns the client ID and secret to use, preferring
 // cached DCR credentials over statically configured ones.
+// If the cached client secret is expiring soon, it attempts renewal via RFC 7592
+// before returning the credentials.
 func (h *Handler) resolveClientCredentials(ctx context.Context) (clientID, clientSecret string) {
 	// First try to use statically configured credentials
 	clientID = h.config.ClientID
@@ -252,6 +270,18 @@ func (h *Handler) resolveClientCredentials(ctx context.Context) (clientID, clien
 		clientID = h.config.CachedClientID
 		slog.Debug("Using cached DCR client credentials", "client_id", clientID)
 
+		// Proactively renew the client secret if it is expiring soon (RFC 7592)
+		if h.isSecretExpiredOrExpiringSoon() {
+			slog.Debug("Cached client secret is expiring soon, attempting renewal",
+				"expiry", h.config.CachedSecretExpiry)
+			if renewErr := h.renewClientSecret(ctx); renewErr != nil {
+				slog.Warn("Failed to proactively renew client secret; continuing with existing secret",
+					"error", renewErr)
+			} else {
+				slog.Debug("Successfully renewed client secret ahead of expiry")
+			}
+		}
+
 		// 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)
@@ -278,6 +308,27 @@ func (h *Handler) tryRestoreFromCachedTokens(
 		return nil, fmt.Errorf("secret provider not configured, cannot restore cached tokens")
 	}
 
+	// Check if the cached client secret is expired before attempting token refresh.
+	// If it has fully expired and renewal also fails we must force a fresh OAuth flow.
+	if h.isSecretExpiredOrExpiringSoon() {
+		slog.Debug("Cached client secret is expiring or expired; attempting renewal before token restore",
+			"expiry", h.config.CachedSecretExpiry)
+		if renewErr := h.renewClientSecret(ctx); renewErr != nil {
+			slog.Warn("Client secret renewal failed", "error", renewErr)
+			// Hard-fail only when the secret is already past its expiry.
+			// If we are still in the buffer window the existing secret may work.
+			if !h.config.CachedSecretExpiry.IsZero() && time.Now().After(h.config.CachedSecretExpiry) {
+				return nil, fmt.Errorf(
+					"client secret expired at %v and renewal failed: %w",
+					h.config.CachedSecretExpiry, renewErr)
+			}
+			// Still within buffer — log and continue with the existing (still-valid) secret
+			slog.Warn("Proceeding with expiring client secret after failed renewal attempt")
+		} else {
+			slog.Debug("Successfully renewed client secret before token restore")
+		}
+	}
+
 	refreshToken, err := h.secretProvider.GetSecret(ctx, h.config.CachedRefreshTokenRef)
 	if err != nil {
 		return nil, fmt.Errorf("failed to retrieve cached refresh token: %w", err)