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

[Martin Kosek]

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