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
LS