[Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
Nathaniel McCallum
npmccallum at redhat.com
Tue Oct 7 18:48:50 UTC 2014
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 <npmccallum at 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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
More information about the Freeipa-devel
mailing list