[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin

[Nathaniel McCallum]

On Fri, 2014-10-17 at 12:05 +0200, Martin Kosek wrote:
> On 10/16/2014 11:53 PM, Nathaniel McCallum wrote:
> > On Thu, 2014-10-16 at 21:02 +0200, Martin Kosek wrote:
> >> On 10/15/2014 09:22 AM, Martin Kosek wrote:
> >>> On 10/14/2014 09:01 PM, Nathaniel McCallum wrote:
> >>>> On Thu, 2014-10-09 at 18:48 +0200, thierry bordaz wrote:
> >>>>> On 10/09/2014 05:51 PM, Nathaniel McCallum wrote:
> >>>>>
> >>>>>> On Thu, 2014-10-09 at 11:44 +0200, thierry bordaz wrote:
> >>>>>>> On 10/09/2014 12:15 AM, Nathaniel McCallum wrote:
> >>>>>>>
> >>>>>>>> On Wed, 2014-10-08 at 17:19 -0400, Simo Sorce wrote:
> >>>>>>>>> On Wed, 08 Oct 2014 15:53:39 -0400
> >>>>>>>>> Nathaniel McCallum <npmccallum at redhat.com> wrote:
> >>>>>>>>>
> >>>>>>>>>> As I understand my code, all servers will have csnD. Some servers will
> >>>>>>>>>> have valueB and others will have valueD, but valueB == valueD.
> >>>>>>>>>>
> >>>>>>>>>> We *never* discard a CSN. We only discard the counter/watermark mods
> >>>>>>>>>> in the replication operation.
> >>>>>>>>> What Thierry is saying is that the individual attributes in the entry
> >>>>>>>>> have associate the last CSN that modified them. Because you remove the
> >>>>>>>>> mods when ValueD == ValueB the counter attribute will not have the
> >>>>>>>>> associated CSN changed. But it doesn't really matter because the plugin
> >>>>>>>>> will always keep things consistent.
> >>>>>>>> Attached is a new version. It removes this optimization. If server X has
> >>>>>>>> valueB/csnB and receives valueD/csnD and valueB == valueD, the
> >>>>>>>> replication will be applied without any modification. However, if valueB
> >>>>>>>>> valueD and csnD > csnB, the counter mods will still be stripped.
> >>>>>>>> It also collapses the error check from betxnpre to bepre now that we
> >>>>>>>> have a fix for https://fedorahosted.org/389/ticket/47919 committed. The
> >>>>>>>> betxnpre functions are completely removed. Also, a dependency on 389
> >>>>>>>> 1.3.3.4 (not yet released) is added.
> >>>>>>>>
> >>>>>>>> Nathaniel
> >>>>>>> Hello Nathaniel,
> >>>>>>>
> >>>>>>>          For me the code is fine. Ack.
> >>>>>> New attached patch.
> >>>>>>
> >>>>>>>          I have two minor comments:
> >>>>>>>                * in preop_mod, when a direct update moves the counter
> >>>>>>>                  backward you send UNWILLING to perform with a message.
> >>>>>>>                  The message is allocated with slapi_ch_smprintf, you
> >>>>>>>                  may free it with slapi_ch_free_string (rather than
> >>>>>>>                  'free').
> >>>>>> Fixed.
> >>>>>>
> >>>>>>>                * About this message, for example when you have these
> >>>>>>>                  MODS (I admit they make no sens):
> >>>>>>>
> >>>>>>>                  changetype: modify
> >>>>>>>                  ipatokenHOTPcounter: MOD_DELETE
> >>>>>>>                  -
> >>>>>>>                  ipatokenHOTPcounter: MOD_INCREMENT
> >>>>>>>
> >>>>>>>                  The returned message will be "Will not decrement
> >>>>>>>                  ipatokenHOTPcounter", because 'simulate' will return
> >>>>>>>                  'COUNTER_UNSET+1'.
> >>>>>>>                  Is it the message you expected ?
> >>>>>> I changed the logic in simulate(). Please review it.
> >>>>>>
> >>>>>> Nathaniel
> >>>>>>
> >>>>> Hello Nathaniel,
> >>>>>
> >>>>>
> >>>>>          The patch is ok for me. Ack.
> >>>>
> >>>> Since the ACK, the upstream 389 fix actually landed in 1.3.3.5. This
> >>>> patch changes nothing except the dependency version. I have tested it
> >>>> against the 1.3.3.5 build.
> >>>>
> >>>> Nathaniel
> >>>
> >>> Great! As soon as the new build land in Fedora 21 (and we add it to our Copr),
> >>> the patch can be pushed.
> >>>
> >>> Martin
> >>
> >> Ok, 1.3.3.5 is in koji and our Copr repo.
> > 
> > +1
> > 
> >> I almost pushed the patch, but I just 
> >> spotted you forgot to solve the upgrades - so NACK.
> > 
> > You asked me to do that in another patch, not this one. :)
> 
> I know, but that does not change the fact it is still missing :-)
> 
> > 
> >> "cn=IPA OTP Counter,cn=plugins,cn=config" plugin configuration also needs to be 
> >> added in some update file.
> > 
> > So I'm generally confused then. If we have to add the plugin config to
> > an update file, why bother creating
> > daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif or
> > modifying ipaserver/install/dsinstance.py at all? Should these changes
> > be removed?
> 
> Good question. Most LDAP update related changes are added to update plugins
> only. But plugin registration seemed to be still on 2 places, like 'cn=IPA
> Range-Check,cn=plugins,cn=config' registration.
> 
> Up to you/additional developer discussion.

Fixed.

Also note that the same problem was present in the OTP Last Token plugin
(committed for 4.0). I have made a patch which fixes this as well:

https://www.redhat.com/archives/freeipa-devel/2014-October/msg00358.html

Nathaniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0064.9-Create-ipa-otp-counter-389DS-plugin.patch
Type: text/x-patch
Size: 33289 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141017/7540af55/attachment.bin>