[Freeipa-devel] Final OTP Review

Nathaniel McCallum npmccallum at redhat.com
Fri May 10 20:55:49 UTC 2013


On Fri, 2013-05-03 at 09:08 -0400, Simo Sorce wrote:
> On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote:
> > On 05/02/2013 10:27 PM, Nathaniel McCallum wrote:
> > > All issues fixed unless noted below... The attached patches are tested
> > > to work.
> > >
> > > On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote:
> > >
> > >> - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
> > >> (although I know slapi_ch_malloc() currently just aborts on failure, I
> > >> think that is plain wrong which is why I would prefer using
> > >> malloc/strdup, but well, I guess this is not something I am going to
> > >> care too much about for now).
> > > Not fixed.
> > >
> > >> - Is the logic with auth_type_used correct ?
> > >> At least the name of the variable sounds misleading, because you set it
> > >> only if the authentication was successful but not set if it was 'used'
> > >> but was unsuccessful. Made me look at it multiple times to reconstuct
> > >> the logic. The var name could be better, however I also want a comment
> > >> that explain exactly how we are going to manage authentication and
> > >> fallbacks here as that logic is critical and is useful for anyone that
> > >> is going to have to change this code later in order not to break it.
> > > The variable is now gone. I re-factored the section to make the logic
> > > clearer and put a nice big comment up top.
> > >
> > >> - General question: how does this PRE_BIND plugin interact with
> > >> ipapwd_pre_bind() in the ipa-pwd-extop() plugin ?
> > >> Are you going to cause that plugin not to run by returning a result
> > >> directly in this function ?
> > >> Or is this plugin configured to run only after the previous one went
> > >> through ?
> > >> I ask because I do not see any ordering information in the cn=config
> > >> plugin configuration so it is not immediately clear to me.
> > > That is a good question for Nathan since he wrote this part of the
> > > code...
> > We would need to set the precedence if you want a predictable/guaranteed 
> > execution order.  If a pre-BIND plug-in callback returns non-zero (which 
> > you should do when the plug-in sends the result to the client directly), 
> > it will cause other pre-bind plug-ins to not be called.  This is 
> > actually how all pre-op plug-ins work.  If a pre-op callback returns an 
> > error, we don't call the rest of the pre-op plug-ins in the list.
> 
> Ok, but this does not answer my question.
> We definitely need to *always* run our other preop plugin as we do
> sanity checks like verifying if the user is enabled/disabled etc...
> Also we need to understand how to deal with migrating password auth when
> OTP is enabeld.
> 
> TBH I think we should not have a separate OTP-auth plugin but we should
> probably have a single plugin that handles authentication and the 2
> should be merged. Keeping them separate is going to cause more harm than
> good with unexpected interactions.
> 
> We could still have 2 plugins and simply move the prebind action
> currently don in ipa-pwd-extop to the otp plugin by making some more
> code common. But it is probably easier to just merge OTP into
> ipa-pwd-extop right now than try to do a huge refactoring. We can always
> refactor the ipa-pwd-extop plugin later.

The attached patches encompass an initial review of the companion daemon
and merge of ipa-otp into ipa-pwd-extop. Unfortunately, merging ipa-otp
into ipa-pwd-extop appears to have broken something during install, but
I don't have enough familiarity with 389 to understand what I've broken.
If I upgrade after an install, it appears to work.

An RPM with the patches is available here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=5362935

Nathan / Rob / Simo, could you take a look and see what might be broken
in ipa-pwd-extop?

Nathaniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Add-OTP-support-to-ipa-pwd-extop.patch
Type: text/x-patch
Size: 61146 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Remove-unnecessary-prefixes-from-ipa-pwd-extop-files.patch
Type: text/x-patch
Size: 185438 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-the-krb5-FreeIPA-RADIUS-companion-daemon.patch
Type: text/x-patch
Size: 62453 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-ipa-kdb-Add-OTP-support.patch
Type: text/x-patch
Size: 6525 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-IPA-OTP-schema-and-ACLs.patch
Type: text/x-patch
Size: 22589 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ipaUserAuthType-and-ipaUserAuthTypeClass.patch
Type: text/x-patch
Size: 4215 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130510/e0b9d515/attachment-0005.bin>


More information about the Freeipa-devel mailing list