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

Handle password expiry in user_ldap #1023

Merged
merged 8 commits into from
Apr 24, 2017

Conversation

GitHubUser4234
Copy link
Contributor

@GitHubUser4234 GitHubUser4234 commented Aug 23, 2016

This is based on #600, it has been discussed at #25114 and covers the "Password Policies" part that is described there.

With the fact in mind that PHP doesn't support ldappasswd, users with expired passwords wouldn't be able to reset their passwords by themselves through ldapmodify. In general, Super Admins and Group Admins will have to reset the password of their users which can be done once #600 is merged. It would sure be great to still have an additional option for users to renew their expired passwords without bothering the Admins. There is a mechanism available, namely grace authentication, which determines whether and how often a user is allowed to log in after a password has expired.

An implementation in user_ldap would look like this:

Add a logic in the checkPassword() method to check after a successful bind whether the pwdGraceUseTime attribute is holding values (indicating the grace logins after password expiry).

If pwdGraceUseTime doesn't exist or doesn't contain a value, proceed as usual. And if (pwdChangedTime + pwdMaxAge - current time) <= pwdExpireWarning, show a password expiry warning on the page to which the LoginController is going to redirect (using NotificationManager, right?).

If pwdGraceUseTime contains a number of values lower than the value indicated by the pwdGraceAuthNLimit attribute then redirect to a password renewal page where the user can renew his password. After successful renewal, the user is redirected to the default login page where he is prompted to login with his new password.

Otherwise (meaning pwdGraceUseTime contains a number of values >= than the value indicated by the pwdGraceAuthNLimit attribute) there is no more grace login available and the method returns false.

Grace logins are supported by OpenLDAP, but not by Active Directory. This won't have an impact on Active Directory users as the pwdGraceUseTime attribute doesn't exist there.

@blizzz: As usual, your opinion on the above is highly recommended 😸

Code "should" be ready by Tuesday (30.08.).

use OCP\AppFramework\IAppContainer;

class Application extends App {
public function __construct (array $urlParams = array()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the argument, it's never needed these days

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint, done.

@GitHubUser4234
Copy link
Contributor Author

@blizzz: An additional thing to consider is whether and how to make the password policy dn configurable. Currently I'm just using $ppolicyDN = 'cn=default,ou=policies,'.$this->access->connection->getConfiguration()['ldap_base'];, not sure whether this is fine though 😋

@GitHubUser4234 GitHubUser4234 force-pushed the ldap_password_renew_pr branch 2 times, most recently from 7080212 to 81af16b Compare August 30, 2016 10:02
@GitHubUser4234
Copy link
Contributor Author

GitHubUser4234 commented Aug 31, 2016

As discussed with @blizzz , the password policy DN will be configurable in the advanced directory settings.

If the password policy DN is not configured, then password expiry handling is disabled completely. This way, we can safely erase the impact on users who don't require this feature.

Moreover, the logic to handle password expiry will be moved from the checkPassword() method to a post_login hook in User/User.php. In this manner we can avoid an infinite redirect which would be caused by the fact that the RenewPasswordController has to recall the checkPassword() method during password renewal.

@GitHubUser4234
Copy link
Contributor Author

There is a framework issue #1303 on which this PR depends on. Once it is fixed, the code can be finalized.

The documentation for this PR is available also here (search for "Default password policy DN").

@GitHubUser4234 GitHubUser4234 force-pushed the ldap_password_renew_pr branch 2 times, most recently from f86f6ae to de52fcc Compare October 6, 2016 11:05
@MorrisJobke MorrisJobke mentioned this pull request Oct 24, 2016
47 tasks
@nickvergessen nickvergessen mentioned this pull request Dec 14, 2016
59 tasks
@rullzer rullzer added this to the Nextcloud 12.0 milestone Dec 16, 2016
@GitHubUser4234 GitHubUser4234 deleted the ldap_password_renew_pr branch January 10, 2017 10:22
@GitHubUser4234 GitHubUser4234 restored the ldap_password_renew_pr branch January 10, 2017 10:26
@KB7777
Copy link
Contributor

KB7777 commented Mar 13, 2017

Grace logins are supported by OpenLDAP, but not by Active Directory.

Is it possible to implement it for AD by counting it?

PasswordExpirationDate
The password expiration date is not an attribute on the user object. It is a calculated value based on the sum of pwdLastSet for the user and maxPwdAge of the user's domain.

https://msdn.microsoft.com/en-us/library/ms677943(v=vs.85).aspx

It will be very helpfull for AD users to have it :)

End what about notify at desktop client?

@GitHubUser4234
Copy link
Contributor Author

GitHubUser4234 commented Mar 13, 2017

@KB7777 The sentence about grace logins actually means that with AD, it is not possible to prompt users to renew their password in case it has already expired. However regarding notifications about passwords due to expire soon, AD has yet another obstacle which is that there is no pwdExpireWarning attribute or equivalent available. It could be gotten around by shifting the configuration of this password policy rule to Nextcloud for the AD case, but as this isn't a clear-cut solution and would also complicate things, it's not clear whether this is acceptable (a discussion with @blizzz might be needed) and it would have to be done in a separate PR which could be based on this one.

Notifications are issued using the Nextcloud's Notification API, so I would assume that if the desktop client checks for new notifications, the password expiry alerts should show up there, too?

@blizzz
Copy link
Member

blizzz commented Mar 17, 2017

@KB7777 The sentence about grace logins actually means that with AD, it is not possible to prompt users to renew their password in case it has already expired. However regarding notifications about passwords due to expire soon, AD has yet another obstacle which is that there is no pwdExpireWarning attribute or equivalent available. It could be gotten around by shifting the configuration of this password policy rule to Nextcloud for the AD case, but as this isn't a clear-cut solution and would also complicate things, it's not clear whether this is acceptable (a discussion with @blizzz might be needed) and it would have to be done in a separate PR which could be based on this one.

Sounds like a heavy-weight workaround, and likely unreliable. Then, I would leave it out for now, as it should be reported by the server.

Notifications are issued using the Nextcloud's Notification API, so I would assume that if the desktop client checks for new notifications, the password expiry alerts should show up there, too?

Correct

@GitHubUser4234
Copy link
Contributor Author

For the record, the code has been further enhanced by handling the pwdReset attribute. If this attribute is TRUE, the renew password screen will be triggered upon login.

pwdReset
This attribute may be used by the administrator to reset the account. If TRUE the user must change their password prior to the next bind attempt.

Making use of the pwdReset attribute can be useful for example to force new users to change their initial password at first login.

@rullzer
Copy link
Member

rullzer commented Mar 30, 2017

#3832 is in. @GitHubUser4234 you can continue (finally) 😄

@GitHubUser4234
Copy link
Contributor Author

Great that the waiting had an end after all, big thanks @rullzer :)

Hey @blizzz, glad if you could take a peek at this thingie :D

Signed-off-by: Roger Szabo <roger.szabo@web.de>
@GitHubUser4234 GitHubUser4234 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 31, 2017
Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
@blizzz
Copy link
Member

blizzz commented Mar 31, 2017

Hey @blizzz, glad if you could take a peek at this thingie :D

Of course! I see there are commits incoming, just ping me when you're done :)

@GitHubUser4234
Copy link
Contributor Author

Hey @blizzz, glad if you could take a peek at this thingie :D
Of course! I see there are commits incoming, just ping me when you're done :)

:))) Oh yeah, the commits were adjustments due to the utterly outdated code base, but now continuous-integration/drone/pr and Scrutinizer look somewhat happy again, that should be all for now ✌️

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, I'd only like to talk about handlePasswordExpiry() (see comment). Other comments are just formal and no blockers.

