389-ds-base

Clone
Source Code
GIT

#50156 Ticket 50155 - password history check has no way to just check the current password
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket50155  into  master

file modified
+103 -106 
@@ -7,10 +7,11 @@ 
 

  # --- END COPYRIGHT BLOCK ---
 

  #
 

  import pytest
 

+ import time
 

  from lib389.tasks import *
 

  from lib389.utils import *
 

  from lib389.topologies import topology_st
 

- 

+ from lib389.idm.user import UserAccounts, TEST_USER_PROPERTIES
 

  from lib389._constants import DN_DM, DEFAULT_SUFFIX, PASSWORD
 

  

  logging.getLogger(__name__).setLevel(logging.DEBUG)
 
@@ -35,6 +36,10 @@ 
 

          6. Attempt to change the password to previous passwords
 

          7. Reset password by Directory Manager (admin reset)
 

          8. Try and change the password to the previous password before the reset
 

+         9. Test passwordInHistory set to "0" rejects only the current password
 

+         10. Test passwordInHistory set to "2" rejects previous passwords
 

+ 

+ 

      :expectedresults:
 

          1. Password history policy should be configured successfully
 

          2. User should be added successfully
 
@@ -47,10 +52,10 @@ 
 

          7. Password should be successfully reset
 

          8. Password change should be correctly rejected
 

             with Constrant Violation error
 

+         9. Success
 

