PR#50450: Ticket 49421 - Implement password hash upgrade on bind. - 389-ds-base

389-ds-base

#50450 Ticket 49421 - Implement password hash upgrade on bind.

Closed 3 years ago by spichugi. Opened 4 years ago by firstyear.

firstyear/389-ds-base update_on_bind into master

Ticket 49421 - Implement password hash upgrade on bind.

William Brown • 4 years ago

b84669f

Ticket 49421 - on bind password upgrade proof of concept

Emanuel Rietveld • 4 years ago

3aa1416

dirsrvtests/tests/suites/password/pwd_upgrade_on_bind.py

file added

+140

		`@@ -0,0 +1,140 @@`
		`+ # --- BEGIN COPYRIGHT BLOCK ---`
		`+ # Copyright (C) 2019 William Brown <william@blackhats.net.au>`
		`+ # All rights reserved.`
		`+ #`
		`+ # License: GPL (version 3 or any later version).`
		`+ # See LICENSE for details.`
		`+ # --- END COPYRIGHT BLOCK ---`
		`+ #`
		`+ import ldap`
		`+ import pytest`
		`+ from lib389.topologies import topology_st`
		`+ from lib389.idm.user import UserAccounts`
		`+ from lib389._constants import (DEFAULT_SUFFIX, PASSWORD)`
		`+`
		`+ def test_password_hash_on_upgrade(topology_st):`
		`+ """If a legacy password hash is present, assert that on a correct bind`
		`+ the hash is "upgraded" to the latest-and-greatest hash format on the`
		`+ server.`
		`+`
		`+ Assert also that password FAILURE does not alter the password.`
		`+`
		`+ :id: 42cf99e6-454d-46f5-8f1c-8bb699864a07`
		`+ :setup: Single instance`
		`+ :steps: 1. Set a password hash in SSHA256, and hash to pbkdf2 statically`
		`+ 2. Test a faulty bind`
		`+ 3. Assert the PW is SSHA256`
		`+ 4. Test a correct bind`
		`+ 5. Assert the PW is PBKDF2`
		`+ :expectedresults:`
		`+ 1. Successfully set the values`
		`+ 2. The bind fails`
		`+ 3. The PW is SSHA256`
		`+ 4. The bind succeeds`
		`+ 5. The PW is PBKDF2`
		`+ """`
		`+ # Make sure the server is set to pkbdf`
		`+ topology_st.standalone.config.set('passwordStorageScheme', 'PBKDF2_SHA256')`
		`+ topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')`
		`+ topology_st.standalone.config.set('nsslapd-enable-upgrade-hash', 'on')`
		`+`
		`+ users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)`
		`+ user = users.create_test_user()`
		`+ # Static version of "password" in SSHA256.`
		`+ user.set('userPassword', "{SSHA256}9eliEQgjfc4Fcj1IXZtc/ne1GRF+OIjz/NfSTX4f7HByGMQrWHLMLA==")`
		`+ # Attempt to bind with incorrect password.`
		`+ with pytest.raises(ldap.INVALID_CREDENTIALS):`
		`+ badconn = user.bind('badpassword')`
		`+ # Check the pw is SSHA256`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('{SSHA256}')`
		`+`
		`+ # Bind with correct.`
		`+ conn = user.bind(PASSWORD)`
		`+ # Check the pw is now PBKDF2!`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('{PBKDF2_SHA256}')`
		`+`
		`+ def test_password_hash_on_upgrade_clearcrypt(topology_st):`
		`+ """In some deploymentes, some passwords MAY be in clear or crypt which have`
		`+ specific possible application integrations allowing the read value to be`
		`+ processed by other entities. We avoid upgrading these two, to prevent`
		`+ breaking these integrations.`
		`+`
		`+ :id: 27712492-a4bf-4ea9-977b-b4850ddfb628`
		`+ :setup: Single instance`
		`+ :steps: 1. Set a password hash in CLEAR, and hash to pbkdf2 statically`
		`+ 2. Test a correct bind`
		`+ 3. Assert the PW is CLEAR`
		`+ 4. Set the password to CRYPT`
		`+ 5. Test a correct bind`
		`+ 6. Assert the PW is CLEAR`
		`+ :expectedresults:`
		`+ 1. Successfully set the values`
		`+ 2. The bind succeeds`
		`+ 3. The PW is CLEAR`
		`+ 4. The set succeeds`
		`+ 4. The bind succeeds`
		`+ 5. The PW is CRYPT`
		`+ """`
		`+ # Make sure the server is set to pkbdf`
		`+ topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')`
		`+ topology_st.standalone.config.set('nsslapd-enable-upgrade-hash', 'on')`
		`+`
		`+ users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)`
		`+ user = users.create_test_user(1001)`
		`+`
		`+ topology_st.standalone.config.set('passwordStorageScheme', 'CLEAR')`
		`+ user.set('userPassword', "password")`
		`+ topology_st.standalone.config.set('passwordStorageScheme', 'PBKDF2_SHA256')`
		`+`
		`+ conn = user.bind(PASSWORD)`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('password')`
		`+`
		`+ user.set('userPassword', "{crypt}I0S3Ry62CSoFg")`
		`+ conn = user.bind(PASSWORD)`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('{crypt}')`
		`+`
		`+ def test_password_hash_on_upgrade_disable(topology_st):`
		`+ """If a legacy password hash is present, assert that on a correct bind`
		`+ the hash is "upgraded" to the latest-and-greatest hash format on the`
		`+ server. But some people may not like this, so test that we can disable`
		`+ the feature too!`
		`+`
		`+ :id: ed315145-a3d1-4f17-b04c-73d3638e7ade`
		`+ :setup: Single instance`
		`+ :steps: 1. Set a password hash in SSHA256, and hash to pbkdf2 statically`
		`+ 2. Test a faulty bind`
		`+ 3. Assert the PW is SSHA256`
		`+ 4. Test a correct bind`
		`+ 5. Assert the PW is SSHA256`
		`+ :expectedresults:`
		`+ 1. Successfully set the values`
		`+ 2. The bind fails`
		`+ 3. The PW is SSHA256`
		`+ 4. The bind succeeds`
		`+ 5. The PW is SSHA256`
		`+ """`
		`+ # Make sure the server is set to pkbdf`
		`+ topology_st.standalone.config.set('passwordStorageScheme', 'PBKDF2_SHA256')`
		`+ topology_st.standalone.config.set('nsslapd-allow-hashed-passwords', 'on')`
		`+ topology_st.standalone.config.set('nsslapd-enable-upgrade-hash', 'off')`
		`+`
		`+ users = UserAccounts(topology_st.standalone, DEFAULT_SUFFIX)`
		`+ user = users.create_test_user(1002)`
		`+ # Static version of "password" in SSHA256.`
		`+ user.set('userPassword', "{SSHA256}9eliEQgjfc4Fcj1IXZtc/ne1GRF+OIjz/NfSTX4f7HByGMQrWHLMLA==")`
		`+ # Attempt to bind with incorrect password.`
		`+ with pytest.raises(ldap.INVALID_CREDENTIALS):`
		`+ badconn = user.bind('badpassword')`
		`+ # Check the pw is SSHA256`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('{SSHA256}')`
		`+`
		`+ # Bind with correct.`
		`+ conn = user.bind(PASSWORD)`
		`+ # Check the pw is NOT upgraded!`
		`+ up = user.get_attr_val_utf8('userPassword')`
		`+ assert up.startswith('{SSHA256}')`

