Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow user to change expired password #114

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sfieux
Copy link

@sfieux sfieux commented Jun 7, 2017

Hi,

I really needed to solve the issue #96 so I did it myself. Here is the pull request, feel free to add it to master if you feel like it's a good idea.

I tested the following cases with OpenLDAP:

  • it allows users with an expired password to change their password anyway
  • it remembers a wrong password when the old one is incorrect, adding a "pwdFailureTime" attribute
  • it won't change a password if the user is locked out

It might require additional testing with Active Directory for regression testing, and maybe with several other LDAP providers. Also, I did the realtime testing with SSHA-hashed passwords, only the unit test hashes the password with all other possible encryptions.

Thanks @coudot for your help on this one

@coudot
Copy link
Member

coudot commented Jun 7, 2017

Thanks a lot for you contribution.

I am a little confused about how you check if the password is expired. It may indeed work as if the bind is rejected but passwords are the same it should mean that the password is expired. But as you check the pwdAccountLockedTime, you could also check the pwdChangedTime and compute the expiration date.

Anyway, I will think about this and see how to include it in SSP.

@plewin
Copy link
Member

plewin commented Jun 7, 2017

Nice code.

Some remarks :

  1. These 40 lines
    18ce340#diff-9ce97ef165e01124464cb1f193dc6c01R292
    18ce340#diff-9ce97ef165e01124464cb1f193dc6c01R331

    unless I missed something, should be simplifiable by

    $old_password_hash = make_crypt_password_with_salt($old_password, $ldap_password);

  2. Avoid leaving commented out code. Is this part complete ?
    18ce340#diff-4431fdf50cf2590bd15b04fefd137751R170

  3. Are you sure you are using ldap_get_values correctly ?
    18ce340#diff-4431fdf50cf2590bd15b04fefd137751R174
    18ce340#diff-4431fdf50cf2590bd15b04fefd137751R180
    According to the phpdoc, ldap_get_values returns array or false. doesn't these isset always return true ?

  4. I believe a substr would be better
    18ce340#diff-9ce97ef165e01124464cb1f193dc6c01R272

  5. hash_equals is >= PHP 5.6, I'm unsure if it can be accepted. target php versions of SSP is not clearly documented

@sfieux
Copy link
Author

sfieux commented Jun 7, 2017

Thank you @coudot for your remarks.

The way the PR works is:

  • Try to bind as the user first (previous behaviour): this 1/ ensures that the password is fully checked by the LDAP provider, 2/ adds a pwdFailureTime attribute if the provided password is incorrect 3/ if the password was locked a long time ago and the pwdLockoutDuration has passed, a correct bind also resets the obsolete pwdAccountLockedTime attribute.
  • If the bind failed because of the password itself (error 49), and all other configuration conditions are met (change allowed, change will done by the administrator):
    • check for pwdAccountLockedTime attribute: this one is set by OpenLDAP when too many guesses have been tried. If this attribute is set, refuse the change
    • Manually compare the old password provided by the user, and the hashed+salted password read from the LDAP provider: use the LDAP pass to compute the value of the salt that was used, hash the user password with this salt and compare hashed values
    • if hashed values match, our user provided a correct login but was unable to log in: allow password change