+         10. Success
 

      """
 

  

-     USER_DN = 'uid=testuser,' + DEFAULT_SUFFIX
 

- 

      #
 

      # Configure password history policy and add a test user
 

      #
 
@@ -58,37 +63,25 @@ 
 

          topology_st.standalone.config.replace_many(('passwordHistory', 'on'),
 

                                                     ('passwordInHistory', '3'),
 

                                                     ('passwordChange', 'on'),
 

-                                                    ('passwordStorageScheme', 'CLEAR'))
 

+                                                    ('passwordStorageScheme', 'CLEAR'),
 

+                                                    ('nsslapd-auditlog-logging-enabled', 'on'))
 

          log.info('Configured password policy.')
 

      except ldap.LDAPError as e:
 

          log.fatal('Failed to configure password policy: ' + str(e))
 

          assert False
 

      time.sleep(1)
 

  

-     try:
 

-         topology_st.standalone.add_s(Entry((USER_DN, {
 

-             'objectclass': ['top', 'extensibleObject'],
 

-             'sn': 'user',
 

-             'cn': 'test user',
 

-             'uid': 'testuser',
 

-             'userpassword': 'password'})))
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to add test user' + USER_DN + ': error ' + str(e))
 

-         assert False
 

+     users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)
 

+     user = users.create(properties=TEST_USER_PROPERTIES)
 

+     user.set('userpassword', 'password')
 

+     user.rebind('password')
 

  

      #
 

      # Test that password history is enforced.
 

      #
 

-     try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password')
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user: ' + str(e))
 

-         assert False
 

- 

      # Attempt to change password to the same password
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password')])
 

+         user.set('userpassword', 'password')
 

          log.info('Incorrectly able to to set password to existing password.')
 

          assert False
 

      except ldap.CONSTRAINT_VIOLATION:
 
@@ -100,110 +93,109 @@ 
 

      #
 

      # Keep changing password until we fill the password history (3)
 

      #
 

+     user.set('userpassword', 'password1')
 

+     user.rebind('password1')
 

+     user.set('userpassword', 'password2')
 

+     user.rebind('password2')
 

+     user.set('userpassword', 'password3')
 

+     user.rebind('password3')
 

+     user.set('userpassword', 'password4')
 

+     user.rebind('password4')
 

+     time.sleep(1)
 

  

-     # password1
 

-     try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password1')])
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to change password: ' + str(e))
 

-         assert False
 

-     try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password1')
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user using "password1": ' + str(e))
 

+     #
 

+     # Check that we only have 3 passwords stored in history
 

+     #
 

+     pwds = user.get_attr_vals('passwordHistory')
 

+     if len(pwds) != 3:
 

+         log.fatal('Incorrect number of passwords stored in history: %d' %
 

+                   len(pwds))
 

+         log.error('password history: ' + str(pwds))
 

          assert False
 

-     time.sleep(1)
 

+     else:
 

+         log.info('Correct number of passwords found in history.')
 

  

-     # password2
 

+     #
 

+     # Attempt to change the password to previous passwords
 

+     #
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password2')])
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to change password: ' + str(e))
 

+         user.set('userpassword', 'password1')
 

+         log.fatal('Incorrectly able to to set password to previous password1.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

-     try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password2')
 

+     except ldap.CONSTRAINT_VIOLATION:
 

+         log.info('Password change correctly rejected')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user using "password2": ' + str(e))
 

+         log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

-     time.sleep(1)
 

- 

-     # password3
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password3')])
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to change password: ' + str(e))
 

+         user.set('userpassword', 'password2')
 

+         log.fatal('Incorrectly able to to set password to previous password2.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

-     try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password3')
 

+     except ldap.CONSTRAINT_VIOLATION:
 

+         log.info('Password change correctly rejected')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user using "password3": ' + str(e))
 

+         log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

-     time.sleep(1)
 

- 

-     # password4
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password4')])
 

-     except ldap.LDAPError as e:
 

-         log.fatal('Failed to change password: ' + str(e))
 

+         user.set('userpassword', 'password3')
 

+         log.fatal('Incorrectly able to to set password to previous password3.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

-     try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password4')
 

+     except ldap.CONSTRAINT_VIOLATION:
 

+         log.info('Password change correctly rejected')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user using "password4": ' + str(e))
 

+         log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

-     time.sleep(1)
 

  

      #
 

-     # Check that we only have 3 passwords stored in history
 

+     # Reset password by Directory Manager(admin reset)
 

      #
 

      try:
 

-         entry = topology_st.standalone.search_s(USER_DN, ldap.SCOPE_BASE,
 

-                                                 'objectclass=*',
 

-                                                 ['passwordHistory'])
 

-         pwds = entry[0].getValues('passwordHistory')
 

-         if len(pwds) != 3:
 

-             log.fatal('Incorrect number of passwords stored in histry: %d' %
 

-                       len(pwds))
 

-             assert False
 

-         else:
 

-             log.info('Correct number of passwords found in history.')
 

+         topology_st.standalone.simple_bind_s(DN_DM, PASSWORD)
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to get user entry: ' + str(e))
 

+         log.fatal('Failed to bind as rootDN: ' + str(e))
 

          assert False
 

+     user.set('userpassword', 'password-reset')
 

+     time.sleep(1)
 

  

-     #
 

-     # Attempt to change the password to previous passwords
 

-     #
 

+     # Try and change the password to the previous password before the reset
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password1')])
 

-         log.info('Incorrectly able to to set password to previous password1.')
 

+         user.rebind('password-reset')
 

+         user.set('userpassword', 'password4')
 

+         log.fatal('Incorrectly able to to set password to previous password4.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

      except ldap.CONSTRAINT_VIOLATION:
 

          log.info('Password change correctly rejected')
 

      except ldap.LDAPError as e:
 

          log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

-     time.sleep(1)
 

  

+     #
 

+     # Test passwordInHistory to 0
 

+     #
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password2')])
 

-         log.info('Incorrectly able to to set password to previous password2.')
 

+         topology_st.standalone.simple_bind_s(DN_DM, PASSWORD)
 

+     except ldap.LDAPError as e:
 

+         log.fatal('Failed to bind as rootDN: ' + str(e))
 

          assert False
 

-     except ldap.CONSTRAINT_VIOLATION:
 

-         log.info('Password change correctly rejected')
 

+ 

+     try:
 

+         topology_st.standalone.config.replace('passwordInHistory', '0')
 

+         log.info('Configured passwordInHistory to 0.')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to attempt to change password: ' + str(e))
 

+         log.fatal('Failed to configure password policy (passwordInHistory to 0): ' + str(e))
 

          assert False
 

+ 

+     # Verify the older passwords in the entry (passwordhistory) are ignored
 

+     user.rebind('password-reset')
 

+     user.set('userpassword', 'password4')
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password3')])
 

-         log.info('Incorrectly able to to set password to previous password3.')
 

+         user.set('userpassword', 'password4')
 

+         log.fatal('Incorrectly able to to set password to current password4.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

      except ldap.CONSTRAINT_VIOLATION:
 

          log.info('Password change correctly rejected')
 
@@ -211,8 +203,12 @@ 
 

          log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

  

+     # Need to make one successful update so history list is reset
 

+     user.set('userpassword', 'password5')
 

+ 

      #
 

-     # Reset password by Directory Manager(admin reset)
 

+     # Set the history count back to a positive value and make sure things still work
 

+     # as expected
 

      #
 

      try:
 

          topology_st.standalone.simple_bind_s(DN_DM, PASSWORD)
 
@@ -221,33 +217,34 @@ 
 

          assert False
 

  

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword',
 

-                                                    b'password-reset')])
 

+         topology_st.standalone.config.replace('passwordInHistory', '2')
 

+         log.info('Configured passwordInHistory to 2.')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to attempt to reset password: ' + str(e))
 

+         log.fatal('Failed to configure password policy (passwordInHistory to 2): ' + str(e))
 

          assert False
 

      time.sleep(1)
 

  

-     # Try and change the password to the previous password before the reset
 

      try:
 

-         topology_st.standalone.simple_bind_s(USER_DN, 'password-reset')
 

+         user.rebind('password5')
 

+         user.set('userpassword', 'password5')
 

+         log.fatal('Incorrectly able to to set password to current password5.')
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

+         assert False
 

+     except ldap.CONSTRAINT_VIOLATION:
 

+         log.info('Password change correctly rejected')
 

      except ldap.LDAPError as e:
 

-         log.fatal('Failed to bind as user: ' + str(e))
 

+         log.fatal('Failed to attempt to change password: ' + str(e))
 

          assert False
 

-     time.sleep(1)
 

  

+     # Test that old password that was in history is not being checked
 

      try:
 

-         topology_st.standalone.modify_s(USER_DN, [(ldap.MOD_REPLACE,
 

-                                                    'userpassword', b'password4')])
 

-         log.info('Incorrectly able to to set password to previous password4.')
 

-         assert False
 

-     except ldap.CONSTRAINT_VIOLATION:
 

-         log.info('Password change correctly rejected')
 

+         user.set('userpassword', 'password1')
 

      except ldap.LDAPError as e:
 

          log.fatal('Failed to attempt to change password: ' + str(e))
 

+         log.error('password history: ' + str(user.get_attr_vals('passwordhistory')))
 

          assert False
 

  

+     # Done
 

      log.info('Test suite PASSED.')

file modified
+4 -4 
@@ -3407,8 +3407,8 @@ 
 

  int
 

  config_set_pw_inhistory(const char *attrname, char *value, char *errorbuf, int apply)
 

  {
 

-     int retVal = LDAP_SUCCESS;
 

-     long history = 0;
 

+     int32_t retVal = LDAP_SUCCESS;
 

+     int64_t history = 0;
 

      char *endp = NULL;
 

  

      slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();
 
@@ -3420,9 +3420,9 @@ 
 

      errno = 0;
 

      history = strtol(value, &endp, 10);
 

  

-     if (*endp != '\0' || errno == ERANGE || history < 1 || history > 24) {
 

+     if (*endp != '\0' || errno == ERANGE || history < 0 || history > 24) {
 

          slapi_create_errormsg(errorbuf, SLAPI_DSE_RETURNTEXT_SIZE,
 

-                               "password history length \"%s\" is invalid. The password history must range from 1 to 24", value);
 

+                               "password history length \"%s\" is invalid. The password history must range from 0 to 24", value);
 

          retVal = LDAP_OPERATIONS_ERROR;
 

          return retVal;
 

      }

file modified
+6 -1 
@@ -1255,7 +1255,7 @@ 
 

          if (pwpolicy->pw_history == 1) {
 

              Slapi_Value **va = NULL;
 

              attr = attrlist_find(e->e_attrs, "passwordHistory");
 

-             if (attr && !valueset_isempty(&attr->a_present_values)) {
 

+             if (pwpolicy->pw_inhistory && attr && !valueset_isempty(&attr->a_present_values)) {
 

                  /* Resetting password history array if necessary. */
 

                  if (0 == update_pw_history(pb, sdn, NULL)) {
 

                      /* There was an update in the password history.  Retry... */
 
@@ -1418,6 +1418,11 @@ 
 

  

      pwpolicy = new_passwdPolicy(pb, dn);
 

  

+     if (pwpolicy->pw_inhistory == 0){
 

+         /* We are only enforcing the current password, just return */
 

+         return res;
 

+     }
 

+ 

      /* retrieve the entry */
 

      e = get_entry(pb, dn);
 

      if (e == NULL) {

file modified
+7 
@@ -54,6 +54,13 @@ 
 

          inst_clone.open(*args, **kwargs)
 

          return inst_clone
 

  

+     def rebind(self, password):
 

+         """Rebind on the same connection
 

