Persist resolved CIMD clients so Redis session rehydration finds them - #6284
Persist resolved CIMD clients so Redis session rehydration finds them#6284alex-feel wants to merge 2 commits into
Conversation
e9f57ef to
ea441ee
Compare
|
Quick note on the latest push: after #6215 merged, I rebased this branch onto main, and the diff got smaller as a result. Previously this PR carried extra handling for the The rebased diff is down to four files, and the miniredis rehydration test still passes end to end. Happy to adjust if a different approach looks better. |
ea441ee to
c107944
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6284 +/- ##
==========================================
+ Coverage 78.02% 78.10% +0.08%
==========================================
Files 767 767
Lines 74343 74372 +29
==========================================
+ Hits 58003 58091 +88
+ Misses 16335 16276 -59
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Nice catch on the root cause — the diagnosis is right and the tests are good. Closing the document server before rehydration in TestRedisStorage_CIMDClientSessionRehydration to prove no re-fetch is involved is a genuinely well-built test.
One thing I'd want addressed before this lands (the rate-limit gap), plus a few smaller ones inline.
c107944 to
24345eb
Compare
|
Addressed all five points in 24345eb, kept as a separate commit for easier re-review, on top of a rebase over current main. The Lint Go Code failure is fixed too: both findings appear only when this branch is combined with #6400's changes to the same function (gocyclo on fetch, resolved by extracting the scope-resolution branch, plus a redundant embedded-field selector on the write-through call). Also repurposed TestBuildFositeClient_TokenEndpointAuthMethodDefault into a direct test of the declared-"none"-plus-plural-list branch, per your note on #6400. |
|
The one red check (Operator Tests Integration) is the known flake #6155: a deletion spec in the mcp-oidc-config suite timing out at exactly 30.000s against the controller's 30s deletion requeue. This PR touches nothing under cmd/thv-operator, and the same job is green on #6283 and #6288, which sit on the same main base. A re-run should clear it. |
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Went through all the review comments again against the latest commit (24345eb). The rate limiter, the disabled-CIMD shape guard, the RegisterClient doc fix, and the delegate-client validation all look right and match what we discussed. The logging nit is mostly addressed too - duplication is gone, Warn is kept intentionally for diagnosability which I'm fine with.
LGTM.
|
oops sorry it looks like a rebase is needed, likely a conflict with your other PR |
Redis session rehydration resolves the session's client through the bare RedisStorage row lookup, never through the CIMD storage decorator, so a CIMD client that was resolved dynamically at authorize time was not found at the token endpoint and every exchange failed with a bare invalid_grant. Persist the resolved client into the underlying storage on every successful document fetch (best-effort write-through), marked DCR-issued so the row carries the same anti-bloat TTL as DCR registrations. Rehydration then finds the persisted snapshot without a document re-fetch, so established sessions also survive pod restarts and document rotation. The rehydration client lookup now also logs the client id and the wrapped error, which previously left no server-side trace. Closes stacklok#6187 Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
Add a rate.Limiter to /oauth/authorize, gating only requests whose client_id is a CIMD URL, mirroring the existing /oauth/register limiter. The gate fires before fosite's NewAuthorizeRequest so a flood of distinct CIMD client_id values cannot mint persisted rows faster than the limit. DCR (non-URL) client_id values are never gated. Wrap storage with a new shape-guard decorator whenever CIMD is disabled, so GetClient refuses any URL-shaped client_id outright instead of resolving a stale row a prior CIMD-enabled period may have persisted. Document the resulting lifecycle on Config.CIMDEnabled: a persisted row outlives the flag being turned off, and re-enabling it before the row's TTL expires makes that snapshot resolvable again without a fresh fetch. Correct the RegisterClient doc comment to describe the real upsert semantics both backends implement, since the write-through path now depends on re-registration renewing an existing row's TTL. Reject URL-shaped client_id values for pre-provisioned delegate clients in config validation, since such a collision would let an unauthenticated CIMD resolution overwrite a confidential delegate client in place. Remove the duplicate client-id reporting in the Redis session-rehydration failure path: the client id and error are logged once, and the error returned to callers no longer repeats the client id since all call sites already propagate it up to fosite. Repurpose the auth-method-default test to exercise the negotiation branch it actually now covers: a document that already declares "none" short-circuits before the plural token_endpoint_auth_methods_supported list is ever consulted. Rename the scope-resolution helper to resolveScopes, dropping the CIMD prefix its CIMDStorageDecorator receiver already carries, and drop a redundant embedded-field selector on the write-through RegisterClient call.
24345eb to
4a4e7f2
Compare
|
Rebased onto main now that #6283 has landed. The conflict was confined to cimd_decorator.go, where the two PRs carried overlapping structure. The rebase keeps the landed #6283 code as the base, including the resolveScopes rename this branch already used, and re-applies this PR's delta on top. Everything from the reviewed 24345eb survives unchanged: the write-through persistence, the authorize rate limiter, the disabled-CIMD shape guard, the delegate-client validation, and the doc and logging fixes. The diff shrank from +582/−38 to +554/−21 because the helper extraction now comes from main. go build, go test ./pkg/authserver/..., and golangci-lint run clean locally. The two red checks on this head are both setup-stage network failures on the kindest v1.35.1 legs, before any test ran: the lifecycle job hit |
2 participants
Closes #6187
Problem
RedisStorage's session rehydration (unmarshalRequester) resolves the session's client through the bare storage's ownGetClientrow lookup — never through theCIMDStorageDecorator. A CIMD client is resolved dynamically at authorize time and never persisted, so at the token endpoint the lookup fails and every exchange for a CIMD client ends in a bareinvalid_grant, with no server-side log line. Memory storage keeps the live client object, so only the CIMD × Redis combination breaks (see #6187 for the analysis and repro).Change
Of the two directions proposed in the issue, this implements the persistence one, as laid out in #6187 (comment):
RegisterClient(write-through; a persistence failure WARN-logs and does not fail the resolution).registration.MarkDCRIssued(since Accept localhost dynamic-port loopback redirect_uris #6215, a CIMD-resolved client is always a shape it already handles), so the row carries the same anti-bloat TTL as DCR registrations andRenewClientTTLkeeps actively-used rows alive — unauthenticated/oauth/authorizetraffic can mint these rows, so they must never be permanent.unmarshalRequesternow WARN-logs the client id and the wrapped error when the client lookup fails, so this failure mode is no longer silent server-side./oauth/authorizebeforeNewAuthorizeRequest, URL-shaped client ids from storage are refused while CIMD is disabled, URL-shaped ids are rejected for pre-provisioned delegate clients in config validation, theRegisterClientdoc comment now states the upsert contract, and the expired-client log path is deduplicated.Tests
RegisterClientdoes not fail resolution.Note: rebased on top of the merged #6283; the two changes share the decorator file, and this PR's delta is re-derived over the landed structure.