weave-gitops icon indicating copy to clipboard operation
weave-gitops copied to clipboard

refresh OIDC tokens after expiry

Open alichaddad opened this issue 3 years ago • 14 comments

What changed? Added a refresh OIDC token mechanism when current token in the session is expired

Why was this change made? There were issues with tokens with a short expiration that was less than the session time which led to the user being forced to log in multiple times in a single session

How was this change implemented? In the auth middleware when a a request is unauthenticated. The code attempts to refresh the tokens before reporting to the user that he is unauthenticated. The refresh token is added to a cookie and used to send a refresh request to the OIDC server.

How did you validate the change? I ran an OIDC server using dex and configured the expiration to be 2min and checked that the refresh flow was triggered.

alichaddad avatar Aug 30 '22 07:08 alichaddad

To quote from the Dex docs...

Depending on the connectors limitations in protocols can prevent dex from issuing refresh tokens or returning group membership claims.

So, we might need to think about what happens if the upstream doesn't allow it, do we get an error?

bigkevmcd avatar Aug 30 '22 09:08 bigkevmcd

To quote from the Dex docs...

Depending on the connectors limitations in protocols can prevent dex from issuing refresh tokens or returning group membership claims.

So, we might need to think about what happens if the upstream doesn't allow it, do we get an error?

I think this will depend on the provider used, ideally the server should check the grant_type and if it isn't supported(in that case refersh_token) it should return an error, but I am not sure if we can depend on this. This might be reported even earlier when sending the scope

alichaddad avatar Aug 30 '22 11:08 alichaddad

It's not just a matter of whether the provider technically support it - a refresh_token is intended to be used "forever", so an administrator might not want us to have refresh_tokens.

If anything, if the system administrator has decided to limit lifetimes of tokens to very, very short times, then I would think it's pretty likely that the same system administrator would also refuse to issue refresh_tokens.

There were issues with tokens with a short expiration that was less than the session time which led to the user being forced to log in multiple times in a single session

Can you please provide some more information about this? I'm not clear on what type of time windows you're talking about, which expiration, or where it expires (OIDC server or k8s API server).

In particlar, are you sure that the problem is not that we require the user to specify tokenDuration as a hard-coded value in the secret, instead of using expires_in from the token itself?

ozamosi avatar Aug 30 '22 12:08 ozamosi

@foot @saeedfazal can you provide more context here?

alichaddad avatar Aug 30 '22 13:08 alichaddad

If anything, if the system administrator has decided to limit lifetimes of tokens to very, very short times, then I would think it's pretty likely that the same system administrator would also refuse to issue refresh_tokens.

Interesting. I'm unfamiliar with setting this up. We've had reports from people (cc @makkes and others) trying to do demos that their IdP's have configured id_token to only last 15mins (even 5mins sometimes!). I would have guessed that the refresh_token would have been the way to support 5/15mins id_token expirations and still keep an okay UX.

In particlar, are you sure that the problem is not that we require the user to specify tokenDuration as a hard-coded value in the secret, instead of using expires_in from the token itself?

This is 1h by default I believe and so not the issue we're seeing, but as you say the cookie expiration should really match the expiration provided by the IdP.

a refresh_token is intended to be used "forever", so an administrator might not want us to have refresh_tokens.

@alichaddad have you noticed an expiration coming back w/ the refresh_tokens?

foot avatar Aug 30 '22 13:08 foot

their IdP's have configured id_token to only last 15mins (even 5mins sometimes!).

Thank you - that is way less indeed!

Is the id_token and the access_token configured to the same duration?

ozamosi avatar Aug 30 '22 13:08 ozamosi

Other question - I'm sorry for this barrage of questions, I shouldn't do all this after work was already being done.

With e.g. google I get tokens that are valid way shorter (hours) than my logged in session (days) - so if I use google as my OIDC provider, then every once in a while my token expires, I get redirected to google, I already have a valid session there so they immediately issue a new token, and then I get redirected back, and the only thing I see is an extra second to load the page and a bit of flickering in the URL bar. I don't think we handle this properly, but is this how it is supposed to work on these providers?

ozamosi avatar Aug 30 '22 13:08 ozamosi

@foot refresh tokens have expiry but it is generally for a long duration. If it is configured for a short time then there isn't much we can do about it. Although certain providers will reset the expiry every time the refresh token is used.

@ozamosi they can have different expiration, a lot of providers allow giving them different values, so it depends on the way they were configured. The way it is done now there shouldn't be a redirect. If the user becomes unauthenticated we attempt to send a request with the values in the cookies to refresh the tokens. I am not sure if that's the best approach though. Initial thought was making the UI refresh the token and retry the failed request but this involved a refactor in the way API calls are made in the UI

alichaddad avatar Aug 30 '22 13:08 alichaddad

@foot refresh tokens have expiry but it is generally for a long duration

Is that like 1d or 14d, does dex have a default here?

I get redirected to google, I already have a valid session there so they immediately issue a new token.

Yeah that sounds like it would fly every day or so, every 5mins might still be a bit rough (maybe we don't have a choice though I'm not sure). We don't do this automatically, but would be kind of neat.

foot avatar Aug 30 '22 14:08 foot

For dex it depends on what's in the configuration file, otherwise no expiry is set.

@bigkevmcd there is way to know if an OIDC provider supports refreshing the tokens by checking the openid configuration that should be be provided by all providers.

alichaddad avatar Aug 30 '22 14:08 alichaddad

@alichaddad scopes_supported ?

bigkevmcd avatar Aug 30 '22 15:08 bigkevmcd

@bigkevmcd Yes and grant_types_supported

alichaddad avatar Aug 30 '22 15:08 alichaddad

Is the id_token and the access_token configured to the same duration?

Both seem to be 1h by default https://github.com/weaveworks/weave-gitops/blob/44cbabd49c37c81b21c4881cd98e7aeff3273e9e/pkg/server/auth/server.go#L84-L87

foot avatar Aug 31 '22 08:08 foot

This is a great discussion (and it shows how unspecific much of the OIDC spec actually is). One thing that would greatly improve the UX would be if Wego would automatically redirect the user to the IdP because there the user might still be logged in. However, as a user I always have to hit "Login with OIDC Provider" as soon as the token expires.

makkes avatar Aug 31 '22 14:08 makkes

As much as I want to see this resolved, this PR hasn't seen any movement for over 3 months. I'm closing this now but not without stating that we should continue to strive towards a better OIDC UX. A discussion might be a better fit at this point, though.

makkes avatar Dec 09 '22 09:12 makkes

Let's keep the branch around just now, as it's likely that we'll need to do this work (and some further OIDC) work sooner rather than later.

bigkevmcd avatar Dec 09 '22 09:12 bigkevmcd

I think this issue has been reactivated as weaveworks/weave-gitops-enterprise#2383

kingdonb avatar Feb 15 '23 05:02 kingdonb