+         :param password: An entry password
 

+         :type password: str
 

+         """
 

+         self._instance.simple_bind_s(self.dn, password)
 

+ 

      def sasl_bind(self, *args, **kwargs):
 

          """Open a new connection and bind with the entry via SASL.
 

          You can pass arguments that will be pass to clone.

Description:

Currently if you set passwordinhistory 1, it checks the last recorded password and the current password. To get it to just check the current password we need to allow "0" in passwordinhistory. Then only check the current password, and not the entry's passwordHistory attributes (if any).

https://pagure.io/389-ds-base/issue/50155

LGTM!

While you're touching it, it could be worth changing "long" to int32_t here (similar with retVal), but not necessary

Hey,

I think this is the kind of change that requires a test case to show that it hasn't broken the existing functions (Which I think exists), but also that we add a test case for the 0 case.

I also commented that there is a possible code cleanup, but it's not required.

Thanks,

Hey,
I think this is the kind of change that requires a test case to show that it hasn't broken the existing functions (Which I think exists), but also that we add a test case for the 0 case.

Agreed, I'll work on that tomorrow, and the code cleanup!

I also commented that there is a possible code cleanup, but it's not required.
Thanks,

Thank you!

@mreynolds just a dummy remark. pw_inhistory is used as the number of password kept in the history array (update_pw_history). Have you tested that when it is set to 0, the all history is not cleared ?

