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 <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?
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?