Intersect default scopes with scopes_supported when scope is omitted - #6283
Conversation
jhrozek
left a comment
There was a problem hiding this comment.
The core fix looks right to me. RFC 7591 §2 says the AS MAY register a client with a default set of scopes and doesn't constrain what that set is, and §3.2.1 lets the server substitute metadata, so narrowing to the intersection instead of rejecting is well within spec. The registered scope is a ceiling rather than a grant, and the DCR response echoes it, so a conformant client can see what it got. Tests pin the new behaviour at all three layers, including the empty-intersection rejection and the exact dropped set. One nice side effect: the old code returned the package-level DefaultScopes slice itself, and the new code builds a fresh one.
Most of my comments are about the logging that came along with the fix rather than the fix itself. The one I'd most like to see addressed is the level: droppedDefaults is a pure function of operator config, so the WARN carries no per-request information, and it's non-empty by default in the operator path — controllerutil/authserver.go passes oidcConfig.Scopes straight through as ScopesSupported, and the CRD documents that default as ["openid", "offline_access"], so profile and email are always dropped. That's one WARN per unauthenticated /oauth/register call forever on a correct config.
One thing outside the diff: the BaselineClientScopes doc comment at pkg/authserver/config.go:86-87 still says "registered scope = client-requested, or DefaultScopes if empty", which this change makes inaccurate. That comment is a swagger source, so it's already published in docs/server/swagger.yaml:621, swagger.json:511 and docs.go:518 — fixing it means regenerating docs/server/. No docs/arch/ page describes DCR scope resolution (grepped for scopes_supported and DefaultScopes, no hits), so nothing there needs updating.
Also worth a note in the PR body under user-facing changes: a registration that used to fail with 400 invalid_client_metadata now succeeds with a narrower scope set.
|
Thanks for the thorough pass — all points addressed in 4f6f612:
Also fixed the stale |
4f6f612 to
04a3f70
Compare
|
On the one open item — Info vs. Warn for the startup narrowing-case log — I'm not attached to Info: either level is defensible, so I'm happy to switch it to Warn (or adjust the wording/placement any other way) if that's the preference; no need to make a case for it either way. Whenever the CI workflows run, I'll treat anything they flag as part of finishing this up and address it promptly. Happy to adjust anything else as well. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6283 +/- ##
==========================================
+ Coverage 78.01% 78.03% +0.02%
==========================================
Files 767 767
Lines 74273 74310 +37
==========================================
+ Hits 57947 57991 +44
+ Misses 16321 16314 -7
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
04a3f70 to
e75b82c
Compare
When a registration omits scope, ValidateScopes required every entry of DefaultScopes to be present in scopes_supported and rejected the client otherwise, so any server whose scopes_supported lacks even one default (e.g. profile) rejected every scope-less registration with a generic invalid_client, on both the DCR and the CIMD registration paths. Fall back to the intersection of DefaultScopes with scopes_supported instead, and report the dropped defaults so both callers WARN-log them with the client identity. An empty intersection is still rejected, with an error description that names the full default set. CIMD rejection paths now also WARN-log the client_id and the reason, which previously left no server-side trace at any level. Closes stacklok#6186 Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com>
Demote the per-registration dropped-defaults record to Debug and move it after the baseline union: on the DCR path it rides the existing 'registered new DCR client' record keyed on client_id, on the CIMD path it logs the final post-union scope set. The drop is a pure function of startup configuration, so the operator-facing signal is now a one-time startup log in Config.applyDefaults (Info when the intersection narrows the default set, Warn when it is empty and scope-omitting clients will be rejected), reusing ValidateScopes so the startup message cannot drift from request-path behavior. Name the residual gap in ValidateScopes: the omitted-scope fallback draws from DefaultScopes only, so entries in scopes_supported outside the default set remain unreachable on that path; the explicit opt-in is baseline_client_scopes = scopes_supported. Update the stale BaselineClientScopes doc comments that described the pre-intersection fallback and regenerate docs/server. Signed-off-by: Aleksandr Filippov <71711753+alex-feel@users.noreply.github.com> # Conflicts: # docs/server/docs.go # docs/server/swagger.json # docs/server/swagger.yaml
RegisterClientHandler and CIMDStorageDecorator.fetch exceeded gocyclo's threshold once dropped-defaults logging was added. Pull the self-contained log-attribute construction and scope-resolution blocks into dedicated helpers with no behavior change.
b45d019 to
7947bb3
Compare
Closes #6186
Problem
When a registration omits
scope,registration.ValidateScopesrequired every entry ofDefaultScopesto be present in the server'sscopes_supportedand rejected the client otherwise. Any server whosescopes_supportedlacks even one default (e.g.profile) therefore rejected every scope-less registration — on both the DCR and the CIMD paths — and on the CIMD path the failure surfaced to the client as a genericinvalid_clientwith no server-side log line at any level (see #6186 for the analysis and repro).Change
ValidateScopesempty-input fallback now returns the intersection ofDefaultScopeswithallowedScopes, plus the list of dropped defaults (RFC 7591 §2 permits the server to register a client with a default set of scopes). Only an empty intersection is rejected, with an error description that names the full default set. Explicit-scope validation is unchanged and strict.dropped_defaultsattribute of the existingregistered new DCR clientrecord (after the baseline union and theclient_idmint, soscopesis the final set and the record correlates byclient_id); on the CIMD path as a post-union Debug record keyed on theclient_idURL. The operator-facing signal is a one-time startup log inConfig.applyDefaultsreusingValidateScopes(nil, ...): Info naming the intersection and the dropped defaults whenscopes_supporteddoes not cover the default set, Warn when the intersection is empty (scope-omitting clients will be rejected).fetch()(fetch failure,token_endpoint_auth_method,grant_types,response_types, scope) WARN-logs theclient_idand the reason, since fosite's production error rendering drops the hint and these failures previously left no server-side trace.BaselineClientScopesdoc comments (theRunConfigswagger source and the server config struct) now describe the intersection fallback;docs/server/regenerated.Behavior
scopes_supportedcarries all defaultsscopes_supportedlacks some defaultsinvalid_client)scopes_supporteddisjoint from defaultsbaseline_client_scopesunionUser-facing change
A registration that omits
scopeagainst a server whosescopes_supportedlacks part of the default set previously failed with HTTP 400 (invalid_client_metadataon DCR,invalid_clienton CIMD); it now succeeds and registers the intersection. The DCR response'sscopefield reflects the granted set.Known residual gap (deliberately out of scope)
The omitted-scope fallback draws from
DefaultScopesonly, never from the rest ofscopes_supported. A scope-omitting client against a server advertising non-default entries (e.g.mcp:tools) registers without them — and since protected-resource metadata advertisesscopes_supportedand MCP guidance tells clients to request all of it absent a narrower hint, such a client can still fail at/oauth/authorizewithinvalid_scope. Defaulting toallowedScopesitself would close that case but hands every anonymous registration the full advertised set — a policy change with an existing explicit opt-in (baseline_client_scopes = scopes_supported). The gap is also named in a code comment at the fallback site.Tests
ValidateScopestable extended: intersection with one dropped default (the exact CIMD clients that omitscopeare silently rejected as 'Client does not exist' when scopes_supported does not include all DefaultScopes (breaks ChatGPT connector) #6186 shape), single-scope intersection, disjoint-set rejection.TestFetch_ScopeResolution: the previous "restrictive ScopesSupported requires explicit scope" case now pins the intersection grant; a disjoint case pins the remaining rejection.scopes_supportedlackingprofilereturns 201 with the intersection in the response.As proposed in #6186 (comment). Happy to split the diagnosability WARNs into a separate PR if preferred.