Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/17/2014 07:22 PM, Nathaniel McCallum wrote: 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 npmccal...@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 Thanks, upgrade part works for me, ACK for that. Pushed to: master: 41bf0ba9403962140a75b28e0b279248ec101a0a ipa-4-1: 2f8dc3b6cc6cdee8230afe50cce694a762010b37 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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 From 7220238f3489dfb84e537dd2debda53ab7ffa14d Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-counter 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 +
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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. :) 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? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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 From e8bd6a7fd024d4150b90114ac91548ac7acfcedd Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-counter 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-slapi-plugins/ipa-otp-counter/Makefile.am | 25 ++ daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c | 96 + daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h | 66 +++ .../ipa-otp-counter/ipa-otp-counter.sym| 1 + .../ipa-otp-counter/ipa_otp_counter.c | 454 + .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.c| 110 + .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.h| 54 +++ .../ipa-otp-counter/otp-counter-conf.ldif | 15 + freeipa.spec.in| 8 +- ipaserver/install/dsinstance.py| 4 + 12 files changed, 832 insertions(+), 3 deletions(-) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa-otp-counter.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa_otp_counter.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/09/2014 06:48 PM, 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 McCallumnpmccal...@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 forhttps://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. Thank you thierry Great! Thanks for tough review. However, we will still need to wait for 389 to release so that we can add the new required DS version at least to FreeIPA Copr. Otherwise all development/CI would break. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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. 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'). * 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 ? thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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 From 2356ad22f009f8a99b1ce8801ee1506ce9433a1d Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-counter 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-slapi-plugins/ipa-otp-counter/Makefile.am | 25 ++ daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c | 96 + daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h | 66 +++ .../ipa-otp-counter/ipa-otp-counter.sym| 1 + .../ipa-otp-counter/ipa_otp_counter.c | 454 + .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.c| 110 + .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.h| 54 +++ .../ipa-otp-counter/otp-counter-conf.ldif | 15 + freeipa.spec.in| 8 +- ipaserver/install/dsinstance.py| 4 + 12 files changed, 832 insertions(+), 3 deletions(-) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa-otp-counter.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa_otp_counter.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index b4507a6d972f854331925e72869898576bdfd76f..bfcdeadcd1dc73762d8c773ee50210d9bdb91e92 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -314,6 +314,7 @@ AC_CONFIG_FILES([ ipa-slapi-plugins/ipa-dns/Makefile ipa-slapi-plugins/ipa-enrollment/Makefile ipa-slapi-plugins/ipa-lockout/Makefile +ipa-slapi-plugins/ipa-otp-counter/Makefile ipa-slapi-plugins/ipa-otp-lasttoken/Makefile ipa-slapi-plugins/ipa-pwd-extop/Makefile ipa-slapi-plugins/ipa-extdom-extop/Makefile diff --git
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
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 npmccal...@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. Thank you thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/07/2014 08:48 PM, Nathaniel McCallum wrote: On Tue, 2014-10-07 at 10:52 -0700, Noriko Hosoi wrote: On 2014/10/07 10:48, Nathaniel McCallum wrote: On Tue, 2014-10-07 at 18:54 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Hi Nathaniel, Just a rapid question about that dependency on https://fedorahosted.org/389/ticket/47919. Using txnpreop_mod you manage to workaround the DS issue. Do you need a fix for the DS issue in 1.3.2 or can it be fixed in a later version ? I would strongly prefer a fix ASAP. Thanks, Nathaniel, Do you need the fix just in 389-ds-base-1.3.3.x on F21 and newer? Or any other versions, e.g., 1.3.2 on F20, 1.3.3.1-x on RHEL-7.1, etc... ? I think we are just shipping 4.1 on F21. Someone please correct me if I'm wrong. FreeIPA 4.x already requires DS 1.3.3.*, so fixing from this version is sufficient for us. We have a Copr repo for Fedora 20 anyway, so we will just rebuild the updated DS there. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/07/2014 06:00 PM, Nathaniel McCallum wrote:
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
For me the code is good. It took me some time to understand the benefit
of removing mods in preop.
In fact I think it is a good idea, as it prevents extra repair ops and
also make more easy the computation of the value to set in repair mod.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
I think we should rather let the mods going on. So that the full
topology will have
valueD (or valueB)/csnD rather having a set of servers having
valueD/csnB and an other set valueD/csnD.
thanks
thierry
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 2014-10-08 at 17:30 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Third, I believe we are now handling replication correct. An error is never returned. When a replication would cause the counter to decrease, we remove all counter/watermark related mods from the operation. This will allow the replication to apply without decrementing the value. There is also a new bepost method which check to see if the replication was discarded (via CSN) while having a higher counter value. If so, we apply the higher counter value. For me the code is good. It took me some time to understand the benefit of removing mods in preop. In fact I think it is a good idea, as it prevents extra repair ops and also make more easy the computation of the value to set in repair mod. Here is the scenario. Server X receives two quick authentications; replications A and B are sent to server Y. Before server Y can process server X's replications, an authentication is performed on server Y; replication C is sent to server X. The following logic holds true: * csnA csnB csnC * valueA = valueC, valueB valueC When server X receives replication C, ipa-otp-counter will strip out all counter mod operations; applying the update but not the lower value. The value of replication B is retained. This is the correct behavior. When server Y processes replications A and B, ipa-otp-counter will detect that a higher value has a lower CSN and will manually set the higher value (in bepost). This initiates replication D, which is sent to server X. Here is the logic: * csnA csnB csnC csnD * valueA = valueC, valueB = valueD, valueD valueC Server X receives replication D. D has the highest CSN. It has the same value as replication B (server X's current value). Because the values are the same, ipa-otp-counter will strip all counter mod operations. This reduces counter write contention which might become a problem in N-way replication when N2. I think we should rather let the mods going on. So that the full topology will have valueD (or valueB)/csnD rather having a set of servers having valueD/csnB and an other set valueD/csnD. I think you misunderstand. The value for csnD is only discarded when the server already has valueB (valueB == valueD). Only the value is discarded, so csnD is still applied. The full topology will have either valueB w/ csnD or valueD w/ csnD. Since, valueB always equals valueD, by substitution, all servers have valueD w/ csnD. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/08/2014 07:30 PM, Nathaniel McCallum wrote: On Wed, 2014-10-08 at 17:30 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Third, I believe we are now handling replication correct. An error is never returned. When a replication would cause the counter to decrease, we remove all counter/watermark related mods from the operation. This will allow the replication to apply without decrementing the value. There is also a new bepost method which check to see if the replication was discarded (via CSN) while having a higher counter value. If so, we apply the higher counter value. For me the code is good. It took me some time to understand the benefit of removing mods in preop. In fact I think it is a good idea, as it prevents extra repair ops and also make more easy the computation of the value to set in repair mod. Here is the scenario. Server X receives two quick authentications; replications A and B are sent to server Y. Before server Y can process server X's replications, an authentication is performed on server Y; replication C is sent to server X. The following logic holds true: * csnA csnB csnC * valueA = valueC, valueB valueC When server X receives replication C, ipa-otp-counter will strip out all counter mod operations; applying the update but not the lower value. The value of replication B is retained. This is the correct behavior. When server Y processes replications A and B, ipa-otp-counter will detect that a higher value has a lower CSN and will manually set the higher value (in bepost). This initiates replication D, which is sent to server X. Here is the logic: * csnA csnB csnC csnD * valueA = valueC, valueB = valueD, valueD valueC Server X receives replication D. D has the highest CSN. It has the same value as replication B (server X's current value). Because the values are the same, ipa-otp-counter will strip all counter mod operations. This reduces counter write contention which might become a problem in N-way replication when N2. I think we should rather let the mods going on. So that the full topology will have valueD (or valueB)/csnD rather having a set of servers having valueD/csnB and an other set valueD/csnD. I think you misunderstand. The value for csnD is only discarded when the server already has valueB (valueB == valueD). Only the value is discarded, so csnD is still applied. The full topology will have either valueB w/ csnD or valueD w/ csnD. Since, valueB always equals valueD, by substitution, all servers have valueD w/ csnD. Nathaniel There are several parts where the CSN are stored. One is used to allow replication protocol to send the approriate updates. This part is stored into a dedicated entry: RUV. In fact when the update valudD/CSND will be received and applied, the RUV will be updated with csnD. An other part is the attribute/attribute values. An attribute value contains the actual value and the CSN associated to that value. This CSN is updated by entry_apply_mod_wsi when it decides which value to keep and which CSN is associated to this value. In the example above, on the server X, the counter attribute has valueB/csnB. Then it receives the update ValueD/csnD it discard this update because valueD=ValueB. That means that on serverX we will have valueB/csnB. Now if on an other server we receive the updates in the reverse order: valueD/csnD first then valueB/csnB. This server will apply and valueD/csnD then will discard valueB/csnB. ValueD and ValueB being identical it is not a big issue. But we will have some server with csnD and others with csnB. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/08/2014 01:45 PM, thierry bordaz wrote: On 10/08/2014 07:30 PM, Nathaniel McCallum wrote: On Wed, 2014-10-08 at 17:30 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Third, I believe we are now handling replication correct. An error is never returned. When a replication would cause the counter to decrease, we remove all counter/watermark related mods from the operation. This will allow the replication to apply without decrementing the value. There is also a new bepost method which check to see if the replication was discarded (via CSN) while having a higher counter value. If so, we apply the higher counter value. For me the code is good. It took me some time to understand the benefit of removing mods in preop. In fact I think it is a good idea, as it prevents extra repair ops and also make more easy the computation of the value to set in repair mod. Here is the scenario. Server X receives two quick authentications; replications A and B are sent to server Y. Before server Y can process server X's replications, an authentication is performed on server Y; replication C is sent to server X. The following logic holds true: * csnA csnB csnC * valueA = valueC, valueB valueC When server X receives replication C, ipa-otp-counter will strip out all counter mod operations; applying the update but not the lower value. The value of replication B is retained. This is the correct behavior. When server Y processes replications A and B, ipa-otp-counter will detect that a higher value has a lower CSN and will manually set the higher value (in bepost). This initiates replication D, which is sent to server X. Here is the logic: * csnA csnB csnC csnD * valueA = valueC, valueB = valueD, valueD valueC Server X receives replication D. D has the highest CSN. It has the same value as replication B (server X's current value). Because the values are the same, ipa-otp-counter will strip all counter mod operations. This reduces counter write contention which might become a problem in N-way replication when N2. I think we should rather let the mods going on. So that the full topology will have valueD (or valueB)/csnD rather having a set of servers having valueD/csnB and an other set valueD/csnD. I think you misunderstand. The value for csnD is only discarded when the server already has valueB (valueB == valueD). Only the value is discarded, so csnD is still applied. The full topology will have either valueB w/ csnD or valueD w/ csnD. Since, valueB always equals valueD, by substitution, all servers have valueD w/ csnD. Nathaniel There are several parts where the CSN are stored. One is used to allow replication protocol to send the approriate updates. This part is stored into a dedicated entry: RUV. In fact when the update valudD/CSND will be received and applied, the RUV will be updated with csnD. An other part is the attribute/attribute values. An attribute value contains the actual value and the CSN associated to that value. This CSN is updated by entry_apply_mod_wsi when it decides which value to keep and which CSN is associated to this value. In the example above, on the server X, the counter attribute has valueB/csnB. Then it receives the update ValueD/csnD it discard this update because valueD=ValueB. That means that on serverX we will have valueB/csnB. Now if on an other server we receive the updates in the reverse order: valueD/csnD first then valueB/csnB. This server will apply and valueD/csnD then will discard valueB/csnB. ValueD and ValueB being identical it is not a big issue. But we will have some server with csnD and others with csnB. The CSN is also the key in the changelog database. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 2014-10-08 at 21:45 +0200, thierry bordaz wrote: On 10/08/2014 07:30 PM, Nathaniel McCallum wrote: On Wed, 2014-10-08 at 17:30 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Third, I believe we are now handling replication correct. An error is never returned. When a replication would cause the counter to decrease, we remove all counter/watermark related mods from the operation. This will allow the replication to apply without decrementing the value. There is also a new bepost method which check to see if the replication was discarded (via CSN) while having a higher counter value. If so, we apply the higher counter value. For me the code is good. It took me some time to understand the benefit of removing mods in preop. In fact I think it is a good idea, as it prevents extra repair ops and also make more easy the computation of the value to set in repair mod. Here is the scenario. Server X receives two quick authentications; replications A and B are sent to server Y. Before server Y can process server X's replications, an authentication is performed on server Y; replication C is sent to server X. The following logic holds true: * csnA csnB csnC * valueA = valueC, valueB valueC When server X receives replication C, ipa-otp-counter will strip out all counter mod operations; applying the update but not the lower value. The value of replication B is retained. This is the correct behavior. When server Y processes replications A and B, ipa-otp-counter will detect that a higher value has a lower CSN and will manually set the higher value (in bepost). This initiates replication D, which is sent to server X. Here is the logic: * csnA csnB csnC csnD * valueA = valueC, valueB = valueD, valueD valueC Server X receives replication D. D has the highest CSN. It has the same value as replication B (server X's current value). Because the values are the same, ipa-otp-counter will strip all counter mod operations. This reduces counter write contention which might become a problem in N-way replication when N2. I think we should rather let the mods going on. So that the full topology will have valueD (or valueB)/csnD rather having a set of servers having valueD/csnB and an other set valueD/csnD. I think you misunderstand. The value for csnD is only discarded when the server already has valueB (valueB == valueD). Only the value is discarded, so csnD is still applied. The full topology will have either valueB w/ csnD or valueD w/ csnD. Since, valueB always equals valueD, by substitution, all servers have valueD w/ csnD. Nathaniel There are several parts where the CSN are stored. One is used to allow replication protocol to send the approriate updates. This part is stored into a dedicated entry: RUV. In fact when the update valudD/CSND will be received and applied, the RUV will be updated with csnD. An other part is the attribute/attribute values. An attribute value contains the actual value and the CSN associated to that value. This CSN is updated by entry_apply_mod_wsi when it decides which value to keep and which CSN is associated to this value. In the example above, on the server X, the counter attribute has valueB/csnB. Then it receives the update ValueD/csnD it discard this update because valueD=ValueB. That means that on serverX we will have valueB/csnB. It does not discard the update (CSN). It discards the value because valueD == valueB. So csnD will be applied, it just won't touch the counter values; valueB will be retained. Now if on an other server we receive the updates in the reverse order: valueD/csnD first then valueB/csnB. This server will apply and valueD/csnD then will discard valueB/csnB. This server will apply valueD/csnD AND csnB, but not valueB. This is because valueB == valueD. ValueD and ValueB being identical it is not a big issue. But we will have some server with csnD and others with csnB. 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. Nathaniel ___ Freeipa-devel mailing list
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 2014-10-08 at 13:53 -0600, Rich Megginson wrote: On 10/08/2014 01:45 PM, thierry bordaz wrote: On 10/08/2014 07:30 PM, Nathaniel McCallum wrote: On Wed, 2014-10-08 at 17:30 +0200, thierry bordaz wrote: On 10/07/2014 06:00 PM, Nathaniel McCallum wrote: Attached is the latest patch. I believe this includes all of our discussions up until this point. However, a few bits of additional information are needed. First, I have renamed the plugin to ipa-otp-counter. I believe all replay prevention work can land inside this plugin, so the name is appropriate. Second, I uncovered a bug in 389 which prevents me from validating the non-replication request in bepre. This is the reason for the additional betxnpre callback. If the upstream 389 bug is fixed, we can merge this check back into bepre. https://fedorahosted.org/389/ticket/47919 Third, I believe we are now handling replication correct. An error is never returned. When a replication would cause the counter to decrease, we remove all counter/watermark related mods from the operation. This will allow the replication to apply without decrementing the value. There is also a new bepost method which check to see if the replication was discarded (via CSN) while having a higher counter value. If so, we apply the higher counter value. For me the code is good. It took me some time to understand the benefit of removing mods in preop. In fact I think it is a good idea, as it prevents extra repair ops and also make more easy the computation of the value to set in repair mod. Here is the scenario. Server X receives two quick authentications; replications A and B are sent to server Y. Before server Y can process server X's replications, an authentication is performed on server Y; replication C is sent to server X. The following logic holds true: * csnA csnB csnC * valueA = valueC, valueB valueC When server X receives replication C, ipa-otp-counter will strip out all counter mod operations; applying the update but not the lower value. The value of replication B is retained. This is the correct behavior. When server Y processes replications A and B, ipa-otp-counter will detect that a higher value has a lower CSN and will manually set the higher value (in bepost). This initiates replication D, which is sent to server X. Here is the logic: * csnA csnB csnC csnD * valueA = valueC, valueB = valueD, valueD valueC Server X receives replication D. D has the highest CSN. It has the same value as replication B (server X's current value). Because the values are the same, ipa-otp-counter will strip all counter mod operations. This reduces counter write contention which might become a problem in N-way replication when N2. I think we should rather let the mods going on. So that the full topology will have valueD (or valueB)/csnD rather having a set of servers having valueD/csnB and an other set valueD/csnD. I think you misunderstand. The value for csnD is only discarded when the server already has valueB (valueB == valueD). Only the value is discarded, so csnD is still applied. The full topology will have either valueB w/ csnD or valueD w/ csnD. Since, valueB always equals valueD, by substitution, all servers have valueD w/ csnD. Nathaniel There are several parts where the CSN are stored. One is used to allow replication protocol to send the approriate updates. This part is stored into a dedicated entry: RUV. In fact when the update valudD/CSND will be received and applied, the RUV will be updated with csnD. An other part is the attribute/attribute values. An attribute value contains the actual value and the CSN associated to that value. This CSN is updated by entry_apply_mod_wsi when it decides which value to keep and which CSN is associated to this value. In the example above, on the server X, the counter attribute has valueB/csnB. Then it receives the update ValueD/csnD it discard this update because valueD=ValueB. That means that on serverX we will have valueB/csnB. Now if on an other server we receive the updates in the reverse order: valueD/csnD first then valueB/csnB. This server will apply and valueD/csnD then will discard valueB/csnB. ValueD and ValueB being identical it is not a big issue. But we will have some server with csnD and others with csnB. The CSN is also the key in the changelog database. Right. We *never* discard a replication operation. We only discard some of its mods if and only if those mods would result in either no change at all or an illegal change. If an illegal change would occur, we also issue a new fixup replication request so that everyone quickly gets consistency. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 08 Oct 2014 15:53:39 -0400 Nathaniel McCallum npmccal...@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. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 2014-10-08 at 17:19 -0400, Simo Sorce wrote: On Wed, 08 Oct 2014 15:53:39 -0400 Nathaniel McCallum npmccal...@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. Oh, I thought this was only being tracked on a per-entry basis. If it really matters, I can undo this optimization (it is a single character change). It will just be some extra writes. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Wed, 2014-10-08 at 17:19 -0400, Simo Sorce wrote: On Wed, 08 Oct 2014 15:53:39 -0400 Nathaniel McCallum npmccal...@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 From 368eb782ec7e4d4c245f4cee5bb819eac4ef2a30 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-counter 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-slapi-plugins/ipa-otp-counter/Makefile.am | 25 ++ daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c | 96 + daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h | 66 .../ipa-otp-counter/ipa-otp-counter.sym| 1 + .../ipa-otp-counter/ipa_otp_counter.c | 436 + .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.c| 110 ++ .../ipa-slapi-plugins/ipa-otp-counter/ldapmod.h| 54 +++ .../ipa-otp-counter/otp-counter-conf.ldif | 15 + freeipa.spec.in| 8 +- ipaserver/install/dsinstance.py| 4 + 12 files changed, 814 insertions(+), 3 deletions(-) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/berval.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa-otp-counter.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ipa_otp_counter.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/ldapmod.h create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-counter/otp-counter-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index b4507a6d972f854331925e72869898576bdfd76f..bfcdeadcd1dc73762d8c773ee50210d9bdb91e92 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -314,6 +314,7 @@ AC_CONFIG_FILES([ ipa-slapi-plugins/ipa-dns/Makefile ipa-slapi-plugins/ipa-enrollment/Makefile ipa-slapi-plugins/ipa-lockout/Makefile +ipa-slapi-plugins/ipa-otp-counter/Makefile ipa-slapi-plugins/ipa-otp-lasttoken/Makefile ipa-slapi-plugins/ipa-pwd-extop/Makefile ipa-slapi-plugins/ipa-extdom-extop/Makefile diff --git a/daemons/ipa-slapi-plugins/Makefile.am b/daemons/ipa-slapi-plugins/Makefile.am index 06e6ee8b86f138cce05f2184ac98c39ffaf9757f..07733921e43ac2eb9e248b276351d915a854bf7e 100644 --- a/daemons/ipa-slapi-plugins/Makefile.am +++ b/daemons/ipa-slapi-plugins/Makefile.am @@ -7,6 +7,7 @@ SUBDIRS = \ ipa-enrollment \ ipa-lockout \ ipa-modrdn \ + ipa-otp-counter \ ipa-otp-lasttoken \ ipa-pwd-extop \ ipa-extdom-extop \ diff --git a/daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am b/daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am new file mode 100644 index ..6b18467613e9bd301ce7432b7052f0fb15aae886 --- /dev/null +++ b/daemons/ipa-slapi-plugins/ipa-otp-counter/Makefile.am @@ -0,0 +1,25 @@ +MAINTAINERCLEANFILES = *~ Makefile.in +PLUGIN_COMMON_DIR = ../common +AM_CPPFLAGS = \ + -I. \ + -I$(srcdir) \ + -I$(PLUGIN_COMMON_DIR) \ + -I/usr/include/dirsrv \ + -DPREFIX=\$(prefix)\ \ + -DBINDIR=\$(bindir)\\ + -DLIBDIR=\$(libdir)\ \ + -DLIBEXECDIR=\$(libexecdir)\ \ + -DDATADIR=\$(datadir)\\ + $(AM_CFLAGS) \ + $(LDAP_CFLAGS) \ + $(WARN_CFLAGS) + +plugindir =
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 10/07/2014 06:00 PM, Nathaniel McCallum wrote:
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Hi Nathaniel,
Just a rapid question about that dependency on
https://fedorahosted.org/389/ticket/47919.
Using txnpreop_mod you manage to workaround the DS issue.
Do you need a fix for the DS issue in 1.3.2 or can it be fixed in a
later version ?
thanks
thierry
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE,
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Tue, 2014-10-07 at 18:54 +0200, thierry bordaz wrote:
On 10/07/2014 06:00 PM, Nathaniel McCallum wrote:
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Hi Nathaniel,
Just a rapid question about that dependency on
https://fedorahosted.org/389/ticket/47919.
Using txnpreop_mod you manage to workaround the DS issue.
Do you need a fix for the DS issue in 1.3.2 or can it be fixed
in a later version ?
I would strongly prefer a fix ASAP.
thanks
thierry
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to
know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 2014/10/07 10:48, Nathaniel McCallum wrote:
On Tue, 2014-10-07 at 18:54 +0200, thierry bordaz wrote:
On 10/07/2014 06:00 PM, Nathaniel McCallum wrote:
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Hi Nathaniel,
Just a rapid question about that dependency on
https://fedorahosted.org/389/ticket/47919.
Using txnpreop_mod you manage to workaround the DS issue.
Do you need a fix for the DS issue in 1.3.2 or can it be fixed
in a later version ?
I would strongly prefer a fix ASAP.
Thanks, Nathaniel,
Do you need the fix just in 389-ds-base-1.3.3.x on F21 and newer? Or any
other versions, e.g., 1.3.2 on F20, 1.3.3.1-x on RHEL-7.1, etc... ?
--noriko
thanks
thierry
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Tue, 2014-10-07 at 10:52 -0700, Noriko Hosoi wrote:
On 2014/10/07 10:48, Nathaniel McCallum wrote:
On Tue, 2014-10-07 at 18:54 +0200, thierry bordaz wrote:
On 10/07/2014 06:00 PM, Nathaniel McCallum wrote:
Attached is the latest patch. I believe this includes all of our
discussions up until this point. However, a few bits of additional
information are needed.
First, I have renamed the plugin to ipa-otp-counter. I believe all
replay prevention work can land inside this plugin, so the name is
appropriate.
Second, I uncovered a bug in 389 which prevents me from validating the
non-replication request in bepre. This is the reason for the additional
betxnpre callback. If the upstream 389 bug is fixed, we can merge this
check back into bepre. https://fedorahosted.org/389/ticket/47919
Hi Nathaniel,
Just a rapid question about that dependency on
https://fedorahosted.org/389/ticket/47919.
Using txnpreop_mod you manage to workaround the DS issue.
Do you need a fix for the DS issue in 1.3.2 or can it be fixed
in a later version ?
I would strongly prefer a fix ASAP.
Thanks, Nathaniel,
Do you need the fix just in 389-ds-base-1.3.3.x on F21 and newer? Or any
other versions, e.g., 1.3.2 on F20, 1.3.3.1-x on RHEL-7.1, etc... ?
I think we are just shipping 4.1 on F21. Someone please correct me if
I'm wrong.
--noriko
thanks
thierry
Third, I believe we are now handling replication correct. An error is
never returned. When a replication would cause the counter to decrease,
we remove all counter/watermark related mods from the operation. This
will allow the replication to apply without decrementing the value.
There is also a new bepost method which check to see if the replication
was discarded (via CSN) while having a higher counter value. If so, we
apply the higher counter value.
Here is the scenario. Server X receives two quick authentications;
replications A and B are sent to server Y. Before server Y can process
server X's replications, an authentication is performed on server Y;
replication C is sent to server X. The following logic holds true:
* csnA csnB csnC
* valueA = valueC, valueB valueC
When server X receives replication C, ipa-otp-counter will strip out all
counter mod operations; applying the update but not the lower value. The
value of replication B is retained. This is the correct behavior.
When server Y processes replications A and B, ipa-otp-counter will
detect that a higher value has a lower CSN and will manually set the
higher value (in bepost). This initiates replication D, which is sent to
server X. Here is the logic:
* csnA csnB csnC csnD
* valueA = valueC, valueB = valueD, valueD valueC
Server X receives replication D. D has the highest CSN. It has the same
value as replication B (server X's current value). Because the values
are the same, ipa-otp-counter will strip all counter mod operations.
This reduces counter write contention which might become a problem in
N-way replication when N2.
On Fri, 2014-10-03 at 19:52 +0200, thierry bordaz wrote:
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal
operation, the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from
the original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the
orginal/final value and can trigger the repair op.
This time being outside of the main operation txn, the repair
op will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
Hello Nathaniel,
An additional comment about the patch.
When the new value is detected to be invalid, it is fixed by a
repair operation (trigger_replication).
I did test it and it is fine to update, with an internal operation,
the same entry that is currently updated.
Now if you apply the repair operation into a be_preop or a
betxn_preop, when it returns from preop the txn of the current
operation will overwrite the repaired value.
An option is to register a bepost that checks the value from the
original entry (SLAPI_ENTRY_PRE_OP) and post entry
(SLAPI_ENTRY_POST_OP). Then this postop checks the orginal/final
value and can trigger the repair op.
This time being outside of the main operation txn, the repair op
will be applied.
thanks
thierry
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/30/2014 10:49 PM, Nathaniel McCallum wrote:
On Tue, 2014-09-30 at 18:30 +0200, thierry bordaz wrote:
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses = slapi_entry_attr_get_charray(entry, objectClass);
+if (clsses == NULL)
+return NULL;
+
+for (size_t i = 0; attr == NULL clsses[i] != NULL; i++) {
+for (size_t j = 0; attr == NULL
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses = slapi_entry_attr_get_charray(entry, objectClass);
+if (clsses == NULL)
+return NULL;
+
+for (size_t i = 0; attr == NULL clsses[i] != NULL; i++) {
+for (size_t j = 0; attr == NULL table[j].clss != NULL;
j++) {
+if (PL_strcasecmp(table[j].clss, clsses[i]) == 0)
+attr =
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Tue, 2014-09-30 at 18:30 +0200, thierry bordaz wrote:
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses =
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/20/2014 09:39 PM, Nathaniel McCallum wrote:
On Sat, 2014-09-20 at 00:25 +0200, thierry bordaz wrote:
Hello Nathaniel,
sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It
looks good but difficult to think to all possible cases.
I think to the following corner case:
The initial entry has ipatokenHOTPcounter=5
ldapmodify..
changetype: modify
add: ipatokenHOTPcounter
ipatokenHOTPcounter: 6
-
replace: ipatokenHOTPcounter
ipatokenHOTPcounter: 7
It translates
add: 6
del: 5
add: 7
This operation will fail because ipatokenHOTPcounter is
single-valued although IMHO it should succeed.
No. It should fail. There can only ever be one ipatokenHOTPcounter.
This is a so special operation that is may not really be a
concern.
+1
It is important that attribute are single valued. The
replication changelog will replicated MOD/DEL + MOD/ADD for a
MOD/REPL.
That means that if the attributes are updated on several
masters, the number of values can likely increase. Where for
single value it should only keep the most recent value.
That is a concern, at least for now. Eventually we won't use replication
for this at all. But for now, we will.
Here is the problem I foresee. You have two servers: A and B.
The user authenticates on A. This triggers a MOD/DEL(0)+MOD/ADD(1).
Replication is sent to server B.
Before the replication is performed on server B, the user authenticates
with the next token. This triggers a MOD/DEL(0)+MOD/ADD(2). Replication
is sent to server A. This replication will fail because A has a value of
1, not a value of 0.
The end result is that there will be different values for
ipatokenHOTPcounter on the two different servers. A will have 1 and B
will have 2. Once this happens, the replications can never reconcile
(this is a big problem). I see two options here, both theoretical.
The final value (when all updates has been replicated/applied) will rely
on the timestamp of the operation (CSN). When a CSN is generated on a
master it is guaranteed that it is greater (more recent) than any known
CSNs. In your scenario nothing can guaranty that the CSN of the
second update (B) is larger than the CSN of the first update. If CSN(B)
CSN(A), the final value will be '2' else it will be '1'.
When the server B will receive the first update ('1') its current value
is '2'. So it will reject the update, that may break replication.
If we decide to not control replicated update and if CSN(A) CSN(B),
then the final value will go backward.
Option #1 is to hook 389 in a different place: before the mods are
performed by after the replication changelog is generated.
Alternatively, we could insert a hook after the replication changelog is
generated, but before it is sent. We could consolidate the MOD/DEL
+MOD/ADD here into a single MOD/REPLACE operation.
With current replication it would be major change. Update of the
changelog is done in betxn_postop based on the operations mods.
Option #2 is to have some way to translate the MOD/REPLACE(X) into a
MOD/DEL(=X)+MOD/ADD(X).
I think it is already what the plugin is doing. It retrieves the current
value from the entry and do a mod/del on this value then a mod/add of
the new value. (at the condition new_valuecurrent_value).
My concern is that in your scenario, due to parallel update with several
new_values we may select an incorrect new_value.
The 'Dynamic Master proxy' (
http://www.freeipa.org/page/V4/OTP_Replay_Prevention#Replication_Counter_Race)
solution looks as a good one.
Are either of these possible? Is there another option?
Nathaniel
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Mon, 2014-09-22 at 11:22 +0200, thierry bordaz wrote:
On 09/20/2014 09:39 PM, Nathaniel McCallum wrote:
On Sat, 2014-09-20 at 00:25 +0200, thierry bordaz wrote:
Hello Nathaniel,
sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It
looks good but difficult to think to all possible cases.
I think to the following corner case:
The initial entry has ipatokenHOTPcounter=5
ldapmodify..
changetype: modify
add: ipatokenHOTPcounter
ipatokenHOTPcounter: 6
-
replace: ipatokenHOTPcounter
ipatokenHOTPcounter: 7
It translates
add: 6
del: 5
add: 7
This operation will fail because ipatokenHOTPcounter is
single-valued although IMHO it should succeed.
No. It should fail. There can only ever be one ipatokenHOTPcounter.
This is a so special operation that is may not really be a
concern.
+1
It is important that attribute are single valued. The
replication changelog will replicated MOD/DEL + MOD/ADD for a
MOD/REPL.
That means that if the attributes are updated on several
masters, the number of values can likely increase. Where for
single value it should only keep the most recent value.
That is a concern, at least for now. Eventually we won't use replication
for this at all. But for now, we will.
Here is the problem I foresee. You have two servers: A and B.
The user authenticates on A. This triggers a MOD/DEL(0)+MOD/ADD(1).
Replication is sent to server B.
Before the replication is performed on server B, the user authenticates
with the next token. This triggers a MOD/DEL(0)+MOD/ADD(2). Replication
is sent to server A. This replication will fail because A has a value of
1, not a value of 0.
The end result is that there will be different values for
ipatokenHOTPcounter on the two different servers. A will have 1 and B
will have 2. Once this happens, the replications can never reconcile
(this is a big problem). I see two options here, both theoretical.
The final value (when all updates has been replicated/applied) will rely
on the timestamp of the operation (CSN). When a CSN is generated on a
master it is guaranteed that it is greater (more recent) than any known
CSNs. In your scenario nothing can guaranty that the CSN of the
second update (B) is larger than the CSN of the first update. If CSN(B)
CSN(A), the final value will be '2' else it will be '1'.
When the server B will receive the first update ('1') its current value
is '2'. So it will reject the update, that may break replication.
If we decide to not control replicated update and if CSN(A) CSN(B),
then the final value will go backward.
The whole point of this plugin is, in a sense, to break replication. CSN
is *not* sufficient to determine valid updates for the
counter/watermark. We don't care what the CSN is, we only care whether
the counter/watermark has increased.
The counter/watermark MUST NEVER decrease.
Please see my new patch here:
http://www.redhat.com/archives/freeipa-devel/2014-September/msg00435.html
Option #1 is to hook 389 in a different place: before the mods are
performed by after the replication changelog is generated.
Alternatively, we could insert a hook after the replication changelog is
generated, but before it is sent. We could consolidate the MOD/DEL
+MOD/ADD here into a single MOD/REPLACE operation.
With current replication it would be major change. Update of the
changelog is done in betxn_postop based on the operations mods.
Option #2 is to have some way to translate the MOD/REPLACE(X) into a
MOD/DEL(=X)+MOD/ADD(X).
I think it is already what the plugin is doing. It retrieves the current
value from the entry and do a mod/del on this value then a mod/add of
the new value. (at the condition new_valuecurrent_value).
My concern is that in your scenario, due to parallel update with several
new_values we may select an incorrect new_value.
The 'Dynamic Master proxy' (
http://www.freeipa.org/page/V4/OTP_Replay_Prevention#Replication_Counter_Race)
solution looks as a good one.
Synchronous synchronization is the better plan on that page. But this is
a *huge* task. Too big for 4.1.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses = slapi_entry_attr_get_charray(entry, objectClass);
+if (clsses == NULL)
+return NULL;
+
+for (size_t i = 0; attr == NULL clsses[i] != NULL; i++) {
+for (size_t j = 0; attr == NULL table[j].clss != NULL;
j++) {
+if
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
Hello Nathaniel, I have a separated remark about updating the mods. modifications of the entry occurs in two phases: * call BE_PREOP plugins then apply the mods on the entry * call BE_TXN_PREOP plugin then apply *only* extra mods on the entry The plugin (BE_TXN_PREOP) translates a MOD/replace from MOD/DEL (current value) + MOD/ADD(new value). The translation is done like this: MOD[0]: MOD/REPL ipatokenHOTPcounter new_value MOD[1]: MOD/REPL modifiersname MOD[2]: MOD/REPL modifytimestamp MOD[3]: MOD/REPL entryusn -- MOD[0]: MOD/DEL ipatokenHOTPcounter current_value MOD[1]: MOD/ADD ipatokenHOTPcounter new_value MOD[2]: MOD/REPL modifiersname MOD[3]: MOD/REPL modifytimestamp MOD[4]: MOD/REPL entryusn That means, we have one added mod (MOD[4]=entryusn) after BE_TXN_PREOP that will be (re)applied on the second phase. So the MOD/DEL+MOD/ADD will not be processed. MOD/REPL will be processed before MOD/DEL+MOD/ADD are generated. If MOD/DEL+MOD/ADD is needed, then we may need to replace REPL-DEL/ADD in BE_PREOP callback. The control done in plugin BE_TXN_PREOP is working fine and guaranties the counter does not decrease. thanks thierry On 09/19/2014 10:15 PM, Nathaniel McCallum wrote: This new version fixes a small style issue pointed out to me by richm (thanks!). On Fri, 2014-09-19 at 13:39 -0400, Nathaniel McCallum wrote: The attached version of the patch should solve all of these issues. It should also be more performant and use less memory. Nathaniel On Wed, 2014-09-17 at 15:33 +0200, thierry bordaz wrote: On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, More thoughts.. I think being betxnpreoperation you are safe with parallel client ops and the check is atomic. I have few comments: * Shouldn't be implemented SLAPI_PLUGIN_CLOSE_FN/SLAPI_PLUGIN_START_FN callback, even if they are empty. * In load_counters, in case we have a target entry that has not 'objectclass' 'ipatokenHOTP|ipatokenTOTP' and the mod operation: dn: entry changetype: modify replace: ipatokenHOTPcounter ipatokenHOTPcounter: 200 - add: objectclass objectclass: ipatokenHOTP I wonder if the operation will not fail although IMHO it should succeeds. Shouldn't let the schema checking reject the operation if the attribute is not granted by the entry objectclass * in load_counters, I am under the impression it may return ipatokenHOTPcounter or ipatokenTOTPwatermark (ipatokenHOTPcounter is missing). But then how the caller knows that the returned value is a counter or a watermark ? * in ldapmod_is_attrs you may prefer PL_strcasecmp to strcasecmp About replicated updates, if updates of counters/watermark are done on several servers. Then a replicated operation may want to set counters/watermark with a smaller value that the existing one. In that case, it will return unwilling to perform. That could break replication. If the update comes from replication and the value is going backward, we could make the operation a nop operation (setting the attribute to its current value). thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Sat, 2014-09-20 at 17:33 -0400, Simo Sorce wrote: On Sat, 20 Sep 2014 15:39:48 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: On Sat, 2014-09-20 at 00:25 +0200, thierry bordaz wrote: Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. No. It should fail. There can only ever be one ipatokenHOTPcounter. This is a so special operation that is may not really be a concern. +1 It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. That is a concern, at least for now. Eventually we won't use replication for this at all. But for now, we will. Here is the problem I foresee. You have two servers: A and B. The user authenticates on A. This triggers a MOD/DEL(0)+MOD/ADD(1). Replication is sent to server B. Before the replication is performed on server B, the user authenticates with the next token. This triggers a MOD/DEL(0)+MOD/ADD(2). Replication is sent to server A. This replication will fail because A has a value of 1, not a value of 0. The end result is that there will be different values for ipatokenHOTPcounter on the two different servers. A will have 1 and B will have 2. Once this happens, the replications can never reconcile (this is a big problem). I see two options here, both theoretical. Option #1 is to hook 389 in a different place: before the mods are performed by after the replication changelog is generated. Alternatively, we could insert a hook after the replication changelog is generated, but before it is sent. We could consolidate the MOD/DEL +MOD/ADD here into a single MOD/REPLACE operation. Option #2 is to have some way to translate the MOD/REPLACE(X) into a MOD/DEL(=X)+MOD/ADD(X). Are either of these possible? Is there another option? I think the only option would be to intercept modifications on replicas, check that the mod is due to a replication event and discard/change the delete and change the add into a replace. This means replay attacks will still be possible, but there is no way around that until we handle the replication/quorum thing ourselves. Attached is a new version. This version replays the counter value through the mod steps. This allows us to discard delete values when doing replication and fill them in with the current counter value. It also allows us to play all the steps and check our parameters on the final state. I've tested with some fairly complex ldifs and I haven't seen any bugs. I think this should fix all the outstanding problems. Nathaniel From 1f3b1454d5fa665029f898c3b5eeead22a40eb23 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-decrement 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-otp-decrement/Makefile.am | 25 ++ .../ipa-otp-decrement/ipa-otp-decrement.sym| 1 + .../ipa-otp-decrement/ipa_otp_decrement.c | 375 + .../ipa-otp-decrement/otp-decrement-conf.ldif | 15 + freeipa.spec.in| 2 + ipaserver/install/dsinstance.py| 4 + 8 files changed, 424 insertions(+) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/otp-decrement-conf.ldif diff --git a/daemons/configure.ac
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Sat, 2014-09-20 at 00:25 +0200, thierry bordaz wrote: Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. No. It should fail. There can only ever be one ipatokenHOTPcounter. This is a so special operation that is may not really be a concern. +1 It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. That is a concern, at least for now. Eventually we won't use replication for this at all. But for now, we will. Here is the problem I foresee. You have two servers: A and B. The user authenticates on A. This triggers a MOD/DEL(0)+MOD/ADD(1). Replication is sent to server B. Before the replication is performed on server B, the user authenticates with the next token. This triggers a MOD/DEL(0)+MOD/ADD(2). Replication is sent to server A. This replication will fail because A has a value of 1, not a value of 0. The end result is that there will be different values for ipatokenHOTPcounter on the two different servers. A will have 1 and B will have 2. Once this happens, the replications can never reconcile (this is a big problem). I see two options here, both theoretical. Option #1 is to hook 389 in a different place: before the mods are performed by after the replication changelog is generated. Alternatively, we could insert a hook after the replication changelog is generated, but before it is sent. We could consolidate the MOD/DEL +MOD/ADD here into a single MOD/REPLACE operation. Option #2 is to have some way to translate the MOD/REPLACE(X) into a MOD/DEL(=X)+MOD/ADD(X). Are either of these possible? Is there another option? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Fri, 2014-09-19 at 18:46 -0400, Simo Sorce wrote: On Sat, 20 Sep 2014 00:25:34 +0200 thierry bordaz tbor...@redhat.com wrote: Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. This is a so special operation that is may not really be a concern. It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. Hi thierry, this behavior is actually intentional, and we want to fail the operation if someone else updates the counter because it means a replay attack has happened. +1 We will not replicate the counters via normal replication, because it would be too much traffic anyway, we have drafted a plan to use a special plugin to handle multi-master updates specific for OTPs and their requirements. See: http://www.freeipa.org/page/V4/OTP_Replay_Prevention#Replication_Counter_Race This is a short-term concern since in the meantime we will be using replication. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Sat, 20 Sep 2014 15:39:48 -0400 Nathaniel McCallum npmccal...@redhat.com wrote: On Sat, 2014-09-20 at 00:25 +0200, thierry bordaz wrote: Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. No. It should fail. There can only ever be one ipatokenHOTPcounter. This is a so special operation that is may not really be a concern. +1 It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. That is a concern, at least for now. Eventually we won't use replication for this at all. But for now, we will. Here is the problem I foresee. You have two servers: A and B. The user authenticates on A. This triggers a MOD/DEL(0)+MOD/ADD(1). Replication is sent to server B. Before the replication is performed on server B, the user authenticates with the next token. This triggers a MOD/DEL(0)+MOD/ADD(2). Replication is sent to server A. This replication will fail because A has a value of 1, not a value of 0. The end result is that there will be different values for ipatokenHOTPcounter on the two different servers. A will have 1 and B will have 2. Once this happens, the replications can never reconcile (this is a big problem). I see two options here, both theoretical. Option #1 is to hook 389 in a different place: before the mods are performed by after the replication changelog is generated. Alternatively, we could insert a hook after the replication changelog is generated, but before it is sent. We could consolidate the MOD/DEL +MOD/ADD here into a single MOD/REPLACE operation. Option #2 is to have some way to translate the MOD/REPLACE(X) into a MOD/DEL(=X)+MOD/ADD(X). Are either of these possible? Is there another option? I think the only option would be to intercept modifications on replicas, check that the mod is due to a replication event and discard/change the delete and change the add into a replace. This means replay attacks will still be possible, but there is no way around that until we handle the replication/quorum thing ourselves. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
The attached version of the patch should solve all of these issues. It should also be more performant and use less memory. Nathaniel On Wed, 2014-09-17 at 15:33 +0200, thierry bordaz wrote: On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, More thoughts.. I think being betxnpreoperation you are safe with parallel client ops and the check is atomic. I have few comments: * Shouldn't be implemented SLAPI_PLUGIN_CLOSE_FN/SLAPI_PLUGIN_START_FN callback, even if they are empty. * In load_counters, in case we have a target entry that has not 'objectclass' 'ipatokenHOTP|ipatokenTOTP' and the mod operation: dn: entry changetype: modify replace: ipatokenHOTPcounter ipatokenHOTPcounter: 200 - add: objectclass objectclass: ipatokenHOTP I wonder if the operation will not fail although IMHO it should succeeds. Shouldn't let the schema checking reject the operation if the attribute is not granted by the entry objectclass * in load_counters, I am under the impression it may return ipatokenHOTPcounter or ipatokenTOTPwatermark (ipatokenHOTPcounter is missing). But then how the caller knows that the returned value is a counter or a watermark ? * in ldapmod_is_attrs you may prefer PL_strcasecmp to strcasecmp About replicated updates, if updates of counters/watermark are done on several servers. Then a replicated operation may want to set counters/watermark with a smaller value that the existing one. In that case, it will return unwilling to perform. That could break replication. If the update comes from replication and the value is going backward, we could make the operation a nop operation (setting the attribute to its current value). thanks theirry From 3658531e16ad914b1b8e8eff573180aa5e3947d1 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-decrement 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-otp-decrement/Makefile.am | 25 ++ .../ipa-otp-decrement/ipa-otp-decrement.sym| 1 + .../ipa-otp-decrement/ipa_otp_decrement.c | 362 + .../ipa-otp-decrement/otp-decrement-conf.ldif | 15 + freeipa.spec.in| 2 + ipaserver/install/dsinstance.py| 4 + 8 files changed, 411 insertions(+) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/otp-decrement-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index b4507a6d972f854331925e72869898576bdfd76f..936cba576818e9e6d2c5399bbfe85de7a27141ab 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -314,6 +314,7 @@ AC_CONFIG_FILES([ ipa-slapi-plugins/ipa-dns/Makefile ipa-slapi-plugins/ipa-enrollment/Makefile ipa-slapi-plugins/ipa-lockout/Makefile +ipa-slapi-plugins/ipa-otp-decrement/Makefile ipa-slapi-plugins/ipa-otp-lasttoken/Makefile ipa-slapi-plugins/ipa-pwd-extop/Makefile ipa-slapi-plugins/ipa-extdom-extop/Makefile diff --git a/daemons/ipa-slapi-plugins/Makefile.am b/daemons/ipa-slapi-plugins/Makefile.am index 06e6ee8b86f138cce05f2184ac98c39ffaf9757f..9c0f048c390dac81f58cd0270a002614693a62d8 100644 --- a/daemons/ipa-slapi-plugins/Makefile.am +++ b/daemons/ipa-slapi-plugins/Makefile.am @@ -7,6 +7,7 @@ SUBDIRS = \ ipa-enrollment
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
This new version fixes a small style issue pointed out to me by richm (thanks!). On Fri, 2014-09-19 at 13:39 -0400, Nathaniel McCallum wrote: The attached version of the patch should solve all of these issues. It should also be more performant and use less memory. Nathaniel On Wed, 2014-09-17 at 15:33 +0200, thierry bordaz wrote: On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, More thoughts.. I think being betxnpreoperation you are safe with parallel client ops and the check is atomic. I have few comments: * Shouldn't be implemented SLAPI_PLUGIN_CLOSE_FN/SLAPI_PLUGIN_START_FN callback, even if they are empty. * In load_counters, in case we have a target entry that has not 'objectclass' 'ipatokenHOTP|ipatokenTOTP' and the mod operation: dn: entry changetype: modify replace: ipatokenHOTPcounter ipatokenHOTPcounter: 200 - add: objectclass objectclass: ipatokenHOTP I wonder if the operation will not fail although IMHO it should succeeds. Shouldn't let the schema checking reject the operation if the attribute is not granted by the entry objectclass * in load_counters, I am under the impression it may return ipatokenHOTPcounter or ipatokenTOTPwatermark (ipatokenHOTPcounter is missing). But then how the caller knows that the returned value is a counter or a watermark ? * in ldapmod_is_attrs you may prefer PL_strcasecmp to strcasecmp About replicated updates, if updates of counters/watermark are done on several servers. Then a replicated operation may want to set counters/watermark with a smaller value that the existing one. In that case, it will return unwilling to perform. That could break replication. If the update comes from replication and the value is going backward, we could make the operation a nop operation (setting the attribute to its current value). thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel From 9ab5c00f483eff2da74c01499d33a9c93932f1ec Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-decrement 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. Because this plugin also ensures internal operations behave properly, this also gives ipa-pwd-extop the appropriate behavior for OTP authentication. https://fedorahosted.org/freeipa/ticket/4493 https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-otp-decrement/Makefile.am | 25 ++ .../ipa-otp-decrement/ipa-otp-decrement.sym| 1 + .../ipa-otp-decrement/ipa_otp_decrement.c | 362 + .../ipa-otp-decrement/otp-decrement-conf.ldif | 15 + freeipa.spec.in| 2 + ipaserver/install/dsinstance.py| 4 + 8 files changed, 411 insertions(+) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/otp-decrement-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index b4507a6d972f854331925e72869898576bdfd76f..936cba576818e9e6d2c5399bbfe85de7a27141ab 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -314,6 +314,7 @@ AC_CONFIG_FILES([ ipa-slapi-plugins/ipa-dns/Makefile ipa-slapi-plugins/ipa-enrollment/Makefile ipa-slapi-plugins/ipa-lockout/Makefile +ipa-slapi-plugins/ipa-otp-decrement/Makefile ipa-slapi-plugins/ipa-otp-lasttoken/Makefile ipa-slapi-plugins/ipa-pwd-extop/Makefile
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. This is a so special operation that is may not really be a concern. It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. thanks theirry On 09/19/2014 10:15 PM, Nathaniel McCallum wrote: This new version fixes a small style issue pointed out to me by richm (thanks!). On Fri, 2014-09-19 at 13:39 -0400, Nathaniel McCallum wrote: The attached version of the patch should solve all of these issues. It should also be more performant and use less memory. Nathaniel On Wed, 2014-09-17 at 15:33 +0200, thierry bordaz wrote: On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, More thoughts.. I think being betxnpreoperation you are safe with parallel client ops and the check is atomic. I have few comments: * Shouldn't be implemented SLAPI_PLUGIN_CLOSE_FN/SLAPI_PLUGIN_START_FN callback, even if they are empty. * In load_counters, in case we have a target entry that has not 'objectclass' 'ipatokenHOTP|ipatokenTOTP' and the mod operation: dn: entry changetype: modify replace: ipatokenHOTPcounter ipatokenHOTPcounter: 200 - add: objectclass objectclass: ipatokenHOTP I wonder if the operation will not fail although IMHO it should succeeds. Shouldn't let the schema checking reject the operation if the attribute is not granted by the entry objectclass * in load_counters, I am under the impression it may return ipatokenHOTPcounter or ipatokenTOTPwatermark (ipatokenHOTPcounter is missing). But then how the caller knows that the returned value is a counter or a watermark ? * in ldapmod_is_attrs you may prefer PL_strcasecmp to strcasecmp About replicated updates, if updates of counters/watermark are done on several servers. Then a replicated operation may want to set counters/watermark with a smaller value that the existing one. In that case, it will return unwilling to perform. That could break replication. If the update comes from replication and the value is going backward, we could make the operation a nop operation (setting the attribute to its current value). thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Sat, 20 Sep 2014 00:25:34 +0200 thierry bordaz tbor...@redhat.com wrote: Hello Nathaniel, sanitize_input translates MOD/REPLACE into MOD/DEL+MOD/ADD. It looks good but difficult to think to all possible cases. I think to the following corner case: The initial entry has ipatokenHOTPcounter=5 ldapmodify.. changetype: modify add: ipatokenHOTPcounter ipatokenHOTPcounter: 6 - replace: ipatokenHOTPcounter ipatokenHOTPcounter: 7 It translates add: 6 del: 5 add: 7 This operation will fail because ipatokenHOTPcounter is single-valued although IMHO it should succeed. This is a so special operation that is may not really be a concern. It is important that attribute are single valued. The replication changelog will replicated MOD/DEL + MOD/ADD for a MOD/REPL. That means that if the attributes are updated on several masters, the number of values can likely increase. Where for single value it should only keep the most recent value. Hi thierry, this behavior is actually intentional, and we want to fail the operation if someone else updates the counter because it means a replay attack has happened. We will not replicate the counters via normal replication, because it would be too much traffic anyway, we have drafted a plan to use a special plugin to handle multi-master updates specific for OTPs and their requirements. See: http://www.freeipa.org/page/V4/OTP_Replay_Prevention#Replication_Counter_Race Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, More thoughts.. I think being betxnpreoperation you are safe with parallel client ops and the check is atomic. I have few comments: * Shouldn't be implemented SLAPI_PLUGIN_CLOSE_FN/SLAPI_PLUGIN_START_FN callback, even if they are empty. * In load_counters, in case we have a target entry that has not 'objectclass' 'ipatokenHOTP|ipatokenTOTP' and the mod operation: dn: entry changetype: modify replace: ipatokenHOTPcounter ipatokenHOTPcounter: 200 - add: objectclass objectclass: ipatokenHOTP I wonder if the operation will not fail although IMHO it should succeeds. Shouldn't let the schema checking reject the operation if the attribute is not granted by the entry objectclass * in load_counters, I am under the impression it may return ipatokenHOTPcounter or ipatokenTOTPwatermark (ipatokenHOTPcounter is missing). But then how the caller knows that the returned value is a counter or a watermark ? * in ldapmod_is_attrs you may prefer PL_strcasecmp to strcasecmp About replicated updates, if updates of counters/watermark are done on several servers. Then a replicated operation may want to set counters/watermark with a smaller value that the existing one. In that case, it will return unwilling to perform. That could break replication. If the update comes from replication and the value is going backward, we could make the operation a nop operation (setting the attribute to its current value). thanks theirry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, Starting looking at it, I have just a question about sanitize_input. If the modification (replace) is related to counter/watermark, it triggers an internal search on the target entry itself. The original/modified entry is also present in the pblock. The internal search will check the filter but except that what is the benefit vs taking the entry directly in the pblock. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Tue, 2014-09-16 at 19:24 +0200, thierry bordaz wrote: On 09/15/2014 09:05 PM, Nathaniel McCallum wrote: This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, Starting looking at it, I have just a question about sanitize_input. If the modification (replace) is related to counter/watermark, it triggers an internal search on the target entry itself. The original/modified entry is also present in the pblock. The internal search will check the filter but except that what is the benefit vs taking the entry directly in the pblock. I didn't know the entry was already in the pblock. What loads it? And when? How do I access it? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 From 9256d83755bb9a19eae98a248eb0a33a4fecd089 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum npmccal...@redhat.com Date: Wed, 10 Sep 2014 17:31:37 -0400 Subject: [PATCH] Create ipa-otp-decrement 389DS plugin This plugin ensures that all counter/watermark operations are atomic and never decrement. Also, deletion is not permitted. https://fedorahosted.org/freeipa/ticket/4494 --- daemons/configure.ac | 1 + daemons/ipa-slapi-plugins/Makefile.am | 1 + .../ipa-otp-decrement/Makefile.am | 25 ++ .../ipa-otp-decrement/ipa-otp-decrement.sym| 1 + .../ipa-otp-decrement/ipa_otp_decrement.c | 385 + .../ipa-otp-decrement/otp-decrement-conf.ldif | 15 + freeipa.spec.in| 2 + ipaserver/install/dsinstance.py| 4 + 8 files changed, 434 insertions(+) create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c create mode 100644 daemons/ipa-slapi-plugins/ipa-otp-decrement/otp-decrement-conf.ldif diff --git a/daemons/configure.ac b/daemons/configure.ac index b4507a6d972f854331925e72869898576bdfd76f..936cba576818e9e6d2c5399bbfe85de7a27141ab 100644 --- a/daemons/configure.ac +++ b/daemons/configure.ac @@ -314,6 +314,7 @@ AC_CONFIG_FILES([ ipa-slapi-plugins/ipa-dns/Makefile ipa-slapi-plugins/ipa-enrollment/Makefile ipa-slapi-plugins/ipa-lockout/Makefile +ipa-slapi-plugins/ipa-otp-decrement/Makefile ipa-slapi-plugins/ipa-otp-lasttoken/Makefile ipa-slapi-plugins/ipa-pwd-extop/Makefile ipa-slapi-plugins/ipa-extdom-extop/Makefile diff --git a/daemons/ipa-slapi-plugins/Makefile.am b/daemons/ipa-slapi-plugins/Makefile.am index 06e6ee8b86f138cce05f2184ac98c39ffaf9757f..9c0f048c390dac81f58cd0270a002614693a62d8 100644 --- a/daemons/ipa-slapi-plugins/Makefile.am +++ b/daemons/ipa-slapi-plugins/Makefile.am @@ -7,6 +7,7 @@ SUBDIRS = \ ipa-enrollment \ ipa-lockout \ ipa-modrdn \ + ipa-otp-decrement \ ipa-otp-lasttoken \ ipa-pwd-extop \ ipa-extdom-extop \ diff --git a/daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am b/daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am new file mode 100644 index ..24aea58d22de57fcf88cbbe27f88c162ad47077c --- /dev/null +++ b/daemons/ipa-slapi-plugins/ipa-otp-decrement/Makefile.am @@ -0,0 +1,25 @@ +MAINTAINERCLEANFILES = *~ Makefile.in +PLUGIN_COMMON_DIR = ../common +AM_CPPFLAGS = \ + -I. \ + -I$(srcdir) \ + -I$(PLUGIN_COMMON_DIR) \ + -I/usr/include/dirsrv \ + -DPREFIX=\$(prefix)\ \ + -DBINDIR=\$(bindir)\\ + -DLIBDIR=\$(libdir)\ \ + -DLIBEXECDIR=\$(libexecdir)\ \ + -DDATADIR=\$(datadir)\\ + $(AM_CFLAGS) \ + $(LDAP_CFLAGS) \ + $(WARN_CFLAGS) + +plugindir = $(libdir)/dirsrv/plugins +plugin_LTLIBRARIES = libipa_otp_decrement.la +libipa_otp_decrement_la_SOURCES = ipa_otp_decrement.c +libipa_otp_decrement_la_LDFLAGS = -avoid-version -export-symbols ipa-otp-decrement.sym +libipa_otp_decrement_la_LIBADD = $(LDAP_LIBS) + +appdir = $(IPA_DATA_DIR) +app_DATA = otp-decrement-conf.ldif +EXTRA_DIST = $(app_DATA) diff --git a/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym b/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym new file mode 100644 index ..27829ead6cc5fcb13b0fbff0b7ac3d638cbf4e2f --- /dev/null +++ b/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa-otp-decrement.sym @@ -0,0 +1 @@ +ipa_otp_decrement_init diff --git a/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c b/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c new file mode 100644 index ..86e851bd991318cfe88fdbebbe1725e6b22d6b06 --- /dev/null +++ b/daemons/ipa-slapi-plugins/ipa-otp-decrement/ipa_otp_decrement.c @@ -0,0 +1,385 @@ +/** BEGIN COPYRIGHT BLOCK + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see http://www.gnu.org/licenses/. + * + * Additional permission under GPLv3
