Enforce OIDC code flow access token verification only if JWT is in the application code

[sberyozkin]

Fixes #39717

This PR avoids enforcing the code flow access token JsonWebToken is the application code does not use it.
Just to summarize, with the authorization code flow, ID token is always verified. But the code flow access token is verified optionally because it is not used for the local security decisions, unless the user requests with quarkus.oidc.authentication.verify-access-token=true. Bearer access tokens are always verified or it is expected to contain the roles.

We made, IMHO, a good hardening fix with #39458, which will minimize the risk of the user code working with the unverified code flow access tokens and making some decisions, by forgetting to request its verification. This feature has been requested a few times, Paulo was asling about it. We just have to be careful because some access tokens are binary ones, from Google, etc, which can not be verified without the user enabling the indirect user info verification which may not always work. In fact I already enabled it by default awhile back and had to immediately revert it for this reason.

So the fix from Michal is much better - we enable it if we detect the user code has an intention to use the code flow JWT access token but it picks up JsonWebToken from smallrye-jwt.

So this PR filters out smallrye-jwt and the OIDC token propagation, the only known OIDC related dependencies that have JsonWebToken injected. smallrye-jwt has a producer which may not be used at all in the application code, and the OIDC token propagation (resteasy based one) has a feature related to JsonWebToken, if it is available.

We can still imagine some custom producers working with JsonWebToken - but in that case the users will have a control around it - and remove the dependency if they don't need JsonWebToken in the code. And if really necessary, they can disable the verification with quarkus.oidc.authentication.verify-access-token=false, IMHO we should try to keep the hardening fix by all means.

@pedroigor @gastaldi If @michalvavrik approves, then IMHO it is good to go, in which case, please approve if you don't have any other concerns, as #39717 is a 3.9.0 regression . We don't change the OIDC logic at all, only would like to enforce the code flow access token verification whenever possible

[alesj]

No need for the "Test" in the class name?

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5f60a6d.

✅ 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.

[michalvavrik]

The OIDC Token propagation line basically says that we register filters that are not actually used while we have all build time information:

1. We can detect when the filter is needed
2. If the JsonWebToken is actually injected inside bean used by the application (by actively used JAX-RS filter), then it should use verified JWT?

As for SmallRye JWT package unfortunately I don't know it well enough, but to have unused beans registered by default surprises me.

[sberyozkin]

@michalvavrik

The OIDC Token propagation line basically says that we register filters that are not actually used while we have all build time information...We can detect when the filter is needed

This is only available in the Resteasy Classic based optional filter which as far as I know noone has used, this is why I did not make the same feature available in the reactive propagation filter. That feature is about using a JWT build API to construct a new JWT token from the coming JWT token and then resigning it with the new key, setting a new audience. But users never used it because the token propagation supports the token exchange grants.
So this check is just to avoid that unused feature interfering, IMHO it is not worth the effort and start checking at the quarkus-oidc level if that filter is used given that filter is also expected to work with smallrye-jwt.

If the JsonWebToken is actually injected inside bean used by the application (by actively used JAX-RS filter), then it should use verified JWT?

If it is meant to be propagated then not really, we won't use for any security decisions locally and users won't access it directly.

We are talking about this feature: https://quarkus.io/guides/security-openid-connect-client-reference#restclient-jsonwebtokenrequestfilter

As I said I haven' seen any evidence it being used, instead the exchange token grants are used to set a new audience, resign etc. And like I said, in cases where it is not enforced users can just enable with the property. But if resteasy easy client users will use that extension to propagate Google binary access token, it will cause a failure
because we will detect JsonWebToken - so if someone, theoretically at least, uses that feature, and wants to have it verified they can enable the verification, I suppose I can update the migration guide.

As for SmallRye JWT package unfortunately I don't know it well enough, but to have unused beans registered by default surprises me.

That producer is probably 7 years old, it may have been myself or Scott who coded it, fair to say it is not the best CDI code written but we just should not let in interfere in this feature

[michalvavrik]

Copy that. Thanks for in detail explanation.

[michalvavrik]

I approve (when the Ales comment about the test name is resolved) as this is time sensitive.

However we only detect active injection points, so I believe this is a workaround that is hiding actual bug - we should only have registered beans that we actually use. And the injection in application code requires verification, so should the framework injection.

My point is, with this PR In place there can be an actively used injection point that should use verified access token but does not.

Update: see #39718 (comment)

[sberyozkin]

@michalvavrik

However we only detect active injection points, so I believe this is a workaround that is hiding actual bug

