Add quarkus-oidc-db-state-manager extension

[sberyozkin]

Describe the extension
Quarkus OIDC CodeAuthenticationMechanism uses a default TokenStateManager implementation which stores all the session content in a cookie, which, by default, will include 3 tokens: ID, access and refresh tokens. When all 3 of them are in a JWT format as is with Keycloak, the cookie value size may exceed 4096 when JWTs have a lot of claims, at which point some browsers may drop it.

Default TokenStateManager can be configured to create a cookie per each of these 3 tokens, thus, avoiding the size limitation problem, likely in most cases.

However, a cookie per token solution is not ideal, and also, a general approach where JWT tokens are stored as cookies may become a weak point as far as the data privacy is concerned, since, a signed JWT token, if leaked, can be easily introspected (its payload is just a base64-url encoded JSON - hence a claim like Address might become visible).

So, as discussed earlier with @pedroigor and @stuartwdouglas, the proposal is to ship a simple Hibernate based extension which will only implement DbTokenStateManager, store 3 tokens in a DB of choice, and return a DB pointer to OIDC CodeAuthenticationMechanism thus allowing for a session cookie to only have a short encoded binary value:

import javax.enterprise.context.ApplicationScoped;

import io.quarkus.oidc.AuthorizationCodeTokens;
import io.quarkus.oidc.OidcTenantConfig;
import io.quarkus.oidc.TokenStateManager;
import io.vertx.ext.web.RoutingContext;

@ApplicationScoped
public class DbTokenStateManager implements TokenStateManager {

    @Override
    public String createTokenState(RoutingContext routingContext, OidcTenantConfig oidcConfig,
            AuthorizationCodeTokens tokens) {
        // save tokens in DB, return DB pointer
    }

    @Override
    public AuthorizationCodeTokens getTokens(RoutingContext routingContext, OidcTenantConfig oidcConfig, String tokenState) {
            //tokenState is a DB pointer - use it to get the tokens from DB
    }

    @Override
    public void deleteTokens(RoutingContext routingContext, OidcTenantConfig oidcConfig, String tokenState) {
          // delete DB table pointed to by tokenState
    }
}

The DB-node synchronization would be out of scope - as it is a general concern for a multi-node Quarkus DB deployment.

Interested in this extension, please +1 via the emoji/reaction feature of GitHub (top right).

Also CC @Sanne

[ghost]

/cc @Sanne, @gsmet, @pedroigor

[pedroigor]

Sounds reasonable for me, but I would keep this on hold until we have a real demand for becoming stateful.

In Keycloak, for instance, we are planning to support reference tokens which should help with this. Also, people should always consider how much data they put into tokens.

[sberyozkin]

Hey @pedroigor Sure about the demand; but I've marked it as a good 1st issue, if someone finds it to be a useful feature for them then IMHO we should let them do it, we are not going ahead with the stateful idea; our default will remain tokens in the cookies :-)
I'd like to consider the token data amount concern be orthogonal, they should be able to put as many claims as they need per their application requirements; it is just our implementation detail that we keep tokens in the cookies :-).
Reference tokens - is it just the binary tokens ? The solution would ideally be portable.

[SumaiyaSafdar]

hey, I'm new to all of this . But I'm highly interested in contributing I'll be highly obliged if anyone would help me. Do I need to install quarkus or learn it? Or do I need to learn maven or graalvm? Please someone help.

[maxandersen]

Hi @SumaiyaSafdar - please see #12522 (comment) where I gave you answer to similar question. Thanks.

[gauravrmsc]

@sberyozkin I am interested in working on this extension. I would love to get started if no one else has started working on it yet.

[sberyozkin]

@gauravrmsc
Sure, give it a try please and let Pedro @pedroigor and myself know how is it going... As I said to Pedro, this is an optional solution, in fact users can just do it i their own code :-), but when they do need it it could be good to let them just pick up a light-weight ready to go extension IMHO :-)
What would be good to have, by the time this extension is completed, is a basic recommendation for the users how to take care of the DB synchronization across the micro services nodes. We can ask @gsmet and @Sanne for some advice.
thanks

[SumaiyaSafdar]

thanks @maxandersen I'm really obliged you replied.

[gauravrmsc]

@sberyozkin
Do I need to create a separate extension or just add one more implementation in the OIDC extension package.

[gauravrmsc]

@sberyozkin
I was trying to add quarkus-oidc extension as a dependency in my extension as I will need to write an implementation for TokenStateManager interface. But now if I am trying to test it, I am getting runtime exception - auth-server-url not configured. Can you guide me how to include the TokenStateManager interface in my extension.

[sberyozkin]

@gauravrmsc sorry, I've missed your questions.

