Renew public client TTL on read in Redis store#5469
Open
mani-muon wants to merge 1 commit into
Open
Conversation
Public clients registered via DCR get DefaultPublicClientTTL stamped on their Redis registration at registration time, and GetClient only reads it back. The TTL is therefore a fixed window from registration: an actively-used public client (e.g. a long-lived MCP desktop/IDE client that caches its client_id) is evicted DefaultPublicClientTTL after it registered, regardless of use. Its next token request then fails at the token endpoint with invalid_client, and the only recovery is to fully re-register the client. Renew the TTL to DefaultPublicClientTTL on a successful GetClient for public clients, turning expiry into a sliding window of inactivity. Abandoned clients still expire after the window, preserving the anti-bloat intent of RegisterClient; confidential clients have no TTL and are left untouched. Renewal is best-effort: a failure is logged and does not fail the lookup. The in-memory backend stores clients without a TTL, so it is unaffected. Add a unit test covering both the public-client renewal and the confidential-client no-TTL invariant. Signed-off-by: Mani Japra <mani@muonspace.com>
mani-muon
requested review from
ChrisJBurns,
JAORMX,
jhrozek,
rdimitrov and
tgrunnagle
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Public clients registered through Dynamic Client Registration (DCR) get
DefaultPublicClientTTL(30 days) stamped on their Redis registration at registration time, andGetClientonly reads it back. Because the read path never renews the TTL, expiry is a fixed window from registration rather than from last use: an actively-used public client — e.g. a long-lived MCP desktop/IDE client that caches itsclient_idindefinitely — is evicted 30 days after it registered regardless of how often it is used. Its next token request then fails at the token endpoint withinvalid_client, and the only recovery is to fully re-register the client (which silently recurs ~monthly).DefaultPublicClientTTLon a successfulGetClientfor public clients, turning expiry into a sliding window of inactivity.RegisterClient.slog.Warn) and does not fail the lookup.Type of change
Test plan
task test)task test-e2e)task lint-fix)Ran the
pkg/authserver/storageunit tests with-race(the Taskfile'sgo testinvocation, scoped to that package) — all pass. Verified the new test fails without the fix (the aged TTL is not renewed) and passes with it, and that confidential clients gain no TTL on read. Rango veton the package. golangci-lint was not run locally; leaving the full lint to CI.Does this introduce a user-facing change?
Yes. Long-lived public OAuth/MCP clients (e.g. Claude Desktop, Cursor, VS Code) backed by the Redis storage no longer stop working ~30 days after they were first authorized; an in-use client's registration stays valid as long as it keeps being used. No configuration change is required. The in-memory backend is unaffected.
Special notes for reviewers
memory.go) stores clients in a bare map with no TTL, so public clients there never expire — the two backends already diverge on this and this PR does not change in-memory behavior.GetClientissues an extraEXPIREper call for public clients. If the per-request round-trip is a concern, a natural follow-up is to renew only when the remaining TTL is below a threshold; I kept this change minimal and can add that if preferred.