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