[Freeipa-devel] Final OTP Review

Martin Kosek mkosek at redhat.com
Tue May 14 17:12:01 UTC 2013

On 05/14/2013 03:53 PM, Nathaniel McCallum wrote:
> On Fri, 2013-05-10 at 16:55 -0400, Nathaniel McCallum wrote:
>> 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?
> While I'm not quite sure what the problem was, I do know it appeared on
> the stock 3.2 F19 RPMs. I also fixed it by accident. I am certain it is
> unrelated to these patches.
> I have now tested install and upgrade with the six patches in the
> previous email and everything is in order, including permissions. At
> this point, we just need reviews/ACKs.
> Nathaniel

I tested IPA server upgrades, new installs and also adding 3.2+OTP replica for
F18 3.1.4 IPA master. Everything seemed to work fine (when I added my patch 407
fixing the replication), I did not see any breakage.

Issues I found with too much logging I reported should now be fixed on github,
so this should be OK.

So it is an ACK from my side if Rob does not discover some blocking issue.


More information about the Freeipa-devel mailing list