@mreynolds, rereading the ticket description I realize that clear of the history array was the not a concern. My understanding is that the ticket is to have the ability to reject a password update if the new value/current value are the same.
A concern is that we can not have both: ability to reject update if new_value=current_value AND new_value exist in history array. I feel I am missing something here.

@mreynolds just a dummy remark. pw_inhistory is used as the number of password kept in the history array (update_pw_history). Have you tested that when it is set to 0, the all history is not cleared ?

I have it works fine

@mreynolds, rereading the ticket description I realize that clear of the history array was the not a concern. My understanding is that the ticket is to have the ability to reject a password update if the new value/current value are the same.
A concern is that we can not have both: ability to reject update if new_value=current_value AND new_value exist in history array. I feel I am missing something here.

If you set passwordinhistry to "0", it only checks the current password, if you set it to "1" it checks the last password and the current password, etc. The only behavior change is when you use "0", in that case it never checks the entry's attributes, just the current password.

rebased onto 74a9cdb6f68b9932542389fdb2992103b645c7ea
5 years ago

rebased onto 2e50629e0792379c463c28318ff69da35e5c33fb
5 years ago

Changes made, and CI test updated. Please review...

I think we should use UserAccounts for all of these operations...

There is no need in these try-except blocks because the test case fails it will report all of the information to the output including a stack trace.

rebased onto d68fcc7d5cdaa5522dac6afc2c151b593f190bdf
5 years ago

I think we should use UserAccounts for all of these operations...

Done, also had to add a new function to Accounts class to rebind on the same connection. Please review
Edited 5 years ago by mreynolds

rebased onto 7cf40d15a679711f91e84e0400079ecab3fd3414
5 years ago

Ohhh as a follow up, when you write the test, run the code with asan too please :)

Ohhh as a follow up, when you write the test, run the code with asan too please :)

Test was written up, but I can not test it anymore because setup-ds.pl is currently broken. I'll test it once that all gets straightened out. Do you still require a ASAN run for this patch (the CI was passing)?

@mreynolds Anytime I touch the C code, I run it with ASAN. It's caught so many mistakes of my own that it's invaluable when used with testing. I think it would be great for you to run it with ASAN as tthat would help give you more confidence in the code changes :)

rebased onto cab38f9
5 years ago

ASAN tests pass, merging...

Pull-Request has been merged by mreynolds
5 years ago

Thank you!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3215

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi
3 years ago
Metadata
None
No Tags
Changes Summary 4
+103 -106
file changed
dirsrvtests/tests/suites/password/pwp_history_test.py
+4 -4
file changed
ldap/servers/slapd/libglobs.c
+6 -1
file changed
ldap/servers/slapd/pw.c
+7 -0
file changed
src/lib389/lib389/idm/account.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.