On (04/08/16 11:38), Jakub Hrozek wrote:
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".
Hmm,
attached patch is createn on top of current master
2a03170b6990c37ac2f7376ea740613c47ef2573
LS