syncope-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Isuranga Perera <isurangamper...@gmail.com>
Subject Re: Token creation is not thread safe
Date Mon, 09 Apr 2018 09:10:51 GMT
Since such condition can happen only if the same user tries to login from 2
mediums at the same, this is rarely happen. However that slight chance may
prevent that particular user from login to the system until all or all-1
access tokens are expired.
Using the UNIQUE constraint will definitely will provide a better security
and furthermore will make the model more meaningful. On the other hand this
will break the token replacing functionality since it first create a token
(at this time there are 2 tokens in the db) and delete the last one.

What I propose is writing a separate query to replace tokens instead of
using save & delete queries separately and furthermore we can use a new
query to save tokens without affecting the UNIQUE constraints so that no
need to mess with threading & @Transactional properties.

On Mon, Apr 9, 2018 at 2:06 PM, Francesco Chicchiriccò <ilgrosso@apache.org>
wrote:

> On 09/04/2018 10:14, Isuranga Perera wrote:
>
>> The Read Committed isolation level ensures that data can only be read by a
>> transaction if it is in the committed state. It doesn't completely isolate
>> this transaction(create) hence some other transaction can still use this
>> method which results in the behavior I explained previously. Ideally If
>> we're gonna use @Transactional annotation the isolation level should be
>> serialized for create operation. Please correct me If I'm wrong.
>>
>
> I see your point - while I don't completely agree about the likelihood of
> such race condition to actually happen.
>
> At worse, you might end up in having two distinct JWT (Access Tokens)
> values for a single user, which will anyway be subject to expiration due to
>
> https://github.com/apache/syncope/blob/master/core/provision
> ing-java/src/main/java/org/apache/syncope/core/provisioni
> ng/java/job/ExpiredAccessTokenCleanup.java
>
> For additional security, we might want to impose a UNIQUE constraint on
>
> https://github.com/apache/syncope/blob/master/core/persisten
> ce-jpa/src/main/java/org/apache/syncope/core/persistenc
> e/jpa/entity/JPAAccessToken.java#L48
>
> (not sure to remember why the column is currently set as nullable, though).
> With UNIQUE owner, the step (5) in your sequence below will fail anyway.
>
> Again, given the chances that the race condition applies, and considering
> what would be the harm (nearly none, to me), I would rather avoid any
> modification rather than imposing the UNIQUE constraint.
>
>
> Regards.
>
> On Mon, Apr 9, 2018 at 1:12 PM, Francesco Chicchiriccò <
>> ilgrosso@apache.org>
>> wrote:
>>
>> On 09/04/2018 09:30, Isuranga Perera wrote:
>>>
>>> Hi Francesco,
>>>>
>>>> Yes there is @Transactional annotation. But it haven't set the isolation
>>>> property as well as the propagation property. Based on the default
>>>> values
>>>> set this thread safe problem will still occur. Please correct me if I'm
>>>> wrong.
>>>>
>>>> The transaction isolation level is set in
>>>
>>> https://github.com/apache/syncope/blob/master/core/persisten
>>> ce-jpa/src/main/resources/domains/MasterDomain.xml#L57-L59
>>>
>>> Regards.
>>>
>>>
>>> On Mon, Apr 9, 2018 at 12:20 PM, Francesco Chicchiriccò <
>>>
>>>> ilgrosso@apache.org> wrote:
>>>>
>>>> On 09/04/2018 08:46, Isuranga Perera wrote:
>>>>
>>>>> Hi Francesco,
>>>>>
>>>>>> I will take a scenario. Suppose a scenario where thread A & thread
B
>>>>>> try
>>>>>> to login user admin.
>>>>>>
>>>>>>    1. thread A checks if a token exist for the user admin (suppose
>>>>>>       currently there is no token associated with the admin)
>>>>>>    2. Then thread A execute following logic[1] to create and save
the
>>>>>> token.
>>>>>>    3. Before thread A save the token for user admin thread B checks
>>>>>> if a
>>>>>>       token exist for user admin (since the toked created by thread
A
>>>>>> is
>>>>>>       not yet saved *exist == null*)
>>>>>>    4. Then thread A complete the creation of token (and saving)
>>>>>>    5. Thread B also complete the creation and saving of the token.
>>>>>>
>>>>>> That way there can be 2 tokens for a particular user.
>>>>>>
>>>>>>
>>>>>> You analysis does not take into any account the fact of the
>>>>>> constraints
>>>>>>
>>>>> imposed by the @Service annotation in
>>>>>
>>>>> https://github.com/apache/syncope/blob/master/core/rest-cxf/
>>>>> src/main/java/org/apache/syncope/core/rest/cxf/service/Acces
>>>>> sTokenServiceImpl.java#L35
>>>>>
>>>>> (e.g. the place when external requests can come in) nor by the
>>>>> @Transactional annotation injected into
>>>>>
>>>>> https://github.com/apache/syncope/blob/master/core/logic/
>>>>> src/main/java/org/apache/syncope/core/logic/AccessTokenLogic.java#L80
>>>>>
>>>>> via
>>>>>
>>>>> https://github.com/apache/syncope/blob/master/core/logic/
>>>>> src/main/java/org/apache/syncope/core/logic/AbstractTra
>>>>> nsactionalLogic.java#L29
>>>>>
>>>>> Regards.
>>>>>
>>>>> [1] https://github.com/apache/syncope/blob/master/core/provision
>>>>>
>>>>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L119
>>>>>>
>>>>>> Best Regards
>>>>>> Isuranga Perera
>>>>>>
>>>>>> On Mon, Apr 9, 2018 at 11:42 AM, Francesco Chicchiriccò <
>>>>>> ilgrosso@apache.org <mailto:ilgrosso@apache.org>> wrote:
>>>>>>
>>>>>>       On 09/04/2018 07:07, Isuranga Perera wrote:
>>>>>>
>>>>>>           Hi All,
>>>>>>
>>>>>>           Token create method in AccessTokenDataBinderImpl[1] is
not
>>>>>>           thread safe.
>>>>>>
>>>>>>
>>>>>>       Could you please explain why you're affirming this?
>>>>>>
>>>>>>           This could result in several problems including
>>>>>>
>>>>>>             * Exist 2 different access token for a particular user
at
>>>>>> a
>>>>>>           given
>>>>>>               time which may result in an exception thrown by method
>>>>>> call[2]
>>>>>>               since it expects a single token a given user.
>>>>>>
>>>>>>           In addition to that token replace is implemented as a
>>>>>>           combination of 2 different functionalities. Since the method
>>>>>>           is not thread safe this may cause some unexpected behaviors
>>>>>>           (since there can be 2 tokens exist for a particular user.
>>>>>> same
>>>>>>           scenario as above).
>>>>>>
>>>>>>           Appreciate your insight on the $subject.
>>>>>>
>>>>>>
>>>>>>           [1]
>>>>>>           https://github.com/apache/sync
>>>>>> ope/blob/master/core/provision
>>>>>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L104
>>>>>>           <https://github.com/apache/syn
>>>>>> cope/blob/master/core/provisio
>>>>>> ning-java/src/main/java/org/apache/syncope/core/provisioni
>>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L104>
>>>>>>
>>>>>>           [2]
>>>>>>           https://github.com/apache/sync
>>>>>> ope/blob/master/core/provision
>>>>>> ing-java/src/main/java/org/apache/syncope/core/provisioni
>>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L113
>>>>>>           <https://github.com/apache/syn
>>>>>> cope/blob/master/core/provisio
>>>>>> ning-java/src/main/java/org/apache/syncope/core/provisioni
>>>>>> ng/java/data/AccessTokenDataBinderImpl.java#L113>
>>>>>>
>>>>>>           Best Regards
>>>>>>           Isuranga Perera
>>>>>>
>>>>>
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message