ldap/servers/slapd/bind.c

file modified

+10

		`@@ -762,6 +762,15 @@`
		`goto free_and_return;`
		`}`
		`}`
		`+`
		`+ /*`
		`+ * If required, update the pw hash to the "current setting" on bind`
		`+ * if it was successful.`
		`+ */`
		`+ if (config_get_enable_upgrade_hash()) {`
		`+ 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 +792,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);`

ldap/servers/slapd/libglobs.c

file modified

+42 -2

		`@@ -249,6 +249,7 @@`
		`#endif`
		`slapi_onoff_t init_extract_pem;`
		`slapi_onoff_t init_ignore_vattrs;`
		`+ slapi_onoff_t init_enable_upgrade_hash;`

		`static int`
		`isInt(ConfigVarType type)`
		`@@ -1232,8 +1233,11 @@`
		`NULL, 0,`
		`(void **)&global_slapdFrontendConfig.tls_check_crl,`
		`CONFIG_SPECIAL_TLS_CHECK_CRL, (ConfigGetFunc)config_get_tls_check_crl,`
		`- "none" /* Allow reset to this value */}`
		`-`
		`+ "none" /* Allow reset to this value */},`
		`+ {CONFIG_ENABLE_UPGRADE_HASH, config_set_enable_upgrade_hash,`
		`+ NULL, 0,`
		`+ (void **)&global_slapdFrontendConfig.enable_upgrade_hash,`
		`+ CONFIG_ON_OFF, (ConfigGetFunc)config_get_enable_upgrade_hash, &init_enable_upgrade_hash}`
		`/* End config */`
		`};`

		`@@ -1751,6 +1755,18 @@`
		`#endif`

		`init_extract_pem = cfg->extract_pem = LDAP_ON;`
		`+ /*`
		`+ * Default upgrade hash to on - this is an important security step, meaning that old`
		`+ * or legacy hashes are upgraded on bind. It means we are proactive in securing accounts`
		`+ * that may have infrequent on no password changes (which is current best practice in`
		`+ * computer security).`
		`+ *`
		`+ * A risk is that some accounts may use clear/crypt for other application integrations`
		`+ * where the hash is "read" from the account. To avoid this, these two hashes are NEVER`
		`+ * upgraded - in other words, "ON" means only MD5, SHA*, are upgraded to the "current"`
		`+ * scheme set in cn=config`
		`+ */`
		`+ init_enable_upgrade_hash = cfg->enable_upgrade_hash = LDAP_ON;`

		`/* Done, unlock! */`
		`CFG_UNLOCK_WRITE(cfg);`
		`@@ -7589,6 +7605,30 @@`
		`return retVal;`
		`}`

		`+ int32_t`
		`+ config_get_enable_upgrade_hash()`
		`+ {`
		`+ int32_t retVal;`
		`+ slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();`
		`+ CFG_LOCK_READ(slapdFrontendConfig);`
		`+ retVal = slapdFrontendConfig->enable_upgrade_hash;`
		`+ CFG_UNLOCK_READ(slapdFrontendConfig);`
		`+`
		`+ return retVal;`
		`+ }`
		`+`
		`+ int32_t`
		`+ config_set_enable_upgrade_hash(const char attrname, char value, char *errorbuf, int32_t apply)`
		`+ {`
		`+ int32_t retVal = LDAP_SUCCESS;`
		`+ slapdFrontendConfig_t *slapdFrontendConfig = getFrontendConfig();`
		`+`
		`+ retVal = config_set_onoff(attrname, value,`
		`+ &(slapdFrontendConfig->enable_upgrade_hash),`
		`+ errorbuf, apply);`
		`+ return retVal;`
		`+ }`
		`+`
		`static char *`
		`config_initvalue_to_onoff(struct config_get_and_set cgas, char initvalbuf, size_t initvalbufsize)`
		`{`

