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

Tue Dec 2 19:57:57 UTC 2014

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Preliminary-refactoring-of-libotp-files.patch
Type: text/x-patch
Size: 24175 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141202/5efbd906/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Move-authentication-configuration-cache-into-libotp.patch
Type: text/x-patch
Size: 38471 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141202/5efbd906/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Enable-last-token-deletion-when-password-auth-type-i.patch
Type: text/x-patch
Size: 10633 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141202/5efbd906/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Make-token-auth-and-sync-windows-configurable.patch
Type: text/x-patch
Size: 33048 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141202/5efbd906/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Create-an-OTP-help-topic.patch
Type: text/x-patch
Size: 1801 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141202/5efbd906/attachment-0004.bin>