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 N>2. > >>> > >>> 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 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 = table[j].attr; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> + slapi_ch_array_free(clsses); > >>>>>>> + return attr; > >>>>>>> +} > >>>>>> Can you put a comment here that explains what you are going to do in > >>>>>> each cases in plain English ? This will help people in future figuring > >>>>>> out if/how to modify the code or why something happens a specific way. > >>>>>> It will also help the reviewer follow what is going on. > >>>>>> > >>>>>> > >>>>>>> +static int > >>>>>>> +preop_mod(Slapi_PBlock *pb) > >>>>>>> +{ > >>>>>>> + size_t count = 0, attrs = 0, extras = 0; > >>>>>>> + Slapi_Entry *entry = NULL; > >>>>>>> + const char *attr = NULL; > >>>>>>> + LDAPMod **inmods = NULL; > >>>>>>> + LDAPMod **mods = NULL; > >>>>>>> + counter ctr, orig; > >>>>>>> + > >>>>>>> + /* Get the input values. */ > >>>>>>> + slapi_pblock_get(pb, SLAPI_ENTRY_PRE_OP, &entry); > >>>>>>> + slapi_pblock_get(pb, SLAPI_MODIFY_MODS, &inmods); > >>>>>>> + if (entry == NULL || inmods == NULL) > >>>>>>> + return 0; > >>>>>>> + > >>>>>>> + /* Get the attribute name. */ > >>>>>>> + attr = get_attribute(entry); > >>>>>>> + if (attr == NULL) > >>>>>>> + return 0; /* Not a token. */ > >>>>>>> + > >>>>>>> + /* Count items. */ > >>>>>>> + while (inmods != NULL && inmods[count] != NULL) { > >>>>>> ^^ this one would read much better as: > >>>>>> for (count = 0; inmods[count] != NULL; count++) { > >>>>>> > >>>>>> You do not need to check for insmods != NULL as you already check for > >>>>>> it and return 0 a few lines above. > >>>>>> > >>>>>>> + LDAPMod *mod = inmods[count++]; > >>>>>>> + > >>>>>>> + if (PL_strcasecmp(mod->mod_type, attr) != 0) > >>>>>>> + continue; > >>>>>>> + > >>>>>>> + attrs++; > >>>>>>> + switch (mod->mod_op & LDAP_MOD_OP) { > >>>>>>> + case LDAP_MOD_REPLACE: > >>>>>>> + case LDAP_MOD_INCREMENT: > >>>>>>> + extras++; > >>>>>>> + break; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> + /* Not operating on the counter/watermark. */ > >>>>>>> + if (attrs == 0) > >>>>>>> + return 0; > >>>>>>> + > >>>>>>> + /* Get the counter. */ > >>>>>>> + orig = ctr = get_counter(entry, attr); > >>>>>>> + > >>>>>>> + /* Filter the modify operations. */ > >>>>>>> + mods = ch_calloc(count + extras + 1, LDAPMod*); > >>>>>>> + for (size_t i = 0, j = 0; inmods != NULL && inmods[i] != NULL; > >>>>>>> mods[j++] = inmods[i++]) { > >>>>>> Please remove check for insmods != NULL, it is useless, and removing it > >>>>>> will bring back the line under 80columns > >>>>>> > >>>>>>> + LDAPMod *mod = inmods[i]; > >>>>>>> + > >>>>>>> + if (PL_strcasecmp(mod->mod_type, attr) != 0) > >>>>>>> + continue; > >>>>>>> + > >>>>>>> + convert_ldapmod_to_bval(mod); > >>>>>>> + > >>>>>>> + switch (mod->mod_op & LDAP_MOD_OP) { > >>>>>>> + case LDAP_MOD_DELETE: > >>>>>>> + ctr.exists = false; > >>>>>>> + if (mod->mod_bvalues != NULL && mod->mod_bvalues[0] == > >>>>>>> NULL) > >>>>>>> + berval_free_array(&mod->mod_bvalues); > >>>>>>> + > >>>>>>> + if (is_replication(pb)) > >>>>>>> + berval_free_array(&mod->mod_bvalues); > >>>>>>> + > >>>>>>> + if (mod->mod_bvalues == NULL) > >>>>>>> + mod->mod_bvalues = make_berval_array(ctr.value); > >>>>>>> + break; > >>>>>> I am not sure I understand this segment, why are you touching the > >>>>>> delete operation outside of a replication event ? > >>>>>> Doesn't this defeat and admin tool trying to correctly delete/add to > >>>>>> avoid races ? > >>>>>> > >>>>>>> + case LDAP_MOD_INCREMENT: > >>>>>>> + mods[j++] = make_ldapmod(LDAP_MOD_DELETE, attr, > >>>>>>> ctr.value); + > >>>>>>> + ctr.value += get_value(mod, 1); > >>>>>> uhmmm if you had an ADD followed by INCREMENT operation, would this > >>>>>> cause the value to become value*2+1 instead of just value+1 ? > >>>>>> > >>>>>>> + berval_free_array(&mod->mod_bvalues); > >>>>>>> + mod->mod_op &= ~LDAP_MOD_OP; > >>>>>>> + mod->mod_op |= LDAP_MOD_ADD; > >>>>>>> + mod->mod_bvalues = make_berval_array(ctr.value); > >>>>>>> + break; > >>>>>> uhmm why are you converting mod_increment in all cases ? (including > >>>>>> replication) > >>>>>> > >>>>>>> + case LDAP_MOD_REPLACE: > >>>>>>> + if (ctr.exists) > >>>>>>> + mods[j++] = make_ldapmod(LDAP_MOD_DELETE, attr, > >>>>>>> ctr.value); + > >>>>>>> + mod->mod_op &= ~LDAP_MOD_OP; > >>>>>>> + mod->mod_op |= LDAP_MOD_ADD; > >>>>>> same question here, why are you converting a replace coming from > >>>>>> replication into a delete/add ? > >>>>>> > >>>>>>> + case LDAP_MOD_ADD: > >>>>>>> + ctr.value = get_value(mod, 0); > >>>>>>> + ctr.exists = true; > >>>>>>> + break; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> + /* Set the modified operations. */ > >>>>>>> + slapi_pblock_set(pb, SLAPI_MODIFY_MODS, mods); > >>>>>>> + ch_free(inmods); > >>>>>>> + > >>>>>>> + /* Ensure we aren't deleting the counter. */ > >>>>>>> + if (orig.exists && !ctr.exists) > >>>>>>> + return send_error(pb, LDAP_UNWILLING_TO_PERFORM, > >>>>>>> + "Will not delete %s", attr); > >>>>>>> + > >>>>>>> + /* Ensure we aren't rolling back the counter. */ > >>>>>>> + if (orig.value > ctr.value) > >>>>>>> + return send_error(pb, LDAP_UNWILLING_TO_PERFORM, > >>>>>>> + "Will not decrement %s", attr); > >>>>>>> + > >>>>>>> + return 0; > >>>>>>> +} > >>>>>> see above about send_error(). > >>>>>> > >>>>>> I think what is needed most is the comment that explains the process > >>>>>> at the to of the main function. > >>>>>> > >>>>>> Simo. > >>>>> All of the above are fixed. > >>>>> > >>>>> Also, I have addressed Thierry's concern about putting the plugin in > >>>>> betxnpreoperation by splitting the plugin into two phases: modification > >>>>> and validation. Now all modifications occur in bepreoperation and all > >>>>> validation occurs in betxnpreoperation. > >>>>> > >>>>> Additionally, I have new code to trigger a new replication in the case > >>>>> where a validation error occurs and we are in a replication transaction. > >>>>> Thus, when A attempts to push an old counter value to B, B will now > >>>>> replicate the latest value back to A. > >>>>> > >>>>> Nathaniel > >>>>> > > > > _______________________________________________ > > 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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel