Support for verifying OIDC JWT claims with custom Jose4j Validator

[michalvavrik]

closes: #39425

[sberyozkin]

Hi Michal, @michalvavrik, thanks very much.

This PR resolves the enhancement request indeed, thanks for the effort, but it looks significantly more sophisticated than I was imagining it would be. Do we really need all of it ?
I know there is no explicit injection point for Validator but I'd rather document that users should add @Unremovable - this is what I already did for OidcRequestFilter and it works for users just fine. Then we just use Arc in OidcProvider to check it - it may end up slightly suboptimal in the end but code-wise, we'll just have a few lines only.

What do you think ?

[michalvavrik]

but it looks significantly more sophisticated than I was imagining it would be. Do we really need all of it ?

Honest answer is no. It all goes down to my honest conviction how we should design Quarkus features. I am well aware how io.quarkus.oidc.runtime.TokenCustomizerFinder#find works and how simple would be to add one static method like that.

The point here really is to do things during the build if possible and as little as possible during static init. You can say it makes no difference in comparation to that finder, but it only makes no difference to one feature. It will make difference to 500 features. That's why there is more deployment code.

I know there is no explicit injection point for Validator but I'd rather document that users should add @unremovable - this is what I already did for OidcRequestFilter and it works for users just fine.

I have no problem to drop that, but before that, I'd like to provide you my POV:

That is just one build step producing Java predicate applied during the build time. Only reason why the @Unremovable is needed is that we access the beans programmatically instead of injecting them into the bean. That's implementation detail users shouldn't care about. If we expect to use all validator beans, we should also expect they will not be removed.

Then we just use Arc in OidcProvider to check it - it may end up slightly suboptimal in the end but code-wise, we'll just have a few lines only.

I answer that in the first paragraph of my code. I will respect if you require that and I'll rewrite this, but I think my PR is better. I was thinking the other way around - to make my code more generic so that anything annotated with @TenantFeature like TokenCustomizer would be handled by this code as well. Only reason why I didn't do it is that we have just 2 features like that ATM and it would require at least one new Java class.

[sberyozkin]

Sure, @michalvavrik, you are right in principle, I support it, it makes sense, I suppose we should keep moving toward optimizing things for OIDC.
Would the same code apply to OidcRequestFilter, and JwtTokenCustomizer ? May be this kind of work can be done in a dedicated PR, as part of the recent issue to do with unremovable beans ? IMHO it can make sense to do it one go and fix it for all such beans like OidcRequestFilter and JwtTokenCustomizer...

What do you think ?

I think there could be some asks for this Validator feature be backported, which will have to be discussed with the team and PM, but we may have problems convincing it can be possible with currently quite involved PR.

FYI, have to sign off right now, will comment later, thanks

[michalvavrik]

Sure, @michalvavrik, you are right in principle, I support it, it makes sense, I suppose we should keep moving toward optimizing things for OIDC. Would the same code apply to OidcRequestFilter, and JwtTokenCustomizer ? May be this kind of work can be done in a dedicated PR, as part of the recent issue to do with unremovable beans ? IMHO it can make sense to do it one go and fix it for all such beans like OidcRequestFilter and JwtTokenCustomizer...

What do you think ?

I agree to simplify this PR. When this gets in, I'll open a PR that refactors the OidcRequestFilter, the JwtTokenCustomizer and the TokenCustomizer. I think we can unify it with a little difference all there (though the OidcRequestFilter is in OIDC commons which currently doesn't have a processor, but I think it should be fine). Thanks

I think there could be some asks for this Validator feature be backported, which will have to be discussed with the team and PM, but we may have problems convincing it can be possible with currently quite involved PR.

That's convincing point even considering it might never happen (which is not relevant).

[sberyozkin]

Sounds good @michalvavrik, indeed, for now, please expect users will add @Unremovable, just to keep the options open, the user who is currently at LTS asked, but we indeed don't know how it will go.
Then later we can have a dedicated enhancement/optimization for users not to be required to add Unremovable fo Validator, OidcRequestFilter, whis is also used in OidcClient, and JwtTokenCustomizer.
Thanks

[michalvavrik]

I'll do the refactoring as part of the #39799. This PR is really trivial, but I am not sure when we have all the information at the build time, we should do this at the runtime. We will say how refactored version will go down...

[sberyozkin]

It is a simple and good start, thanks @michalvavrik, if you don't mind, can you only do a minor doc update early next week, have it under a Custom Jose4j validators section so that you can link to it from the OIDC web-app reference doc from the section with the same name (The question of using the Jose4j validators came up recently in scope of discussing the web-app specific issues).
Please don't spend any more time on this issue this weekend, enjoy the rest of it

[sberyozkin]

@michalvavrik Let me push that doc update a bit later to save you a tiny bit of time :-)

[sberyozkin]

LGTM, many thanks @michalvavrik, ready to go pending a minor doc update, this feature is currently scheduled for 3.10.
CC @calvernaz

[michalvavrik]

@michalvavrik Let me push that doc update a bit later to save you a tiny bit of time :-)

thanks!

[sberyozkin]

By the way @michalvavrik, something else to check before we merge next week, if we need to use ClientProxy to unwrap the bean, as I recall Martin recommended it for the issue we had with @OidcRequestFilter where annotations like Endpoint.Type.JWKS were ignored without using ClientProxy

[michalvavrik]

By the way @michalvavrik, something else to check before we merge next week, if we need to use ClientProxy to unwrap the bean, as I recall Martin recommended it for the issue we had with @OidcRequestFilter where annotations like Endpoint.Type.JWKS were ignored without using ClientProxy

I don't know what problems were there, but I don't do what io.quarkus.oidc.common.runtime.OidcCommonUtils#getOidcRequestFilters does. Instead of listing all the beans (which I didn't know how to get "value" from the literal without reflection API), I divide it into 2 calls - one listing default beans (without any TenantFeature qualifier) and one with TenantFeature qualifier. I'll propose refactoring PR anyway next month.

[sberyozkin]

Did a minor doc update, waiting for the CI to pass

[sberyozkin]

Looks like Keycloak was not loaded in time for the native oidc-code-flow test failure, this PR does not impact it

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 76b035b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 76b035b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[github-actions]

🙈 The PR is closed and the preview is expired.