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:24:45 GMT
Sure will work on that. Shall I create a JIRA?

Sorry for the delay will submit the ICLA asap

Best Regards
Isuranga Perera

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

> On 09/04/2018 11:10, Isuranga Perera wrote:
>
>> 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.
>>
>
> If you can come up with a proposal which works with all the supported
> DBMSes, then please go on.
>
> As already asked as comment in your recent PR: did you submit an ICLA for
> your contributions? Thanks.
>
> Regards.
>
> 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