4:38 a.m.
On Thu, Aug 04, 2016 at 11:26:41AM +0200, Jakub Hrozek wrote:
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?
Hmm, this still doesn't work:
jhrozek@hendrix devel/sssd (review %) » git reset --hard origin/master
HEAD is now at 2a03170 Fixed some typos in man pages
jhrozek@hendrix devel/sssd (review %) » git am
0001-SYSDB-Fix-setting-dataExpireTimestamp-if-sysdb-is-su.patch
Applying: SYSDB: Fix setting dataExpireTimestamp if sysdb is supposed to set the current
time
jhrozek@hendrix devel/sssd (review %) » git am
0001-SYSDB-Avoid-optimisation-with-modifyTimestamp-for-us.patch
Applying: SYSDB: Avoid optimisation with modifyTimestamp for users
error: patch failed: src/db/sysdb_ops.c:2465
error: src/db/sysdb_ops.c: patch does not apply
Patch failed at 0001 SYSDB: Avoid optimisation with modifyTimestamp for users
The copy of the patch that failed is found in:
/home/remote/jhrozek/devel/sssd/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".