Yes, it has to be a new extension, in its runtime it will have this @ApplicationScoped TokenStateManager implementation, in its deployment register it as an unremovabable bean

I think you can copy and paste integration-tests/oidc-code-flow and there in the CodeFlowTest keep the very 1st test only and in the application.properties - the very first (default) configuration

[sberyozkin]

@gauravrmsc Hi - have you had a chance to look into it a bit further ? I'd like this extension added sooner or later - if you are busy with something else then it is np, just let me know

[gauravrmsc]

@sberyozkin I want to work on this extension but this month I am a bit busy with my internship and university exams. Can I continue on this in Feb

[sberyozkin]

@gauravrmsc Sure - that sounds fine, good luck with the university work; let me know in Feb please - if you will have time to continue - this issue is not very urgent but we have some users which may benefit from it, so I'd prefer not to delay its resolution indefinitely. thanks

[sberyozkin]

@gauravrmsc - Hi, how are things - can you let me now please if you are intending to resume working on this issue ? Np if not - i can suggest a few other issues to look at which can take less time, etc
Thanks

[ahmadshabib]

@sberyozkin Hello, was navigating through good first issues label as I am interested in quarkus project and would like to contribute.
What is your estimation of the time required for this one? if it is too much I would like to pick up something easier so I can get myself familiar with the codebase(at least some part of it) any suggestions on that as well?
Thanks.

[sberyozkin]

@ahmadshabib Hi, missed it, yes, it would not be the best issue to start getting familiar with Quarkus, please pick up something else, I'm going to remove a good first issue label to avoid the confusions, thanks

[sberyozkin]

Hi @pedroigor @stuartwdouglas,

I think this issue should be addressed somehow after a recent case of the Azure producing the code flow tokens with the size > 4096 and the authentication failing. So far the default token state manager has managed to save the day by having a single cookie combining 3 tokens split into into 3 cookies. However the case where it won't be able to handle a > 4096 case is not far away. Example in the Azure case, the bulk of it is the ID token, with the access token likely be a small binary token and trying to re-optimize ID token is not going go be a generic solution.
So we'll have cases (and possibly already have - as the users started creating the custom managers) when the only option for the current user to try quarkus-oidc is to start from writing a custom manager and it is not the ideal start and can discourage - we should have the options for it to always work when the user is only getting started.
Hence I believe this extension should be done so that we can sure that if there will be a case of quarkus-oidc not being able to handle the session cookie with its default manager then a user should be able just add this extension and it all works - and then the user will decide if a further customization is needed or not.

It probably should be based on a cache though such as quarkus-cache. It really should be a lightweight extension where it saves AuthorizationTokens and returns a key and then uses this key to remove or extract them

@cemnura would like to have look at this issue.

thanks

[cemnura]

@sberyozkin are we looking for a caching mechanism like the following? Caching/Invalidating would be based on the tokenState?

quarkus/extensions/oidc-cache/runtime/src/main/java/io/quarkus/oidc/runtime/CacheTokenStateManager.java

Lines 22 to 32 in dda8d67

	@Override
	@CacheResult(cacheName = "tokenState")
	public Uni<AuthorizationCodeTokens> getTokens(RoutingContext routingContext, OidcTenantConfig oidcConfig, @CacheKey String tokenState, OidcRequestContext<AuthorizationCodeTokens> requestContext) {
	return TokenStateManager.super.getTokens(routingContext, oidcConfig, tokenState, requestContext);
	}
	@Override
	@CacheInvalidate(cacheName = "tokenState")
	public Uni<Void> deleteTokens(RoutingContext routingContext, OidcTenantConfig oidcConfig, @CacheKey String tokenState, OidcRequestContext<Void> requestContext) {
	return TokenStateManager.super.deleteTokens(routingContext, oidcConfig, tokenState, requestContext);
	}

[cemnura]

@sberyozkin I gave it my best shot and tried to implement a caching mechanism. Please have a look here.

Perhaps we can give the user the ability modify the caching properties such as

quarkus.cache.caffeine."tokenState".initial-capacity=10 
quarkus.cache.caffeine."tokenState".maximum-size=20
quarkus.cache.caffeine."tokenState".expire-after-write=60S

[sberyozkin]

Hi @cemnura Thanks for starting so quickly.
A few initial comments - I believe the extension should be named differently, we already have a cache for the token introspection/userinfo, so probably oidc-token-state-cache or may be even better oidc-authorization-code-tokens-cache. Hmm... But that is a bit long...Hard to come up with the optimal name :-). So how about oidc-code-tokens-cache and the description will clarify it with "Use the cache for storing authorization code flow tokens" or similar.

The current cache token manager is a copy of the default one - all of it has to go, OidcTenantConfig.TokenStateManager configuration is not related. You should introduce OidcCodeTokensCache configuration where the cache related properties will be set, I propose to avoid using caffeine there in case we decide to switch to Infinispan later.

