Support OIDC token propagation during SecurityIdentity augmentation

[michalvavrik]

fixes: #33994

Currently you can't use AccessTokenRequest filter on the REST client during security identity augmentation as:

• identity is not yet available
• Arc request context can't be activated yet, for example io.quarkus.vertx.http.runtime.CurrentVertxRequest had not been set yet etc.
• which means we can't access request context, routing context, previous headers with authorization nor authorization request

Only way I actually found was putting local data to duplicated context. Reproducer I created always had context marked as safe, I didn't find any core extension that marks context as unsafe. Only extensions that I found which are using duplicated context local data are Hibernate Reactive, Mongo Panache Reactive, Hibernate Reactive Panache, OpenTelemetry and Micrometer. I understand this is not preferrable way to propagate context data. I also found this answer from @cescoffier https://stackoverflow.com/questions/71242590/can-i-store-sensitive-data-in-a-vert-x-context-in-a-quarkus-application that made me more optimistic that it is safe to use duplicated context when available. And https://groups.google.com/g/quarkus-dev/c/lBmQkCi_VK0/m/_O9b9mUxBQAJ made me less optimistic.

This PR propose enabling token propagation during auth only when explicitly enabled and with caveats.

[sberyozkin]

Thanks @michalvavrik for giving it a try, will have a look soon, cheers

[cescoffier]

I confirm it's the proper approach to propagate data along async (and sync) processing.

Note that you can also use the request scope which is written in the duplicated context if available.

BTW, a lot of extensions are using the duplicated context without knowing about it.

About the email it's because you must not write data in the root context (shared by concurrent requests) and only in the duplicated context. No worries if you try to do that in Quarkus, it bites... (well, it just throws an exception). So if you were able to write in the context, it's fine.

[michalvavrik]

I confirm it's the proper approach to propagate data along async (and sync) processing.

Note that you can also use the request scope which is written in the duplicated context if available.

I just put breakpoint there, request scope is not yet written in the duplicated context. This code is run during authentication when proactive auth is enabled.

BTW, a lot of extensions are using the duplicated context without knowing about it.

About the email it's because you must not write data in the root context (shared by concurrent requests) and only in the duplicated context. No worries if you try to do that in Quarkus, it bites... (well, it just throws an exception). So if you were able to write in the context, it's fine.

Thank you very much!

[michalvavrik]

FYI @sberyozkin: when testing Clement suggestion I realized previous solution didn't work for disabled proactive auth when Arc request context is already active, however we can't use for accessing SecurityIdentity (token credential that comes from it) it as it is circular reference; so I pushed slight adjustment.

[sberyozkin]

Hi Michal thanks for the PR and Clement for your help as well,

It does look like a technically precise PR, I'm a little bit uncertain if we really want to do it though, given a number of caveats. Let me clarify.

The use case is, use the current token in SecurityIdentityAugmentor to fetch from the remote service more roles associated with this token and OIDC token propagation filters don't work because they require an injected (verified) token (it is expected to be verified to avoid for the endpoint act as a direct propagator).

But this is a very specific case requiring a specific client interface support - not an application level rest client interface which endpoints will use themselves. So users can type in an interface with the @HeaderParam("Authorization") and pass it Bearer + the value of the current token and it is done.

This is why I'm a little bit uncertain, given the complexities/caveats involved, if we want to generalize it around this specific use case which can be easily solved with alternative means.

Michal, as always I appreciate very much your help :-), I'm just having some doubts :-).
Lets keep discussing, cheers

[michalvavrik]

The use case is, use the current token in SecurityIdentityAugmentor to fetch from the remote service more roles associated with this token and OIDC token propagation filters don't work because they require an injected (verified) token (it is expected to be verified to avoid for the endpoint act as a direct propagator).

Just FYI: when I was developer few years back, using Keycloak as SSO, we had exactly this use case when microservice needed to access SAP database with user information and roles. I presume this is the edge because noone opened issue about this till now, but common sense suggest that is is not. Especially if you Keycloak user federation between organisations, you will have SSO for auth, but in order to avoid buttleneck, you might want to have other database specific to application / department for authorization that can be accessed without IT engineers (for example: SAP).

Full disclosure: I don't really know how users are using Quarkus OIDC, you will have much better idea. I just meant to let you know this is not unseen use case, that's all.

This is why I'm a little bit uncertain, given the complexities/caveats involved, if we want to generalize it around this specific use case which can be easily solved with alternative means.

