[Freeipa-devel] [PATCH 0074] Make token window sizes configurable
Petr Vobornik
pvoborni at redhat.com
Tue Dec 2 16:56:48 UTC 2014
On 12/02/2014 05:39 PM, thierry bordaz wrote:
> 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.
Just a remainder: the python part of patch 0004 is still being
discussed:
http://www.redhat.com/archives/freeipa-devel/2014-November/msg00295.html
>
> 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
>>
--
Petr Vobornik
More information about the Freeipa-devel
mailing list