So the config would read something like this:

quarkus.oidc-code-tokens-cache.initial-capacity=10 

I'd not be concerned about the max size here, as the authentication should always work and if the cache will restrict then some users won't be authenticated if the is size is max size. And the cache like Caffeine, Infinispan will take care of storing as many entries as needed.

The expiry should probably be calculated the same way the q_session coookie lifespan is calculated plus add a few mins to it just in case.

Thanks

[cemnura]

Hi @sberyozkin

I renamed the extension according to your comments.

Would a configuration class like this be suitable?

https://github.com/cemnura/quarkus/blob/c79212360ea19ba25cea71ae3af36aa586939523/extensions/oidc-code-tokens-cache/runtime/src/main/java/io/quarkus/oidc/runtime/OidcCodeTokensCacheConfiguration.java#L1-L11

I believe quarkus-cache is using caffeine as an underlying caching provider as documented here. How could be enable the user to switch to Infinispan if he wanted? Would we need to use infinispan-client?

I couldn't find where q_session cookie is being calculated? Could you please show an example?

Thanks

[sberyozkin]

Hi @cemnura

The config class like that one should do for now, I believe ...Configuration in its name should be changed to ...Config - it looks like this is the convention.
We'll see from the user's feedback how quarkus-cache can handle it and if needed we can switch completely to infinispan-client.
One thing I believe we should do is to run the integration tests with kubernetes to verify that with more than one pod it all works.

The session lifetime/max age calculated here: https://github.com/quarkusio/quarkus/blob/main/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java#L379

but the good news this value is already available as a routing context attribute, but may be add 30 secs to it.

[cemnura]

Hi @sberyozkin

I changed the config class as you pointed out.

Sounds like a good idea. Perhaps the quarkus-cache extension can provide a infinispan implementation. Thought for the future.

The expiry after write should be a bit more (30 secs) then the max age. Therefore, the token state cache would die when the token reached its max age. Is the max age completely dependent on oidc build time config like quarkus.oidc.credentials.jwt.lifespan? Because if that is the case perhaps we can configure the cache expire-after-write based on the lifespan configuration at build time?

Also, how could I configure the quarkus-cache extension parameters from within the oidc-code-tokens-cache extension? I need to somehow configure the quarkus-cache from this extension to set the parameters?

[sberyozkin]

Hi @cemnura

The expiry after write should be a bit more (30 secs) then the max age. Therefore, the token state cache would die when the token reached its max age.

Why would the token cache would be gone ? It should be set individually for a specific token entry, not for the whole cache

Is the max age completely dependent on oidc build time config like quarkus.oidc.credentials.jwt.lifespan?

It is a dynamic runtime value, it is calculated from the current ID token returned from Keycloak/etc

Also, how could I configure the quarkus-cache extension parameters from within the oidc-code-tokens-cache extension? I need to somehow configure the quarkus-cache from this extension to set the parameters?

I think you'll need to use the Cache provider API directly :-)

[cemnura]

Hi @sberyozkin

Hi @cemnura

The expiry after write should be a bit more (30 secs) then the max age. Therefore, the token state cache would die when the token reached its max age.

Why would the token cache would be gone ? It should be set individually for a specific token entry, not for the whole cache

Sorry, I meant as in cache entry would die for each specific token entry. Each entry would expire token lifespan + 30 seconds. If for example the token lifespan is constant at 60 seconds the cache entry would expire at 90 seconds.

If so couldn't we just always have the cache expiry set to be current time + lifespan seconds + 30 seconds for each individual token?

Is the max age completely dependent on oidc build time config like quarkus.oidc.credentials.jwt.lifespan?

It is a dynamic runtime value, it is calculated from the current ID token returned from Keycloak/etc

Yes it should be dynamic for sure depending on the time it was created. But will it always be the same duration? For example, the lifespan is set to 60 seconds then the token state max age will be current time + 60 seconds?

Also, how could I configure the quarkus-cache extension parameters from within the oidc-code-tokens-cache extension? I need to somehow configure the quarkus-cache from this extension to set the parameters?

I think you'll need to use the Cache provider API directly :-)

Do you mean directly use the caffeine cache rather then quarkus-cache extension?

Thanks for your help 😃

[sberyozkin]

Hi @cemnura

Sorry, I meant as in cache entry would die for each specific token entry. Each entry would expire token lifespan + 30 seconds. If for example the token lifespan is constant at 60 seconds the cache entry would expire at 90 seconds.
If so couldn't we just always have the cache expiry set to be current time + lifespan seconds + 30 seconds for each individual token?

Makes sense, I thought I may have suggested something similar, but sorry if my comments were confusing

