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.
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
