Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-10 Thread Martin Kosek 
On 06/09/2014 05:54 PM, Dmitri Pal wrote:
 On 06/09/2014 10:03 AM, Nathaniel McCallum wrote:
 On Mon, 2014-06-09 at 09:01 -0400, Simo Sorce wrote:
 From: Martin Kosek mko...@redhat.com
 Given all sort of issues we get, I am thinking we should just revert it
 unless
 there is a quick fix available.
 Instead of reverting I am thinking we may want to make this optional by
 adding a configuration parameter that defaults to False for now. Once we can
 manage better the password change we can turn it on by deault, in the
 meanwhile admins can choose by themselves the lesser evil.

 Thoughts?
 I'm not a fan of introducing a new configuration parameter for a
 temporary workaround.

 My preference is to revert it and have a small project for the next
 release which handles all the non-authenticated corner cases. This
 would include:
 * Expired passwords
 * Password changes
 * Token syncing
 * Unauthenticated RPCs (rpcserver.py rework)
 * others?

 I think there is some value to be gained by thinking about these
 problems as a whole and devising a set of consistent mechanisms for
 them.

 Nathaniel

 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel
 +1
 

This is my preference as well, as written in other part of this thread.

I reverted the patch, added an appropriate description (attached) and pushed to
master.

I updated the ticket, added Nathaniel's suggestions and moved it to needs 
triage.

Thanks,
Martin
From c41b782bc59cd0cb70cbd62d543f9c538109d410 Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Tue, 10 Jun 2014 08:42:03 +0200
Subject: [PATCH] Revert Check for password expiration in pre-bind

This reverts commit bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6.

Forceful validation of password expiration date in a BIND pre-callback
breaks LDAP password change extended operation as the password change
is only allowed via authenticated (bound) channel. Passwords could be
only changed via kadmin protocol. This change would thus break
LDAP-only clients and Web UI password change hook.

This patch will need to be revisited so that unauthenicated corner
cases are also revisited.

