feat: add external client resolution and RFC 8414 metadata - #131
Conversation
luikyv
left a comment
luikyv
left a comment
There was a problem hiding this comment.
Hey, thanks for taking the time to contribute to this repo. I really appreciate it, this is a nice feature to have in the module
I've requested a few changes. Let me know if anything isn't clear. Feel free to reach out via email or linkedin if you'd like to discuss anything in more detail
| if ctx.ResolveClientFunc != nil && !errors.Is(err, goidc.ErrNotFound) { | ||
| return fmt.Errorf("could not load the client: %w", err) | ||
| } |
There was a problem hiding this comment.
Could you remove this? There's a bug in this error handling that I'm fixing in another MR. I'll rebase mine once this one is merged
| // return the latest client state, including key rotations and disablement. | ||
| // The function may be called concurrently and must return a snapshot that is | ||
| // safe for the provider to read for the duration of the request. | ||
| type ResolveClientFunc func(context.Context, string) (*Client, error) |
There was a problem hiding this comment.
Could you rename this to maybe ClientFunc? We already use "resolve" a lot for federation
| c, err := ctx.ResolveClient(id) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if c == nil { | ||
| return nil, errors.New("client resolver returned a nil client") | ||
| } | ||
| if c.ID != id { | ||
| return nil, fmt.Errorf("client resolver returned client %q for %q", c.ID, id) | ||
| } | ||
| return c, nil |
There was a problem hiding this comment.
You can just return ctx.ResolveClient(id). No need to check if the client is non-nil or if the ID matches, that is the implementer’s responsibility
| // It also caches the keys if they are fetched. | ||
| // Fetched keys are cached unless the client came from ResolveClientFunc. The | ||
| // latter is deliberately re-fetched so key rotation takes effect immediately. | ||
| func JWKS(ctx oidc.Context, c *goidc.Client) (*goidc.JSONWebKeySet, error) { |
There was a problem hiding this comment.
Could you undo the changes in this function? The JWKS cache is request-scoped in the normal flow because the client is passed by pointer through the code paths that use it
If an implementation returns the same *Client across multiple requests, cache invalidation should stay on that implementation's side. For example, it can clear the cached JWKS before returning from ctx.Client(id) or return a fresh *Client populated from its backing store
Also, SignedJWKSURI doesn't need to require federation, it could be used in other deployments.
2 participants
Summary
Security behavior
Resolver results are rejected when nil or when the returned client ID does not match the requested ID. signed_jwks_uri fails safely when Federation is unavailable. Authorization requests preserve resolver-store failures as server errors rather than misclassifying them as unknown clients.
Verification
References: RFC 8414 and OpenID Connect Discovery 1.0.