[Freeipa-devel] [PATCH 0074] Make token window sizes configurable

Tue Nov 18 19:26:30 UTC 2014

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)?

>>>>
>>>
>>> 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.
-- 
Petr Vobornik