Add CIMD storage decorator for embedded AS#5343
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5343 +/- ##
==========================================
+ Coverage 68.72% 68.77% +0.04%
==========================================
Files 625 626 +1
Lines 63422 63520 +98
==========================================
+ Hits 43588 43687 +99
+ Misses 16585 16580 -5
- Partials 3249 3253 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
The CIMDStorageDecorator wraps storage.Storage and intercepts GetClient calls for HTTPS client_id values. When the embedded AS receives a client_id like https://vscode.dev/oauth/client-metadata.json, the decorator fetches the CIMD document via pkg/oauthproto/cimd, validates it, builds a fosite.Client, caches the result with a configurable fallback TTL, and deduplicates concurrent fetches for the same URL via singleflight. Key design decisions: - Embeds storage.Storage so all ~30 other methods delegate transparently - Unwrap() exposes the underlying storage for the DCRCredentialStore and RedisStorage type assertions in server_impl.go to reach the concrete backend through the decorator layer - LoopbackClient wraps clients with loopback redirect URIs for RFC 8252 §7.3 dynamic port matching - NewCIMDStorageDecorator returns base unchanged when enabled=false (no allocation); fails loudly for invalid cacheMaxSize runLegacyMigration extracted from newServer to keep the function under the gocyclo limit after the Unwrap additions; both the DCRCredentialStore assertion and the RedisStorage migration now use the same Unwrap pattern. Incorporates all changes from PR 1 (pkg/oauthproto/cimd sub-package, networking.FetchJSON with WithMaxResponseSize, IsPrivateIP reuse). Relates to #4825 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
87e6996 to
24b7094
Compare
PR size has been reduced below the XL threshold. Thank you for splitting this up!
|
✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up! |
There was a problem hiding this comment.
Pull request overview
Adds a storage-layer decorator to enable CIMD (Client ID Metadata Document) client resolution for the embedded authorization server, allowing client_id values that are HTTPS URLs to be fetched/validated on-demand instead of requiring pre-registration via DCR.
Changes:
- Introduces
CIMDStorageDecoratorthat interceptsGetClientfor HTTPSclient_ids, fetches CIMD documents, and caches the resultingfosite.Clientwith LRU + TTL andsingleflightdeduplication. - Adds unit tests covering constructor behavior, delegation for opaque IDs, cache hit/miss/TTL/LRU eviction, singleflight behavior, and loopback redirect wrapping.
- Updates server construction to support decorator-wrapped storages via
Unwrap(), and extracts Redis legacy migration logic intorunLegacyMigration.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| pkg/authserver/storage/cimd_decorator.go | Adds the CIMD-aware Storage decorator, caching, and CIMD→fosite client conversion logic (including loopback redirect handling). |
| pkg/authserver/storage/cimd_decorator_test.go | Adds comprehensive unit tests for decorator behavior (caching, concurrency, unwrap, and client building). |
| pkg/authserver/server_impl.go | Adds Unwrap() handling for storage type assertions and extracts Redis legacy migration into a helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cimd_decorator.go: - Fix docstring: TTL is fixed (not from Cache-Control); Cache-Control parsing is a documented follow-up - Force token_endpoint_auth_method to "none": the embedded AS only advertises "none" in discovery, so accepting other values creates an inconsistent client; always override regardless of what the document says - Fix LoopbackClient dropping TokenEndpointAuthMethod: was passing defaultClient (no auth method) instead of openIDClient.DefaultClient (carries the auth method) to NewLoopbackClient - Fix singleflight context: use context.WithoutCancel(ctx) so one caller cancelling does not abort the shared in-flight fetch for other waiters; HTTP client inside FetchClientMetadataDocument already enforces its own 5-second timeout cimd_decorator_test.go: - Replace flaky time.Sleep with a startBarrier channel: goroutines signal before calling fetchOrCached; draining all signals before closing ready makes singleflight deduplication deterministic - Fix CIDM → CIMD typo in test name server_impl.go: - Fix error message: report baseStore type (concrete backend) not stor type (possibly a decorator) when DCRCredentialStore assertion fails - Extract unwrapStorage helper to deduplicate the identical Unwrap() inline logic in newServer and runLegacyMigration Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Issue 1 (blocking): actually fix LoopbackClient dropping TokenEndpointAuthMethod. Change LoopbackClient to embed *fosite.DefaultOpenIDConnectClient instead of *fosite.DefaultClient, preserving TokenEndpointAuthMethod through the wrapper. Update NewLoopbackClient signature, registration.New(), buildFositeClient, and all test call sites accordingly. Previous "fix" was wrong: openIDClient.DefaultClient is the same pointer as defaultClient — the behaviour was identical to the original broken code. Issue 2: remove dead `enabled bool` field from CIMDStorageDecorator. The constructor returns base unchanged when enabled=false so the struct is never instantiated with enabled=false; the field was never read. Issue 3: add TestCIMDStorageDecorator_FetchOrCached_FetchFailureReturnsNotFound covering the error path and verifying errors.Is(err, fosite.ErrNotFound). Issue 4: add CIMD decorator section to docs/arch/11-auth-server-storage.md covering what the decorator does, the Unwrap() pattern, and the air-gapped caveat. Issue 5: replace //nolint:forcetypeassert with a safe type assertion that returns an observable error instead of panicking on type mismatch. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- server_impl.go: remove stray blank comment line - cimd_decorator_test.go: add TokenEndpointAuthMethod assertion to LoopbackClient test to catch any future regression Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
jhrozek
left a comment
There was a problem hiding this comment.
One question on the token_endpoint_auth_method handling.
The embedded AS only supports "none" for CIMD clients. Previously, buildFositeClient silently overrode any declared auth method to "none", which was misleading: a client that claimed private_key_jwt would be treated as public without any indication of the mismatch. Now fetch() rejects documents that declare a method other than "none" before buildFositeClient is reached. buildFositeClient only fills in "none" as a default when the field is omitted, which matches the comment on defaultCIMDTokenEndpointAuthMethod. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Fix CacheFallbackTTL comment to say it is a fixed TTL (not fallback); matches the fix already applied in PR #5343 - Add TODO(cimd) comment above CIMDRunConfig noting the CRD exposure gap - Add discovery handler tests: CIMDEnabled=true advertises the flag, CIMDEnabled=false omits it, for both AS metadata and OIDC endpoints - Add config defaults tests: CIMDEnabled=true fills in cache size/TTL defaults; CIMDEnabled=false leaves zero fields unchanged - Add resolveCIMDConfig nil-guard test: nil input returns zero values, non-nil passes all fields through Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Summary
Phase 2 PR 2 of CIMD embedded AS support — depends on PR #5320 (merged).
The embedded authorization server currently requires clients to call
/oauth/register(DCR) before the authorization flow. With this PR, clients can present an HTTPS URL asclient_idand the server fetches and validates the CIMD document on demand, with no prior registration.CIMDStorageDecoratorwrapsstorage.Storage, overrides onlyGetClient, and delegates all other methods transparently. When fosite callsGetClient("https://vscode.dev/oauth/client-metadata.json"), the decorator fetches the document viapkg/oauthproto/cimd.FetchClientMetadataDocument, builds afosite.Client, caches it with a configurable TTL viahashicorp/golang-lru/v2, and deduplicates concurrent fetches for the same URL viasingleflight.Unwrap()exposes the underlying storage so theDCRCredentialStoreandRedisStoragetype assertions inserver_impl.gostill reach the concrete backend through the decorator layer.runLegacyMigrationhelper extracted fromnewServerto keep cyclomatic complexity under the lint limit after the Unwrap additions.Type of change
Test plan
go test ./pkg/authserver/storage/...— all pass including cache hit/miss, singleflight deduplication, TTL expiry,Unwrap, disabled path, DCR client unaffectedgo test ./pkg/authserver/...— 13 packages passSpecial notes for reviewers
NewCIMDStorageDecoratorreturnsbaseunchanged whenenabled=false— no allocation, no behavior change for existing deploymentsLoopbackClientwraps clients whoseredirect_uriscontain loopback addresses, enabling RFC 8252 §7.3 dynamic port matching for native app clientsGenerated with Claude Code