Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 2014-09-18 at 14:20 -0400, Simo Sorce wrote: > On Thu, 18 Sep 2014 13:59:34 -0400 > Nathaniel McCallum wrote: > > > On Thu, 2014-09-18 at 14:00 +0200, Petr Vobornik wrote: > > > On 15.9.2014 21:08, Nathaniel McCallum wrote: > > > > On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: > > > >> This prevents any local attempt at rapid token code replay. If > > > >> two token codes hit the system at roughly the same moment, only > > > >> the first write will succeed. All subsequent authentications > > > >> will fail. > > > >> > > > >> This obviates the need for an OTP authentication lock. > > > >> > > > >> https://fedorahosted.org/freeipa/ticket/4493 > > > > > > > > I still need a review of this. This is targeted for 4.1. > > > > > > > > Nathaniel > > > > > > > > > > > > > Works fine with HTOP but fails for new TOTP tokens. > > > > > > New TOTP token doesn't have a watermark attribute set so there is > > > nothing to delete and therefore standard login procedure fails on > > > writeattr call (libotp.c:223). > > > > I have fixed this by making ipatokenTOTPwatermark a required attribute > > (MAY -> MUST). I did this in a separate patch (0066) because I thought > > it was cleaner. > > This can easily break stuff, and is not allowed, sorry you need to find > a way that will not cause objects, even temporarily to be incomplete. > > (think of a replica getting the new schema while an older one pushes > the object via replication) I rescind this patch. It is no longer necessary. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 18 Sep 2014 13:59:34 -0400 Nathaniel McCallum wrote: > On Thu, 2014-09-18 at 14:00 +0200, Petr Vobornik wrote: > > On 15.9.2014 21:08, Nathaniel McCallum wrote: > > > On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: > > >> This prevents any local attempt at rapid token code replay. If > > >> two token codes hit the system at roughly the same moment, only > > >> the first write will succeed. All subsequent authentications > > >> will fail. > > >> > > >> This obviates the need for an OTP authentication lock. > > >> > > >> https://fedorahosted.org/freeipa/ticket/4493 > > > > > > I still need a review of this. This is targeted for 4.1. > > > > > > Nathaniel > > > > > > > > > Works fine with HTOP but fails for new TOTP tokens. > > > > New TOTP token doesn't have a watermark attribute set so there is > > nothing to delete and therefore standard login procedure fails on > > writeattr call (libotp.c:223). > > I have fixed this by making ipatokenTOTPwatermark a required attribute > (MAY -> MUST). I did this in a separate patch (0066) because I thought > it was cleaner. This can easily break stuff, and is not allowed, sorry you need to find a way that will not cause objects, even temporarily to be incomplete. (think of a replica getting the new schema while an older one pushes the object via replication) Simo. > https://www.redhat.com/archives/freeipa-devel/2014-September/msg00386.html > > There is no change to this patch, but it now depends on my patch 0066 > (linked above). > > Nathaniel > > ___ > Freeipa-devel mailing list > Freeipa-devel@redhat.com > https://www.redhat.com/mailman/listinfo/freeipa-devel -- 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 0062] Use delete/add for OTP counter/watermark updates
On Thu, 2014-09-18 at 14:00 +0200, Petr Vobornik wrote: > On 15.9.2014 21:08, Nathaniel McCallum wrote: > > On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: > >> This prevents any local attempt at rapid token code replay. If two > >> token codes hit the system at roughly the same moment, only the > >> first write will succeed. All subsequent authentications will fail. > >> > >> This obviates the need for an OTP authentication lock. > >> > >> https://fedorahosted.org/freeipa/ticket/4493 > > > > I still need a review of this. This is targeted for 4.1. > > > > Nathaniel > > > > > Works fine with HTOP but fails for new TOTP tokens. > > New TOTP token doesn't have a watermark attribute set so there is > nothing to delete and therefore standard login procedure fails on > writeattr call (libotp.c:223). I have fixed this by making ipatokenTOTPwatermark a required attribute (MAY -> MUST). I did this in a separate patch (0066) because I thought it was cleaner. https://www.redhat.com/archives/freeipa-devel/2014-September/msg00386.html There is no change to this patch, but it now depends on my patch 0066 (linked above). Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On 15.9.2014 21:08, Nathaniel McCallum wrote: On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: This prevents any local attempt at rapid token code replay. If two token codes hit the system at roughly the same moment, only the first write will succeed. All subsequent authentications will fail. This obviates the need for an OTP authentication lock. https://fedorahosted.org/freeipa/ticket/4493 I still need a review of this. This is targeted for 4.1. Nathaniel Works fine with HTOP but fails for new TOTP tokens. New TOTP token doesn't have a watermark attribute set so there is nothing to delete and therefore standard login procedure fails on writeattr call (libotp.c:223). -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: > This prevents any local attempt at rapid token code replay. If two > token codes hit the system at roughly the same moment, only the > first write will succeed. All subsequent authentications will fail. > > This obviates the need for an OTP authentication lock. > > https://fedorahosted.org/freeipa/ticket/4493 I still need a review of this. This is targeted for 4.1. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
On Thu, 2014-08-28 at 22:54 -0400, Nathaniel McCallum wrote: > This prevents any local attempt at rapid token code replay. If two > token codes hit the system at roughly the same moment, only the > first write will succeed. All subsequent authentications will fail. > > This obviates the need for an OTP authentication lock. > > https://fedorahosted.org/freeipa/ticket/4493 LGTM. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0062] Use delete/add for OTP counter/watermark updates
This prevents any local attempt at rapid token code replay. If two
token codes hit the system at roughly the same moment, only the
first write will succeed. All subsequent authentications will fail.
This obviates the need for an OTP authentication lock.
https://fedorahosted.org/freeipa/ticket/4493
From a414a5873e47880328ef578c3ea5ff078e096cd5 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum
Date: Thu, 28 Aug 2014 22:45:18 -0400
Subject: [PATCH] Use delete/add for OTP counter/watermark updates
This prevents any local attempt at rapid token code replay. If two
token codes hit the system at roughly the same moment, only the
first write will succeed. All subsequent authentications will fail.
This obviates the need for an OTP authentication lock.
https://fedorahosted.org/freeipa/ticket/4493
---
daemons/ipa-slapi-plugins/libotp/libotp.c | 67 +--
1 file changed, 36 insertions(+), 31 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/libotp/libotp.c b/daemons/ipa-slapi-plugins/libotp/libotp.c
index 41f9e7b4809fbca82452d260b9aa7d1d3059fd2e..d41fc2886987b0ab2d69bcb1bbb9986d6935774e 100644
--- a/daemons/ipa-slapi-plugins/libotp/libotp.c
+++ b/daemons/ipa-slapi-plugins/libotp/libotp.c
@@ -124,31 +124,39 @@ static const struct berval *entry_attr_get_berval(const Slapi_Entry* e,
return slapi_value_get_berval(v);
}
+/**
+ * Changes an attribute from oldval to newval.
+ * If oldval has changed on disk since it was read, fail.
+ */
static bool writeattr(const struct otptoken *token, const char *attr,
- int value)
+ long long oldval, long long newval)
{
-Slapi_Value *svals[] = { NULL, NULL };
Slapi_PBlock *pb = NULL;
-Slapi_Mods *mods = NULL;
bool success = false;
+char oldvalue[32];
+char newvalue[32];
int ret;
-/* Create the value. */
-svals[0] = slapi_value_new();
-if (slapi_value_set_int(svals[0], value) != 0) {
-slapi_value_free(&svals[0]);
-return false;
-}
+LDAPMod *mods[] = {
+&(LDAPMod) {
+LDAP_MOD_DELETE, (char *) attr,
+.mod_values = (char *[]) { oldvalue, NULL }
+},
+&(LDAPMod) {
+LDAP_MOD_ADD, (char *) attr,
+.mod_values = (char *[]) { newvalue, NULL }
+},
+NULL
+};
-/* Create the mods. */
-mods = slapi_mods_new();
-slapi_mods_add_mod_values(mods, LDAP_MOD_REPLACE, attr, svals);
+/* Convert the values to strings. */
+snprintf(oldvalue, sizeof(oldvalue), "%lld", oldval);
+snprintf(newvalue, sizeof(newvalue), "%lld", newval);
/* Perform the modification. */
pb = slapi_pblock_new();
slapi_modify_internal_set_pb(pb, slapi_sdn_get_dn(token->sdn),
- slapi_mods_get_ldapmods_byref(mods),
- NULL, NULL, token->plugin_id, 0);
+ mods, NULL, NULL, token->plugin_id, 0);
if (slapi_modify_internal_pb(pb) != 0)
goto error;
if (slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &ret) != 0)
@@ -160,9 +168,7 @@ static bool writeattr(const struct otptoken *token, const char *attr,
error:
slapi_pblock_destroy(pb);
-slapi_mods_free(&mods);
return success;
-
}
/**
@@ -174,12 +180,14 @@ static bool validate(struct otptoken *token, time_t now, ssize_t step,
uint32_t first, const uint32_t *second)
{
const char *attr;
+long long last;
uint32_t tmp;
/* Calculate the absolute step. */
switch (token->type) {
case OTPTOKEN_TOTP:
attr = T("watermark");
+last = token->totp.watermark;
step = (now + token->totp.offset) / token->totp.step + step;
if (token->totp.watermark > 0 && step < token->totp.watermark)
return false;
@@ -188,6 +196,7 @@ static bool validate(struct otptoken *token, time_t now, ssize_t step,
if (step < 0) /* NEVER go backwards! */
return false;
attr = H("counter");
+last = token->hotp.counter;
step = token->hotp.counter + step;
break;
default:
@@ -208,26 +217,22 @@ static bool validate(struct otptoken *token, time_t now, ssize_t step,
if (*second != tmp)
return false;
+}
+/* Write the step value. */
+if (!writeattr(token, attr, last, step))
+return false;
+
+/* Save our modifications to the object. */
+switch (token->type) {
+case OTPTOKEN_TOTP:
/* Perform optional synchronization steps. */
-switch (token->type) {
-case OTPTOKEN_TOTP:
+if (second != NULL) {
tmp = (step - now / token->totp.step) * token->totp.step;
-if (!writeattr(token, T("clockOffset"), tmp))
+if (!writeattr(token, T("clockOffset"), token->totp.offset, tmp))
return false;
-break;
-default:
-break;
+
