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

Wed Dec 3 13:43:54 UTC 2014

On 12/02/2014 08:57 PM, Nathaniel McCallum wrote:
> On Tue, 2014-11-18 at 20:26 +0100, 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)?
> 
> Patches 0001, 0002, 0003 are ACKed by Thierry, but not merged. They can
> and should be merged as they fix an independent bug.
> 
>>>>> 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)
> 
> In the latest patch, I merged totpconfig-* and hotpconfig-* into a
> single otpconfig-* plugin.
> 
>> 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
> 
> I added a fifth patch (0005) which creates an otp umbrella topic. We can
> merge it or not.
> 
>>>> 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.
> 
> Fixed.
> 
>> 2. git diff HEAD~4 -U0 | pep8 --diff
>> I would ignore E124 and fix E302 (5x)
> 
> Fixed.
> 
>> I did not test actual functionality yet.
> 
> The attached patches I think have a much better overall aesthetic. Now
> patch 0004 introduces only two new commands:
> * otpconfig-mod
> * otpconfig-show
> 
> Under the covers, a single configuration entity is used:
> * cn=otp,cn=etc,$SUFFIX
> 
> Other than these small changes, there are no changes to patch 0004. I
> have not tested the latest changes, however, due to an unrelated build
> issue I'm working on.
> 
> Patch 0005 introduces an umbrella help topic for all OTP related
> commands (currently: otpconfig, otptoken, otptoken-yubikey).
> 
> Nathaniel
> 

Patches 0001-0003 pushed to master, ipa-4-1.

Was the typo you found yesterday ("I found a small typo in patch 0002. I will
attach it in reply to Petr's review email") fixed in this version?

Martin