Refresh tokens are no longer reusable. This is done by setting the client sessions timestamp when a new refresh token is issued. If the refresh tokens iat value is less than the client sessions timestamp it's not permitted. If anyone has time I'd appreciate a review of the changes: https://github.com/keycloak/keycloak/pull/1732 For anyone that runs into issues with this policy there's an option to disable it in the admin console in the realms token settings. This does not apply to offline tokens (at least yet). We need to add it to offline tokens as well though as it's even more important for those. There's two problems with offline tokens though, firstly the setTimestamp is not permitted on offline client sessions. Secondly if we allow setting it we would have to persist it, unless someone can come up with something clever.

_______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

On 10/14/2015 5:49 PM, Marek Posolda wrote: > On 14/10/15 20:24, Stian Thorgersen wrote: >> Refresh tokens are no longer reusable. This is done by setting the >> client sessions timestamp when a new refresh token is issued. If the >> refresh tokens iat value is less than the client sessions timestamp >> it's not permitted. >> >> If anyone has time I'd appreciate a review of the changes: >> <https://github.com/keycloak/keycloak/pull/1732> https://github.com/keycloak/keycloak/pull/1732 >> >> For anyone that runs into issues with this policy there's an option to >> disable it in the admin console in the realms token settings. >> >> This does not apply to offline tokens (at least yet). We need to add >> it to offline tokens as well though as it's even more important for >> those. There's two problems with offline tokens though, firstly the >> setTimestamp is not permitted on offline client sessions. Secondly if >> we allow setting it we would have to persist it, unless someone can >> come up with something clever. > I think we don't need to persist, but just save clientSession with > updated timestamp into infinispan/memory. Then during startup, the > timestamp of clientSessions will be updated to startup time similarly > like we have for lastSessionRefresh of user sessions. The refresh will > be allowed if (iat == clientSession.timestamp OR startupTime == > clientSession.timestamp) . In other words, first refresh after server > restart will be always allowed. > > There is some chance that there can be same refresh token used two > times (if attacker will do second refresh after server restart). But > then clientSession timestamp will be updated and regular user won't be > allowed to refresh his token and will recognize error. > > > But question is, do we really want refreshTokenReusable to be disabled > by default? For offline tokens, people will often need to save the > offline token into their database on application side. With > refreshTokenReusable disabled, they will need to always write into their > DB and save new offline token after each refresh. > My own personal opinion is that we are making this fix to pass some random company's security audit that I don't particularly agree with. If a client has been compromised, then its offline tokens should be revoked and a revocation not-before policy should be pushed out. As it is, the only reason we need to regenerate the refresh token is to update its timestamp for idle checks. -- Bill Burke JBoss, a division of Red Hat http://bill.burkecentral.com _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

On 15/10/15 06:36, Stian Thorgersen wrote: That was my initial idea as well, but then again it already works with our adapters, we already regenerate the tokens, so why not add this extra layer of defence? End of the day as refresh tokens can be stored on the client side how well they are secured can vary. If users want long sessions or even worse with offline tokens it makes sense to add this that enables users to at least notice something is going wrong. The issue is that you may not notice that a client has been compromised, but if all tokens stop working you will. I don't have an issue with setting it to true by default (so refresh tokens are reusable), but since our adapters already work and I can't see any big side effects of preventing refresh token reuse I set it to false by default. As I said, I can see the side effect just for offline tokens, that people will always need to write new offline token into their DB on application side after each refresh.

Marek On 15 October 2015 at 01:27, Bill Burke <bburke(a)redhat.com&gt; wrote: > > > On 10/14/2015 5:49 PM, Marek Posolda wrote: > > On 14/10/15 20:24, Stian Thorgersen wrote: > >> Refresh tokens are no longer reusable. This is done by setting the > >> client sessions timestamp when a new refresh token is issued. If the > >> refresh tokens iat value is less than the client sessions timestamp > >> it's not permitted. > >> > >> If anyone has time I'd appreciate a review of the changes: > >> <https://github.com/keycloak/keycloak/pull/1732> > <https://github.com/keycloak/keycloak/pull/1732> > https://github.com/keycloak/keycloak/pull/1732 > >> > >> For anyone that runs into issues with this policy there's an option to > >> disable it in the admin console in the realms token settings. > >> > >> This does not apply to offline tokens (at least yet). We need to add > >> it to offline tokens as well though as it's even more important for > >> those. There's two problems with offline tokens though, firstly the > >> setTimestamp is not permitted on offline client sessions. Secondly if > >> we allow setting it we would have to persist it, unless someone can > >> come up with something clever. > refreshTokenReusable > I think we don't need to persist, but just save > clientSession with > > updated timestamp into infinispan/memory. Then during startup, the > > timestamp of clientSessions will be updated to startup time similarly > > like we have for lastSessionRefresh of user sessions. The refresh will > > be allowed if (iat == clientSession.timestamp OR startupTime == > > clientSession.timestamp) . In other words, first refresh after server > > restart will be always allowed. > > > > There is some chance that there can be same refresh token used two > > times (if attacker will do second refresh after server restart). But > > then clientSession timestamp will be updated and regular user won't be > > allowed to refresh his token and will recognize error. > > > > > > But question is, do we really want refreshTokenReusable to be disabled > > by default? For offline tokens, people will often need to save the > > offline token into their database on application side. With > > refreshTokenReusable disabled, they will need to always write into their > > DB and save new offline token after each refresh. > > > > My own personal opinion is that we are making this fix to pass some > random company's security audit that I don't particularly agree with. > If a client has been compromised, then its offline tokens should be > revoked and a revocation not-before policy should be pushed out. As it > is, the only reason we need to regenerate the refresh token is to update > its timestamp for idle checks.refreshTokenReusable > > > -- > Bill Burke > JBoss, a division of Red Hat > http://bill.burkecentral.com > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev > _______________________________________________ keycloak-dev mailing listkeycloak-dev@lists.jboss.orghttps://lists.jboss.org/mailman/listinfo/keycloak-dev

A small extract from OAuth2 spec: The authorization server MAY issue a new refresh token, in which case the client MUST discard the old refresh token and replace it with the new refresh token. The authorization server MAY revoke the old refresh token after issuing a new refresh token to the client. If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request. Revoking old refresh tokens is optional. With that in mind and the fact that we didn't use to revoke old refresh tokens my vote goes to the default is not to revoke old refresh tokens, but still have the option to enable this for those that want to. I'm going to change it to that.

On 15 October 2015 at 08:49, Stian Thorgersen <sthorger(a)redhat.com <mailto:sthorger@redhat.com>> wrote: On 15 October 2015 at 08:44, Marek Posolda <mposolda(a)redhat.com <mailto:mposolda@redhat.com>> wrote: On 15/10/15 06:36, Stian Thorgersen wrote: > That was my initial idea as well, but then again it already > works with our adapters, we already regenerate the tokens, so > why not add this extra layer of defence? End of the day as > refresh tokens can be stored on the client side how well they > are secured can vary. If users want long sessions or even > worse with offline tokens it makes sense to add this that > enables users to at least notice something is going wrong. > The issue is that you may not notice that a client has been > compromised, but if all tokens stop working you will. > > I don't have an issue with setting it to true by default (so > refresh tokens are reusable), but since our adapters already > work and I can't see any big side effects of preventing > refresh token reuse I set it to false by default. As I said, I can see the side effect just for offline tokens, that people will always need to write new offline token into their DB on application side after each refresh. Thing is for offline tokens it's even more important to block old tokens, so would it not make sense to have it on by default as a security measure? Marek > > On 15 October 2015 at 01:27, Bill Burke <bburke(a)redhat.com > <mailto:bburke@redhat.com>> wrote: > > > > On 10/14/2015 5:49 PM, Marek Posolda wrote: > > On 14/10/15 20:24, Stian Thorgersen wrote: > >> Refresh tokens are no longer reusable. This is done by > setting the > >> client sessions timestamp when a new refresh token is > issued. If the > >> refresh tokens iat value is less than the client > sessions timestamp > >> it's not permitted. > >> > >> If anyone has time I'd appreciate a review of the changes: > >> <https://github.com/keycloak/keycloak/pull/1732>https://github.com/keyc... > >> > >> For anyone that runs into issues with this policy > there's an option to > >> disable it in the admin console in the realms token > settings. > >> > >> This does not apply to offline tokens (at least yet). > We need to add > >> it to offline tokens as well though as it's even more > important for > >> those. There's two problems with offline tokens > though, firstly the > >> setTimestamp is not permitted on offline client > sessions. Secondly if > >> we allow setting it we would have to persist it, > unless someone can > >> come up with something clever. > refreshTokenReusable > I think we don't need to persist, > but just save clientSession with > > updated timestamp into infinispan/memory. Then during > startup, the > > timestamp of clientSessions will be updated to startup > time similarly > > like we have for lastSessionRefresh of user sessions. > The refresh will > > be allowed if (iat == clientSession.timestamp OR > startupTime == > > clientSession.timestamp) . In other words, first > refresh after server > > restart will be always allowed. > > > > There is some chance that there can be same refresh > token used two > > times (if attacker will do second refresh after server > restart). But > > then clientSession timestamp will be updated and > regular user won't be > > allowed to refresh his token and will recognize error. > > > > > > But question is, do we really want refreshTokenReusable > to be disabled > > by default? For offline tokens, people will often need > to save the > > offline token into their database on application side. With > > refreshTokenReusable disabled, they will need to always > write into their > > DB and save new offline token after each refresh. > > > > My own personal opinion is that we are making this fix to > pass some > random company's security audit that I don't particularly > agree with. > If a client has been compromised, then its offline tokens > should be > revoked and a revocation not-before policy should be > pushed out. As it > is, the only reason we need to regenerate the refresh > token is to update > its timestamp for idle checks.refreshTokenReusable > > > -- > Bill Burke > JBoss, a division of Red Hat > http://bill.burkecentral.com > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > <mailto:keycloak-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/keycloak-dev > > > > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > <mailto:keycloak-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/keycloak-dev _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev