-
Notifications
You must be signed in to change notification settings - Fork 123
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
Authorization Handler / Client Scopes enhancement for Dynamic Scopes / RAR #325
base: main
Are you sure you want to change the base?
Authorization Handler / Client Scopes enhancement for Dynamic Scopes / RAR #325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @dgozalo, I'm @tnorimat working at FAPI-SIG (Financial-grade API Security : Special Interest Group) that tries to support RAR.
I've added some review comments. Could you check them?
design/authorization-handler.md
Outdated
|
||
### AuthorizationContext initiator | ||
|
||
At the initial step, the authorization data sent by the client can be retrieved from the initial OIDC Authorization-Endpoint request (or Token-Request in the use-cases like CIBA or OAuth2 resource-owner-password-credentials grant). This retrieved data will be referred to as AuthorizationRequest, which means the initial request to Keycloak to start the authentication process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CIBA, an authentication request is sent to Backchannel Authentication Endpoint.
There are other endpoints that receives an authorization request.
In Device Grant, an authorization request is sent to Device Authorization Endpoint.
In PAR, an authorization request is sent to Pushed Authorization Request Endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnorimat @dgozalo This is vagulemy mentioned in the text above as it mentions initial OIDC Authorization-Endpoint request (or Token-Request in the use-cases like CIBA or OAuth2 resource-owner-password-credentials grant)
.
But maybe to clearify and have it more explicit, we can add the note like this:
NOTE: The text above mentions "initial OIDC Authorization-Endpoint request". This is the the initial request in the
default OAuth2/OIDC Authorization-code flow. However various grants and specifications related to OAuth2 and
OpenID Connect allows that initial request is sent in different way. For example BackchannelAuthenticationEndpoint
in case of CIBA, Pushed-Authorization-Request endpoint in case of PAR, DeviceEndpoint in case of OAuth2 Device
Grant or TokenEndpoint in case of OAuth2 Resource-Owner-Password-Credentials Grant. All those endpoints are
entry point for particular authentication use-case and they can accept `scope` or `authorization_details` parameters.
In the text below, when it mentions "Initial Authorization-Endpoint request", it is assumed that it can be any of these
"initial request" type f orequests. It won't be mentioned again in the further text for the sake of simplicity.
And so the text in the paragraph would have "initial OIDC Authorization-Endpoint request (see NOTE below for the clarification)"
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mposolda +1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also confused and I @mposolda helps to makt it more clear. But isn't it just saying that "Access/Authorization data is represented by a AuthorizationRequest
"? So that no matter what grant type / request mode you are using it means the access/authorization data sent along with an authorization request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably the "initial OIDC Authorization-Endpoint request" can be simplified to "Authorization Request" to make it shorter in the text?
But IMO it will be clear to still include this NOTE and mention the various type of requests we're talking about (EG. BackchannelAuthenticationEndpoint in case of CIBA etc).
design/authorization-handler.md
Outdated
|
||
* In the authorization_details parameter as defined in the [RAR specification](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-rar-07). | ||
|
||
Once Keycloak parses the data from any of these 2 parameters, it will save them in the AuthorizationContext object. The idea is to unify the data in the AuthorizationContext in some structure, which will serve as an abstraction layer over the data that was sent via the scope parameter or authorization_details. However Keycloak will still need to have some flag in AuthorizationContext to track how the data were sent. This may be needed for various reasons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to mention that it can parse both scope
and authorization_details
when an authorization request contains both of them. RAR mentions such the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnorimat +1. We can add another sentence and change the text like this:
it will save them in the AuthorizationContext object. Note that same request can contain both `scope` and
`authorization_details` parameter and hence Keycloak should be able to parse both of them in the same request.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mposolda +1
design/authorization-handler.md
Outdated
|
||
For example [Grant Management API specification - section 6.4](https://openid.net/specs/fapi-grant-management-01.html#section-6.4) (not yet supported by Keycloak) allows clients to query Keycloak about the granted consents. Note that the returned structure contains both scope and authorization_details and hence it is needed by KC to track how the particular request was sent. | ||
|
||
### AuthorizationContext object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that the current keycloak's authorization service has already use the AuthorizationContext
class.
IMO, it might be better to use different class name (EG. OAuth2AuthorizationContext
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnorimat @dgozalo Yes. This can work, on the other hand, I am not 100% sure if we want to mention OAuth2AuthorizationContext
as it implies that this is specific to OAuth2 specification? Which IMO is some limitation as in the future, it may not necessarily be specific to OAuth2/OIDC. How about ClientAuthorizationContext
?
Similarly we can have ClientAuthorizationHandler
for the AuthorizationHandler (Similarly like you, I have also some fear that having purely AuthorizationHandler can introduce some confusions and conflicts with existing authorization services). "Authorization" is unfortunately quite overloaded term in the Keycloak world, so some clarification of this may be nice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgozalo RequestAuthorizationContext
looks good to me as it is clear from it, that it is request-scoped object. One more idea is ClientRequestContext
(does not exists in Keycloak, but in Resteasy dependency). Leaving to you what you prefer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there's a class called AuthorizationRequestContext
used in https://github.com/keycloak/keycloak/blob/31345c49b12352639de9406ccefd51f19afdf426/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java#L167.
It's used in the OIDC protocol endpoint but it does something quite similar to what we have in mind, even thought this one will be more generic, to be used in other grants and specifications.
Maybe we could "generify" and reuse this class for our purposes instead of creating a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure as AuthorizationRequestContext
is currently used for client policies and it is part of some infrastructure for client policies. Re-use the same class for both client policies and this new functionality might be confusing and doesn't help with very clear "separation of responsibilities" in the sources IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I was't sure what it was being used for, but it's clear that it's doing a completely different thing and we shouldn't reuse.
|
||
Assumption of this approach is that there is no limitation on Authorization Handler whether it can be added to the request by “scope” parameter or by “authorization_details” parameter. | ||
|
||
Requests will go through the chain of parsers. By default, we will have three parsers defined: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good if you would write how to distinguish static scope and dynamic scope during the chain of parsers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnorimat @dgozalo Yes, I am not sure if we need to have separate parser definitions for StaticScopeParser
and ScopeParameterParser
. It seems we may consider having only single ScopeParameterParser
as static parser is in fact just subset of the dynamic scope parser, where the static parser have just "static" regex configured.
So for example if regex is configured like group:(.*)
, it is assumed that it is dynamic scope, which would support scope parameter values like scope=group:123
. However if regex is configured like group
, it is assumed that it is static scope with only available value for scope parameter like scope=group
(This is what Keycloak supports today).
The alternative is, that static-Scope would be just the dynamic scope without any regex configured, which would imply that only supported value of scope parameter is the name of the client scope (or name of the authorization handler). This will be fine for backwards compatibility as we won't need to explicitly configure regex for all the static scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tnorimat on this. The most we can avoid touching the current data in the database, the better. If new, dynamic scopes will be created with a new, regexp field, it should not impact the already existing scopes at all. Otherwise they would need to be migrated to have a valid regexp in the database.
design/authorization-handler.md
Outdated
|
||
Policies can be configured at the client scope (or AuthorizationHandler) level. In addition to tab Mappers and Scope, there can be another tab like Policies . By default, each client scope can have only Persistent Consent Policy automatically set (See below for Persistent Consent Policy details). | ||
|
||
### When |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to check AuthorizationDetails
is acceptable or not at the beginning of the request to AuthorizationEndpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true.
I've just checked that current Keycloak also checks this at the beginning of the request ( EG. AuthorizationEndpointChecker.checkValidScope()
in case of AuthorizationEndpoint request). So it will be good to keep it like that and check at the beginning though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added it in the ### ClientAuthorizationContext initiator
section to make sure it's accounted for.
``` | ||
|
||
|
||
## Parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that static scope and dynamic scope are translated into the representation of RAR's authorization_details
. How about writing it explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, We can perhaps add another sentence after this code sample like:
Note that AuthorizationDetailsJSONRepresentation is supposed to represent both authorization_details, but also
static and dynamic scopes. See below for the examples how the JSON from the parsed scope representation looks
like.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mposolda +1
…ear and added a few aclaratory notes
Thank you for the thorough review @tnorimat. I've gone through all the comments and made a few modifications according to what was discussed with Marek. Apologies for taking so long to come back to this, but we'll shift focus to this again soon. |
@dgozalo Thank you for your update. I've read your update and confirmed that points I've commented on have been incorporated. I have 2 comments on that. I think there is an authorization that can be used as one shot.
When a client sends this authorization request to an authorization server, it gets a consent from a user and sends an access token to the client. After that, when the client sends the same authorization request to the authorization server, it again gets a consent from a resource owner because this payment initiation is different from the former one. Considering this point, IMO, such the request need not persist. Namely, there is an authorization that requires user consent but does not require persist. What do you think about it? I think we need to log an authorization, namely which client sends which authorization request and who authorized this request when, regardless of whether the authorization persists or not. |
This is good point. If I understand correctly, it will be needed to NOT persist such request as for example user consented the consent like:
But then client may want to send another payment of the same value (EG. It is periodic monthly payment of 125 EUR) and hence it is required that user will need to consent this again as it is different payment. IMO we need to have some logic in the policies to cover use-cases like this. So the policy itself needs to be smart-enough to understand this and decide if this requires consent or not.
I suppose we may use event SPI. This will automatically cover logging (as Keycloak has |
That's a very interesting point. The fact that a RAR payload is the same as a previously consented one doesn't mean that's strictly the same. In the context of a bank, even if a transfer happens again with the same amount, it doesn't mean that I'll consent it this time. When we get to the point of creating the configuration options for the RAR handlers, maybe an option like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dgozalo I've added very minor comments when I quickly went again through this doc yesterday/today. Most of them are only cosmetic changes.
design/authorization-handler.md
Outdated
|
||
### ClientAuthorizationContext initiator | ||
|
||
In the text below, when it mentions "Initial Authorization-Endpoint request", it is assumed that it can be any of these "initial request" type f orequests. It won't be mentioned again in the further text for the sake of simplicity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Not sure if remove this paragraph and merge it with the following paragraphs, as it seems to be this part is repeated couple of times. This is only cosmetic change, so it is more friendly for readers :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, it seems like it was mixed up when I was making the change, correcting it.
design/authorization-handler.md
Outdated
|
||
Policy is also responsible for processing the consent screen (The moment when a user submits the consent screen, which can happen multiple times during a single “authentication session” due the fact that more consent screens would be possible with this proposal). | ||
|
||
### Naming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: For some reason, this is not shown as "Header" when I try to view this text as HTML. See https://github.com/keycloak/keycloak-community/blob/1f6b5f86c40eb7688069ee39ef3e2c8d4fd8d81d/design/authorization-handler.md . It may be only github bug, not sure. This is also only minor cosmetic thing.
Same issue can be seen for multiple places (EG. for "When" section below as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it happened on quite a few headers in the document for some reason. Fixing them all.
|
||
These selections will be subjected to the policies defined by the handler, which will execute every time there’s a new value being added. | ||
|
||
**Question**: Should we also consider further consent screen enhancement that may be out of scope from this proposal now that we are improving it? I’m thinking of showing a client image, or name and UX improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. Something like this is supported by OIDC specification and we recently have community contribution, which added some of those enhancements to the consent screen. See for more details: keycloak/keycloak@63c9845#diff-3033beba91b6de9a2ae714e742dff4894b2fd9f9caeaea72dffdff0b17e50062
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add it as an "answer" to the question in the document to keep track of it.
…a lead on the consent screen enhancements section
private List<AuthorizationDetailsEntry> entries; | ||
} | ||
|
||
class AuthorizationDetailsEntry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be better in terms of design if we introduce an interface and specific sub-types for scope-based access and json-based access formats?
|
||
} | ||
|
||
enum Source { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the source matters? At the end you are interested on what is being requested, right? Or this is because you want to use this information later to build authorization/token responses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. In the token response, you may need to know if you include stuff in the scopes
or authorization_details
. The Grant Management API also has some differentiation regarding this.
We may not need "Source" in case if we can always know that particular "type" can be addressed either by scope
parameter or by RAR (authorization_details
parameter). So for example type "group" will be always handled by scope parameter like scope=group:123
when the type payment
will be always covered by RAR. But not sure if such restriction is good as it is a chance that some types might be covered by both scope
parameter and RAR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client code (using this API) should know where the information should be included. That is why I was wondering if the source really matters when building responses.
In addition to that, why do we need to treat scopes and authorization details as two different things? Internally, we could probably translate scopes to follow the authorization details (more generic/flexible) semantics and make the API work with a single input.
In your example, scope=group:123
would be translated to:
{
"type": "http://keycloak.org/auth-type/group",
"Identifier": "123"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly the point. It will be ideal if your example can be used and the group
can be addressed by both the scope
format and RAR format, which you used in your example. So internally Keycloak SPIs can treat it as same thing and also consent screen can be smart enough to treat those as same thing. We cover this in the design here : https://github.com/dgozalo/keycloak-community/blob/proposal/authorization-handler-clientscopes-enh/design/authorization-handler.md#how-to-parse-authorization-data
Possibly just token response may need to know the "source", so that client "foo", which used scope parameter scope=group:123
will see it in the scope
parameter in token response and client "bar", which used RAR will have RAR also in the token response. Also Grant Management API may have some requirement around this...
My point was, that if we don't include "Source" in the request data, it will mean that scope "group" type will need to have hardcoded whether it should be included in the token response in scope
or in the RAR. Which means that the example you used won't be possible as "group" client scope will have hardcoded whether it can be invoked by the scope
parameter or by rar.
```java | ||
interface ClientScopePolicy { | ||
|
||
PolicyVote applyPolicy(ClientScopePolicyContext ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, a better design is to follow the same approach used in authentication so that specific methods are provided to infer information from the handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what exactly you mean by "authentication" ? Did not you mean Authorization services or something else? Can you mention any more concrete class/use-cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant authentication as you did for AuthenticationFlowContext
. There you have methods such as success
, failure
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. This approach should work. I am not 100% sure if it is better to use "context" based mechanism like Authentication SPI uses or the mechanism based on something like "vote", which is used for example in Client Policies (See ClientPolicyConditionProvider.applyPolicy
) . It seems both have some advantages.
@dgozalo WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to prefer relying on implementations to expose their behaviour and data given a set of methods instead of being given a context to work with.
Any change to the context can have a huge impact on these classes, while extensibility can certainly be easier when only dealing with the internals of each implementation, or even just creating new ones. So I believe the AuthenticationFlowContext
would be preferred.
But then again, I'd love to define a common Policy Engine that would manage all the policies executed in Keycloak.
As I can see right now, there are at least three implementations doing that.
@mposolda @tnorimat @dgozalo In general, I think this design kind of overlaps with what we have in authorization services and future plans to enable fine-grained specification of access. For me, looks like we can leverage some key concepts from authorization services and come up with a simpler and general-purpose authorization API. A gist of what I have in mind is:
Basically, we have an entry point, the
Then you have the different
For last, the client code would process the outcome by looking at the evaluation
Note that policies should be able to provide advice/obligations to the client code to perform certain actions with the result such as to show a consent page. On top of that, we could be enabling other aspects such as:
|
I would be greatly in favour of reusing as much as possible. It would make sense to reuse the Policies Engine used in Authorization Services to evaluate any policies that we define in here. Also, I like what you are suggesting about evaluating permissions, if others agree on this approach we should add it to the design. This proposal is also defining how we would parse scopes and define a common As @mposolda mentioned in another comment, it would simplify code quite a lot on SPIs because we'd work with a single object. |
IMO, If an authorization includes unique id like |
Hi, following this discussion it seems to me that dynamic scope could solve my problems.
I want to filter this claim for instance giving:
Or at least a filtering feature with scope: ["policy:policy1"] in order that keycloak issue the policy claim only with policy:policy1 In order to do this:
|
@alexisdondon Dynamic scopes are partially available within the feature When feature is enabled, there are some configuration options on the client scope, which can allow some dynamic aspects. You can try and see if it suits your needs. |
Hi, thanks for the response i have the feature activated on the last keycloak version But i can't find really where to go after in the admin app to configure a client or a scope. |
Hi @mposolda. I have a similar problem. I have activated the feature, but I cannot see any difference in the client scope form view, and it doesn't work. I just see these warnings in the startup. I'm using this image |
Does this means we can use authorization_details with |
After considering how Dynamic Scopes and RAR would interact with the system and the possible interactions between them, the team has decided to come up with a mechanism that will abstract the authorization mechanism used in the authorization request.
In this proposal, we’ll be merging the Dynamic Scopes discussion and the RAR Design Proposal into a single place.
So, many considerations, benefits and implementation details of both proposals can be considered true here, as well as some of the ideas proposed in the Dynamic Scopes vs RAR discussion.