Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fixes #4481] - RP-Initiated Logout and session verification #8512

Merged
merged 1 commit into from
Apr 20, 2020

Conversation

pedroigor
Copy link
Contributor

No description provided.

@pedroigor
Copy link
Contributor Author

I'll spend a bit more time trying to figure out a way to prevent the logout endpoint from CSRF attacks. As we discussed, it seems there is no way to prevent CSRF without impacting UX. At the same time, the logout is not so risky and OPs can prevent that by asking the user if they want to log out as mentioned in the specs. So ...

The way it is, it is quite simple.

I'm probably going to implement logging too.

@pedroigor
Copy link
Contributor Author

@sberyozkin Btw, another thing we can do is to silently refresh tokens regardless of failures due to token expiration. That could allow users to have their tokens updated with whatever access information that is updated at the OP.

The way it works right now is to refresh tokens only in case the token expires.

@pedroigor
Copy link
Contributor Author

@sberyozkin I'm going to work on the suggestions. I also forgot to add a concurrency check when refreshing tokens ....

@pedroigor pedroigor force-pushed the issue-4481 branch 2 times, most recently from 7750043 to fca62d1 Compare April 10, 2020 13:24
@sberyozkin sberyozkin added this to the 1.5 milestone Apr 14, 2020
@stuartwdouglas
Copy link
Member

Where is the blocking call that is the problem?

@sberyozkin
Copy link
Member

sberyozkin commented Apr 15, 2020

@sberyozkin
Copy link
Member

sberyozkin commented Apr 15, 2020

@pedroigor Hi Pedro, I was thinking about the refreshTimeout while I was offline, at the moment we have a condition refreshTimeout > now - iat. But how to set this property correctly ? The iat may vary depending on when a given user has signed in but the property should work for all the users.

IMHO we should instead have refreshTimeout > now - exp which means the configuration just sets, for example, 60 (sec) and it works irrespectively of when the token was issued at, if the token has been expired for less than 1 min then try to refresh, if someone submitted a token which has been expired for 1 month, then no refresh is possible.

What do you think ? I can push it to your branch if you agree

@sberyozkin
Copy link
Member

sberyozkin commented Apr 15, 2020

@pedroigor Hi, one more thing, while looking at the issue to do with dropping the redirect query params, I've realized that in this PR we will lose the encoded path parameters since VetxWeb apparently decodes context.getPath. I've seen so many issues related to it before that I'd rather use URI.getRawPath() which is what is done on the master with the calculated absoluteUri.
The target JAX-RS request resource after all of these redirects may have @Encoded attached to a PathParam.
So, I'm very sorry, but lets get back to keeping the original buildRedirectUri and simply add buildLogoutUri. It is a very small change in the PR.
That said, I'm not 100% sure if it will really affect the protected JAX-RS resource or not, decoding and then URL encoding is always a bit messy, it is not guaranteed to restore the original encoded path as far as I recall as the path component is a bit more lenient in what characters it may keep unencoded which would otherwise be encoded in the query component.
Not a big deal I guess, but please consider a minor update, I'll leave it up to you
Thanks

@pedroigor
Copy link
Contributor Author

@sberyozkin Didn't the exp may be also different for different users too?

The way it is, it just means that we refresh whenever x seconds have elapsed since the last time the user authenticated. It can be even before the session actually expires.

@sberyozkin
Copy link
Member

sberyozkin commented Apr 15, 2020

@pedroigor Hi Pedro

The way it is, it just means that we refresh whenever x seconds have elapsed since the last time the user authenticated. It can be even before the session actually expires

That sounds good, yes. But I'm a bit confused. I don't think it can happen before the session expires though, as we initiate an optional refresh only when the id token expires ?

So if we have refreshTimeout > now-iat, lets say refreshTimeout is 60 secs. And the token has expired 2 mins ago. Then the refresh will not happen, unless we set to say 180 sec. But then again if the token expired say 3 mins ago then it will not work again as iat is even further behind.

