[Freeipa-devel] [PATCH 0074] Make token window sizes configurable
Nathaniel McCallum
npmccallum at redhat.com
Tue Dec 2 17:20:24 UTC 2014
On Tue, 2014-12-02 at 17:56 +0100, Petr Vobornik wrote:
> 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
Correct. Please merge 0001-0003 as they are ACKed, but NOT 0004. I will
have an alternative proposal for 0004 shortly.
Nathaniel
More information about the Freeipa-devel
mailing list