-
Notifications
You must be signed in to change notification settings - Fork 224
Add CIMD storage decorator for embedded AS #5343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
amirejaz
wants to merge
7
commits into
main
Choose a base branch
from
cimd-phase2-pr2-storage-decorator
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
24b7094
Add CIMD storage decorator for embedded AS (Phase 2 PR 2)
amirejaz 030a8e3
Address Copilot review comments on CIMD storage decorator
amirejaz a721736
Address agent review feedback on CIMD storage decorator
amirejaz 9ff0c4c
Fix two remaining nits
amirejaz 223c488
Reject CIMD docs that declare unsupported token_endpoint_auth_method
amirejaz 225df90
Merge branch 'main' into cimd-phase2-pr2-storage-decorator
amirejaz 52fe1dd
Merge branch 'main' into cimd-phase2-pr2-storage-decorator
amirejaz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package storage | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/url" | ||
| "strings" | ||
| "time" | ||
|
|
||
| lru "github.com/hashicorp/golang-lru/v2" | ||
| "github.com/ory/fosite" | ||
| "golang.org/x/sync/singleflight" | ||
|
|
||
| "github.com/stacklok/toolhive/pkg/authserver/server/registration" | ||
| "github.com/stacklok/toolhive/pkg/oauthproto" | ||
| "github.com/stacklok/toolhive/pkg/oauthproto/cimd" | ||
| ) | ||
|
|
||
| // CIMDStorageDecorator wraps storage.Storage and intercepts GetClient calls | ||
| // for HTTPS client_id values, fetching and caching the corresponding Client | ||
| // ID Metadata Document instead of requiring prior DCR registration. | ||
| // | ||
| // All other Storage methods delegate to the underlying storage unchanged. | ||
| // Only GetClient is overridden. DCR clients (opaque IDs) continue to work | ||
| // exactly as before. | ||
| type CIMDStorageDecorator struct { | ||
| Storage // embed full interface — all methods delegate | ||
| sf singleflight.Group // deduplicates concurrent fetches for the same URL | ||
| cache *lru.Cache[string, *cimdCacheEntry] | ||
| ttl time.Duration | ||
| } | ||
|
|
||
| type cimdCacheEntry struct { | ||
| client fosite.Client | ||
| expires time.Time | ||
| } | ||
|
|
||
| // NewCIMDStorageDecorator wraps base with CIMD client lookup. When enabled=false | ||
| // it returns base unchanged (no allocation). cacheMaxSize must be >= 1; | ||
| // fallbackTTL is the fixed TTL applied to every cache entry (Cache-Control | ||
| // header parsing is not yet implemented; all entries use this value). | ||
| func NewCIMDStorageDecorator( | ||
| base Storage, | ||
| enabled bool, | ||
| cacheMaxSize int, | ||
| fallbackTTL time.Duration, | ||
| ) (Storage, error) { | ||
|
amirejaz marked this conversation as resolved.
|
||
| if !enabled { | ||
| return base, nil | ||
| } | ||
|
|
||
| if cacheMaxSize < 1 { | ||
| return nil, fmt.Errorf("CIMD storage decorator cacheMaxSize must be >= 1, got %d", cacheMaxSize) | ||
| } | ||
|
|
||
| c, err := lru.New[string, *cimdCacheEntry](cacheMaxSize) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create CIMD LRU cache: %w", err) | ||
| } | ||
|
|
||
| return &CIMDStorageDecorator{ | ||
| Storage: base, | ||
| cache: c, | ||
| ttl: fallbackTTL, | ||
| }, nil | ||
| } | ||
|
|
||
| // GetClient intercepts HTTPS client_id values to resolve them via CIMD. | ||
| // Opaque DCR-issued IDs are delegated to the underlying storage unchanged. | ||
| func (d *CIMDStorageDecorator) GetClient(ctx context.Context, id string) (fosite.Client, error) { | ||
| if !oauthproto.IsClientIDMetadataDocumentURL(id) { | ||
| return d.Storage.GetClient(ctx, id) | ||
| } | ||
| return d.fetchOrCached(ctx, id) | ||
| } | ||
|
|
||
| // Unwrap returns the underlying storage so that type assertions (e.g. for | ||
| // storage.DCRCredentialStore in server_impl.go) can reach the concrete type. | ||
| func (d *CIMDStorageDecorator) Unwrap() Storage { | ||
| return d.Storage | ||
| } | ||
|
|
||
| func (d *CIMDStorageDecorator) fetchOrCached(ctx context.Context, id string) (fosite.Client, error) { | ||
| // Check cache first (outside singleflight to avoid holding the group lock for cache hits) | ||
| if entry, ok := d.cache.Get(id); ok && time.Now().Before(entry.expires) { | ||
| return entry.client, nil | ||
| } | ||
|
|
||
| // Deduplicate concurrent fetches for the same URL. The shared fetch uses a | ||
| // context detached from the caller so that one caller cancelling does not | ||
| // abort the in-flight request for other waiters. The HTTP client inside | ||
| // FetchClientMetadataDocument enforces its own 5-second timeout. | ||
| fetchCtx := context.WithoutCancel(ctx) | ||
| result, err, _ := d.sf.Do(id, func() (interface{}, error) { | ||
| // Re-check cache inside singleflight (another goroutine may have populated it) | ||
| if entry, ok := d.cache.Get(id); ok && time.Now().Before(entry.expires) { | ||
| return entry.client, nil | ||
| } | ||
| return d.fetch(fetchCtx, id) | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| client, ok := result.(fosite.Client) | ||
| if !ok { | ||
| return nil, fmt.Errorf("CIMD singleflight returned unexpected type %T", result) | ||
| } | ||
| return client, nil | ||
| } | ||
|
|
||
| func (d *CIMDStorageDecorator) fetch(ctx context.Context, id string) (fosite.Client, error) { | ||
| doc, err := cimd.FetchClientMetadataDocument(ctx, id) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: %w", fosite.ErrNotFound.WithHint("CIMD fetch failed"), err) | ||
| } | ||
|
|
||
| client := buildFositeClient(doc) | ||
|
|
||
| d.cache.Add(id, &cimdCacheEntry{ | ||
| client: client, | ||
| expires: time.Now().Add(d.ttl), | ||
| }) | ||
|
|
||
| return client, nil | ||
| } | ||
|
|
||
| // defaultCIMDGrantTypes are the OAuth 2.0 grant types applied when the CIMD | ||
| // document omits grant_types. CIMD clients are typically public native apps | ||
| // that use the authorization code flow with refresh token rotation. | ||
| var defaultCIMDGrantTypes = []string{"authorization_code", "refresh_token"} | ||
|
|
||
| // defaultCIMDResponseTypes are the OAuth 2.0 response types applied when the | ||
| // CIMD document omits response_types. | ||
| var defaultCIMDResponseTypes = []string{"code"} | ||
|
|
||
| // defaultCIMDTokenEndpointAuthMethod is the token endpoint authentication | ||
| // method applied when the CIMD document omits token_endpoint_auth_method. | ||
| // CIMD clients are always public — no pre-shared secret is established. | ||
| const defaultCIMDTokenEndpointAuthMethod = "none" | ||
|
|
||
| // buildFositeClient converts a ClientMetadataDocument into a fosite.Client. | ||
| // Redirect URIs containing http://localhost are wrapped in a LoopbackClient | ||
| // so that RFC 8252 §7.3 dynamic port matching applies. | ||
| func buildFositeClient(doc *cimd.ClientMetadataDocument) fosite.Client { | ||
| grantTypes := doc.GrantTypes | ||
| if len(grantTypes) == 0 { | ||
| grantTypes = defaultCIMDGrantTypes | ||
| } | ||
|
|
||
| responseTypes := doc.ResponseTypes | ||
| if len(responseTypes) == 0 { | ||
| responseTypes = defaultCIMDResponseTypes | ||
| } | ||
|
|
||
| // The embedded AS advertises only "none" in token_endpoint_auth_methods_supported. | ||
| // Override any other value from the document to avoid an inconsistent client | ||
| // configuration — CIMD clients are always public and have no pre-shared secret. | ||
| tokenEndpointAuthMethod := defaultCIMDTokenEndpointAuthMethod | ||
|
amirejaz marked this conversation as resolved.
Outdated
|
||
|
|
||
| var scopes []string | ||
| if doc.Scope != "" { | ||
| scopes = strings.Fields(doc.Scope) | ||
| } | ||
|
|
||
| defaultClient := &fosite.DefaultClient{ | ||
| ID: doc.ClientID, | ||
| RedirectURIs: doc.RedirectURIs, | ||
| GrantTypes: grantTypes, | ||
| ResponseTypes: responseTypes, | ||
| Scopes: scopes, | ||
| // CIMD clients don't pre-declare audience; leave empty so the AS | ||
| // applies its own audience policy rather than rejecting all values. | ||
| Audience: nil, | ||
| Public: true, | ||
| } | ||
|
|
||
| openIDClient := &fosite.DefaultOpenIDConnectClient{ | ||
| DefaultClient: defaultClient, | ||
| TokenEndpointAuthMethod: tokenEndpointAuthMethod, | ||
| } | ||
|
|
||
| // Wrap in LoopbackClient when any redirect URI targets localhost so that | ||
| // RFC 8252 §7.3 dynamic port matching works for native app clients. | ||
| // Pass openIDClient directly so TokenEndpointAuthMethod is preserved — | ||
| // LoopbackClient now embeds *fosite.DefaultOpenIDConnectClient. | ||
| if hasLoopbackRedirectURI(doc.RedirectURIs) { | ||
| return registration.NewLoopbackClient(openIDClient) | ||
| } | ||
|
|
||
| return openIDClient | ||
|
amirejaz marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // hasLoopbackRedirectURI returns true when any of the redirect URIs in the | ||
| // list targets a loopback address over HTTP. The host is parsed from each URI | ||
| // to prevent bypass via hosts like "http://localhost.evil.com/". | ||
| func hasLoopbackRedirectURI(uris []string) bool { | ||
| for _, uri := range uris { | ||
| parsed, err := url.Parse(uri) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if parsed.Scheme == "http" && oauthproto.IsLoopbackHost(parsed.Hostname()) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.