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

Tue Dec 2 16:39:33 UTC 2014

On 12/02/2014 05:24 PM, Nathaniel McCallum wrote:
> On Tue, 2014-12-02 at 17:12 +0100, Martin Kosek wrote:
>> 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.
> Python code is confined to patch 0004, so I think we just need Thierry's
> ACK on 0001-0003.

Nathaniel,

Thanks for all your explanations. ACK for the pacthes 0001-0002-0003.

regarding the DS plugin part of 0004, the patch is good to  me. For the 
ipa plugins part I am too novice.

thanks
thierry
>> By the fourth part you mean patch 0004, right? It somehow ended as second in my
>> MUA.
> Yes. This patch 0004 is basically the rework of patch 0074 which is the
> main topic of this thread. But doing it properly meant sharing code
> (0001 and 0002) with another bugfix (0003). Thus, if we merge 0001-0003
> we're back to just evaluating 0004/0074.
>
> Nathaniel
>
>