Yes it should be dynamic for sure depending on the time it was created. But will it always be the same duration? For example, the lifespan is set to 60 seconds then the token state max age will be current time + 60 seconds?

I'm not sure it will be the same duration, probably it will be in most cases except when multiple tenants are used where it might differ, but we should just take RoutingContext max_age attribute whose value is calculated by the code auth mechanism and add a few secs like 30 secs to it.

Do you mean directly use the caffeine cache rather then quarkus-cache extension?

Good point, indeed, quarkus-caffeine is what should be used, I just assumed quarkus-cache was the same as Caffeine but it builds on top of quarkus-caffeine; so yeah, if needed, in time, we will replace it with the infinispan client, but for now it is more of a POC so going with quarkus-caffeine would be OK for a start

Thanks

[sberyozkin]

Hey @cemnura Lets try to have some progress on this one :-), how is it going ?

[cemnura]

Hey @cemnura Lets try to have some progress on this one :-), how is it going ?

Sure thing. I'll get back to it today. :)

[cemnura]

Implemented a quarkus-cache based caching solution but after revisiting with @sberyozkin that the cache would be per instance/deployment beating the purpose to cache in the first place. Decided to try out a db persistence approach. Later will attempt to make a infinispan-client based cache.

[sberyozkin]

@cemnura Thanks, good plan IMHO.

[michalvavrik]

My conclusion is that there should be 2 extensions

• quarkus-oidc-db-state-manager based on Hibernate Reactive so that we don't have to block for switching of thread on reactive endpoint would cost us. We should have it for there will always be applications that don't need cache so they don't have it
• Quarkus Cache based extension that can be used for not in memory cache implementations as Redis and Infinispan client (latter will require different impl. than Quarkus Cache) for this is something real time applications want to use - it is quicker and you are likely to have such cache in place already.

/cc @sberyozkin

[michalvavrik]

I had a look into this, and it turns out that at the moment, Hibernate ORM and Hibernate Reactive can't coexist. It doesn't seem right to provide quarkus-oidc-db-state-manager given Hibernate ORM is used in most cases by Quarkus app (AFAICT). But providing quarkus-oidc-db-state-manager for Hibernate ORM means execute blocking and most likely switch threads.

@sberyozkin I'd like to use cache after all. I can't understand arguments why it was previously ruled of as long as it is not in-memory cache. I won't look into this in next weeks, so please take your time to answer, but I'd like to see your POV. Thanks

[michalvavrik]

I presume argument against cache would be that the simplest apps probably don't have in place Redis / Infinispan. On the other hand, using cache would make this feature more usable for prod apps as it is faster and we don't have to take care of TTL.

[sberyozkin]

Hi @michalvavrik.

I presume argument against cache would be that the simplest apps probably don't have in place Redis / Infinispan.

I agree.

I presume argument against cache would be that the simplest apps probably don't have in place Redis / Infinispan.

using cache would make this feature more usable for prod apps as it is faster and we don't have to take care of TTL.

Possibly but I'm not sure we want to start recommending a stateful approach in prod.

IMHO, we can start with quarkus-oidc-db-state-manager and Hibernare ORM and that will do to address simple cases, if there will be strong demand - we can add quarkus-oidc-db-state-manager-reactive and likewise, we can have quarkus-oidc-cache-state-manager if Redis or Infinispan users would really prefer it.

Does it sound reasonable to you, start with the simple quarkus-oidc-db-state-manager and Hibernare ORM option ?

[michalvavrik]

Hi @michalvavrik.

I presume argument against cache would be that the simplest apps probably don't have in place Redis / Infinispan.

I agree.

I presume argument against cache would be that the simplest apps probably don't have in place Redis / Infinispan.

using cache would make this feature more usable for prod apps as it is faster and we don't have to take care of TTL.

Possibly but I'm not sure we want to start recommending a stateful approach in prod.

IMHO, we can start with quarkus-oidc-db-state-manager and Hibernare ORM and that will do to address simple cases, if there will be strong demand - we can add quarkus-oidc-db-state-manager-reactive and likewise, we can have quarkus-oidc-cache-state-manager if Redis or Infinispan users would really prefer it.

Does it sound reasonable to you, start with the simple quarkus-oidc-db-state-manager and Hibernare ORM option ?

Yep, this explanation makes sense to me. I'm glad to hear this, because I planned to go in the different direction.

[sberyozkin]

@michalvavrik Sounds good, let give users a simple option to keep the tokens in the local DB state and see what demand will be there to push it to the next level with Hibernate Reactive, Redis etc :-). I'm hoping each of such extensions would be very light-weight, say PanacheEntity TokenStateManager only (with some minor bootstrap/cleanup support if needed) for Hibernate ORM, etc