On (04/08/16 12:59), Lukas Slebodnik wrote:
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
and does not apply anymore on current master
Updated patch is attached.
LS