https://fedorahosted.org/freeipa/ticket/1539
---
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 33 +++
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
index 6786c6ddb4de4158ec680e271cae29318bc150ce..23c7cb18c9a1cb5256254f20080c5d9aaec25579 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c
@@ -1217,35 +1217,13 @@ static bool ipapwd_pre_bind_otp(const char *bind_dn, Slapi_Entry *entry,
 }
 
 static int ipapwd_authenticate(const char *dn, Slapi_Entry *entry,
-   const struct berval *credentials,
-   const char **errmsg)
+   const struct berval *credentials)
 {
 Slapi_Value **pwd_values = NULL; /* values of userPassword attribute */
 Slapi_Value *value = NULL;
 Slapi_Attr *attr = NULL;
-struct tm expire_tm;
-char *expire;
-char *p;
 int ret;
 
-/* check the if the krbPrincipalKey attribute is present */
-ret = slapi_entry_attr_find(entry, krbprincipalkey, attr);
-if (!ret) {
-/* check that the password is not expired */
-expire = slapi_entry_attr_get_charptr(entry, krbpasswordexpiration);
-if (expire) {
-memset(expire_tm, 0, sizeof (expire_tm));
-p = strptime(expire, %Y%m%d%H%M%SZ, expire_tm);
-if (*p) {
-LOG(Invalid expiration date string format);
-return 1;
-} else if (time(NULL)  mktime(expire_tm)) {
-*errmsg = The user password is expired;
-return 1;
-}
-}
-}
-
 /* retrieve userPassword attribute */
 ret = slapi_entry_attr_find(entry, SLAPI_USERPWD_ATTR, attr);
 if (ret) {
@@ -1403,7 +1381,7 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
 static const char *attrs_list[] = {
 SLAPI_USERPWD_ATTR, ipaUserAuthType, krbprincipalkey, uid,
 krbprincipalname, objectclass, passwordexpirationtime,
-passwordhistory, krbprincipalexpiration, krbpasswordexpiration,
+passwordhistory, krbprincipalexpiration,
 NULL
 };
 struct berval *credentials = NULL;
@@ -1416,7 +1394,6 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
 time_t expire_time;
 char *principal_expire = NULL;
 struct tm expire_tm;
-const char *errmsg = NULL;
 
 /* get BIND parameters */
 ret |= slapi_pblock_get(pb, SLAPI_BIND_TARGET, dn);
@@ -1477,12 +1454,10 @@ static int ipapwd_pre_bind(Slapi_PBlock *pb)
 }
 
 /* Authenticate the user. */
-ret = ipapwd_authenticate(dn, entry,

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Martin Kosek 
On 06/09/2014 10:59 AM, Martin Kosek wrote:
 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

 Martin
 
 I just read a question in the ticket from alonbl about how password change
 should work on the LDAP level:
 
 https://fedorahosted.org/freeipa/ticket/1539#comment:29
 
 Are we ready to say that from now on, expired passwords cannot be changed via
 LDAP? I.e. this workflow will no longer work:
 
 $ ipa user-add --first=Foo --last Bar expired --random
 
 Added user expired
 
   User login: expired
   First name: Foo
   Last name: Bar
   Full name: Foo Bar
   Display name: Foo Bar
   Initials: FB
   Home directory: /home/expired
   GECOS: Foo Bar
   Login shell: /bin/sh
   Kerberos principal: expi...@mkosek-fedora20.test
   Email address: expi...@mkosek-fedora20.test
   Random password: qiCjofo.2pfT
   UID: 1170600026
   GID: 1170600026
   Password: True
   Member of groups: ipausers
   Kerberos keys available: True
 
 $ ldappasswd -h `hostname` -D
 uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w qiCjofo.2pfT
 uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -a qiCjofo.2pfT -s
 Secret123
 ldap_bind: Invalid credentials (49)
   additional info: The user password is expired
 
 Thanks,
 Martin

As noted in other part of this thread, this also breaks password changes via
Web UI as the hooks use LDAP (and not kadmin) to update passwords.

Given all sort of issues we get, I am thinking we should just revert it unless
there is a quick fix

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Alon Bar-Lev 


- Original Message -
 From: Martin Kosek mko...@redhat.com
 To: Nathaniel McCallum npmccal...@redhat.com, Simo Sorce 
 s...@redhat.com
 Cc: freeipa-devel freeipa-devel@redhat.com
 Sent: Monday, June 9, 2014 1:11:17 PM
 Subject: Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP
 
 On 06/09/2014 10:59 AM, Martin Kosek wrote:
  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
 
  Martin
  
  I just read a question in the ticket from alonbl about how password change
  should work on the LDAP level:
  
  https://fedorahosted.org/freeipa/ticket/1539#comment:29
  
  Are we ready to say that from now on, expired passwords cannot be changed
  via
  LDAP? I.e. this workflow will no longer work:
  
  $ ipa user-add --first=Foo --last Bar expired --random
  
  Added user expired
  
User login: expired
First name: Foo
Last name: Bar
Full name: Foo Bar
Display name: Foo Bar
Initials: FB
Home directory: /home/expired
GECOS: Foo Bar
Login shell: /bin/sh
Kerberos principal: expi...@mkosek-fedora20.test
Email address: expi...@mkosek-fedora20.test
Random password: qiCjofo.2pfT
UID: 1170600026
GID: 1170600026
Password: True
Member of groups: ipausers
Kerberos keys available: True
  
  $ ldappasswd -h `hostname` -D
  uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w
  qiCjofo.2pfT
  uid

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Simo Sorce 
 On 06/09/2014 12:15 PM, Alon Bar-Lev wrote:
  From: Martin Kosek mko...@redhat.com

  Given all sort of issues we get, I am thinking we should just revert it
  unless
  there is a quick fix available.

  The fix should be for the password modify to work within anonymous bind if
  old password is specified. I am not sure why IPA enforces non anonymous
  bind for this extended request.
  
  Applications should also be modified to perform anonymous bind, exactly per
  this reason.
  
  Searching why IPA requires non anonymous bind is what led me to this bug...
  :)
 
 Simo, do you know the historical reason why this is enforced in
 ipapwd_chpwop?

When we started we wanted to allow password changes using GSSAPI for bind 
instead of password based authentication, and we ended up not implementing the 
old-password based one at all...

 By quickly looking at the code it should not be difficult to fix, but devil
 is in details and we need to be very cautious in this function.

We just need to be careful about what operations are done, but indeed it 
shouldn't be difficult, I am just not sure it is quick enough for you.
I can take a look in a few.

Simo.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Simo Sorce 
   From: Martin Kosek mko...@redhat.com
 
   Given all sort of issues we get, I am thinking we should just revert it
   unless
   there is a quick fix available.

Instead of reverting I am thinking we may want to make this optional by adding 
a configuration parameter that defaults to False for now. Once we can manage 
better the password change we can turn it on by deault, in the meanwhile admins 
can choose by themselves the lesser evil.

Thoughts?

Simo.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Dmitri Pal 

On 06/09/2014 09:01 AM, Simo Sorce wrote:

From: Martin Kosek mko...@redhat.com
Given all sort of issues we get, I am thinking we should just revert it
unless
there is a quick fix available.

Instead of reverting I am thinking we may want to make this optional by adding 
a configuration parameter that defaults to False for now. Once we can manage 
better the password change we can turn it on by deault, in the meanwhile admins 
can choose by themselves the lesser evil.

Thoughts?

Simo.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


I am also concerned about the OTP flows with this change.
IMO we might not be ready for this change one way or another.
Backing out or adding a default switch turning the feature off works for me.

--
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Martin Kosek 
On 06/09/2014 03:08 PM, Dmitri Pal wrote:
 On 06/09/2014 09:01 AM, Simo Sorce wrote:
 From: Martin Kosek mko...@redhat.com
 Given all sort of issues we get, I am thinking we should just revert it
 unless
 there is a quick fix available.
 Instead of reverting I am thinking we may want to make this optional by
 adding a configuration parameter that defaults to False for now. Once we can
 manage better the password change we can turn it on by deault, in the
 meanwhile admins can choose by themselves the lesser evil.

 Thoughts?

 Simo.

 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel
 
 I am also concerned about the OTP flows with this change.
 IMO we might not be ready for this change one way or another.
 Backing out or adding a default switch turning the feature off works for me.

I do not like the proposal very much. It sounds like oops, this breaks
FreeIPA, let's hide it with configuration option and fix later.

This would not be a simple fix, we know that Web UI and possibly other
workflows are broken unless we introduce password changes via anonymous binds
(and thus utilize oldPassword piece). We would also need to make sure the
setting is not read with every LDAP bind, otherwise it would also have some
performance impact, our BINDs are already slow (see
https://fedorahosted.org/freeipa/ticket/3892).

If this can be indeed fixed, let us do it before 4.0 Beta (we are talking about
2 weeks of time in ideal scenario) or revert until we are ready.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP

2014-06-09 Thread Dmitri Pal 

On 06/09/2014 10:03 AM, Nathaniel McCallum wrote:

On Mon, 2014-06-09 at 09:01 -0400, Simo Sorce wrote:

From: Martin Kosek mko...@redhat.com
Given all sort of issues we get, I am thinking we should just revert it
unless
there is a quick fix available.

Instead of reverting I am thinking we may want to make this optional by adding 
a configuration parameter that defaults to False for now. Once we can manage 
better the password change we can turn it on by deault, in the meanwhile admins 
can choose by themselves the lesser evil.

Thoughts?

I'm not a fan of introducing a new configuration parameter for a
temporary workaround.

My preference is to revert it and have a small project for the next
release which handles all the non-authenticated corner cases. This
would include:
* Expired passwords
* Password changes
* Token syncing
* Unauthenticated RPCs (rpcserver.py rework)
* others?

I think there is some value to be gained by thinking about these
problems as a whole and devising a set of consistent mechanisms for
them.

Nathaniel

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

+1

--
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel