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