Skip to content

Add dcrConfig.rotationKey to re-register upstream DCR clients - #6825

Merged
jhrozek merged 1 commit into
stacklok:mainfrom
aron-muon:dcr-rotation
Oct 15, 2026
Merged

jhrozek merged 1 commit into
stacklok:mainfrom
aron-muon:dcr-rotation

Conversation

@aron-muon

Copy link
Copy Markdown
Contributor

Summary

An embedded auth server that registers its upstream OAuth client through RFC 7591
Dynamic Client Registration resolves the registration once per process start and
serves it from the credential store on every later start. The only invalidation
is the upstream's client_secret_expires_at, which public-client upstreams never
send, so when the upstream drops or invalidates the registration the proxy keeps
presenting the dead client_id indefinitely. The rejection happens in the user's
browser on the upstream's /authorize page, so nothing in ToolHive observes it.
Today the only remedy is deleting the dcr: row from Redis by hand and
restarting (#6821 covers the automatic recovery path; this change adds the
operator-driven one, which is also needed when the upstream does not
authenticate clients before validating grants or when a rotation is wanted on
request).

  • Add dcrConfig.rotationKey, an optional operator-chosen label
    (^[A-Za-z0-9._-]{1,64}$) on MCPExternalAuthConfig upstream providers and
    on the authserver.DCRUpstreamConfig run-config. It is never sent to the
    upstream.
  • Fold the value into the DCR cache identity (storage.DCRKey, redisDCRKey,
    the memory-store map key, the persisted row and the resolver's singleflight
    key) only when non-empty. Changing it makes the next start register a fresh
    client; an empty value addresses exactly the pre-existing key, so existing
    rows stay reachable and no migration is needed.
  • Log the rotation key on the dcr: cache hit and dcr: registered new client
    records so an operator can confirm which generation a replica runs.
  • Validate the alphabet/length on the run-config path (mirrors the CRD markers
    for YAML/CLI configs) and regenerate CRDs and CRD API docs.

Related to #6821, #6496, #6508.

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test) — every package touched here passes with -race
    (pkg/auth/dcr, pkg/authserver, pkg/authserver/storage, pkg/authserver/runner,
    cmd/thv-operator/pkg/controllerutil, cmd/thv-operator/api/...); the remaining
    failures on the dev machine are pre-existing environment failures in untouched
    packages (in-cluster runtime detection, root user). New coverage:
    TestResolveDCRCredentials_RotationKeyForcesFreshRegistration,
    TestFlightKeyOf_RotationKey, TestRedisStorage_DCRCredentials_RotationKeyRoundTrip,
    the redisDCRKey with rotation key format case, rotation-key entries in the
    memory/store distinct-key tests, run-config validation cases, newDCRRequest
    propagation and the operator CRD-to-run-config conversion.
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

rotationKey is a new optional string field on DCRUpstreamConfig; existing
objects are unaffected. The persisted row gains an omitempty field and the Redis
key format is unchanged for an empty rotation key.

Changes

File Change
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go rotationKey on DCRUpstreamConfig with Pattern/MaxLength markers
cmd/thv-operator/pkg/controllerutil/authserver.go copy the field into the run-config
pkg/authserver/config.go rotation_key on DCRUpstreamConfig; alphabet/length validation
pkg/authserver/runner/dcr_adapter.go pass the field to dcr.Request
pkg/auth/dcr/request.go, resolver.go, store.go Request.RotationKey; key construction, singleflight key, log attributes
pkg/authserver/storage/types.go, redis_keys.go, redis.go DCRKey.RotationKey; key encoding when non-empty; persisted key_rotation_key
deploy/charts/operator-crds/**, docs/operator/crd-api.md generated
docs/arch/11-auth-server-storage.md describe the key and the rotation behaviour

Does this introduce a user-facing change?

Yes. Operators can set dcrConfig.rotationKey on an embedded auth server's
upstream provider (CRD or YAML run-config). Changing the value makes the proxy
register a new client with the upstream at its next start instead of reusing
the cached registration. The previous registration's row stays in storage, and
on upstreams that bind refresh tokens to the client, users signed in through the
previous client re-authenticate once.

Special notes for reviewers

  • Scope is deliberately limited to the explicit rotation lever. The automatic
    path (probe the cached client at startup, replace the row) is being handled
    under Cached upstream DCR registration never recovers after the upstream evicts it #6821; the two do not touch the same lines and can land in either
    order. A time-based maxRegistrationAge would also fit on top of that work.
  • The old row is intentionally not deleted: the resolver does not know the
    previous key value, and the row carries no TTL unless the upstream asserted
    client_secret_expires_at, which matches the existing behaviour for any
    key-format change (see the note in redis_keys.go).
  • RotationKey is appended to the Redis key and the singleflight key as a
    length-prefixed segment after the hex scopes hash, so the empty and non-empty
    shapes cannot be confused.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.68%. Comparing base (6851793) to head (dd6ef24).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6825   +/-   ##
=======================================
  Coverage   79.68%   79.68%           
=======================================
  Files         815      815           
  Lines       82940    82955   +15     
=======================================
+ Hits        66088    66100   +12     
- Misses      16847    16850    +3     
  Partials        5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

An embedded auth server resolves its upstream RFC 7591 registration once
per process start and then serves the cached client_id from the credential
store forever: the only invalidation is client_secret_expires_at, which
public-client upstreams never send. When the upstream drops the
registration, the rejection surfaces only in the user's browser, the proxy
keeps presenting the dead client_id on every start, and the sole remedy is
deleting the Redis row by hand and restarting.

Add dcrConfig.rotationKey, an optional operator-chosen label that is folded
into the DCR cache identity (DCRKey, the Redis key, the persisted row, the
memory map key and the singleflight key) only when non-empty and is never
sent to the upstream. Bumping it makes the next start register a fresh
client; an empty value addresses exactly the pre-existing key, so existing
rows need no migration. The run-config validates the same alphabet and
length the CRD markers enforce, and the cache-hit and registration logs
carry the key so operators can confirm which generation a replica runs.

Related to stacklok#6821 (automatic recovery), stacklok#6496, stacklok#6508.

Signed-off-by: Aron Gates <aron@muonspace.com>

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jhrozek
jhrozek merged commit 14c6066 into stacklok:main Oct 15, 2026
44 checks passed
@github-actions github-actions Bot mentioned this pull request Oct 19, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants