On Thu, Aug 04, 2016 at 11:19:36AM +0200, Lukas Slebodnik wrote: > On (04/08/16 11:15), Jakub Hrozek wrote: > >On Thu, Aug 04, 2016 at 08:45:00AM +0200, Lukas Slebodnik wrote: > >> On (03/08/16 18:56), Lukas Slebodnik wrote: > >> >On (29/07/16 16:41), Jakub Hrozek wrote: > >> >>On Thu, Jul 28, 2016 at 01:56:50PM +0200, Jakub Hrozek wrote: > >> >>> On Thu, Jul 28, 2016 at 01:33:32PM +0200, Lukas Slebodnik wrote: > >> >>> > On (28/07/16 12:06), thierry bordaz wrote: > >> >>> > >On 07/28/2016 09:39 AM, Jakub Hrozek wrote: > >> >>> > >> On Wed, Jul 27, 2016 at 04:09:07PM +0200, thierry bordaz wrote: > >> >>> > >> > > >> >>> > >> > On 07/27/2016 03:36 PM, Jakub Hrozek wrote: > >> >>> > >> > > On Wed, Jul 27, 2016 at 02:55:37PM +0200, thierry bordaz > >> >>> > >> > > wrote: > >> >>> > >> > > > On 07/27/2016 01:56 PM, Jakub Hrozek wrote: > >> >>> > >> > > > > On Wed, Jul 27, 2016 at 01:03:59PM +0200, Jakub Hrozek > >> >>> > >> > > > > wrote: > >> >>> > >> > > > > > On Wed, Jul 27, 2016 at 12:22:46PM +0200, Lukas > >> >>> > >> > > > > > Slebodnik wrote: > >> >>> > >> > > > > > > On (27/07/16 12:08), Jakub Hrozek wrote: > >> >>> > >> > > > > > > > On Wed, Jul 27, 2016 at 12:02:24PM +0200, Jakub > >> >>> > >> > > > > > > > Hrozek wrote: > >> >>> > >> > > > > > > > > On Wed, Jul 27, 2016 at 11:54:16AM +0200, Lukas > >> >>> > >> > > > > > > > > Slebodnik wrote: > >> >>> > >> > > > > > > > > > ehlo, > >> >>> > >> > > > > > > > > > > >> >>> > >> > > > > > > > > > attached patch fixes acces denied after > >> >>> > >> > > > > > > > > > activating user in 389ds. > >> >>> > >> > > > > > > > > > Jakub had some comments/ideas in ticket but I > >> >>> > >> > > > > > > > > > think it's better to discuss > >> >>> > >> > > > > > > > > > about virtual attributes and timestamp cache on > >> >>> > >> > > > > > > > > > mailing list. > >> >>> > >> > > > > > > > > Yes, so the comment I have is that while this > >> >>> > >> > > > > > > > > works, it might break some > >> >>> > >> > > > > > > > > strange LDAP servers. > >> >>> > >> > > > > > > > > > >> >>> > >> > > > > > > > > We use modifyTimestamp as a 'positive' indicator > >> >>> > >> > > > > > > > > that the entry has not > >> >>> > >> > > > > > > > > changed -- if the modifyTimestamp didn't change, > >> >>> > >> > > > > > > > > we consider the cached > >> >>> > >> > > > > > > > > entry the same as what is on the server and only > >> >>> > >> > > > > > > > > bump the timestamp > >> >>> > >> > > > > > > > > cache. If the timestamp is different, we do a > >> >>> > >> > > > > > > > > deep-comparison of cached > >> >>> > >> > > > > > > > > attribute values with what is on the LDAP server > >> >>> > >> > > > > > > > > and write the sysdb > >> >>> > >> > > > > > > > > cache entry only if the attributes differ. > >> >>> > >> > > > > > > > > > >> >>> > >> > > > > > > > > I was wondering if we can use the modifyTimestamp > >> >>> > >> > > > > > > > > at all, then, because > >> >>> > >> > > > > > > > > even if it's the same, we might want to check the > >> >>> > >> > > > > > > > > attributes to see if > >> >>> > >> > > > > > > > > some of the values are different because some of > >> >>> > >> > > > > > > > > the attributes might be > >> >>> > >> > > > > > > > > this operational/virtual attribute.. > >> >>> > >> > > > > > > > Sorry, sent too soon. > >> >>> > >> > > > > > > > > >> >>> > >> > > > > > > > I think the questions are -- 1) can we enumerate > >> >>> > >> > > > > > > > the virtual attributes? > >> >>> > >> > > > > > > That might be a question for 389-ds developers. > >> >>> > >> > > > > > > But it's very likely it will be different on other > >> >>> > >> > > > > > > LDAP servers. > >> >>> > >> > > > > > > > >> >>> > >> > > > > > > > 2) Would different LDAP servers have different > >> >>> > >> > > > > > > > virtual attributes. > >> >>> > >> > > > > > > > > >> >>> > >> > > > > > > > For 2) maybe a possible solution might be to set a > >> >>> > >> > > > > > > > non-existing > >> >>> > >> > > > > > > > modifyTimestamp attribute value, but I would > >> >>> > >> > > > > > > > consider that only a > >> >>> > >> > > > > > > > kludge, we shouldn't break existing setups.. > >> >>> > >> > > > > > > I am not satisfied with this POC solution either. > >> >>> > >> > > > > > > > >> >>> > >> > > > > > > So should we remove usage of modifyTimestamp for > >> >>> > >> > > > > > > detecting changes? > >> >>> > >> > > > > > I would prefer to ask the DS developers before removing > >> >>> > >> > > > > > it completely. > >> >>> > >> > > > > > > >> >>> > >> > > > > > At least for large groups it might take a long time to > >> >>> > >> > > > > > compare all attribute > >> >>> > >> > > > > > values and IIRC we don't depend on any virtual > >> >>> > >> > > > > > attributes for groups. Maybe > >> >>> > >> > > > > > we could parametrize that part of the code and enable > >> >>> > >> > > > > > the fast way with > >> >>> > >> > > > > > modifyTimestamps for 'known' server types, that is for > >> >>> > >> > > > > > setups with AD and > >> >>> > >> > > > > > IPA providers. > >> >>> > >> > > > > > > >> >>> > >> > > > > > For users, there is typically not as many attributes so > >> >>> > >> > > > > > we should be > >> >>> > >> > > > > > fine deep-comparing all attributes. > >> >>> > >> > > > > I'm adding Thierry (so please reply-to-all to keep him in > >> >>> > >> > > > > the thread). > >> >>> > >> > > > > > >> >>> > >> > > > > Thierry, in the latest sssd version we tried to add a > >> >>> > >> > > > > performance > >> >>> > >> > > > > improvement related to how we store SSSD entries in the > >> >>> > >> > > > > cache. The short > >> >>> > >> > > > > version is that we store the modifyTimestamp attribute in > >> >>> > >> > > > > the cache and > >> >>> > >> > > > > when we fetch an entry, we compare the entry > >> >>> > >> > > > > modifyTimestamp with what > >> >>> > >> > > > > is on the server. When the two are the same, we say that > >> >>> > >> > > > > the entry did > >> >>> > >> > > > > not change and don't update the cache. > >> >>> > >> > > > > > >> >>> > >> > > > > This works fine for most attributes, but not for > >> >>> > >> > > > > attributes like > >> >>> > >> > > > > nsAccountLock which do not change modifyTimestamp when > >> >>> > >> > > > > they are > >> >>> > >> > > > > modified. So when an entry was already cached but then > >> >>> > >> > > > > nsAccountLock > >> >>> > >> > > > > changed, we treated the entry as the same and never read > >> >>> > >> > > > > the new > >> >>> > >> > > > > nsAccountLock value. > >> >>> > >> > > > > > >> >>> > >> > > > > To fix this, I think we have several options: > >> >>> > >> > > > > 1) special-case the nsAccountLock. This seems a > >> >>> > >> > > > > bit dangerous, > >> >>> > >> > > > > because I'm not sure we can say that some other > >> >>> > >> > > > > attribute we are > >> >>> > >> > > > > interested in behaves the same as nsAccountLock. > >> >>> > >> > > > > 2) drop the modifyTimestamp optimization > >> >>> > >> > > > > completely. Then we fall > >> >>> > >> > > > > back to comparing the attribute values, which > >> >>> > >> > > > > might work, but for > >> >>> > >> > > > > huge objects like groups with thousands of > >> >>> > >> > > > > members, this might be > >> >>> > >> > > > > too expensive. > >> >>> > >> > > > > 3) only use the modifyTimestamp optimization for > >> >>> > >> > > > > cases where we know > >> >>> > >> > > > > we don't read any virtual attributes. > >> >>> > >> > > > > > >> >>> > >> > > > > And my question is -- can we, in general, know if the > >> >>> > >> > > > > modifyTimestamp > >> >>> > >> > > > > way of detecting changes is realiable for all LDAP > >> >>> > >> > > > > servers? Or do you > >> >>> > >> > > > > think it should only be used for cases where we know we > >> >>> > >> > > > > are not > >> >>> > >> > > > > interested in any virtual attributes (that would mostly > >> >>> > >> > > > > be storing > >> >>> > >> > > > > groups from servers where we know exactly what is on the > >> >>> > >> > > > > server side, > >> >>> > >> > > > > like IPA or AD). > >> >>> > >> > > > Hello, > >> >>> > >> > > > > >> >>> > >> > > > Relying on modifytimestamp looks a good idea. Any > >> >>> > >> > > > MOD/MODRDN will update it, > >> >>> > >> > > > except I think it is unchanged when updating some password > >> >>> > >> > > > policy > >> >>> > >> > > > attributes. > >> >>> > >> > > > > >> >>> > >> > > > Regarding virtual attribute, the only one I know in IPA is > >> >>> > >> > > > nsaccountlock. > >> >>> > >> > > > nsaccountlock is an operational attribute (you need to > >> >>> > >> > > > request it to see it) > >> >>> > >> > > > and is also a virtual attribute BUT only for 'staged' and > >> >>> > >> > > > 'deleted' users. > >> >>> > >> > > > It is a stored attribute for regular users and we should > >> >>> > >> > > > update > >> >>> > >> > > > modifytimestamp when it is set. > >> >>> > >> > > > > >> >>> > >> > > > thanks > >> >>> > >> > > > thierry > >> >>> > >> > > OK, in that case it seems like we can special-case it. But do > >> >>> > >> > > you know > >> >>> > >> > > about any other attributes in any other LDAP servers? > >> >>> > >> > Any LDAP server following standard should provide > >> >>> > >> > modifytimestamp that > >> >>> > >> > reflect the last update of the entry. Now virtual attribute > >> >>> > >> > values may be > >> >>> > >> > "attached" to the entry and its value change without > >> >>> > >> > modification of > >> >>> > >> > modifytimestamp. > >> >>> > >> > For 389-ds and IPA it is fine as virtual value of nsaccountlock > >> >>> > >> > is changed > >> >>> > >> > only when the DN change. > >> >>> > >> > For others LDAP servers I suppose it exists the same ability to > >> >>> > >> > define > >> >>> > >> > service providers that return virtual attribute values. The > >> >>> > >> > difficulty is > >> >>> > >> > that the schema may not give any hint if the retrieved > >> >>> > >> > attributes values > >> >>> > >> > were stored or computed and consequently trust modifytimestamp > >> >>> > >> > to know if > >> >>> > >> > the values changed or not. > >> >>> > >> > For example in ODSEE, memberof is a virtual attribute. > >> >>> > >> Thank you, for the explanation Thierry. > >> >>> > >> > >> >>> > >> Then to be on the safe side I propose: > >> >>> > >> 1) We add an (probably undocumented?) flag that says whether > >> >>> > >> to use > >> >>> > >> modifyTimestamp to detect entry changes or not > >> >>> > >> 2) for the generic LDAP provider we always really compare the > >> >>> > >> attribute values, in other words the option would be set > >> >>> > >> to > >> >>> > >> false. If there is anyone with performance issues with a > >> >>> > >> generic > >> >>> > >> setup, we tell them to flip the option. > >> >>> > >> 3) For the IPA and AD providers, we set this option to true > >> >>> > >> and use > >> >>> > >> the modifyTimestamp value to detect changes > >> >>> > >> 4) We special case nsAccountLock > >> >>> > >> > >> >>> > >> Lukas, do you agree? > >> >>> > >Hi Jakub, > >> >>> > > > >> >>> > >digging further into the server, it appears that a DS plugin > >> >>> > >'acctpolicy' > >> >>> > >updates an entry without changing the mofidytimestamp. > >> >>> > >The updated attribute is 'lastlogintime' (by default) but i think > >> >>> > >can be any > >> >>> > >attribute configured in the entry account policy. > >> >>> > >I need to do further tests to confirm this. > >> >>> > > > >> >>> > > >> >>> > IMHO, the problems with nsAccountLock just revealed the fact that > >> >>> > it might happen that the attribute modifyTimestamp/whenChanged > >> >>> > needn't be > >> >>> > a reliable way how to determine change in entry for any LDAP Server. > >> >>> > > >> >>> > We improved the performance but there might be other corner cases > >> >>> > with > >> >>> > other LDAP servers. > >> >>> > > >> >>> > Jakub proosed in 2) that we should always really compare the the > >> >>> > attibutes > >> >>> > from sssd cache and from LDAP search. But it would mean that we > >> >>> > would not > >> >>> > improve a performance for generic LDAP providers. > >> >>> > >> >>> We would still avoid the cache writes, "only" after comparing the > >> >>> attribute values. So essentially by using the modifyTimestamp we save a > >> >>> single LDB base search and a number of attribute-value comparisons, > >> >>> depending on how large the object is. > >> >>> > >> >>> > > >> >>> > We cannot generally detect virtual attributes for any LDAP server. > >> >>> > What about adding an option where user could list virtual attributes. > >> >>> > It would be a kind of proposed solution 4) but it would not be a > >> >>> > special case for nsAccountLock but for more attributes which could > >> >>> > be changed > >> >>> > in configuration. > >> >>> > >> >>> OK, so your proposal is to keep the more aggressive cache optimization? > >> >>> I think I'm mostly afraid about the more exotic LDAP servers, like IBM > >> >>> Tivoli or Novell eDirectory which I already know from experience don't > >> >>> follow the established standards closely. I guess I'm fine with that, > >> >>> we > >> >>> can always flip the default back. Worst case for people who start > >> >>> using > >> >>> 1.14, the admin can always define modifyTimestamp to some non-existing > >> >>> attribute and force the attribute value comparison check. > >> >>> > >> >>> Yes, we can make the list configurable. We also need to be > >> >>> very careful about printing the reason for (not) updating the cache to > >> >>> the admin (#3060). > >> >> > >> >>OK, so after some discussion with Simo on IRC, there is a different > >> >>proposal - let responder control the optimization level, so that PAM > >> >>responder would always trigger a full cache write, or at least > >> >>deep-compare the attributes and NSS responder would rely on > >> >>modifyTimestamp. > >> >> > >> >>But that's a larger fix, so for short-term fix I propose to only use > >> >>modifyTimestamp for group objects and always compare attributes for > >> >>users. Then later, as another patch we can let the responder send a flag > >> >>to control the optimization (would probably be done as a flag for a > >> >>sysdb transaction). > >> >> > >> >>If you agree, I would file a ticket for the second part and you can > >> >>instead write a patch to disable modifyTimestamp checks for users. > >> > > >> >As you wish. > >> >The updated patch is attached. > >> > > >> >LS > >> > >> >From ae01ffdbbc74c5b43c2b644f8847d856cd2bf997 Mon Sep 17 00:00:00 2001 > >> >From: Lukas Slebodnik <lsleb...@redhat.com> > >> >Date: Wed, 3 Aug 2016 18:48:04 +0200 > >> >Subject: [PATCH] SYSDB: Avoid optimisation with modifyTimestamp for users > >> > > >> >The usage of modifyTimestamp needn't be a reliable way > >> >for detecting of changes in user entry in LDAP. > >> >The authorisation need to rely current data from LDAP > >> >and therefore we will temporary disable optimisation with > >> >modifyTimestamp and we will rather rely on deep comparison > >> >of attributes. In he future, it might be changed and > >> >responders might control the optimization level. > >> > > >> And now with version witout failures in unit test and without compiler > >> warnings > >> :-) > >> > >> LS > > > >Hi, > > > >this patch doesn't apply atop origin/master, do I need some patches > >before this one? > > I looks like I created patch on top of your patch > [SSSD] [PATCH] SYSDB: Fix setting dataExpireTimestamp if sysdb is supposed to > set the current time > > If you want I can create on origin/master but you would need to rebase your > patch. So it depends on wich patch will be pushed the first
I don't mind waiting and testing the patches together. I take it you'll review my patch, then? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
