#49952 Ticket 49950 - PassSync not setting pwdLastSet attribute in Active Directory after Pw update from LDAP sync for normal user
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base winsync-pwdlastset  into  master

@@ -74,7 +74,6 @@ 

  static int automember_parse_grouping_attr(char *value, char **grouping_attr, char **grouping_value);

  static int automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileDesc *ldif_fd);

  static int automember_add_member_value(Slapi_Entry *member_e, const char *group_dn, char *grouping_attr, char *grouping_value, PRFileDesc *ldif_fd);

- const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);

  

  /*

   * task functions
@@ -1927,25 +1926,6 @@ 

      int scope;

  } task_data;

  

- /*

-  * extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Value *val = NULL;

-     Slapi_Attr *attr;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0) {

-         return default_val;

-     }

-     slapi_attr_first_value(attr, &val);

- 

-     return slapi_value_get_string(val);

- }

- 

  static void

  automember_task_destructor(Slapi_Task *task)

  {

@@ -22,7 +22,6 @@ 

  static void linked_attrs_fixup_links(struct configEntry *config);

  static int linked_attrs_remove_backlinks_callback(Slapi_Entry *e, void *callback_data);

  static int linked_attrs_add_backlinks_callback(Slapi_Entry *e, void *callback_data);

- static const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);

  

  /*

   * Function Implementations
@@ -459,22 +458,3 @@ 

  

      return rc;

  }

- 

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0) {

-         return default_val;

-     }

- 

-     slapi_attr_first_value(attr, &val);

- 

-     return slapi_value_get_string(val);

- }

@@ -142,7 +142,6 @@ 

  static int memberof_modop_one_replace_r(Slapi_PBlock *pb, MemberOfConfig *config, int mod_op, Slapi_DN *group_sdn, Slapi_DN *op_this_sdn, Slapi_DN *replace_with_sdn, Slapi_DN *op_to_sdn, memberofstringll *stack);

  static int memberof_task_add(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *eAfter, int *returncode, char *returntext, void *arg);

  static void memberof_task_destructor(Slapi_Task *task);

- static const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);

  static void memberof_fixup_task_thread(void *arg);

  static int memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td);

  static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data);
@@ -2871,22 +2870,6 @@ 

                    "memberof_fixup_task_thread - refcount decremented.\n");

  }

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

-     slapi_attr_first_value(attr, &val);

-     return slapi_value_get_string(val);

- }

- 

  int

  memberof_task_add(Slapi_PBlock *pb,

                    Slapi_Entry *e,

@@ -42,22 +42,6 @@ 

  static int

  posix_group_fix_memberuid_callback(Slapi_Entry *e, void *callback_data);

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

-     slapi_attr_first_value(attr, &val);

-     return slapi_value_get_string(val);

- }

- 

  /* e configEntry */

  int

  posix_group_task_add(Slapi_PBlock *pb __attribute__((unused)),
@@ -82,7 +66,7 @@ 

  

      /* get arg(s) */

      /* default: set replication basedn */

-     if ((dn = fetch_attr(e, "basedn", slapi_sdn_get_dn(posix_winsync_config_get_suffix()))) == NULL) {

+     if ((dn = fetch_attr(e, "basedn", (char *)slapi_sdn_get_dn(posix_winsync_config_get_suffix()))) == NULL) {

          *returncode = LDAP_OBJECT_CLASS_VIOLATION;

          rv = SLAPI_DSE_CALLBACK_ERROR;

          goto out;

@@ -1353,19 +1353,6 @@ 

      return LDAP_SUCCESS;

  }

  

- const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

- 

-     slapi_attr_first_value(attr, &val);

-     return slapi_value_get_string(val);

- }

- 

  static int

  replica_cleanall_ruv_task(Slapi_PBlock *pb __attribute__((unused)),

                            Slapi_Entry *e,

@@ -720,39 +720,79 @@ 

      } else {

          Slapi_Attr *attr = NULL;

          int force_reset_pw = 0;

+         int pwd_already_reset = 0;

+         int ds_must_change = config_get_pw_must_change();

+ 

          /*

-              * If AD entry has password must change flag is set,

-              * we keep the flag (pwdLastSet == 0).

-              * msdn.microsoft.com: Windows Dev Centor - Desktop

-              * To force a user to change their password at next logon,

-              * set the pwdLastSet attribute to zero (0).

-              */

+          * If AD entry has password must change flag is set,

+          * we keep the flag (pwdLastSet == 0).

+          * msdn.microsoft.com: Windows Dev Centor - Desktop

+          * To force a user to change their password at next logon,

+          * set the pwdLastSet attribute to zero (0).

+          */

          if (remote_entry &&

              (0 == slapi_entry_attr_find(remote_entry, "pwdLastSet", &attr)) &&

-             attr) {

+             attr)

+         {

              Slapi_Value *v = NULL;

              int i = 0;

+ 

              for (i = slapi_attr_first_value(attr, &v);

                   v && (i != -1);

-                  i = slapi_attr_next_value(attr, i, &v)) {

+                  i = slapi_attr_next_value(attr, i, &v))

+             {

                  const char *s = slapi_value_get_string(v);

                  if (NULL == s) {

                      continue;

                  }

                  if (0 == strcmp(s, "0")) {

-                     slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name,

-                                   "%s: AD entry %s set \"user must change password at next logon\". ",

-                                   agmt_get_long_name(prp->agmt), slapi_entry_get_dn(remote_entry));

                      force_reset_pw = 1;

+                     if (ds_must_change) {

+                         /*

+                          * DS already enforces "password must be changed after reset".

+                          * Do an internal search and check the passwordExpirationtime

+                          * to see if is it actually needs to be reset.  If it doesn't,

+                          * then set pwdLastSet to -1

+                          */

+                         char *expiration_val;

+                         int rc = 0;

+                         Slapi_DN *local_sdn = NULL;

+ 

+                         rc = map_entry_dn_inbound(remote_entry, &local_sdn, prp->agmt);

+                         if ((0 == rc) && local_sdn) {

+                             Slapi_Entry *local_entry = NULL;

+                             /* Get the local entry if it exists */

+                             rc = windows_get_local_entry(local_sdn, &local_entry);

+                             if ((0 == rc) && local_entry) {

+                                 expiration_val = (char *)fetch_attr(local_entry, "passwordExpirationtime", NULL);

+                                 if (expiration_val && parse_genTime(expiration_val) != NO_TIME){

+                                     /* The user did reset their password */

+                                     slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name,

+                                         "send_password_modify - entry (%s) password was reset by user send that info to AD\n",

+                                         slapi_sdn_get_dn(local_sdn));

+                                     pwd_already_reset = 1;

+                                     force_reset_pw = 0;

+                                 }

+                                 slapi_entry_free(local_entry);

+                             }

+                         }

+                         slapi_sdn_free(&local_sdn);

+                     } else {

+                         slapi_log_err(SLAPI_LOG_REPL, windows_repl_plugin_name,

+                                       "%s: AD entry %s set \"user must change password at next logon\n",

+                                       agmt_get_long_name(prp->agmt), slapi_entry_get_dn(remote_entry));;

+                     }

                  }

              }

          }

