syncope-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Francesco Chicchiriccò <ilgro...@apache.org>
Subject Re: Token creation is not thread safe
Date Mon, 09 Apr 2018 10:57:01 GMT
On 09/04/2018 11:24, Isuranga Perera wrote:
> Sure will work on that. Shall I create a JIRA?

Yes, please.
Do set both 2.0.9 and 2.1.0 as fix-for-versions since I will apply your 
PR both to branches master and 2_0_X.

> Sorry for the delay will submit the ICLA asap

Ok, thanks.

Regards.

> 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
View raw message