Scope handling changes to accomodate client_credentials grant
Description
Environment
has dependent
Activity
Scott CantorJanuary 5, 2022 at 1:17 AM
Merged logic and moved action. Need to add some more unit tests to check more of this behavior.
Scott CantorJanuary 5, 2022 at 12:12 AM
I believe the full fix is to combine the two scope-handling/validating actions into one step that combines both, and move the combined ValidateScope action down to where the ReduceValidatedScope action step appears in the token flow.
None of the intervening steps requires validated scopes be known, and I don’t even see any optimizing going on in the case where none of the scopes are registered in metadata, the original code wasn’t failing on that case, it just continued processing with no scopes set.
Moving it down to the Reduce spot allows it to detect that encoded claims exist and dispose of them when the requested set filters down the effective scopes.
Scott CantorJanuary 4, 2022 at 11:53 PM
Actually, I spotted a second action bean, net.shibboleth.idp.plugin.oidc.op.profile.impl.ReduceValidatedScope that I think was doing this job. I needed to revisit this to support the new grant anyway, so I’ll just do a second pass and adjust this action in light of the change I made.
Scott CantorJanuary 4, 2022 at 11:01 PM
I committed an initial attempt at a fix in 1764b5645bc3585650037203c99b55e8f1c1eb22
Included in larger changes to start adding client_credentials grant support, but hopefully addressing this as well.
@Henri Mikkonen I’ll leave this assigned to you for review in the existing flows and feature set.
A concern I had is whether the unit tests for e.g. the token endpoint are strictly correct in that I think they end up (and did before) with empty scopes in the tokens issued. The only difference is that the field is actually null in the context rather than just an empty Scope object. It was the two secret JWT tests that seemed to fail because of that. Not sure why those specifically, maybe the other tests ended up populating the scope somehow.
The scope validation logic is flawed in a couple of ways:
There are code paths that assume the Scope field in the OIDC response context object is non-null, but it's typed as nullable and the code path involving no scopes registered in metadata will lead to a null value there. That's probably not exploitable because something might cause it to bail out earlier, but it did come up in my testing once I made other changes. It's possible the unit tests just don't produce that case.
The more significant issue is that the spec allows requests to the token endpoint to pass a scope parameter that overrides any pre-existing scope in the code or token as long as it's a subset. I imagine that doesn't come up much if they don't even test for it during certification.
The fix happens to be required to make the client_credentials grant work since the only scope input there is the token request itself, triggering all these edge cases to start appearing.