Hello all, A last patch is available for testing with the given improvement : * add a security property that disable the feature by default * add a security property that enable the production mode (impersonated user is informed about the impersonation and cannot act during the process) * add a control that prevent ADMIN impersonation * add a control that prevent impersonation of a more privilege user * add some documentation
Lastly for the criticity of depersonation (coming back to admin user), i added a verification of the existence of an impersonation process. That would ensure that this method could not be abused (the impersonation process can only be set through a impersonation itself, or simulated with access to database, so pretty reliable) Any feedback are welcome :) Gil Le samedi 08 sept. 2018 à 11:32:17 (+0200), Pierre Smits a écrit : > Are we confident that this will not lead to a CVE when it will appear in a > release? > > > Best regards, > > Pierre Smits > > Apache Trafodion <https://trafodion.apache.org>, Vice President > Apache Directory <https://directory.apache.org>, PMC Member > Apache Incubator <https://incubator.apache.org>, committer > *Apache OFBiz <https://ofbiz.apache.org>, contributor (without privileges) > since 2008* > Apache Steve <https://steve.apache.org>, committer > > On Mon, Aug 20, 2018 at 4:00 PM, Jacques Le Roux < > jacques.le.r...@les7arts.com> wrote: > > > Anyway an admin can do that and much other things by directly poking in > > the DB > > > > So not really a concern for me > > > > Jacques > > > > > > Le 20/08/2018 à 14:04, Pierre Smits a écrit : > > > >> Consider this: > >> - having it enabled by default (as suggested by many).... > >> - enabling a user with higher privileges (suggested to be the OFBiz Admin) > >> to impersonate someone with lower privileges > >> - this user with higher privileges can now create/alter/etc... > >> transactions > >> in accounting, ordermgr, warehousing, ecommence, etc. through OFBiz as the > >> impersonated user > >> - this user can prevent - through OFBiz - that others can see > >> UserLoginHistory. > >> - impersonation is in many countries - by law - a criminal offence > >> - this goes directly against GDPR > >> > >> Caution is the Mother of the Porcelain Cabinet (as a saying in the > >> Netherlands goes). Especially when introducing features that have an > >> impact > >> on security aspects. > >> > >> > >> Best regards, > >> > >> Pierre Smits > >> > >> Apache Trafodion <https://trafodion.apache.org>, Vice President > >> Apache Directory <https://directory.apache.org>, PMC Member > >> Apache Incubator <https://incubator.apache.org>, committer > >> *Apache OFBiz <https://ofbiz.apache.org>, contributor (without > >> privileges) > >> since 2008* > >> > >> Apache Steve <https://steve.apache.org>, committer > >> > >> On Mon, Aug 20, 2018 at 12:04 PM, Taher Alkhateeb < > >> slidingfilame...@gmail.com> wrote: > >> > >> I don't have a strong opinion on this, and I am open. My personal > >>> preference is pehaps to just 'login as' instead of impersonate with > >>> normal > >>> user login history. The reason for my preference is having the least > >>> amount > >>> of code written and least security worries. I find the impersonate > >>> feature > >>> also lovely and quite useful. So both directions are okay for me. > >>> > >>> On Mon, Aug 20, 2018, 12:07 PM Gil Portenseigne < > >>> gil.portensei...@nereide.fr> > >>> wrote: > >>> > >>> Hello Taher, > >>>> > >>>> Thanks for your ideas, i think that had helped making it pop into > >>>> Nicolas answer to Pierre (that i just annoted). > >>>> > >>>> I hope the idea, that seem a mix of yours could be good enough, a > >>>> > >>> property > >>> > >>>> that : > >>>> * by default allow any impersonation to be done in non-preproduction > >>>> env, > >>>> without logging out the user. (less security requirement) > >>>> * else, impersonation will logout the user impersonated, and restrict > >>>> impersonation to one only for this user, during impersonation time. > >>>> > >>>> The audit could be done in UserLoginHistory entity, storing > >>>> impersonation period. > >>>> > >>>> Regards > >>>> > >>>> Gil > >>>> > >>>> Le mardi 14 août 2018 à 09:37:06 (+0300), Taher Alkhateeb a écrit : > >>>> > >>>>> One idea that comes to my mind which might be useful is that we add a > >>>>> flag in general.properties that by default enables this feature, and > >>>>> we can then specify in the documentation that to secure OFBiz we need > >>>>> to disable this feature so that it can be used in development but > >>>>> disabled in production for people who prefer to be on the safe side. I > >>>>> also like the suggestions from Pierre on perhaps having some kind of > >>>>> audit trail, just like we have a log of party visits, we can have a > >>>>> log of impersonation visits for example. > >>>>> > >>>>> Another idea, is perhaps to avoid completely persisting the session > >>>>> into the system. In other words, once I impersonate some user, it's > >>>>> done, I AM that user and I cannot go back. I have to log out and log > >>>>> back in to access the system again as an admin. That might be a bit > >>>>> more secure because we don't touch the session data. > >>>>> > >>>>> All food for thought, and I appreciate getting more feedback from the > >>>>> > >>>> community. > >>>> > >>>>> On Mon, Aug 13, 2018 at 11:09 AM Pierre Smits <pierresm...@apache.org> > >>>>> > >>>> wrote: > >>>> > >>>>> Impressive... > >>>>>> > >>>>>> This seems to be an in-OFBiz equivalent of an OS take-over tool like > >>>>>> Microsoft's Remote Desktop. The business case (and use cases) are > >>>>>> > >>>>> explained > >>>> > >>>>> insufficiently in this thread or in the ticket ([1]) why > >>>>>> > >>>>> incorporating > >>> > >>>> this > >>>> > >>>>> in the repo should be favourable over having the adopting business > >>>>>> implement implement the OS take-over tool. What I feel missing here > >>>>>> > >>>>> (and in > >>>> > >>>>> the ticket) is the reference to the previous thread, which might > >>>>>> > >>>>> explain > >>>> > >>>>> the business case. I suggest to have a link to this thread also in > >>>>>> > >>>>> the > >>> > >>>> ticket > >>>>>> > >>>>>> Based on a cursory review of the patch, it is lacking serious aspects > >>>>>> > >>>>> that > >>>> > >>>>> will boost the confidence of any business adopter that this feature > >>>>>> > >>>>> will > >>>> > >>>>> not jeopardise their business operations. As it is now, I find the > >>>>>> > >>>>> patch to > >>>> > >>>>> basic to be committed to the repo and be included in any new release. > >>>>>> > >>>>>> As I see it it allows anybody with the IMPERSONATE_ADMIN permission > >>>>>> > >>>>> take > >>>> > >>>>> over any other ID and perform actions under that ID at anytime. I did > >>>>>> > >>>>> not > >>>> > >>>>> see any functionality (I am spitballing here) that: > >>>>>> > >>>>>> 1. would exclude any particular ID from being taken over (as a > >>>>>> > >>>>> default > >>>> > >>>>> configuration) > >>>>>> 2. would allow a user to disable the feature for their own account > >>>>>> (overriding the default permission of impersonation) > >>>>>> 3. would allow a user to explicitly allow its ID to be taken over > >>>>>> > >>>>> by > >>> > >>>> someone else, AND limit it for a specific amount of time > >>>>>> > >>>>> (overriding aspect > >>>> > >>>>> #2 above). > >>>>>> 4. would prohibit the impersonator to take over an ID when the > >>>>>> > >>>>> user > >>> > >>>> of > >>>> > >>>>> the ID is NOT logged in (which should be an additional default > >>>>>> > >>>>> aspect). > >>>> > >>>>> This feature seems 'impersonator' driven as the permission would not > >>>>>> > >>>>> be on > >>>> > >>>>> a case-by-case scenario, but rather on a semi-permanent permission > >>>>>> assignment and by a user who has the - technical - permission to set > >>>>>> > >>>>> such > >>>> > >>>>> a permission. > >>>>>> > >>>>>> What I furthermore feel lacking or underdeveloped is the audit and > >>>>>> > >>>>> logging > >>>> > >>>>> trail regarding this feature. Nowhere can be seen what actions (for > >>>>>> > >>>>> the > >>> > >>>> authentic ID) have been undertaken by the impersonator while the > >>>>>> impersonation was in progress. Neither in logfiles, nor in screens in > >>>>>> > >>>>> the > >>>> > >>>>> Partymgr component (e.g. for the user to see). > >>>>>> > >>>>>> I advise the community to be very careful to commit this, without > >>>>>> consideration of the above, into the repo. > >>>>>> > >>>>>> > >>>>>> [1] https://issues.apache.org/jira/browse/OFBIZ-10515 > >>>>>> > >>>>>> > >>>>>> Best regards, > >>>>>> > >>>>>> Pierre Smits > >>>>>> > >>>>>> Apache Trafodion <https://trafodion.apache.org>, Vice President > >>>>>> Apache Directory <https://directory.apache.org>, PMC Member > >>>>>> Apache Incubator <https://incubator.apache.org>, committer > >>>>>> Apache OFBiz <https://ofbiz.apache.org>, contributor since 2008 > >>>>>> Apache Steve <https://steve.apache.org>, committer > >>>>>> > >>>>>> On Mon, Aug 13, 2018 at 6:19 AM, Zhang Wei <tzn...@msn.com> wrote: > >>>>>> > >>>>>> +1 > >>>>>>> ________________________________ > >>>>>>> 发件人: Rajesh Mallah <mallah.raj...@gmail.com> > >>>>>>> 发送时间: 2018年8月11日 11:10 > >>>>>>> 收件人: dev@ofbiz.apache.org > >>>>>>> 主题: Re: New Impersonate Feature : OFBIZ-10515 > >>>>>>> > >>>>>>> This feature has valid use cases. > >>>>>>> +1 > >>>>>>> > >>>>>>> On Sat, Aug 11, 2018 at 1:30 AM, Gil Portenseigne < > >>>>>>> gil.portensei...@nereide.fr> wrote: > >>>>>>> > >>>>>>> Hello ! > >>>>>>>> > >>>>>>>> I would like to introduce to you a new feature, i already talked > >>>>>>>> > >>>>>>> about > >>>> > >>>>> some > >>>>>>> > >>>>>>>> time ago (last year?). We needed it for one of our customer, that > >>>>>>>> > >>>>>>> is > >>>> > >>>>> using it for some time and is very happy with it (like we are). > >>>>>>>> > >>>>>>>> Indeed this impersonation feature comes to be very useful when we > >>>>>>>> > >>>>>>> need > >>>> > >>>>> to validate some behaviour or to assist a user in production > >>>>>>>> > >>>>>>> without > >>>> > >>>>> asking for its credential. It's became so easy to use that even > >>>>>>>> > >>>>>>> in > >>> > >>>> preproduction/integration environment we use it daily to > >>>>>>>> > >>>>>>> impersonate > >>>> > >>>>> specific configured userlogin without trying to remember the > >>>>>>>> > >>>>>>> password... > >>>> > >>>>> It's kinda basic, a new permission is created and can be granted > >>>>>>>> > >>>>>>> to an > >>>> > >>>>> authorized user, that will be offered a way to select a userlogin > >>>>>>>> > >>>>>>> to > >>>> > >>>>> impersonate. > >>>>>>>> > >>>>>>>> It's a common feature that can be found for example in Gitlab. > >>>>>>>> > >>>>>>>> If you wanna try it out it's available here : > >>>>>>>> https://issues.apache.org/jira/browse/OFBIZ-10515 > >>>>>>>> > >>>>>>>> Feedback are welcomed :), although i'll be partly offline next > >>>>>>>> > >>>>>>> week. > >>>> > >>>>> Looking forward reading you ! > >>>>>>>> > >>>>>>>> Gil > >>>>>>>> > >>>>>>>> > >