@@ -21,7 +21,6 @@
*
*/

/** @var $this \OCP\Route\IRouter */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it removed?

$c->query('UserManager'),
$server->getConfig(),
$c->query('OCP\IL10N'),
//$c->query('Session'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this line can go

* @param IURLGenerator $urlGenerator
*/
function __construct($appName,
IRequest $request,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation seems a bit off

if($this->config->getUserValue($user, 'user_ldap', 'needsPasswordReset') !== 'true') {
return new RedirectResponse($this->urlGenerator->linkToRouteAbsolute('core.login.showLoginForm'));
}
$parameters = array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use the short notation here: $parameters = []; as you did on other places.

&& $pwdExpireWarning && (count($pwdExpireWarning) > 0)) {
$pwdMaxAgeInt = intval($pwdMaxAge[0]);
$pwdExpireWarningInt = intval($pwdExpireWarning[0]);
//pwdMaxAge=0 -> password never expires
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this and the following comment go?

*
* @param array $params
*/
public function handlePasswordExpiry($params) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have several readAttributes in this method, this all costs. A web login does not happen so often, but what about logins via Clients (btw, did you test what happens when the password expired while you connected to Nextcloud with a client?). Perhaps some values can be loaded during login (see Manager::getAttributes() and User::processAttributes) and/or cached, if it makes sense.

@GitHubUser4234
Copy link
Contributor Author

GitHubUser4234 commented Apr 3, 2017

Thanks @blizzz for the comments :)

I'll solve all the formal issues in one go, once we have pinpointed a solution for the handlePasswordExpiry() comment.

[...]btw, did you test what happens when the password expired while you connected to Nextcloud with a client?[...]

Yes, e.g. the Nextcloud desktop client continues to work normally when the password expires (cookies?), however when a logout and login is performed after expiry, the sync indicator doesn't become green again which is somewhat expected.

We have several readAttributes in this method, this all costs. A web login does not happen so often, but what about logins via Clients (...). Perhaps some values can be loaded during login (see Manager::getAttributes() and User::processAttributes) and/or cached, if it makes sense.

Well, I guess logins for a client only happen often when it doesn't use cookies and therefore triggers a login for each request?

Alright, here is a list of all the 7 attributes in question:

Global attributes:

pwdGraceAuthNLimit
pwdMaxAge
pwdExpireWarning

General user attribute:
pwdPolicySubentry

Transient user attributes:

pwdGraceUseTime
pwdReset
pwdChangedTime

While transient user attributes and the general user attribute could be loaded during login, the global attributes could be cached. What would be the appropriate method that could be used for caching the global attributes?

But then there's another thing to consider: The whole "handle password expiry" logic currently resides in a code block which is surrounded by a check whether this feature is actually enabled. When we put the attributes used by this feature into several other locations, it would at least not be as clear cut as it is now and code is called even with the feature not supported/enabled. Do performance benefits outweigh this?

Thanks a lot 🐱

@blizzz
Copy link
Member

blizzz commented Apr 3, 2017

[...]btw, did you test what happens when the password expired while you connected to Nextcloud with a client?[...]

Yes, e.g. the Nextcloud desktop client continues to work normally when the password expires (cookies?), however when a logout and login is performed after expiry, the sync indicator doesn't become green again which is somewhat expected.

Yes, session cookies. I assume the client periodically tries to re-login in the latter case. Is this a problem?

We have several readAttributes in this method, this all costs. A web login does not happen so often, but what about logins via Clients (...). Perhaps some values can be loaded during login (see Manager::getAttributes() and User::processAttributes) and/or cached, if it makes sense.

Well, I guess logins for a client only happen often when it doesn't use cookies and therefore triggers a login for each request?

Exactly.

While transient user attributes and the general user attribute could be loaded during login, caching the global attributes seems to make sense. What would be the appropriate method that could be used for caching the global attributes?

The connection instance has a cache object, cf. https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L315 and hhttps://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L320

But then there's another thing to consider: The whole "handle password expiry" logic currently resides in a code block which is surrounded by a check whether this feature is actually enabled. When we put the attributes used by this feature into several other locations, it would at least not be as clear cut as it is now and code is called even with the feature not supported/enabled. Do performance benefits outweigh this?

That's a valid point. On the other hand there are not only performance benefits for the user, but also frequent request to the LDAP server which would could get more significant with big active user bases. A compromise could be to use Access::searchUsers with the user DN as base instead. It can read and return any number of attributes. (Or more work can be put in to either make readAttribute multi-attribute ready or to touch less other crud to implement an new readAttributes() method and deprecate the former one. The benefit is the search scope, but probably this would not differ too much from the searchUsers approach).

Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
@GitHubUser4234
Copy link
Contributor Author

Thanks @blizzz

[...]btw, did you test what happens when the password expired while you connected to Nextcloud with a client?[...]

Yes, e.g. the Nextcloud desktop client continues to work normally when the password expires (cookies?), however when a logout and login is performed after expiry, the sync indicator doesn't become green again which is somewhat expected.

Yes, session cookies. I assume the client periodically tries to re-login in the latter case. Is this a problem?

One has to keep in mind that this scenario eats up grace logins and if the pwdGraceAuthNLimit attribute is only set to a small value, no more grace logins might be available and thus an admin needs to be bothered again to help reset the password. Therefore, the pwdGraceAuthNLimit attribute should be set to an adequate value.

We have several readAttributes in this method, this all costs. A web login does not happen so often, but what about logins via Clients (...). Perhaps some values can be loaded during login (see Manager::getAttributes() and User::processAttributes) and/or cached, if it makes sense.

Well, I guess logins for a client only happen often when it doesn't use cookies and therefore triggers a login for each request?

Exactly.

While transient user attributes and the general user attribute could be loaded during login, caching the global attributes seems to make sense. What would be the appropriate method that could be used for caching the global attributes?

The connection instance has a cache object, cf. https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L315 and hhttps://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/User/User.php#L320

But then there's another thing to consider: The whole "handle password expiry" logic currently resides in a code block which is surrounded by a check whether this feature is actually enabled. When we put the attributes used by this feature into several other locations, it would at least not be as clear cut as it is now and code is called even with the feature not supported/enabled. Do performance benefits outweigh this?

That's a valid point. On the other hand there are not only performance benefits for the user, but also frequent request to the LDAP server which would could get more significant with big active user bases. A compromise could be to use Access::searchUsers with the user DN as base instead. It can read and return any number of attributes. (Or more work can be put in to either make readAttribute multi-attribute ready or to touch less other crud to implement an new readAttributes() method and deprecate the former one. The benefit is the search scope, but probably this would not differ too much from the searchUsers approach).

As discussed, we replace the 7 readAttribute() calls by 2 LDAP searches, whereas one searches for the user's relevant attributes and the other one searches for the global password policy attributes. Latter attributes are also cached.

All issues including the formal ones should be addressed now 🐱

Signed-off-by: Roger Szabo <roger.szabo@web.de>
Signed-off-by: Roger Szabo <roger.szabo@web.de>
@MorrisJobke
Copy link
Member

@blizzz Could you have another look? cc @nextcloud/ldap

@johnripper1
Copy link

Why dont you implement a(nother) grace period where the user is only able to login and change there password:

  • Either show content readonly or
  • Redirect to a login/change password side
  • Or actively confirms that he is not willing to do so.

Of course this is only another workaround and it will not address problems discussed above (eg no login in grace period).
Furthermore it will cut the effective usage time by the grace period: e.g. password should change every 90 days and the grace period is 10 Days, you force the user to change it every 80 days.

A general word on expired passwords in AD: for me this is not a relevant problem as my users have the possibilty to login via a windows client. And thats nothibg else as the grace period above.

