[Freeipa-devel] Final OTP Review

[Petr Viktorin]

On 05/17/2013 09:42 AM, Martin Kosek wrote:
> On 05/14/2013 07:12 PM, Martin Kosek wrote:
>> 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.
>>
>> Martin
>
> We have all the acks now (some went off-list). Pushed to master, ipa-3-2.
>
> Thanks!
> Martin
>

Just a note: With these patches, master no longer builds on Fedora 18. 
(no krb5-devel 1.11)

-- 
Petr³