[Freeipa-devel] [PATCH 0074] Make token window sizes configurable
Nathaniel McCallum
npmccallum at redhat.com
Tue Dec 2 19:47:22 UTC 2014
On Tue, 2014-12-02 at 12:20 -0500, Nathaniel McCallum wrote:
> 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.
I found a small typo in patch 0002. I will attach it in reply to Petr's
review email.
Nathaniel
More information about the Freeipa-devel
mailing list