[Freeipa-devel] [PATCH 0074] Make token window sizes configurable
thierry bordaz
tbordaz at redhat.com
Tue Nov 25 16:58:25 UTC 2014
On 11/18/2014 08:26 PM, Petr Vobornik wrote:
> On 13.11.2014 08:53, Martin Kosek wrote:
>> On 11/13/2014 08:51 AM, Nathaniel McCallum wrote:
>>> On Thu, 2014-11-13 at 08:48 +0100, Martin Kosek wrote:
>>>> On 11/12/2014 11:37 PM, Nathaniel McCallum wrote:
>>>>> On Mon, 2014-11-10 at 08:28 +0100, Martin Kosek wrote:
>>>>>> On 11/07/2014 04:44 PM, Petr Vobornik wrote:
>>>>>>> On 7.11.2014 08:58, Martin Kosek wrote:
>>>>>>>> On 11/04/2014 05:17 PM, Nathaniel McCallum wrote:
>>>>>>>>> On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote:
>>>>>>>>>> On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote:
>>>>>>>>>>> On 10/29/2014 10:37 AM, Martin Kosek wrote:
>>>>>>>>>>>> On 10/28/2014 09:59 PM, Nathaniel McCallum wrote:
>>>>>>>>>>>>> On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote:
>>>>>>>>>>>>>> This patch gives the administrator variables to control
>>>>>>>>>>>>>> the size of
>>>>>>>>>>>>>> the authentication and synchronization windows for OTP
>>>>>>>>>>>>>> tokens.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4511
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> NOTE: There is one known issue with this patch which I
>>>>>>>>>>>>>> don't know
>>>>>>>>>>>>>> how to
>>>>>>>>>>>>>> solve. This patch changes the schema in
>>>>>>>>>>>>>> install/share/60ipaconfig.ldif.
>>>>>>>>>>>>>> On an upgrade, all of the new attributeTypes appear
>>>>>>>>>>>>>> correctly.
>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>> the modifications to the pre-existing objectClass do not
>>>>>>>>>>>>>> show up
>>>>>>>>>>>>>> on the
>>>>>>>>>>>>>> server. What am I doing wrong?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> After modifying ipaGuiConfig manually, everything in this
>>>>>>>>>>>>>> patch
>>>>>>>>>>>>>> works
>>>>>>>>>>>>>> just fine.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This new version takes into account the new (proper) OIDs and
>>>>>>>>>>>>> attribute
>>>>>>>>>>>>> names.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks Nathaniel!
>>>>>>>>>>>>
>>>>>>>>>>>>> The above known issue still remains.
>>>>>>>>>>>>
>>>>>>>>>>>> Petr3, any idea what could have gone wrong? ObjectClass MAY
>>>>>>>>>>>> list
>>>>>>>>>>>> extension
>>>>>>>>>>>> should work just fine, AFAIK.
>>>>>>>>>>>
>>>>>>>>>>> You added a blank line to the LDIF file. This is an entry
>>>>>>>>>>> separator, so
>>>>>>>>>>> the objectClasses after the blank line don't belong to
>>>>>>>>>>> cn=schema, so
>>>>>>>>>>> they aren't considered in the update.
>>>>>>>>>>> Without the blank line it works fine.
>>>>>>>>>>
>>>>>>>>>> Thanks for the catch!
>>>>>>>>>>
>>>>>>>>>> Here is a version without the blank line.
>>>>>>>>>
>>>>>>>>> I forgot to remove the old steps defines. This patch performs
>>>>>>>>> this
>>>>>>>>> cleanup.
>>>>>>>>
>>>>>>>> I am now wondering, is the global config object really the nest
>>>>>>>> place to
>>>>>>>> add these OTP specific settings?
>>>>>>>>
>>>>>>>> I would prefer not to overload the object and instead:
>>>>>>>> - create new ipaOTPConfig objectclass
>>>>>>>> - add it to cn=otp,$SUFFIX
>>>>>>>> - create otpconfig-mod and otpconfig-show commands to follow an
>>>>>>>> example
>>>>>>>> of dnsconfig-* and trustconfig-* commands
>>>>>>>>
>>>>>>>> IMO, this would allow more flexibility for the OTP settings and
>>>>>>>> would
>>>>>>>> also scale better for the future updates.
>>>>>>>
>>>>>>> +1
>>>>>>>
>>>>>>> I will comment the patch as if ^^ would not exist because it
>>>>>>> will still be
>>>>>>> needed in the new plugin.
>>>>>>>
>>>>>>> Because of ^^ I did not test, just read.
>>>>>>>
>>>>>>> 1. Got:
>>>>>>> install/ui/src/freeipa/serverconfig.js(135): lint warning: extra
>>>>>>> comma is not
>>>>>>> recommended in array initializers
>>>>>>>
>>>>>>> Please run:
>>>>>>> jsl -nofilelisting -nosummary -nologo -conf jsl.conf
>>>>>>> in install/ui directory
>>>>>>>
>>>>>>> The goal is no have no warnings and errors.
>>>>>>>
>>>>>>> 2. new attrs should be added to 'System: Read Global
>>>>>>> Configuration' managed
>>>>>>> permission
>>>>>>
>>>>>> +1. Though if we go with OTP config, it should be called
>>>>>>
>>>>>> System: Read OTP Configuration
>>>>>>
>>>>>> Martin
>>>>>
>>>>> Attached is a new set of patches that replaces this single patch.
>>>>> This
>>>>> now fixes multiple issues.
>>>>>
>>>>> I now create two new entries:
>>>>> * cn=TOTP,cn=Token Config,cn=etc,$SUFFIX
>>>>> * cn=HOTP,cn=Token Config,cn=etc,$SUFFIX
>>>>>
>>>>> There are two corresponding CLI commands:
>>>>> * totpconfig-(show|mod)
>>>>> * hotpconfig-(show|mod)
>>>>>
>>>>> There is no UI support for this yet (pointers welcome).
>>>>>
>>>>> This is designed so that eventually tokens can grow a per-token
>>>>> override, but I have not yet implemented this feature (it should
>>>>> be easy
>>>>> in the future).
>>>>>
>>>>> Additionally, I had to do some shared refactoring to address
>>>>> issues in
>>>>> ipa-otp-lasttoken, which is why all of these are now merged into a
>>>>> single patch set.
>>>>>
>>>>> Nathaniel
>
> I'm little confused with a state of reviews. Thierry were some of the
> patches ACKed in different threads or are they under review (I'm not
> reviewing DS plugin parts)?
I am sorry for the long delay...
I am having difficulties to apply the patches. I am on master branch.
For example I see those errors:
git apply -v /tmp/0001-Preliminary-refactoring-of-libotp-files.patch
Checking patch
daemons/ipa-slapi-plugins/ipa-otp-lasttoken/Makefile.am...
Checking patch
daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c...
error: while searching for:
# include <config.h>
#endif
#include <libotp.h>
#include <time.h>
#include "util.h"
#define PLUGIN_NAME "ipa-otp-lasttoken"
#define LOG(sev, ...) \
slapi_log_error(SLAPI_LOG_ ## sev, PLUGIN_NAME, \
"%s: %s\n", __func__, __VA_ARGS__), -1
static void *plugin_id;
static const Slapi_PluginDesc preop_desc = {
error: patch failed:
daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c:41
error:
daemons/ipa-slapi-plugins/ipa-otp-lasttoken/ipa_otp_lasttoken.c:
patch does not apply
Checking patch daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am...
error: while searching for:
AM_CPPFLAGS = \
-I. \
-I$(srcdir) \
-I$(srcdir)/../libotp \
-I$(PLUGIN_COMMON_DIR) \
-I$(KRB5_UTIL_DIR) \
-I$(COMMON_BER_DIR) \
error: patch failed:
daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am:10
error: daemons/ipa-slapi-plugins/ipa-pwd-extop/Makefile.am: patch
does not apply
Checking patch daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h...
Checking patch daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c...
Checking patch daemons/ipa-slapi-plugins/ipa-pwd-extop/syncreq.c...
Checking patch daemons/ipa-slapi-plugins/libotp/Makefile.am...
Checking patch daemons/ipa-slapi-plugins/libotp/librfc.c =>
daemons/ipa-slapi-plugins/libotp/hotp.c...
Checking patch daemons/ipa-slapi-plugins/libotp/librfc.h =>
daemons/ipa-slapi-plugins/libotp/hotp.h...
Checking patch daemons/ipa-slapi-plugins/libotp/libotp.c =>
daemons/ipa-slapi-plugins/libotp/otp_token.c...
Checking patch daemons/ipa-slapi-plugins/libotp/libotp.h =>
daemons/ipa-slapi-plugins/libotp/otp_token.h...
Checking patch daemons/ipa-slapi-plugins/libotp/t_librfc.c =>
daemons/ipa-slapi-plugins/libotp/t_hotp.c...
Anyone would help me seeing what I am missing here :-[
>
>
>>>>>
>>>>
>>>> Ah, I meant adding the token config to cn=otp,SUFFIX directly, but
>>>> if we want
>>>> to make TOTP/HOTP token config as separate entries (to enable
>>>> future per-token
>>>> overrides), your approach should make sense. Rather adding Rob to
>>>> CC for sanity.
>>>
>>> That would work too. I'm open to that.
>>>
>>>> I am just not sure we should create them as separate plugins, I
>>>> think the new
>>>> commands should be rather added to otp plugin directly so that they
>>>> show in
>>>> "ipa help otptoken" instead of adding 2 new topics just for OTP
>>>> config.
>>>
>>> I can play with that.
>
> Do you plan to change it? I like the idea of a single point of help
> for OTP but I'm also unsure about the length of the commands. Current
> solution is also more consistent with a rest of the framework. Would
> it be something like:
>
> otptoken-totpconfig-(show|mod)
> otptoken-hotpconfig-(show|mod)
>
> Maybe it would be better to introduce more help topics for otp. This
> concept is used for HBAC already:
>
> $ ipa help hbac
> hbacsvcgroup HBAC Service Groups
> hbacsvc HBAC Services
> hbacrule Host-based access control
>
> $ ipa help hbacrule
> Host-based access control
> ... a lot of text
>
> So we could introduce otp umbrella topic:
>
> $ ipa help otp
> opttoken OTP tokens'
> totpconfig TOTP configuration options
> hotpconfig HOTP configuration options
>
>
>>>
>>> Nathaniel
>>
>> No worries ATM, you can wait for proper review. I was just looking at
>> the new
>> API to make sure we are on the same page - we seem to mostly are.
>>
>> Martin
>>
>
> Commenting just patch 0004:
>
> 1. Requires rebase because of API change.
>
> 2. git diff HEAD~4 -U0 | pep8 --diff
> I would ignore E124 and fix E302 (5x)
>
> I did not test actual functionality yet.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141125/eae31a84/attachment.htm>
More information about the Freeipa-devel
mailing list