-         /* We will attempt to bind to AD with the new password first. We do

-              * this to avoid playing a password change that originated from AD

-              * back to AD.  If we just played the password change back, then

-              * both sides would be in sync, but AD would contain the new password

-              * twice in it's password history, which undermines the password

-              * history policies in AD. */

+         /*

+          * We will attempt to bind to AD with the new password first. We do

+          * this to avoid playing a password change that originated from AD

+          * back to AD.  If we just played the password change back, then

+          * both sides would be in sync, but AD would contain the new password

+          * twice in it's password history, which undermines the password

+          * history policies in AD.

+          */

          if (windows_check_user_password(prp->conn, sdn, password)) {

              char *quoted_password = NULL;

              /* AD wants the password in quotes ! */
@@ -792,9 +832,18 @@ 

                      pw_mod.mod_bvalues = bvals;

  

                      pw_mods[0] = &pw_mod;

-                     if (force_reset_pw) {

-                         reset_bv.bv_len = 1;

-                         reset_bv.bv_val = "0";

+ 

+                     if (force_reset_pw || pwd_already_reset) {

+                         if (force_reset_pw) {

+                             reset_bv.bv_val = "0";

+                             reset_bv.bv_len = 1;

+                         } else if (pwd_already_reset) {

+                             /* Password was reset by the user, there is no

+                              * need to make the user change their password

+                              * again in AD so set pwdLastSet to -1 */

+                             reset_bv.bv_val = "-1";

+                             reset_bv.bv_len = 2;

The fix looks good to me. A minor question, why setting 'pwdLastSet=-1'.
If on AD side there is expiration time, I wonder if there is a risk '(<current_time> - <pwdLastSet>)' is greater than '<pwdMaxAge>', that may expire immediately the reset password

+                         }

                          reset_bvals[0] = &reset_bv;

                          reset_bvals[1] = NULL;

                          reset_pw_mod.mod_type = "pwdLastSet";
@@ -807,7 +856,6 @@ 

                      }

  

                      pw_return = windows_conn_send_modify(prp->conn, slapi_sdn_get_dn(sdn), pw_mods, NULL, NULL);

- 

                      slapi_ch_free((void **)&unicode_password);

                  }

                  PR_smprintf_free(quoted_password);

@@ -187,23 +187,6 @@ 

                    "schemareload_thread <-- refcount decremented.\n");

  }

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

-     slapi_attr_first_value(attr, &val);

- 

-     return slapi_value_get_string(val);

- }

- 

  static void

  schemareload_destructor(Slapi_Task *task)

  {

@@ -43,7 +43,6 @@ 

  static void syntax_validate_task_destructor(Slapi_Task *task);

  static void syntax_validate_task_thread(void *arg);

  static int syntax_validate_task_callback(Slapi_Entry *e, void *callback_data);

- static const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);

  static void syntax_validate_set_plugin_id(void *plugin_id);

  static void *syntax_validate_get_plugin_id(void);

  
@@ -258,25 +257,6 @@ 

      return rc;

  }

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0) {

-         return default_val;

-     }

- 

-     slapi_attr_first_value(attr, &val);

- 

-     return slapi_value_get_string(val);

- }

- 

  /*

   * Plug-in identity management helper functions

   */

@@ -8276,6 +8276,8 @@ 

   */

  uint64_t slapi_atomic_decr_64(uint64_t *ptr, int memorder);

  

+ /* helper function */

+ const char * fetch_attr(Slapi_Entry *e, const char *attrname, char *default_val);

  

  #ifdef __cplusplus

  }

file modified
-17
@@ -80,7 +80,6 @@ 

  static int task_modify(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *eAfter, int *returncode, char *returntext, void *arg);

  static int task_deny(Slapi_PBlock *pb, Slapi_Entry *e, Slapi_Entry *eAfter, int *returncode, char *returntext, void *arg);

  static void task_generic_destructor(Slapi_Task *task);

- static const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);

  static Slapi_Entry *get_internal_entry(Slapi_PBlock *pb, char *dn);

  static void modify_internal_entry(char *dn, LDAPMod **mods);

  static void fixup_tombstone_task_destructor(Slapi_Task *task);
@@ -692,22 +691,6 @@ 

      slapi_ch_free((void **)&task);

  }

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

-     slapi_attr_first_value(attr, &val);

-     return slapi_value_get_string(val);

- }

- 

  /* supply the pblock, destroy it when you're done */

  static Slapi_Entry *

  get_internal_entry(Slapi_PBlock *pb, char *dn)

@@ -116,22 +116,6 @@ 

      slapi_task_finish(task, rv);

  }

  

- /* extract a single value from the entry (as a string) -- if it's not in the

-  * entry, the default will be returned (which can be NULL).

-  * you do not need to free anything returned by this.

-  */

- static const char *

- fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val)

- {

-     Slapi_Attr *attr;

-     Slapi_Value *val = NULL;

- 

-     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

-         return default_val;

-     slapi_attr_first_value(attr, &val);

-     return slapi_value_get_string(val);

- }

- 

  static void

  task_sampletask_destructor(Slapi_Task *task)

  {

file modified
+17
@@ -1607,3 +1607,20 @@ 

          *op_nested_count = 0;

      }

  }

+ 

+ /*

+  * Extract a single value from an entry (as a string) -- if it's not in the

+  * entry, the default will be returned (which can be NULL).  You do not need

+  * to free the returned string value.

+  */

+ const char *

+ fetch_attr(Slapi_Entry *e, const char *attrname, char *default_val)

+ {

+     Slapi_Attr *attr;

+     Slapi_Value *val = NULL;

+ 

+     if (slapi_entry_attr_find(e, attrname, &attr) != 0)

+         return default_val;

+     slapi_attr_first_value(attr, &val);

+     return slapi_value_get_string(val);

+ }

Bug Description:

If a user's password was reset by an "Admin" or directory manager, the
password policy requires a user must change their password after it's
been "reset", and the user then resets their password in DS, this
information was not sent to AD. Then if the user logged in AD after
resetting their password in DS they still get forced to change their
password again in AD.

Fix Description:

When sending a password update to AD, and AD is enforcing password must
be reset, check if the user's did reset thier password. If so, set the
correct "pwdLastSet" value to prevent AD from forcing that user to
change their password again.

But this only works going from DS to AD. The information needed to make
it work from AD -> DS is not available to passSync, and if it was available
it could not be correctly sent to DS anyway (not without a major redesign).

Side Note:

Also moved iand consolidated the function "fetch_attr" to util.c. It
was reused and redefined in many plugins. So I added the definition
to slapi-plugin.h and removed the duplicate definitions.

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

Reviewed by: ?

If this is part of the api, this should be in entry.c, and probably "slapi_entry_fetch_attr" rather than this,

I'm happy with this, but I think that your cleanup of fetch_attr should be "slapi_entry_fetch_attr" and be a function in entry.c instead so that we can expose it cleanly and correctly.

The fix looks good to me. A minor question, why setting 'pwdLastSet=-1'.
If on AD side there is expiration time, I wonder if there is a risk '(<current_time> - <pwdLastSet>)' is greater than '<pwdMaxAge>', that may expire immediately the reset password

The fix looks good to me. A minor question, why setting 'pwdLastSet=-1'.
If on AD side there is expiration time, I wonder if there is a risk '(<current_time> - <pwdlastset>)' is greater than '<pwdmaxage>', that may expire immediately the reset password

My understanding is that pwdlastset is a flag, not a time stamp.

I agree with '0' it is a flag but looking a https://docs.microsoft.com/en-us/windows/desktop/ADSchema/a-pwdlastset, my understanding is that it can also be a timestamp.
I think the any value <>0 should prevent a reset but I am not sure we can compute a AD timestamp

I think the fix improve the situation and even if does not fully conform the semantic of pwdlastset it fixes a known issue.
I agree with the patch. ACK

rebased onto d9437be

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

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/3011

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