On 06/09/2014 08:21 AM, Martin Kosek wrote:
On 06/06/2014 05:47 PM, Nathaniel McCallum wrote:
On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote:
On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote:
On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote:
On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote:
On 05/31/2014 03:27 AM, Simo Sorce wrote:
I have rebased theold patch attached to the ticket, unfortunately I
haven't had time to test it yet, but didn't want to lose it in some
branch.
Simo.
I tested the patch and it worked fine, code also reads OK. Thus, I am willing
to ACK it.
I am just wondering if there is any scenario we could have missed, but I did
not find any. In there is no push back against the patch I may just push it.
The only thing I would draw attention to is the fact that now I am
sending back the error directly once we have a negative return from the
function in which expiration is checked (ipapwd_authenticate).
I could not see why we did, in fact, not do that before and I meant
asking Nathaniel if he had an explicit reason why we do not, as he is
the last one that did some significant refactoring in the bind preop
plugin.
In the current design, if ipapwd_authenticate() fails, we defer to 389ds
for the actual response to the client. The purpose for this is so that
verification of the first factor always behaves the same, regardless of
what is in pre-bind.
So ipapwd_authenticate() is not actually the "final" authentication. It
is a preliminary authentication to determine if the user should be able
to write kerberos keys and perform OTP sync. So checking expiration in
pre-bind only protects these two operations.
I presume that 389ds also checks password expiration. If this assumption
is true, ipapwd_authenticate() SHOULD NOT return any response to the
client. It should simply fail key-write/otp-sync silently and let dirsrv
return the error to the client.
389ds would check it if we were using 389ds native password policies but
we are not. So we need to check on our own, which is what this patch
implements.
The important point here is that so long as 389ds is verifying password
expiration, using a correct-but-expired password will should not result
in a bind in the current code. It will result in kerberos key writing
and OTP sync. Do we actually care about protecting kerberos key writing
and OTP sync from correct-but-expired passwords?
No, but that's not the point.
If 389ds does not check password expiration, then we probably need to
patch upstream 389ds or, at the very least, return an error to the
client.
It is not a 389ds bug, it is just an integration issue due to the fact
IPA uses a different schema for password policies (for compatibility
with the Kerberos schema).
Otherwise, if we don't care about protecting kerberos key writing and
OTP sync from correct-but-expired passwords, this patch is entirely
unnecessary.
Otherwise, the patch is necessary, but should skip kerberos key writing
and OTP sync and fall through silently to 389ds.
If we fall through to 389ds then authentication will succeed.
So from this discussion it seem to me we achieve the goal of the patch
and returning an error directly is ok.
Unless Nathaniel has something *against* returning an error in this
place I think we are good to go.
Looks good to me. ACK.
Nathaniel
Ok, thanks for discussion and double checking!
Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6
Hello,
This patch broke some of our tests.
ipatests.test_ipaserver.test_changepw:test_changepw.test_invalid_auth
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_1_bind_as_test_user
ipatests.test_xmlrpc.test_user_plugin:test_denied_bind_with_expired_principal.test_3_bind_as_renewed_test_user
fail with: INVALID_CREDENTIALS: {'info': 'The user password is expired',
'desc': 'Invalid credentials'}
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_error
fails with: AssertionError: 'invalid-password' != 'policy-error'
ipatests.test_ipaserver.test_changepw:test_changepw.test_pwpolicy_success
fails with: AssertionError: 'invalid-password' != 'ok'
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
