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 Fri, 13 Apr 2018 07:30:57 GMT
Hi,
after our discussion on PR #70 [1] yesterday, I took the chance to 
review the AccessToken creation logic and committed a change [2] which 
should fix your warnings from SYNCOPE-1301.

Please, take a look at let me know if we can consider SYNCOPE-1301 as 
resolved.

Regards.

[1] https://github.com/apache/syncope/pull/70
[2] 
https://github.com/apache/syncope/commit/24f789932141ee05fa12d81eca9d43178953f508

On 09/04/2018 13:19, Isuranga Perera wrote:
> Sure will work on that. I'll give priority to this feature and will
> continue to work on the eclipse project upon the completion of this one.
>
> Best Regards
> Isuranga Perera
>
> On Mon, Apr 9, 2018 at 4:27 PM, Francesco Chicchiriccò <ilgrosso@apache.org>
> wrote:
>
>> 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