ldap/servers/slapd/proto-slap.h

file modified

		`@@ -588,6 +588,9 @@`
		`int config_get_maxsimplepaged_per_conn(void);`
		`int config_get_extract_pem(void);`

		`+ int32_t config_get_enable_upgrade_hash(void);`
		`+ int32_t config_set_enable_upgrade_hash(const char attrname, char value, char *errorbuf, int apply);`
		`+`
		`int is_abspath(const char *);`
		`char rel2abspath(char );`
		`char rel2abspath_ext(char , char *);`

ldap/servers/slapd/pw.c

file modified

+126

		`@@ -661,6 +661,13 @@`
		`"Param error - no password entry/target dn/operation\n");`
		`return -1;`
		`}`
		`+`
		`+ /* If we have been requested to skip updating this data, check now */`
		`+ if (slapi_operation_is_flag_set(operation, OP_FLAG_ACTION_SKIP_PWDPOLICY)) {`
		`+ /* No action required! */`
		`+ return 0;`
		`+ }`
		`+`
		`internal_op = slapi_operation_is_flag_set(operation, SLAPI_OP_FLAG_INTERNAL);`
		`target_dn = slapi_sdn_get_ndn(sdn);`
		`pwpolicy = new_passwdPolicy(pb, target_dn);`
		`@@ -3247,3 +3254,122 @@`
		`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 ),`
		`+ */`
		`+ int32_t update_pw_encoding(Slapi_PBlock orig_pb, Slapi_Entry e, Slapi_DN sdn, char cleartextpassword) {`
		`+ 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;`
		`+ char *hashed_val = NULL;`
		`+ Slapi_PBlock *pb = NULL;`
		`+ int32_t res = 0;`
		`+`
		`+ slapi_mods_init(&smods, 0);`
		`+`
		`+ /*`
		`+ * Does the entry have a pw?`
		`+ */`
		`+ 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",`
		`+ dn);`
		`+ res = -1;`
		`+ goto free_and_return;`
		`+ }`
		`+`
		`+ /*`
		`+ * Get the current system pw policy.`
		`+ */`
		`+ 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;`
		`+ }`
		`+`
		`+ /*`
		`+ * If the scheme is the same as current, do nothing!`
		`+ */`
		`+ 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;`
		`+ }`
		`+ /*`
		`+ * If the scheme is clear or crypt, we also do nothing to prevent breaking some application`
		`+ * integrations. See pwdstorage.h`
		`+ */`
		`+ if (strcmp(curpwsp->pws_name, "CLEAR") == 0 \|\| strcmp(curpwsp->pws_name, "CRYPT") == 0) {`
		`+ res = 0; // Nothing to do`
		`+ goto free_and_return;`
		`+ }`
		`+`
		`+ /*`
		`+ * We are commited to upgrading the hash content now!`
		`+ */`
		`+`
		`+ 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);`
		`+ 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 */`
		`+ 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_ACTION_SKIP_PWDPOLICY); /* 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);`
		`+ if (pb) slapi_pblock_destroy(pb);`
		`+ slapi_mods_done(&smods);`
		`+ return res;`
		`+ }`

ldap/servers/slapd/slap.h

file modified

		`@@ -2198,6 +2198,7 @@`
		`#define CONFIG_MODDN_ACI_ATTRIBUTE "nsslapd-moddn-aci"`
		`#define CONFIG_GLOBAL_BACKEND_LOCK "nsslapd-global-backend-lock"`
		`#define CONFIG_ENABLE_NUNC_STANS "nsslapd-enable-nunc-stans"`
		`+ #define CONFIG_ENABLE_UPGRADE_HASH "nsslapd-enable-upgrade-hash"`
		`#define CONFIG_CONFIG_ATTRIBUTE "nsslapd-config"`
		`#define CONFIG_INSTDIR_ATTRIBUTE "nsslapd-instancedir"`
		`#define CONFIG_SCHEMADIR_ATTRIBUTE "nsslapd-schemadir"`
		`@@ -2531,6 +2532,7 @@`
		`int malloc_mmap_threshold; /* mallopt M_MMAP_THRESHOLD */`
		`#endif`
		`slapi_onoff_t extract_pem; /* If "on", export key/cert as pem files */`
		`+ slapi_onoff_t enable_upgrade_hash; /* If on, upgrade hashes for PW at bind */`
		`} slapdFrontendConfig_t;`

		`/* possible values for slapdFrontendConfig_t.schemareplace */`

ldap/servers/slapd/slapi-plugin.h

file modified

-1

		`@@ -5385,7 +5385,6 @@`
		`*/`
		`#define SLAPI_USERPWD_ATTR "userpassword"`
		`int slapi_pw_find_sv(Slapi_Value *vals, const Slapi_Value v);`
		`-`
		`/* value encoding encoding */`
		`/* checks if the value is encoded with any known algorithm*/`
		`int slapi_is_encoded(char *value);`

ldap/servers/slapd/slapi-private.h

file modified

		`@@ -426,6 +426,9 @@`
		`#define OP_FLAG_BULK_IMPORT 0x800000 /* operation is bulk import */`
		`#define OP_FLAG_NOOP 0x01000000 /* operation results from urp and`
		`* should be ignored */`
		`+ #define OP_FLAG_ACTION_SKIP_PWDPOLICY 0x02000000 /* Skip applying pw policy rules to the password`
		`+ * change operation, as it's from an upgrade on`
		`+ * bind rather than a normal password change */`

		`/* reverse search states */`
		`#define REV_STARTED 1`
		`@@ -803,6 +806,9 @@`
		`int pw_rever_encode(Slapi_Value *vals, char attr_name);`
		`int pw_rever_decode(char cipher, char plain, const char attr_name);`

		`+ int32_t update_pw_encoding(Slapi_PBlock orig_pb, Slapi_Entry e, Slapi_DN sdn, char cleartextpassword);`
		`+`
		`+`
		`/* config routines */`

		`int slapi_config_get_readonly(void);`

firstyear commented 4 years ago

Bug Description: As time goes on, password hash mechanisms
change and need to become more resistant to brute force and
other attacks. However long lived, and service passwords do
not change frequently - and in fact, frequent password changes
is a security anti-pattern which is now discouraged.

As a result, it's important to be able to improve the
cryptographic strength and resitance of our passwords for
users as time goes on.

Fix Description: We can implement this because during a bind
operation we have short amount of access to the plaintext
password - we then use that to upgrade the content of the
hash.

tbordaz commented on line 7 of ldap/servers/slapd/bind.c 4 years ago

Later need_new_pw will check if the password will be updated soon (password expired or about to expire). In such case this is not urgent to check/reencode the current password. Could it be done only if need_new_pwd has not requested a new passwd ?

tbordaz commented 4 years ago

The idea and the patch are nice. I was wondering if the detection should be part of a kind of healthcheck tools (kind of insight rules), to let the admin decide what/when to do the update.

firstyear commented 4 years ago

I think the issue with the healthcheck approach is we would need to substring index on userPassword, and the admin would have to search for these which could be really costly? I think also having a choice of "when" could be really complex to implement policy wise, and also relies on the admin to fix it.

In my mind the healthcheck tool is so that things we can't automatically fix, can be raised. I think given how simple this is, automatically fixing it is more reasonable than a warning/review because we can easily update it. An example we can't automaticalyl fix is weak ACI's for example, or a missing index (because we don't know if the query is flawed or some other issue exists). So I'd rather just have this "as it is" because it's simple, and most admins will not have to think about the process at all.

I think the need_new_pwd idea is good, but I would wonder if it adds complexity to testing an asserting the behaviour? The test case now is "correct and simple". We can really strongly look at the test here and say "yes, it works!". To add in the need_new_pw behaviour, maybe this is an optimisation for time-performance that is not such a big concern, but brings larger complexity in testing that the code works? So I think I'm going to avoid over-complicating this until it becomes a problem, because I think in most cases the upgrade would be avoided (already done), and the cost is low when it is done anyway.

There is another update to the patch about to come which avoids updating clear/crypt to avoid breaking super-legacy and weird integrations, and adds an on-off switch too.

rebased onto 4b5c9c8ecf16fc20e8c1bc492245f6df8c5faa1c

4 years ago

firstyear commented 4 years ago

Okay, this has been updated now. Please review @tbordaz and @codehotter!

codehotter commented 4 years ago

Awesome!

What happened to the concern about repurposing replication flag for a non-obvious purpose?

firstyear commented on line 101 of ldap/servers/slapd/pw.c 4 years ago

@codehotter has reminded me about this issue, so I should double check about the correct way to handle this because op_repl may prevent csn creation?

firstyear commented 4 years ago

Okay, so just thinking about it, using op_flag_repl, even without any other possible side effects, is wrong as it's confusing and not clear, especially if we change op-flag-repl later to do more. So I'm going to add a new flag (OP_FLAG_ACTION_SKIP_PWDPOLICY) which has a specific behaviour to only allow this change to skip pw policy checking and manipulations.

rebased onto 1873d94be70adf6a78e8f6425795c435bab078c6

4 years ago

firstyear commented 4 years ago

Okay, this update changes the flag and adds the needed check to update_pw_info to avoid the history and other update. I'd like @mreynolds to check this though because he knows this part of the code better than I do, and I'd appreciate his expert input if I have done this correctly.

It's probably worth checking in the test the pwdHistory isn't modified - is that as simple as reading the passwordHistory attribute from the entry?

mreynolds commented 4 years ago

Okay, this update changes the flag and adds the needed check to update_pw_info to avoid the history and other update. I'd like @mreynolds to check this though because he knows this part of the code better than I do, and I'd appreciate his expert input if I have done this correctly.

What about FIPS mode and PBDFK2? If you enable fips after server install the default hashing scheme is still PBDFK2. I think there was code to us a different scheme, but I don't recall if it updates the scheme in the internal password policy. Please test this works correctly in FIPS mode. If it does you have my ack!

But I'm wondering if this should go into 1.4.2? Which means I need to branch master to 1.4.1 first.

firstyear commented 4 years ago

@mreynolds I don't have a fips capable system?

Saying that it sounds like the fix is as you say, to have the different scheme used when fips is active ... but I'm not sure what would be the best way to implement that.

An option is to merge this now as default off, then on 1.4.2 we flick it to default on?

firstyear commented 4 years ago

In libglobs.c:4126 couldn't we do something like:

new_scheme = NULL
if (fips) {
    new_scheme = pw_name2scheme(DEFAULT_FIPS_SCHEME)
} else {
    new_scheme = pw_name2scheme(DEFAULT_PASSWORD_SCHEME)
}

That would take care of this and other concerns I think, because then it would cause us to be able to on-upgrade improve both the fips and non-fips value, and means we'd properly respect and change the scheme based on the fips flag.

But I also think this may be out of scope of the ticket here ... thoughts?

mhonek commented 4 years ago

When in FIPS aren't we doomed already since we cannot even check if the password is valid since the algorithm is not allowed? Like:

Jul 12 02:58:08 b ns-slapd[19129]: [12/Jul/2019:02:58:08.611940106 +0000] - ERR - PBKDF2_SHA256 - Unable to extract hash output.
Jul 12 02:58:08 b ns-slapd[19129]: [12/Jul/2019:02:58:08.618619292 +0000] - ERR - PBKDF2_SHA256 - Unable to hash userpwd value

firstyear commented 4 years ago

@mhonek If that's still an issue, I think we need some more concrete fips handling of the pw algo setup then, so this sounds like it's own issue ....

mhonek commented 4 years ago

@mhonek If that's still an issue, I think we need some more concrete fips handling of the pw algo setup then, so this sounds like it's own issue ....

Correct. The hashing fails because PK11_ExtractKeyValue call is disallowed in FIPS. We'd need to find a different way of storing the symmetric key, which is a different issue.

firstyear commented 4 years ago

Okay, I think we are off track here - there is clearly a FIPS and PBKDF2 issue - lets open an issue for that, and focus on the correct fix for it there. I think it's not related to or part of this issue.

With that in mind, any comments on this patch then?

mreynolds commented 4 years ago

ack

rebased onto 3aa1416

4 years ago

Pull-Request has been merged by firstyear

4 years ago

mreynolds commented 4 years ago

@firstyear. This really needs a design doc, and it should be added to the wiki: https://www.port389.org/docs/389ds/design/design.html

The doc team uses the design page for updating the admin guide.

Thanks!

firstyear commented 4 years ago