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 <lslebodn(a)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?