[Freeipa-devel] Final OTP Review

[Nathan Kinder]

On 05/03/2013 09:14 AM, Simo Sorce wrote:
> On Fri, 2013-05-03 at 12:00 -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.
>> +1. Can we do this after 3.2? This is an experimental feature after
>> all...
> You must assure ipa-pwd-extop is always invoked in all bind cases. I am
> not welded on how you do it.
> However *merging* plugins later will be messy as you have to deal with
> configuration changes as the plugin .so will disappear, so I would
> rather do it now.
I think that they must be merged together if you want things to work 
properly.  You can set the precedence to ensure that ipa-pwd-extop 
always runs first, but I don't think it will work properly if one is 
trying to do an OTP auth.  The ipa-pwd-extop pre-bind callback is 
responsible for creating kerberos keys based off of the supplied 
credentials.  It will only do this if the credentials sent by the client 
match one of the userPassword values.  With an OTP auth, this will never 
match since the token code is combined with the password.  The 
ipa-pwd-extop plug-in will just return 0 and the OTP plug-in will be 
called and processed as normal.  This is OK, but it does mean that an 
OTP auth will not trigger generation of new Kerberos keys.

The ideal scenario would be to combind the plug-ins so that 
ipa-pwd-extop could recognize an OTP authentication attempt, split the 
supplied credentials into the password and token code, then use the 
extracted password to generate Kerberos keys if needed.

-NGK
>
> Simo.
>