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

[Nathaniel McCallum]

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