This covers all the OpenLDAP ppolicy possibilities to lock a user:

  • user that is not locked: will pass the bind step successfully, change allowed
  • user with an expired password (that's when current date > pwdChangedTime + ppolicy pwdMaxAge attribute): bind fails, password checked by SSP, change allowed
  • user whose password explicitly needs to be changed (pwdReset set to TRUE): bind fails, password checked by SSP, change allowed, pwdReset is deleted after a successful change
  • user who was previously locked out but ppolicy pwdLockoutDuration has elapsed since then: will pass the bind step successfully (deleting the pwdLockoutDuration attribute in the process), change allowed
  • user still locked out: bind fails, SSP notices the pwdAccountLockedTime, change denied
  • user providing a wrong password: bind fails and adds a pwdFailureTime (eventually leading to pwdAccountLockedTime if too many attempts), password checked by SSP, change denied

Computing the expiration date is actually extremely difficult: it requires the pwdAccountLockedTime but also the ppolicy pwdMaxAge attribute, and this is quite hard to get to: there might be several policies depending of the user (real person VS service account...), and the policy used for a particular user is only referenced in pwdPolicySubentry if it's not the default one (how to guess which one is the default? I truly have no idea). Additionally, it doesn't cover the case when a user explicitly needs to change its password - for instance, when an administrator just created the account with a phony password and the user needs to change it before being able to log in.

@coudot
Copy link
Member

coudot commented Jun 7, 2017

Computing the expiration date is actually extremely difficult: it requires the pwdAccountLockedTime but also the ppolicy pwdMaxAge attribute, and this is quite hard to get to: there might be several policies depending of the user (real person VS service account...), and the policy used for a particular user is only referenced in pwdPolicySubentry if it's not the default one (how to guess which one is the default? I truly have no idea).

Indeed, if the pwdPolicySubentry is defined, you can get pwdMaxAge here, if not, we can have a configuration parameter to set default password max age. Its value will be the one defined in default ppolicy in LDAP server, or no value if no ppolicy (but in this case the password will not be expired).

Additionally, it doesn't cover the case when a user explicitly needs to change its password - for instance, when an administrator just created the account with a phony password and the user needs to change it before being able to log in.

I don't see the point here, what the link with the expired password?

@coudot
Copy link
Member

coudot commented Jun 7, 2017

hash_equals is >= PHP 5.6, I'm unsure if it can be accepted. target php versions of SSP is not clearly documented

We have unit test running with PHP 5.4, so for the moment PHP 5.4 is the minimal version. But it is acceptable to have not compatible code if it's activated only with some options.

@sfieux
Copy link
Author

sfieux commented Jun 7, 2017

To answer @plewin:

You're mostly right, I'll change the code, do a bit of testing on my side and update the PR later.

  1. True, but with $ldap_bytes instead of $ldap_password, because $ldap_password still starts with '{CRYPT}'. Wish I had noticed this before coding it all :(

  2. Code is complete but I forgot to clean up the comments. Looking at it, the 'Not tested' and 'Not testable' in comments also needs to be removed, since I finally found ways to unittest them.

  3. Might require a change then... I wasn't sure how to use these functions so I copied/pasted from change_password (line 363 from my PR's functions.inc.php, 264 from master) - wouldn't this also need an update?

  4. I don't really see why but I don't see why not, so I'll change it. It needs to be consistent with similar checks at lines 284 and 293.

  5. Correct. I couldn't find which PHP version is officially supported by SSP but the Travis CI runs the tests with PHP 5.4, 5.5, 5.6, 7.0, 7.1 and hhvm, so there should be an implementation of hash_equals if the function is not defined. (By the way, Travis fails with hhvm but it seems it's more of a Travis issue.)
    I'll add the hash_equals implementation proposed in here: http://php.net/manual/fr/function.hash-equals.php

#hash_equals implementation for older versions of php
if(!is_callable('hash_equals')) {
  function hash_equals($str1, $str2) {
    if(strlen($str1) != strlen($str2)) {
      return false;
    } else {
      $res = $str1 ^ $str2;
      $ret = 0;
      for($i = strlen($res) - 1; $i >= 0; $i--) $ret |= ord($res[$i]);
      return !$ret;
    }
  }
}

@plewin
Copy link
Member

plewin commented Jun 7, 2017

Might require a change then... I wasn't sure how to use these functions so I copied/pasted from change_password (line 363 from my PR's functions.inc.php, 264 from master) - wouldn't this also need an update?

Yes you spotted a useless if that should be removed in an other pr, I checked the code and found no consequences of this useless if (I mean no bugs, just useless code).

I don't really see why but I don't see why not, so I'll change it.

To be honest I'm not sure. I researched what is the best way to do this for an other PR. I find it subjectively more simplier and efficient. It was seeing the substr in fusiondirectory's code that convinced me that this should the best way.

@sfieux
Copy link
Author

sfieux commented Jun 8, 2017

I don't see the point here, what the link with the expired password?

When an admin sets the pwdReset operational attribute for a user, it means that this user won't be able to log in until he changes his password. It's not directly related to expired passwords, but it's another use-case where the user can't log in but should be allowed to change his password anyway. PAM handles this correctly by forcing the user to change his password before granting him access to the shell. I believe SSP should allow it as well, and this PR enable this possibility.

@plewin: I did all the correction you suggested. Travis still not passing with PHP hhvm, it looks more like a Travis configuration issue.

@coudot
Copy link
Member

coudot commented Jul 12, 2017

Hi @sfieux and @plewin, I think some changes in this PR have been done in other issues (like #115). And also travis file should not be modified.

Other suggestion: do not remove the parameter in AD option, but maybe define the parameter before and reuse it after like:

$ldap_change_expired_password = false;
...
$ad_options['change_expired_password'] = $ldap_change_expired_password;

Note also that true or false configuration values must not be set with quotes, these are boolean values.

If you have some time, could you update the PR?

@coudot coudot added this to the Future milestone Aug 30, 2017
@TheAshwanik
Copy link

Hi, I have noticed the same issue.. I am not able to change expired password.
Is this PR not merged in latest code?

@coudot
Copy link
Member

coudot commented Oct 23, 2020

As you can see, this PR was not yet accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants