389-ds-base

Clone
Source Code
GIT

#50346 #49421: WIP: on bind password upgrade proof of concept
Closed 3 years ago by spichugi. Opened 4 years ago by codehotter.
codehotter/389-ds-base update_on_bind  into  master

file modified
+4 
@@ -762,6 +762,9 @@ 
 

                              goto free_and_return;
 

                          }
 

                      }
 

+ 

+                     update_pw_encoding(pb, bind_target_entry, sdn, cred.bv_val);
 

+ 

                      bind_credentials_set(pb_conn, authtype,
 

                                           slapi_ch_strdup(slapi_sdn_get_ndn(sdn)),
 

                                           NULL, NULL, NULL, bind_target_entry);
 
@@ -783,6 +786,7 @@ 
 

                      /* need_new_pw failed; need_new_pw already send_ldap_result in it. */
 

                      goto free_and_return;
 

                  }
 

+ 

              } else { /* anonymous */
 

                  /* set bind creds here so anonymous limits are set */
 

                  bind_credentials_set(pb_conn, authtype, NULL, NULL, NULL, NULL, NULL);

file modified
+98 
@@ -3247,3 +3247,101 @@ 
 

      return rc;
 

  }
 

  

+ /*
 

+  * Re-encode a user's password if a different encoding scheme is configured
 

+  * in the password policy than the password is currently encoded with.
 

+  *
 

+  * Returns:
 

+  *   success ( 0 )
 

+  *   operationsError ( -1 ),
 

+  */
 

+ int update_pw_encoding(Slapi_PBlock *orig_pb, Slapi_Entry *e, Slapi_DN *sdn, char *cleartextpassword) {

It may be better to consider an enum here for succes vs error. Some places in the code are updated to have this, but not universal.

Alternately, it may be better to define this as int32_t to specify the exact size. 

+     char *dn = (char *)slapi_sdn_get_ndn(sdn);
 

+     Slapi_Attr *pw = NULL;
 

+     Slapi_Value **password_values = NULL;
 

+     passwdPolicy *pwpolicy = NULL;
 

+     struct pw_scheme *curpwsp = NULL;
 

+     Slapi_Mods smods;

You should zero this on the stack with Slapi_Mods smods = {0}; 

+     char *hashed_val = NULL;
 

+     Slapi_PBlock *pb = NULL;
 

+     int res = 0;

Use exact size, IE int32_t. 

+ 

+     slapi_mods_init(&smods, 0);
 

+ 

+     if (e == NULL || slapi_entry_attr_find(e, SLAPI_USERPWD_ATTR, &pw) != 0 || pw == NULL) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Could not read password attribute on '%s'\n",
 

+                       dn);
 

+         res = -1;
 

+         goto free_and_return;
 

+     }
 

+ 

+     password_values = attr_get_present_values(pw);
 

+     if (password_values == NULL || password_values[0] == NULL) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Could not get password values for '%s'\n",
 

+                       dn);
 

+         res = -1;
 

+         goto free_and_return;
 

+     }
 

+     if (password_values[1] != NULL) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Multivalued password attribute not supported: '%s'\n",

In this error, it's always good to consider "what, why, how to fix". So what went wrong - multivalued password. But why? And how to fix? I think you could also add "not supported for hash upgrade on bind: to resolve on %{dn} only have a single userPassword field".

 
+                       dn);
 

+         res = -1;
 

+         goto free_and_return;
 

+     }
 

+ 

+     pwpolicy = new_passwdPolicy(orig_pb, dn);
 

+     if (pwpolicy == NULL || pwpolicy->pw_storagescheme == NULL) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Could not get requested encoding scheme: '%s'\n",
 

+                       dn);
 

+         res = -1;
 

+         goto free_and_return;
 

+     }
 

+ 

+     curpwsp = pw_val2scheme((char *)slapi_value_get_string(password_values[0]), NULL, 1);
 

+     if (curpwsp != NULL && strcmp(curpwsp->pws_name, pwpolicy->pw_storagescheme->pws_name) == 0) {
 

+         res = 0; // Nothing to do
 

+         goto free_and_return;
 

+     }
 

+ 

+     hashed_val = slapi_encode_ext(NULL, NULL, cleartextpassword, pwpolicy->pw_storagescheme->pws_name);
 

+     if (hashed_val == NULL) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Could not re-encode password: '%s'\n",
 

+                       dn);
 

+         res = -1;
 

+         goto free_and_return;
 

+     }
 

+ 

+     slapi_mods_add_string(&smods, LDAP_MOD_REPLACE, SLAPI_USERPWD_ATTR, hashed_val);

Does mods_add_string copy the value? or just use the reference? (I actualy can't remember, want to be sure you have checked. It would be good to comment about that here potentially). 

+     slapi_ch_free((void **)&hashed_val);
 

+ 

+     pb = slapi_pblock_new();
 

+     /* We don't want to overwrite the modifiersname, etc. attributes,
 

+      * so we set a flag for this operation.
 

+      * We also set the repl flag to avoid updating password history */

So you are right to be concerned about the repl flag here, because it could indicate to the server to also NOT replicate this operation to the partner server.

I think a better method would either be a unique flag for OP_FLAG_PASSWORD_UPGRADE.

I'm wondering if a simpler approach could be "if userPassword schema != config scheme -> just apply a mod of userPassword with the cleartext password".

We don't care if the password history is in there twice because I would hope OP_INTERNAL would bypass the reject of the re-used password. It also means you would probably avoid any messiness trying to encode and modify yourself in light of the password migration flag. Saying this, I could be totally wrong too as I have not looked at pw code in a longggg time. :) 

+     slapi_modify_internal_set_pb_ext(pb, sdn,
 

+                                      slapi_mods_get_ldapmods_byref(&smods),
 

+                                      NULL,                         /* Controls */
 

+                                      NULL,                         /* UniqueID */
 

+                                      pw_get_componentID(),         /* PluginID */
 

+                                      OP_FLAG_SKIP_MODIFIED_ATTRS &
 

+                                      OP_FLAG_REPLICATED);          /* Flags */
 

+     slapi_modify_internal_pb(pb);
 

+ 

+     slapi_pblock_get(pb, SLAPI_PLUGIN_INTOP_RESULT, &res);
 

+     if (res != LDAP_SUCCESS) {
 

+         slapi_log_err(SLAPI_LOG_WARNING,
 

+                       "update_pw_encoding", "Modify error %d on entry '%s'\n",
 

+                       res, dn);
 

+     }
 

+ 

+ free_and_return:
 

+     if (curpwsp) free_pw_scheme(curpwsp);

I'm not a fan of inline if, so please make these
if (curpwsp) { ... }

 
+     if (pb) slapi_pblock_destroy(pb);

Doesn't pblock destroy check for nulls? I think you can just call it without the if check. 

+     slapi_mods_done(&smods);
 

+     return res;
 

+ }

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

   */
 

  #define SLAPI_USERPWD_ATTR "userpassword"
 

  int slapi_pw_find_sv(Slapi_Value **vals, const Slapi_Value *v);
 

+ int update_pw_encoding(Slapi_PBlock *orig_pb, Slapi_Entry *e, Slapi_DN *sdn, char *cleartextpassword);
 

  

  /* value encoding encoding */
 

  /* checks if the value is encoded with any known algorithm*/

Implements #49421 : on bind password upgrade

This is a work in progress. At the least, I think it needs:
- Tests
- Disabling this functionality by default - adding a configuration option

I'm also not happy that I have repurposed the repl_op flag for something non-obvious. A better solution here might be to add another custom flag to disable password history updates.

Even though it is not finished, I'd like to post it already to get some feedback on whether this is the right overall direction.

Hey there,

I'm really happy to see you contributing to the server. Thank you!

As you have rightly said, it's a work in progress, and I think tests are an important thing to add here. Additionally, if you are developing in C, it's super valuable to have ASAN setup and working in your environment (./configure --enable-asan) to help you find memory errors.

As for adding the functionality, you are right there should be a feature flag - look at libglobs.c. It's pretty long, but that's where cn=config feature flags are implemented. That's your best bet.

I've put comments inline to the patch, but again, it looks like a great start, and we're excited to see you contributing!

It may be better to consider an enum here for succes vs error. Some places in the code are updated to have this, but not universal.

Alternately, it may be better to define this as int32_t to specify the exact size.

You should zero this on the stack with Slapi_Mods smods = {0};

Use exact size, IE int32_t.

In this error, it's always good to consider "what, why, how to fix". So what went wrong - multivalued password. But why? And how to fix? I think you could also add "not supported for hash upgrade on bind: to resolve on %{dn} only have a single userPassword field".

Does mods_add_string copy the value? or just use the reference? (I actualy can't remember, want to be sure you have checked. It would be good to comment about that here potentially).

So you are right to be concerned about the repl flag here, because it could indicate to the server to also NOT replicate this operation to the partner server.

I think a better method would either be a unique flag for OP_FLAG_PASSWORD_UPGRADE.

I'm wondering if a simpler approach could be "if userPassword schema != config scheme -> just apply a mod of userPassword with the cleartext password".

We don't care if the password history is in there twice because I would hope OP_INTERNAL would bypass the reject of the re-used password. It also means you would probably avoid any messiness trying to encode and modify yourself in light of the password migration flag. Saying this, I could be totally wrong too as I have not looked at pw code in a longggg time. :)

I'm not a fan of inline if, so please make these
if (curpwsp) { ... }

Doesn't pblock destroy check for nulls? I think you can just call it without the if check.

@mreynolds - you probably have some good comments here too about PW upgrade on bind. :)

Hey @codehotter, would you object to me taking this branch and adding tests and finishing it up for you?

Apologize for my slow responses -- I don't mind at all.

Although I will eventually get round to it if you're not in a hurry.

Really appreciate your feedback.

No problems, I wanted to check before "hijacking" your work. I'm currently traveling and working slowly, but I have a few outstanding things to resolve, then I may look at this depending on your time. As always, happy to help and advise if you want to do this yourself, especially around making test cases. Thanks,

Hey mate: I've added the test case for you here in this PR: https://pagure.io/389-ds-base/pull-request/50450

I'm going to add the libglobs.c "on/off" switch next (tomorrow maybe?), and I have some more testing to do then, we can merge this.

Again, amazing work and thank you so much!

Pull-Request has been closed by firstyear
4 years ago

Closing this as #50450 supercedes it. I have taken care to ensure your git authorship information is preserved in the new branch. 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/3405

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 3
+4 -0
file changed
ldap/servers/slapd/bind.c
+98 -0
file changed
ldap/servers/slapd/pw.c
+1 -0
file changed
ldap/servers/slapd/slapi-plugin.h
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.