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

Tue Dec 2 16:12:12 UTC 2014

On 12/02/2014 04:56 PM, Nathaniel McCallum wrote:
> On Tue, 2014-12-02 at 14:05 +0100, thierry bordaz 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
>>>
>>>
>>> _______________________________________________
>>> Freeipa-devel mailing list
>>> Freeipa-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>
>> Hello Nathaniel,
>>
>>         Very few comments.
> 
> Just as a reminder, patch 0001 is already ACKed.
> 
>>         On patch 0002:
>>         
>>         Is it possible that we later define a spec with 'dflt'
>>         contains OTP_CONFIG_AUTH_TYPE_DISABLED ? If yes it needs to be
>>         32bits.
> 
> Fixed. It was just a typo.
> 
>>         When otp_config_fini is it called ?
> 
> Sadly, never. I admit that I am cargo-culting the lack of calling
> otp_config_fini(). Surely there must be a way to sanely tear this down
> when 389 shuts down?
> 
>>         On patch 0003:
>>         
>>         In ipa-otp-lasttoken:58 you may use SLAPI_ATTR_OBJECTCLASS
>>         (slapi-plugin.h).
> 
> Fixed.
> 
>>         In ipa-otp-lasttoken:preop_mod , the test is_allowed is done
>>         on the original entry (SLAPI_ENTRY_PRE_OP). That is the entry
>>         untouched by others BE_PREOP/TXN_BE_PREOP plugins. Is that the
>>         entry you want to check ?
> 
> Yes, the code is correct as written. We check to see if a change to the
> existing state would cause bad behavior. Then, if any such change is
> attempted (ipa_otp_lasttoken.c:205) we reject it. In the future we might
> improve this to be more granular regarding the values of the change. But
> for now it is good enough.
> 
>>         On patch 0004:
>>         In otp_config.c:otp_config_window you may use
>>         SLAPI_ATTR_OBJECTCLASS (slapi-plugin.h)
> 
> Fixed.
> 
>>         in otp_token: bvtod if 'code' contains non digit
>>         character ,'out' is not reset before return.
> 
> Yes. If the input value is invalid, the function returns an error status
> and the state of the output is undefined. This is pretty normal
> behavior.
> 
> I think the first three patches are ready for merge. The last patch
> still needs some polish on my part. However, the first three also fix an
> important, independent bug. So let's merge them as soon as you feel they
> are ready.
> 
> Nathaniel

If the Python parts are also OK and acked by Petr Vobornik or anyone else then
sure, we can merge them.

By the fourth part you mean patch 0004, right? It somehow ended as second in my
MUA.

Martin