@GitHubUser4234
Copy link
Contributor Author

@johnripper1 Not sure whether I have understood correctly, but as grace periods in terms of actual time periods are mentioned (which isn't supported by OpenLDAP), it seems the suggestion is to implement such time period based password policies in Nextcloud? Well, it sounds like out of scope, the goal of this PR is to actually handle password expiry enforced by the LDAP server (see discussions above), not by the client (Nextcloud).

But apart from that, grace periods as described in the example have the problem that users who login seldomly, already passed their grace period (e.g. 10 days) when they login. With the grace login count approach supported by OpenLDAP however, these users can still renew their password, independently from when their last login was. And with this PR, they will be redirected to a "Renew password" page during login.

@rullzer
Copy link
Member

rullzer commented Apr 20, 2017

@GitHubUser4234 @blizzz can you write down some steps to test this?
I'd like to get this in sooner rather than later :)

@GitHubUser4234
Copy link
Contributor Author

GitHubUser4234 commented Apr 21, 2017

@rullzer Besides the phpunit test cases, the following steps could be used to perform some manual tests:

  • Install Nextcloud + this PR / OpenLDAP >=2.4.40
  • Configure OpenLDAP to use ppolicy and create 2 sample policies:
    • default policy: attributes set like pwdGraceAuthNLimit=99999, pwdInHistory=3, pwdMaxAge=7776000 and pwdExpireWarning=1209600 . Also set some password rules like minimum length etc.
    • custom policy: pwdMaxAge=0 (password never expires). Also set some password rules like minimum length etc.
  • Configure Nextcloud to enable LDAP password changes per user and fill in the "Default password policy DN"
  • Create 2 LDAP users:
    • userA:
      • no pwdPolicySubentry attribute (default policy is used)
      • pwdChangedTime set to a value that triggers the password expiry notification
    • userB:
      • pwdPolicySubentry set to the custom policy
      • pwdReset set to "TRUE"
  • Login to Nextcloud with userA, he should see the password expiry warning. Change his password. The notification should disappear automatically within some seconds. And should also not reappear, when he logs in again.
  • Login with userB. He should be prompted to renew his password. Play around with invalid passwords to trigger different error messages returned from LDAP (e.g. password wasn't changed, password too simplistic etc...). Finally renew the password successfully and login with the new password, all should be fine.
  • Patch pwdChangedTime of userA to a value so that it expires.
  • Login with userA. He should be prompted to renew his password. Try to use the initial password (used for user creation) to trigger an error saying that the password has been used before. Finally renew the password successfully and login with the new password, all should be fine.
  • Patch pwdChangedTime of userB to a value years ago.
  • Login with userB. As he's under the custom policy, he should not be prompted for password renewal.

Thanks!

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok did some messing around and it seems to do the job! Lets get this in.

@GitHubUser4234
Copy link
Contributor Author

Ok did some messing around and it seems to do the job! Lets get this in.

Yippie, thanks a lot for testing! 😺

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me! :) 🚢

@blizzz blizzz merged commit 42e805f into nextcloud:master Apr 24, 2017
@GitHubUser4234
Copy link
Contributor Author

Changes look good to me! :) 🚢

@blizzz Awesome, and thanks a lot for the great and friendly help consistently provided to make this PR possible :))

@MorrisJobke
Copy link
Member

@blizzz Awesome, and thanks a lot for the great and friendly help consistently provided to make this PR possible :))

But I really say sorry for this. This simply took too long. Sorry that you waited that long to get it in. Thanks for your patience.

@jospoortvliet
Copy link
Member

jospoortvliet commented May 3, 2017

Congrats to @GitHubUser4234 for this, I realize we didn't talk about this in the release announcement but it is a great improvement! I will think of a way to talk about this still.

Edit: I just added that we support password policies on nextcloud.com/secure ;-)

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.

None yet

8 participants