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

Wed Dec 3 14:32:56 UTC 2014

On Wed, 2014-12-03 at 14:43 +0100, Martin Kosek wrote:
> On 12/02/2014 08:57 PM, Nathaniel McCallum wrote:
> > 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
> > 
> 
> Patches 0001-0003 pushed to master, ipa-4-1.
> 
> Was the typo you found yesterday ("I found a small typo in patch 0002. I will
> attach it in reply to Petr's review email") fixed in this version?

Yes.