I'm not sure what you mean, the bug in the OIDC token propagation filter ? I'm not sure where the bug is , that feature needs the JWT token. But It does not really matter because the idea behind the automatic token verification is about protecting the user code from accessing it and making some decisions, but with the token propagation filters, users don't see the access token, the target server will be verifying it. This is the whole reason why we don't verify the code flow access tokens by default, we don't use them in Quarkus by default.

But in any case, IMHO, this feature, should not depend on how some other places in Quarkus use JsonWebToken. It is all about protecting the user code from getting access to the unverified JWTs if the users forget to enable the verification.

Have you convinced you a little bit ?

[sberyozkin]

@alesj Sure, I'll update that test class name, but I can do it in a follow up PR if you want to merge today, I won't be waiting till another build passes, so it will be then merged in the morning, it will probably won't make it to 3.9.1 then, not sure it will make it in any case...

[michalvavrik]

@michalvavrik

However we only detect active injection points, so I believe this is a workaround that is hiding actual bug

I'm not sure what you mean, the bug in the OIDC token propagation filter ? I'm not sure where the bug is , that feature needs the JWT token.

The feature needs the JWT token and we enable the feature when nobody uses it. I don't want to continue discussing it, you explained it well enough above. Thanks

But It does not really matter because the idea behind the automatic token verification is about protecting the user code from accessing it and making some decisions, but with the token propagation filters, users don't see the access token, the target server will be verifying it. This is the whole reason why we don't verify the code flow access tokens by default, we don't use them in Quarkus by default.

I would not guess it, because protecting the user code from accessing it and making some decisions is just a speculation, users can use the token for propagation as well, we don't know what they use with it. So I didn't realize that was the intention of this feature. It sounds reasonable.

But in any case, IMHO, this feature, should not depend on how some other places in Quarkus use JsonWebToken. It is all about protecting the user code from getting access to the unverified JWTs if the users forget to enable the verification.

That is sound if you are able to say that Quarkus will never use this injected JWT without verifying automatically. I don't see it currently, so you are right.

Have you convinced you a little bit ?

You had me after #39718 (comment), my notes above are just for me to learn. Thanks for the fix.

[sberyozkin]

@michalvavrik

The feature needs the JWT token and we enable the feature when nobody uses it.

We don't enable it by default

protecting the user code from accessing it and making some decisions is just a speculation, users can use the token for propagation as well, we don't know what they use with it

If the users inject it directly in their code, then indeed, we don't know what they will do with it - they may never access the claims, but does it matter ? The risk is there that they will - which is enough. We also document that if the users want to inject the token for the purpose of the manually propagating it, without the filters, then they should use AccessTokenCredential which will give them the encoded token. And here they can decode it manually. So yes we can't cover all the variations. But if we see JsonWebToken injected there is a high chance they will want to access it.

If we follow your argument then when we see UserInfo then we don't know if the users will even access it.

[michalvavrik]

You know @sberyozkin , if that's a case and you are use we always want to inspect only application injection point, I suppose we can find something more robust. For example Quarkus never registers beans from runtime modules by default, so Quarkus knows what application classes are (I suspect ApplicationIndexBuildItem contains them? Didn't check yet, bit busy ATM). If you want, we can make it more robust in the future.

[michalvavrik]

We don't enable it by default

I am sorry, I don't understand. How do you read this line:

quarkus/extensions/oidc-token-propagation/deployment/src/main/java/io/quarkus/oidc/token/propagation/deployment/OidcTokenPropagationBuildStep.java

Line 50 in c23eba2

additionalBeans.produce(AdditionalBeanBuildItem.unremovableOf(JsonWebTokenRequestFilter.class));

[sberyozkin]

additionalBeans.produce(AdditionalBeanBuildItem.unremovableOf(JsonWebTokenRequestFilter.class));

You got me, it is enabled by default. But is it relevant to this PR :-) ?

[michalvavrik]

You got me, it is enabled by default. But is it relevant to this PR :-) ?

I only mentioned it because originally (and incorrectly) I thought every active bean should be checked and I was surprised it breaks applications even without actively using this feature. No. it is not relevant for this PR. Let's get it in.

[sberyozkin]

@michalvavrik Indeed, we can improve the way that token propagation feature is registered (I probably added it as unremovable because it did not work otherwise, but to be honest I don't remember). Similarly for the smallrye-jwt filter. But in general I think it is good that this particular feature does not depend on how some other parts of the Quarkus ecosystem use JsonWebToken.

Thanks for the feedback and good night !