Yes, I agree there is a workaround, but this code that I added is only executed in scenarios where access token propagation is enabled and actually used inside authentication.

Is you set quarkus.oidc-token-propagation.enabled-during-auth=true and use access token propagation filter outside of authentication, TokenCredential is taken from CDI as before. Implementation added here shouldn't have performance downside (it's behind flag and it only access context map, so it should be quick).

So users can type in an interface with the @HeaderParam("Authorization") and pass it Bearer + the value of the current token and it is done.

One question: I don't believe this is just about propagating headers as when exchange token is enabled, filter also exchanges tokens. How will users also do that, or this is not related?

quarkus/extensions/oidc-token-propagation-reactive/runtime/src/main/java/io/quarkus/oidc/token/propagation/reactive/AccessTokenRequestReactiveFilter.java

Line 76 in 2da3206

exchangeToken(accessToken.get().getToken()).subscribe().with(new Consumer<>() {

Michal, as always I appreciate very much your help :-), I'm just having some doubts :-).

This is absolutely fine. I've learned something about context & token propagation, so if this ends up to be abandoned, I'm still happy.

I came with only solution I can see. If you prefer to either:

• document workaround
• or let issue open till there are other ways to fix (personally I don't see them)

no problem at all.

UPDATE: I thought about it and maybe you meant this is a whole lot of a code for something we don't need to solve for user. Did I get it right?

[sberyozkin]

Hey @michalvavrik , as far as the token exchange is concerned, it is about managing audience and token targets correctly in multiple hop propagation scenarios.

I understand the use case itself, sure.

I wonder, can we somehow reformat this PR not to focus on token propagation only, example, users have often requested Routing Context injected in the augmentor ?

[michalvavrik]

Hey @michalvavrik , as far as the token exchange is concerned, it is about managing audience and token targets correctly in multiple hop propagation scenarios.

I understand the use case itself, sure.

I wonder, can we somehow reformat this PR not to focus on token propagation only, example, users have often requested Routing Context injected in the augmentor ?

We are adding routing context to auth request that is passed to IdentityProvider, we could be doing something similar (not RoutingContext explicitly), sure. If there is some open issue, please point me to it and I'll add it to my list. This is just my opinion (maybe someone with better expertise would be needed), but activating Arc request context during authentication when proactive auth is enabled, we are going to have whole lot of problems to solve. If users need to inject RoutingContext to their SecurityIdentityAugmentor, they need to disable proactive auth.

None of that would fix linked issue though. There is no need to stretch it, I suggest that I will close this one.

Instead of putting TokenCredential into duplicated context, we could put there RoutingContext and read header, but it is same situation (and less performant) because we are going to have this PR (just with different object in the context), but I don't want to always put RoutingContext to duplicated context and document it, it will only serve purpose of fixing linked issue.

There is also possible partial solution - we could fix access token propagation filter fix for disabled proactive auth as there we can inject RoutingContext and read headers, but I don't think it is worth it, it will take little more code than this one and proper solution should work for both. How many users would really use it?

I'll let PR open for day or two in case you have some idea and then close it. No need to spend more time on this. Thanks!

[sberyozkin]

@michalvavrik Hi, please don't close PR yet, let it be open for a few days at least. I'll comment more as well. Thanks

[michalvavrik]

@michalvavrik Hi, please don't close PR yet, let it be open for a few days at least. I'll comment more as well. Thanks

:-) In that case I'll add one more note. I didn't emphasize it before because I think the gist of your doubts is that this is change made for the edge case and whether we should add code that will only be used for this case.

This solution should be safe, because propagating data through duplicated context when safe toggle is set to true should be safe according to Clement comment and what I read. IIUC there is effort for other extensions to use duplicated context.

[sberyozkin]

@michalvavrik Can you check the review comments please when you get a chance ?

[michalvavrik]

@michalvavrik Can you check the review comments please when you get a chance ?

I mentioned you commented (thanks btw) and I'll address them tomorrow. I was rather busy today :-)

[michalvavrik]

Failure in native is related somehow, I think it is also related to Unrecognized configuration key logging that I want to avoid. I'll refactor it a bit. (yep, it can't be static)

[michalvavrik]

Added test for SmallRye JWT and fixed PR so that it work properly in native mode.

[sberyozkin]

LGTM, thanks, should be good to go after minor comments related to JavaDocs get addressed

[quarkus-bot]

✖ This workflow run has failed but no jobs reported an error. Something weird happened, please check the workflow run page carefully: it might be an issue with the workflow configuration itself.