Repository navigation
[OID4VCI] Add a client policy to require a credential offer - #47286
Conversation
10cfc1e to
2b8b1d2
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#salesPostSigTestKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#disabledClientTestKeycloak CI - Adapter IT Strict Cookies |
bdb02ed to
fcfc8c7
Compare
b04a10f to
2151ab9
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.AuthenticatorSubflowsTest2#testSubflow2Keycloak CI - Forms IT (chrome) |
71e6f55 to
83497e0
Compare
363597d to
777a198
Compare
There was a problem hiding this comment.
@tdiesler Thanks for the updates.
PR looks better and it is good that it introduces the switch in the UI. However the PR is still doing 2 things, which is not ideal:
- Introduced
credential offer requiredswitch and related logic - Introducing client policies
It can be good if the PR rather does just first thing and omit client policies for now as I've mentioned in one of the previous review comments . In general, it is always better if PRs are small as possible and focus on single thing rather than addressing multiple things. So the client policies might be possibly done as a follow-up if needed.
Besides that, The integration with client policies in this PR is done in very strange way. The OID4VCAuthorizationDetailsProcessor checks that offer is present, however exactly same check is done also by the client policy executor. Client policies are typically used in Keycloak as additional layer of protection. The executors usually have their own configuration (for things like offer required for example) instead of reading configuration from somewhere else (EG. client scope in your case) and duplicating the checked logic in same way like done by OID4VCAuthorizationDetailsProcessor. I suggest to take a look at existing client policy executors, their configuration and client policy tests
Remember, originally there were no client policies involved - only a switch on the client scope. The argument was, that this is no good because "there is already an established way to handle this: client policies". The client policy (as an additional dimension) was added because of that request. Unfortunately, the logic cannot be in the policy executor alone, because it is never called when the policy is disabled. In the past that may have been different (i.e. the executor was called and could see whether the policy was active or not). Anyway, removing the client policy is no big deal - I can do that no problem. For symmetry there would then be no client policies for pre-auth and tx-code either. Are you sure you want to go back to client scope attributes alone? |
|
@mposolda This is hopefully now done as you suggested. The client policy is integrated at the authorization endpoint through the standard mechanism. The client scope dimension is also integrated there through the standard checker. |
There was a problem hiding this comment.
@tdiesler Thanks! Client policies look good. I've added few comments inline. The biggest one is probably related to AuthorizationEndpointChecker (AFAIK this is new thing in your PR as previously, the checks for enforcing credential-offer were triggered from OID4VCAuthorizationDetailsProcessor if I remember correctly)
| @@ -564,6 +564,16 @@ | |||
| "configuration": {} | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "name": "oid4vci-client-profile", | |||
There was a problem hiding this comment.
Will be good to avoid adding this to default config for now, since it is experimental feature.
For testing, the client profile can be manually created in the particular test.
There was a problem hiding this comment.
oid4vci is experimental too - it already has no effect unless a client policy that references this profile is created. When it comes to docs, it'd probably be better to just have to explain how the policy is created.
There was a problem hiding this comment.
Yes, I mean that oid4vci is experimental feature. The client_policies is not experimental, but already supported feature.
We should remove this though until oid4vci becomes supported as referencing the client-profile, which uses experimental feature is not ideal as people will see this when creating client-policy in the admin console even if the feature may not be supported.
| @@ -338,6 +350,47 @@ public void checkParDPoPParams() throws AuthorizationCheckException { | |||
| } | |||
| } | |||
|
|
|||
| public void checkCredentialScope() throws AuthorizationCheckException { | |||
There was a problem hiding this comment.
Is it possible to make sure that AuthorizationEndpointChecker does not have direct dependency on OID4VCI specific logic?
If needed, it can be good to introduce some new SPI like AuthorizationCheckProvider SPI and have implementation (which will be enabled just when OID4VCI feature enabled for the server) and trigger this new SPI from AuthorizationEndpoint
There was a problem hiding this comment.
Yes, I thought about this. Similar to other places we could check dependent on the client protocol. Can we do that in a follow-up?
There was a problem hiding this comment.
Would prefer to do it in this PR instead of the follow-up as it would be good to introduce any OID4VCI related code to the classes like AuthorizationEndpointChecker
There was a problem hiding this comment.
There are various ways to do this ...
- Check the AuthorizationRequest (i.e. fail early), which would require the AuthorizationEndpointChecker to have some sort of plugin mechanism - perhaps by client scope protocol
- Check the TokenRequest, there is already a plugin mechanism that we can use (i.e. put the logic back into OID4VCAuthorizationDetailsProcessor)
- Check the CredentialRequest (i.e. fail late), this is late but the most natural for OID4VCI
If you prefer option 1 and fail as early as possible - is there perhaps already something I could use out-of-the-box? If not, how would I go about it? Perhaps some protocol specific provider?
There was a problem hiding this comment.
The option (2) to add this check to OID4VCAuthorizationDetailsProcessor is OK. AFAIK that was in some previous variant of your PR. If you go this way, then maybe
CredentialClientPolicyExecutor can be updated to act on TOKEN_REQUEST instead of AUTHORIZATION_REQUEST event, so that client policy is checked at the same request (token request) like the credential-scope option? But if you prefer to leave client policy on the AUTHORIZATION_REQUEST that also works.
For the option (1), there is nothing yet, so yes, I am thinking about some specific provider. Something like:
public interface AuthorizationEndpointAddOnCheck extends Provider {
// Context is to retrieve variables available in AuthorizationEndpointChecker
void check(AuthorizationEndpointCheckerContext context) throws AuthorizationCheckException;
}
And then in AuthorizationEndpointChecker trigger something like:
AuthorizationEndpointCheckerContext context new AuthorizationEndpointCheckerContext(this);
try {
Set<AuthorizationEndpointAddOnCheck> additionalChecks = session.getAllProviders(AuthorizationEndpointAddOnCheck.class);
for (AuthorizationEndpointAddOnCheck check : additionalChecks) {
check.check(context);
}
} catch (AuthorizationCheckException ex) {
... handle exception ...
}
I am not sure about option (3) as we're likely going to add soon the support for refresh-tokens ( #48397 ) and I think this may not work well with that. As in case that refreshed access token would be used in the credential-request, the credential offer will not be available.
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.forms.BruteForceTest#testNoFailureResetForPermanentLockout |
| @@ -564,6 +564,16 @@ | |||
| "configuration": {} | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "name": "oid4vci-client-profile", | |||
There was a problem hiding this comment.
Yes, I mean that oid4vci is experimental feature. The client_policies is not experimental, but already supported feature.
We should remove this though until oid4vci becomes supported as referencing the client-profile, which uses experimental feature is not ideal as people will see this when creating client-policy in the admin console even if the feature may not be supported.
| @@ -338,6 +350,47 @@ public void checkParDPoPParams() throws AuthorizationCheckException { | |||
| } | |||
| } | |||
|
|
|||
| public void checkCredentialScope() throws AuthorizationCheckException { | |||
There was a problem hiding this comment.
Would prefer to do it in this PR instead of the follow-up as it would be good to introduce any OID4VCI related code to the classes like AuthorizationEndpointChecker
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestKeyIdAsKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies org.keycloak.testsuite.adapter.servlet.SAMLServletAdapterTest#employeeSigPostNoIdpKeyTestNoKeyNameInKeyInfoKeycloak CI - Adapter IT Strict Cookies |
| @@ -338,6 +350,47 @@ public void checkParDPoPParams() throws AuthorizationCheckException { | |||
| } | |||
| } | |||
|
|
|||
| public void checkCredentialScope() throws AuthorizationCheckException { | |||
There was a problem hiding this comment.
The option (2) to add this check to OID4VCAuthorizationDetailsProcessor is OK. AFAIK that was in some previous variant of your PR. If you go this way, then maybe
CredentialClientPolicyExecutor can be updated to act on TOKEN_REQUEST instead of AUTHORIZATION_REQUEST event, so that client policy is checked at the same request (token request) like the credential-scope option? But if you prefer to leave client policy on the AUTHORIZATION_REQUEST that also works.
For the option (1), there is nothing yet, so yes, I am thinking about some specific provider. Something like:
public interface AuthorizationEndpointAddOnCheck extends Provider {
// Context is to retrieve variables available in AuthorizationEndpointChecker
void check(AuthorizationEndpointCheckerContext context) throws AuthorizationCheckException;
}
And then in AuthorizationEndpointChecker trigger something like:
AuthorizationEndpointCheckerContext context new AuthorizationEndpointCheckerContext(this);
try {
Set<AuthorizationEndpointAddOnCheck> additionalChecks = session.getAllProviders(AuthorizationEndp
10BC8
ointAddOnCheck.class);
for (AuthorizationEndpointAddOnCheck check : additionalChecks) {
check.check(context);
}
} catch (AuthorizationCheckException ex) {
... handle exception ...
}
I am not sure about option (3) as we're likely going to add soon the support for refresh-tokens ( #48397 ) and I think this may not work well with that. As in case that refreshed access token would be used in the credential-request, the credential offer will not be available.
|
I added an AuthorizationEndpointCheckProvider abstraction |
There was a problem hiding this comment.
@tdiesler Nice, Thanks!
Could you please rebase on top of latest main? Hopefully that is the last thing for this PR as that should fix the test failures - (The "Javascript UI" failure was an issue in Keycloak main and it was fixed yesterday. So rebase should help to fix it. The failure in "Base testsuite group 5" also seems unrelated to your changes).
|
@tdiesler I was able to rebase your branch "remotely". So nothing more needed. Hope to merge if tests are OK. |
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Signed-off-by: Thomas Diesler <tdiesler@proton.me> - no functional change - CredentialScopeModelUtils => CredentialScopeUtils - AuthorizationRequestContext.getparsedResponseType() => getParsedResponseType() - AuthorizationEndpointRequest.getRedirectUriParam() => getRedirectUri()
Thanks @mposolda Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
Signed-off-by: Thomas Diesler <tdiesler@proton.me>
closes #44317
We now have this policy ...
which is triggered when a client is oid4vci enabled.
There is also a an attribute
vc.policy.offer.requiredthat can be set on the credential scope. The final outcome is determined by this decision matrixHence, we use an OR with sensible defaults to determine whether an order is required. With the client scope not installed or disabled and the default attr value an offer would not be required (i.e. out-of-the-box) behaviour.