But you are right :-), refreshTimeout > now - exp also suffers but is slightly different - we just say that only recently expired tokens can be refreshed, the closer the exp to now the more chances it has to be refreshed simply because it is easier to cover now-exp as opposed to now-iat

I may be very confused here :-)

@pedroigor
Copy link
Contributor Author

pedroigor commented Apr 15, 2020

The token may be expired but you want to refresh only if the session in the server may be active. So, tracking from iat helps the application to refresh every time X second passes based on the max timeout of the session at the OP (refresh token).

In addition to that, nothing stops us in the future to refresh the token regardless of token expiration after x second. Like I mentioned somewhere here, we may want to provide to applicatios a more dynamic behavior where changes at the OP (claims, roles, etc) are reflected to the application without a re-authentication.

@sberyozkin
Copy link
Member

sberyozkin commented Apr 16, 2020

Hi @pedroigor OK, as I said it does make sense, and sounds good.
But help please with the numbers. What can we recommend to set this value to such that it works consistently for all the users irrespectively of when their ID tokens were issued (how can a given endpoint know about the OP's policy re how long it will itself keep the user session open ?). Please type some examples with a few iat variations.

(I was wondering, may be we should just have a boolean refreshExpiredIdToken if the numbers don't work - it is simple, at the endpoint level we don't worry about what is happening in OP about the user sessions, if refreshExpiredIdToken is true - we try to extend the user session with this endpoint and refresh expired ID tokens, if not - don't refresh).

Like I mentioned somewhere here, we may want to provide to applicatios a more dynamic behavior where changes at the OP (claims, roles, etc) are reflected to the application without a re-authentication.

+1, this is very promising. I guess we'd have another property for it, say a listener which will listen in the backchannel for the updates from OP, this will drive the refresh independently from what we do now with the refresh when the ID token expires. So we will accommodate all the variations easily :-)

Lets finalize this one and I think it will be ready to go in.

Though I'm not sure about this refresh and blocking calls. Probably can be resolved as part of the other issue I opened...

Thanks

@pedroigor
Copy link
Contributor Author

Hi @pedroigor OK, as I said it does make sense, and sounds good.
But help please with the numbers. What can we recommend to set this value to such that it works consistently for all the users irrespectively of when their ID tokens were issued (how can a given endpoint know about the OP's policy re how long it will itself keep the user session open ?). Please type some examples with a few iat variations.

(I was wondering, may be we should just have a boolean refreshExpiredIdToken if the numbers don't work - it is simple, at the endpoint level we don't worry about what is happening in OP about the user sessions, if refreshExpiredIdToken is true - we try to extend the user session with this endpoint and refresh expired ID tokens, if not - don't refresh).

I think I see your point. A boolean would be easier and work too. From a UX perspective it is better. I think initially I was refreshing tokens regardless of expiration exception, thus the timeout.

The name should probably change to refresh-on-expiration-timeout or something like that.

Like I mentioned somewhere here, we may want to provide to applicatios a more dynamic behavior where changes at the OP (claims, roles, etc) are reflected to the application without a re-authentication.

+1, this is very promising. I guess we'd have another property for it, say a listener which will listen in the backchannel for the updates from OP, this will drive the refresh independently from what we do now with the refresh when the ID token expires. So we will accommodate all the variations easily :-)

Yeah, we can easily do that now. I think that is one of the reasons for leaving the timeout in seconds. But we always get back to it.

Lets finalize this one and I think it will be ready to go in.

Though I'm not sure about this refresh and blocking calls. Probably can be resolved as part of the other issue I opened...

Thanks

@sberyozkin
Copy link
Member

sberyozkin commented Apr 16, 2020

@pedroigor OK, can you please respond to

But help please with the numbers. What can we recommend to set this value to such that it works consistently for all the users irrespectively of when their ID tokens were issued (how can a given endpoint know about the OP's policy re how long it will itself keep the user session open ?). Please type some examples with a few iat variations.

? Thanks.
For example, lets say we set refresh-timeout to 300. As I said earlier it will only work if iat was less than 5 mins before now. If we we have an expired token which was issued 6 mins ago then it won't work. In both cases the token lifespan may be the same actually, say 5 mins, but in the latter case we just have a user checking his/her mobile for 1 extra min :-) and their expired token will not be refreshed

refresh-on-expiration-timeout

If the numbers work, then refresh-timeout is fine, we can tweak the docs to say this is used when the token has expired. If they don't work then a boolean refreshExpiredIdToken will do fine.

@pedroigor
Copy link
Contributor Author

@sberyozkin It will not work consistently for all users. They can have different iat values. You usually know how many times a session is active at the OP, especially when you are within the same domain. And if it is not the case, it does not really matter so you can set any value that forces a refresh as soon as the token expires. Let's say 2 seconds. If you set the value too high, a refresh may not happen when the token expires.

The point here is that we changed the code to only process refresh tokens when the token expires. So, for now, we can just have a boolean property as you suggested.

The numbers work just fine but a boolean is much easier from a user perspective, as you suggested. Thus my proposal to name the property as refresh-on-expiration-timeout. The timeout using a number will work better once we support that capability I mentioned.

@sberyozkin
Copy link
Member

sberyozkin commented Apr 16, 2020

@pedroigor I'm a bit lost, so do we go with refreshExpiredIdToken now, right ? I can push to your branch if you confirm.
I'm not sure it is worth having a property which will only be effective for some expired tokens. If we enable something around the refresh then it should work consistently for all the tokens. It just should work without the users guessing what to set the timeout too.

@sberyozkin
Copy link
Member

@pedroigor Hi Pedro,

The point here is that we changed the code to only process refresh tokens when the token expires. So, for now, we can just have a boolean property as you suggested.

This means you agree, I'd like to read it this way :-). I can push to the branch, just confirm

@pedroigor
Copy link
Contributor Author

@pedroigor Hi Pedro,

The point here is that we changed the code to only process refresh tokens when the token expires. So, for now, we can just have a boolean property as you suggested.

This means you agree, I'd like to read it this way :-). I can push to the branch, just confirm

Yeah. I'm glad to get your changes.

@sberyozkin
Copy link
Member

@pedroigor Thanks, I'll have a look. I was also thinking, as per your comments, that indeed introducing an interval may complement the refresh concept well (refresh it while it is not expired). Yeah, actually, having a boolean property as discussed above would apply to the expired tokens, and then to support your idea - we would just add a new property which would apply to any token even if they have not expired.

(if token is expired) {
   if  (refreshExpiredTokens) {
       // refresh
   }
} else if (now > refreshTokenInterval + iat) {
    // token is still valid, but it needs to be refreshed since more than `refreshTokenInterval` passed since `iat`
}

I'll open an issue to discuss later. Thanks

@sberyozkin
Copy link
Member

Hey Pedro @pedroigor I've resolved the conflicts and pushed an update:

  • renamed refresh-timeout to refresh-expired (since we already have .token. as in quarkus.oidc.token.refresh-expired) - we can tweak this name a bit if you'd like.
  • trySilentRefresh is conditionally run if the refresh-expired check in apply is true - will be easier to reuse trySilentRefresh once we return to the interval idea
  • minor tweak to use context.request().query() check

Once you approve my changes :-) (I can push some minor updates if needed) I will then approve and merge

@pedroigor
Copy link
Contributor Author

@sberyozkin I'm OK

@sberyozkin
Copy link
Member

HI @pedroigor Thanks. I'l open a few more issues a bit later on to record your idea about the proactive recycling of the sessions based on the time interval, as well as the custom cookie for the landing post logout page.
@pedroigor It has been a massive effort, thanks a million :-)

@sberyozkin sberyozkin dismissed stuartwdouglas’s stale review April 20, 2020 10:27

The review comments have been addressed

@sberyozkin sberyozkin self-requested a review April 20, 2020 10:28
@sberyozkin sberyozkin merged commit 4b4723d into quarkusio:master Apr 20, 2020
@gsmet
Copy link
Member

gsmet commented Apr 20, 2020

This looks like a brand new feature. Why does it have the backport? tag? I would be more comfortable to push that one to 1.5 except if there are strong reasons?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants