4:45 a.m.
Hi,
I've sent PR https://github.com/keycloak/keycloak/pull/5736 to fix the
issue in subject (corresponding JIRAs are KEYCLOAK-7774 KEYCLOAK-8438).
The cause is that once the user is authenticated in the browser and has
userSession, there is just 1 clientSession per the userSession+client.
So in case of concurrent SSO login in more browser tabs, there can be an
issue that each tab is writing it's state to the same clientSession,
which causes conflicts (especially attributes like nonce, scope or
redirectUri can be possibly different for each tab and shouldn't be
shared).
The possibility can be to revert back when we had more clientSessions
per userSession+client. I think this is step back and has lots of other
issues (memory consumption, more cluster and cross-dc writes, worse
performance in general...). For the future, it will be rather ideal to
remove clientSession entirely or reduce their size even more, as they
will be needed for logout, but hopefully for nothing more.
So the other possibility is to move most of the state to the "code"
parameter and to the refreshToken. Each browser tab has it's own "code"
and it's own refreshToken. Problem with the "code" is, that it's used as
query parameter in the URL and hence has limits for it's size. It seems
ideal is still to stick with 2000 characters per URL. And when
redirectUri itself will be part of the code, it can be problematic as we
don't know how the redirectUri provided by users will be big....
Fortunately refreshToken doesn't have this issue as it needs to be
always sent in the POST request.
So to solve the issue for "code", I've added an entry to the infinispan
cache "actionTokens", which encapsulates needed state and it's supposed
to be valid just for the very short time (between the
AuthenticationResponse to the application and the time when application
sends the code-to-token request) and it's lifespan corresponds to realm
code lifespan (1 minute by default). In the code-to-token request, there
is a call to "cache.remove" to remove the code. Infinispan caches
(local, distributed and hotrod caches) fortunately guarantee that
"remove(123)" is always successful just in 1 thread (Successful means
that it returns previous state). So we still have guarantee for the
single-use code.
Marek
1:54 a.m.
The ideal may be to create a code token. If it's less than 2000 characters
just send it as is. If it's more than we put it in the actionTokens cache
and just send a reference to it in the code param. Not sure if the
reference should just be a uuid, or if it should be something more to make
sure it can't be guessed. The last point is also relevant to reference
tokens once/if we add support for that.
On Mon, 19 Nov 2018 at 11:49, Marek Posolda <mposolda(a)redhat.com> wrote:
Hi,
I've sent PR https://github.com/keycloak/keycloak/pull/5736 to fix the
issue in subject (corresponding JIRAs are KEYCLOAK-7774 KEYCLOAK-8438).
The cause is that once the user is authenticated in the browser and has
userSession, there is just 1 clientSession per the userSession+client.
So in case of concurrent SSO login in more browser tabs, there can be an
issue that each tab is writing it's state to the same clientSession,
which causes conflicts (especially attributes like nonce, scope or
redirectUri can be possibly different for each tab and shouldn't be
shared).
The possibility can be to revert back when we had more clientSessions
per userSession+client. I think this is step back and has lots of other
issues (memory consumption, more cluster and cross-dc writes, worse
performance in general...). For the future, it will be rather ideal to
remove clientSession entirely or reduce their size even more, as they
will be needed for logout, but hopefully for nothing more.
So the other possibility is to move most of the state to the "code"
parameter and to the refreshToken. Each browser tab has it's own "code"
and it's own refreshToken. Problem with the "code" is, that it's used
as
query parameter in the URL and hence has limits for it's size. It seems
ideal is still to stick with 2000 characters per URL. And when
redirectUri itself will be part of the code, it can be problematic as we
don't know how the redirectUri provided by users will be big....
Fortunately refreshToken doesn't have this issue as it needs to be
always sent in the POST request.
So to solve the issue for "code", I've added an entry to the infinispan
cache "actionTokens", which encapsulates needed state and it's supposed
to be valid just for the very short time (between the
AuthenticationResponse to the application and the time when application
sends the code-to-token request) and it's lifespan corresponds to realm
code lifespan (1 minute by default). In the code-to-token request, there
is a call to "cache.remove" to remove the code. Infinispan caches
(local, distributed and hotrod caches) fortunately guarantee that
"remove(123)" is always successful just in 1 thread (Successful means
that it returns previous state). So we still have guarantee for the
single-use code.
Marek
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
2:03 a.m.
On 26/11/2018 08:54, Stian Thorgersen wrote:
The ideal may be to create a code token. If it's less than 2000
characters just send it as is. If it's more than we put it in the
actionTokens cache and just send a reference to it in the code param.
I see. So it
will be something like "hybrid" between both approaches.
Was also a bit thinking about it, but didn't in the end. ATM it is
always adding the item to actionTokens cache. Maybe it's possible to
optimize it and add the "hybrid" possibility, but not sure about it at
this stage? Maybe after new year?
Not sure if the reference should just be a uuid, or if it should be
something more to make sure it can't be guessed. The last point is
also relevant to reference tokens once/if we add support for that.
ATM it is <code-uuid>.<user-session-id>.<client-uuid>
The userSessionId and clientUUID are needed, so if code was already used
(and is removed from actionTokens cache), it is still possible to lookup
userSession and remove clientSession from it as OIDC specs suggests (in
case that attempt of duplicate code use was detected).
Not sure how big probability is to guess UUID for the very short-lived
code. It seems to me like kind of impossible thing. And if it's really
guessed, then when valid user will try to exchange the code, which
attacker already exchanged, the "code" will be expired (client removed
from userSession as pointed above).
Marek
On Mon, 19 Nov 2018 at 11:49, Marek Posolda <mposolda(a)redhat.com
<mailto:mposolda@redhat.com>> wrote:
Hi,
I've sent PR https://github.com/keycloak/keycloak/pull/5736 to fix
the
issue in subject (corresponding JIRAs are KEYCLOAK-7774
KEYCLOAK-8438).
The cause is that once the user is authenticated in the browser
and has
userSession, there is just 1 clientSession per the
userSession+client.
So in case of concurrent SSO login in more browser tabs, there can
be an
issue that each tab is writing it's state to the same clientSession,
which causes conflicts (especially attributes like nonce, scope or
redirectUri can be possibly different for each tab and shouldn't be
shared).
The possibility can be to revert back when we had more clientSessions
per userSession+client. I think this is step back and has lots of
other
issues (memory consumption, more cluster and cross-dc writes, worse
performance in general...). For the future, it will be rather
ideal to
remove clientSession entirely or reduce their size even more, as they
will be needed for logout, but hopefully for nothing more.
So the other possibility is to move most of the state to the "code"
parameter and to the refreshToken. Each browser tab has it's own
"code"
and it's own refreshToken. Problem with the "code" is, that it's
used as
query parameter in the URL and hence has limits for it's size. It
seems
ideal is still to stick with 2000 characters per URL. And when
redirectUri itself will be part of the code, it can be problematic
as we
don't know how the redirectUri provided by users will be big....
Fortunately refreshToken doesn't have this issue as it needs to be
always sent in the POST request.
So to solve the issue for "code", I've added an entry to the
infinispan
cache "actionTokens", which encapsulates needed state and it's
supposed
to be valid just for the very short time (between the
AuthenticationResponse to the application and the time when
application
sends the code-to-token request) and it's lifespan corresponds to
realm
code lifespan (1 minute by default). In the code-to-token request,
there
is a call to "cache.remove" to remove the code. Infinispan caches
(local, distributed and hotrod caches) fortunately guarantee that
"remove(123)" is always successful just in 1 thread (Successful means
that it returns previous state). So we still have guarantee for the
single-use code.
Marek
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org <mailto:keycloak-dev@lists.jboss.org>
https://lists.jboss.org/mailman/listinfo/keycloak-dev
11:08 a.m.
On Mon, 26 Nov 2018, 09:04 Marek Posolda <mposolda(a)redhat.com wrote:
On 26/11/2018 08:54, Stian Thorgersen wrote:
The ideal may be to create a code token. If it's less than 2000 characters
just send it as is. If it's more than we put it in the actionTokens cache
and just send a reference to it in the code param.
I see. So it will be something like "hybrid" between both approaches. Was
also a bit thinking about it, but didn't in the end. ATM it is always
adding the item to actionTokens cache. Maybe it's possible to optimize it
and add the "hybrid" possibility, but not sure about it at this stage?
Maybe after new year?
+1 for the future. I guess one issue would be the single use aspect.
Not sure if the reference should just be a uuid, or if it should be
something more to make sure it can't be guessed. The last point
is also
relevant to reference tokens once/if we add support for that.
ATM it is <code-uuid>.<user-session-id>.<client-uuid>
The userSessionId and clientUUID are needed, so if code was already used
(and is removed from actionTokens cache), it is still possible to lookup
userSession and remove clientSession from it as OIDC specs suggests (in
case that attempt of duplicate code use was detected).
Not sure how big probability is to guess UUID for the very short-lived
code. It seems to me like kind of impossible thing. And if it's really
guessed, then when valid user will try to exchange the code, which attacker
already exchanged, the "code" will be expired (client removed from
userSession as pointed above).
uuid should be sufficiently secure, but there's a potential here for a
brute force attack of sorts though.
Marek
On Mon, 19 Nov 2018 at 11:49, Marek Posolda <mposolda(a)redhat.com> wrote:
> Hi,
>
> I've sent PR https://github.com/keycloak/keycloak/pull/5736 to fix the
> issue in subject (corresponding JIRAs are KEYCLOAK-7774 KEYCLOAK-8438).
> The cause is that once the user is authenticated in the browser and has
> userSession, there is just 1 clientSession per the userSession+client.
> So in case of concurrent SSO login in more browser tabs, there can be an
> issue that each tab is writing it's state to the same clientSession,
> which causes conflicts (especially attributes like nonce, scope or
> redirectUri can be possibly different for each tab and shouldn't be
> shared).
>
> The possibility can be to revert back when we had more clientSessions
> per userSession+client. I think this is step back and has lots of other
> issues (memory consumption, more cluster and cross-dc writes, worse
> performance in general...). For the future, it will be rather ideal to
> remove clientSession entirely or reduce their size even more, as they
> will be needed for logout, but hopefully for nothing more.
>
> So the other possibility is to move most of the state to the "code"
> parameter and to the refreshToken. Each browser tab has it's own
"code"
> and it's own refreshToken. Problem with the "code" is, that it's
used as
> query parameter in the URL and hence has limits for it's size. It seems
> ideal is still to stick with 2000 characters per URL. And when
> redirectUri itself will be part of the code, it can be problematic as we
> don't know how the redirectUri provided by users will be big....
> Fortunately refreshToken doesn't have this issue as it needs to be
> always sent in the POST request.
>
> So to solve the issue for "code", I've added an entry to the
infinispan
> cache "actionTokens", which encapsulates needed state and it's
supposed
> to be valid just for the very short time (between the
> AuthenticationResponse to the application and the time when application
> sends the code-to-token request) and it's lifespan corresponds to realm
> code lifespan (1 minute by default). In the code-to-token request, there
> is a call to "cache.remove" to remove the code. Infinispan caches
> (local, distributed and hotrod caches) fortunately guarantee that
> "remove(123)" is always successful just in 1 thread (Successful means
> that it returns previous state). So we still have guarantee for the
> single-use code.
>
> Marek
>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
2222
days inactive
2230
days old
3 comments
2 participants
participants (2)
-
Marek Posolda -
Stian Thorgersen