Changing OidcTenantConfig returned from TenantConfigResolver::resolve has no effect

[maag03]

We're using Quarkus 3.20.0 and a multi-tenant dynamic Oidc setup. As the title says, when returning OidcTenantConfig in TenantConfigResolver::resolve for a tenant with a changed property (e.g. clientId), the changed property seems to be ignored and the initial value is still being used by Quarkus.

Is this the intented behavior or is it a bug?
Any workaround available?

Br
Magnus

[quarkus-bot[bot]]

/cc @pedroigor (oidc), @radcortez (config), @sberyozkin (oidc)

[sberyozkin]

@maag03 We haven't looked into dealing with such cases, where an already resolved configuration (and therefore, possibly the config related to the already OIDC provider connection) has been updated.

Can you clarify please, is your use case about specifically the client secret recycling/update for the existing OIDC session ?

The challenge is how to handle correctly a case where an already resolved configuration does not match the newly resolved configuration for the current OIDC tenant - it may require just a swap or re-establishing the connection.

Perhaps we can start with taking care of the client secret update only as it is a known use case.

@michalvavrik This is the kind of challenge I thought should be discussed in detail, I'm referring here to my comments to your previous PRs.

[sberyozkin]

@michalvavrik I'd consider with starting supporting a very specific use case, the update of the client secret, since it is generally recommended to recycle client secrets periodically, to minimize the risk of them being leaked somehow, so, for example, the OIDC provider admin goes and updates it in the dashboad for a given OIDC application registration.

It is unclear what to do with any other updates, there are too many variations of what might get changed and what such changes might imply. Cases like OIDC provider gets redeployed therefore the existing OIDC connection for the already running application has to be refreshed seems more theoretical than realistic. Or, what if the updated configuration no longer requires UserInfo while the resolved one does - that can break the code which already expects UserInfo, plus any other of many dozens requirements having changed...

Which is why starting with supporting very specific realistic cases such as the client secret update, while ignoring any other possible mismatches and informing about it via docs, can make it easier to deal with, as opposed to coming up with some general config replacement strategy...

[michalvavrik]

Must admit I am bit afraid to reply (or even start on this topic). Maybe it will be better if you deal with it and leave me out of it. Making sure that only certain properties can change makes total sense, I am +100. But if we don't go with what I have proposed previously, you already do allow users to change whatever they want anyway (and not in a thread safe way). I'll wait for @maag03 use case and let you deal with it. Bye

[sberyozkin]

@michalvavrik, np, we will deal with specific cases

[sberyozkin]

@michalvavrik The problem with building the solution that will provide for the general replacement is that Quarkus OIDC loses control of what is being replaced, and it can lead to all sort of problems.
Situation where the providers are modifying the resolved configurations should be dealt with better.

IMHO, the right solution could be to copy factory construct OidcTenantConfig, and then, if the resolver returns a non null instance again, do not try to make it work generically, but warn about the mismatches unless the mismatch is supported for specific usecases, such as client secret updates.
I'll look into it.
In the meantime, CredentialProvider option may offer a better solution for the client secret update case

[sberyozkin]

@michalvavrik Lastly, I honestly do not expect users doing what I did in the test, modifying existing instances their resolvers may be keeping but instead create new instances.
However I agree quarkus oidc should be protected against such cases

[sberyozkin]

@maag03

It is also possible to register CredentialsProvider with a specific OIDC tenant configuration, https://github.com/quarkusio/quarkus/blob/main/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/config/OidcClientCommonConfigBuilder.java#L222.

I've looked at the code, if the client id and secret are sent as form parameters, then this provider is called on every request. but not if the client id and secret are sent as the Basic authorization scheme value.

So if the client secret post authentication is an option, it will offer a workaround, where your CredentialProvider will cache the current secret and replace it once the updated value is available.

I'll look into treating it consistently between client_secret_basic and client_secret_post (either read the secret once from the provider in both cases, or call the provider on every request)

[maag03]

We have a couple of different use cases, e.g. misconfiguration, changing clientIds, keys for JWTs, URLs etc.

In previous versions of Quarkus we could use:
getTenantConfigBean().getDynamicTenantsConfig().remove(tenantId)
and then recreate the config with different settings. But now remove has been removed ;)

You might also wonder why TenantConfigResolver::resolve is being called upon repeatedly when it has no effect for a already resolved tenant?

[sberyozkin]

@maag03 Do you mean why it is called when the session is already available ? I recall this case, to let resolvers react to a situation where the call with the existing session tries to reach the space covered by another tenant. Resolvers will see the tenant id attribute on the routing context when the session already exists and can return null if the tenant id is correct for a given request url.

As far as `remove' is concerned, the idea was to encapsulate the tenant management.
But I see it caused problems in your case.
I think may be this is what we should do, if the returned config instance is not null and not the same as the already resolved one, we just remove internally the original one instead of trying to merge.
@michalvavrik hope it sounds reasonable. I'll try to fix it quickly and cc you.
The copy construct of the original instances can be dealt with later

Thanks

[maag03]

@sberyozkin Yes that's what I meant.

And I think your suggested solution seems reasonable.

Thx for your attention on this issue!

[sberyozkin]

@maag03 Thanks, I opened a bug issue out of this discussion. FYI, the earlier point about returning null for the existing tenant is doc-ed at https://quarkus.io/guides/security-openid-connect-multitenancy#tenant-resolution-for-web-app, I was not in front of my laptop when I commented earlier...