From 8a7b47602acc910d2f64439b81af3299b60359c8 Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Mon, 18 Sep 2017 10:35:20 -0400 Subject: [PATCH] Ticket 49379 - Allowed sasl mapping requires restart Bug Description: If allowed sasl mechanisms are configured, and the server is restarted, trying to add new sasl mechanisms does not get applied until the server is restarted again. [1] We were also overwriting memory when we stripped the commas from the allowed machanism list. THis lead to the allowed mechanisms to get truncated,and permanently lose certain mechs. [2] A crash with PLAIN sasl mechanism was also found. [3] Fix Description: To address allowed sasl mechs, we no longer explicitly the mechanisms during the sasl_init at server startup. Instead we check the allowed list ourselves during a bind. [1] When setting the allowed sasl mechs, make a copy of the value to apply the changes to(removing coamms), and do not change the original value as it's still being used. [2] The crash when using sasl PLAIN was due to unlocking a rwlock that was not locked. [3] https://pagure.io/389-ds-base/issue/49379 Reviewed by: tbordaz(Thanks!) (cherry picked from commit c78f41db31752a99aadd6abcbf7a1d852a8e7931) --- dirsrvtests/tests/suites/sasl/allowed_mechs.py | 114 ++++++++++++++++++++++-- dirsrvtests/tests/suites/sasl/plain.py | 10 ++- ldap/servers/slapd/libglobs.c | 23 ++--- ldap/servers/slapd/saslbind.c | 116 +++++++++++++------------ 4 files changed, 187 insertions(+), 76 deletions(-) diff --git a/dirsrvtests/tests/suites/sasl/allowed_mechs.py b/dirsrvtests/tests/suites/sasl/allowed_mechs.py index 7958db4..5b1b92c 100644 --- a/dirsrvtests/tests/suites/sasl/allowed_mechs.py +++ b/dirsrvtests/tests/suites/sasl/allowed_mechs.py @@ -8,45 +8,141 @@ # import pytest -import ldap - -import time - +import os from lib389.topologies import topology_st + def test_sasl_allowed_mechs(topology_st): + """Test the alloweed sasl mechanism feature + + :ID: ab7d9f86-8cfe-48c3-8baa-739e599f006a + :feature: Allowed sasl mechanisms + :steps: 1. Get the default list of mechanisms + 2. Set allowed mechanism PLAIN, and verify it's correctly listed + 3. Restart server, and verify list is still correct + 4. Test EXTERNAL is properly listed + 5. Add GSSAPI to the existing list, and verify it's correctly listed + 6. Restart server and verify list is still correct + 7. Add ANONYMOUS to the existing list, and veirfy it's correctly listed + 8. Restart server and verify list is still correct + 9. Remove GSSAPI and verify it's correctly listed + 10. Restart server and verify list is still correct + 11. Reset allowed list to nothing, verify "all" the mechanisms are returned + 12. Restart server and verify list is still correct + + :expectedresults: The supported mechanisms supported what is set for the allowed + mechanisms + """ standalone = topology_st.standalone # Get the supported mechs. This should contain PLAIN, GSSAPI, EXTERNAL at least + standalone.log.info("Test we have some of the default mechanisms") orig_mechs = standalone.rootdse.supported_sasl() print(orig_mechs) assert('GSSAPI' in orig_mechs) assert('PLAIN' in orig_mechs) assert('EXTERNAL' in orig_mechs) - # Now edit the supported mechs. CHeck them again. + # Now edit the supported mechs. Check them again. + standalone.log.info("Edit mechanisms to allow just PLAIN") standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN') + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) # Should always be in the allowed list, even if not set. + assert('GSSAPI' not in limit_mechs) # Should not be there! + # Restart the server a few times and make sure nothing changes + standalone.log.info("Restart server and make sure we still have correct allowed mechs") + standalone.restart() + standalone.restart() limit_mechs = standalone.rootdse.supported_sasl() assert('PLAIN' in limit_mechs) - # Should always be in the allowed list, even if not set. assert('EXTERNAL' in limit_mechs) - # Should not be there! assert('GSSAPI' not in limit_mechs) + # Set EXTERNAL, even though its always supported + standalone.log.info("Edit mechanisms to allow just PLAIN and EXTERNAL") standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN, EXTERNAL') + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' not in limit_mechs) + + # Now edit the supported mechs. Check them again. + standalone.log.info("Edit mechanisms to allow just PLAIN and GSSAPI") + standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN, GSSAPI') + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' in limit_mechs) + assert(len(limit_mechs) == 3) + + # Restart server twice and make sure the allowed list is the same + standalone.restart() + standalone.restart() # For ticket 49379 (test double restart) + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' in limit_mechs) + assert(len(limit_mechs) == 3) + + # Add ANONYMOUS to the supported mechs and test again. + standalone.log.info("Edit mechanisms to allow just PLAIN, GSSAPI, and ANONYMOUS") + standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN, GSSAPI, ANONYMOUS') + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' in limit_mechs) + assert('ANONYMOUS' in limit_mechs) + assert(len(limit_mechs) == 4) + + # Restart server and make sure the allowed list is the same + standalone.restart() + standalone.restart() # For ticket 49379 (test double restart) + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' in limit_mechs) + assert('ANONYMOUS' in limit_mechs) + assert(len(limit_mechs) == 4) + # Remove GSSAPI + standalone.log.info("Edit mechanisms to allow just PLAIN and ANONYMOUS") + standalone.config.set('nsslapd-allowed-sasl-mechanisms', 'PLAIN, ANONYMOUS') limit_mechs = standalone.rootdse.supported_sasl() assert('PLAIN' in limit_mechs) assert('EXTERNAL' in limit_mechs) - # Should not be there! assert('GSSAPI' not in limit_mechs) + assert('ANONYMOUS' in limit_mechs) + assert(len(limit_mechs) == 3) + + # Restart server and make sure the allowed list is the same + standalone.restart() + limit_mechs = standalone.rootdse.supported_sasl() + assert('PLAIN' in limit_mechs) + assert('EXTERNAL' in limit_mechs) + assert('GSSAPI' not in limit_mechs) + assert('ANONYMOUS' in limit_mechs) + assert(len(limit_mechs) == 3) # Do a config reset + standalone.log.info("Reset allowed mechaisms") standalone.config.reset('nsslapd-allowed-sasl-mechanisms') # check the supported list is the same as our first check. + standalone.log.info("Check that we have the original set of mechanisms") final_mechs = standalone.rootdse.supported_sasl() - print(final_mechs) assert(set(final_mechs) == set(orig_mechs)) + # Check it after a restart + standalone.log.info("Check that we have the original set of mechanisms after a restart") + standalone.restart() + final_mechs = standalone.rootdse.supported_sasl() + assert(set(final_mechs) == set(orig_mechs)) + + +if __name__ == '__main__': + # Run isolated + # -s for DEBUG mode + CURRENT_FILE = os.path.realpath(__file__) + pytest.main("-s %s" % CURRENT_FILE) diff --git a/dirsrvtests/tests/suites/sasl/plain.py b/dirsrvtests/tests/suites/sasl/plain.py index 91ccb02..6bf39a8 100644 --- a/dirsrvtests/tests/suites/sasl/plain.py +++ b/dirsrvtests/tests/suites/sasl/plain.py @@ -15,9 +15,11 @@ from lib389.topologies import topology_st from lib389.utils import * from lib389.sasl import PlainSASL from lib389.idm.services import ServiceAccounts +from lib389._constants import (SECUREPORT_STANDALONE1, DEFAULT_SUFFIX) log = logging.getLogger(__name__) + def test_sasl_plain(topology_st): standalone = topology_st.standalone @@ -38,7 +40,7 @@ def test_sasl_plain(topology_st): standalone.rsa.create() # Set the secure port and nsslapd-security # Could this fail with selinux? - standalone.config.set('nsslapd-secureport', '%s' % SECUREPORT_STANDALONE1 ) + standalone.config.set('nsslapd-secureport', '%s' % SECUREPORT_STANDALONE1) standalone.config.set('nsslapd-security', 'on') # Do we need to restart to allow starttls? standalone.restart() @@ -65,12 +67,14 @@ def test_sasl_plain(topology_st): # I can not solve. I think it's leaking state across connections in start_tls_s? # Check that it works with TLS - conn = standalone.openConnection(saslmethod='PLAIN', sasltoken=auth_tokens, starttls=True, connOnly=True, certdir=standalone.get_cert_dir(), reqcert=ldap.OPT_X_TLS_NEVER) + conn = standalone.openConnection(saslmethod='PLAIN', sasltoken=auth_tokens, starttls=True, connOnly=True, + certdir=standalone.get_cert_dir(), reqcert=ldap.OPT_X_TLS_NEVER) conn.close() # Check that it correct fails our bind if we don't have the password. auth_tokens = PlainSASL("dn:%s" % sa.dn, 'password-wrong') with pytest.raises(ldap.INVALID_CREDENTIALS): - standalone.openConnection(saslmethod='PLAIN', sasltoken=auth_tokens, starttls=False, connOnly=True, certdir=standalone.get_cert_dir(), reqcert=ldap.OPT_X_TLS_NEVER) + standalone.openConnection(saslmethod='PLAIN', sasltoken=auth_tokens, starttls=True, connOnly=True, + certdir=standalone.get_cert_dir(), reqcert=ldap.OPT_X_TLS_NEVER) # Done! diff --git a/ldap/servers/slapd/libglobs.c b/ldap/servers/slapd/libglobs.c index bb51827..2fb4bab 100644 --- a/ldap/servers/slapd/libglobs.c +++ b/ldap/servers/slapd/libglobs.c @@ -7137,22 +7137,25 @@ config_set_allowed_sasl_mechs(const char *attrname, char *value, char *errorbuf, /* During a reset, the value is "", so we have to handle this case. */ if (strcmp(value, "") != 0) { - /* cyrus sasl doesn't like comma separated lists */ - remove_commas(value); + char *nval = slapi_ch_strdup(value); - if(invalid_sasl_mech(value)){ - slapi_log_err(SLAPI_LOG_ERR,"config_set_allowed_sasl_mechs", - "Invalid value/character for sasl mechanism (%s). Use ASCII " - "characters, upto 20 characters, that are upper-case letters, " - "digits, hyphens, or underscores\n", value); + /* cyrus sasl doesn't like comma separated lists */ + remove_commas(nval); + + if (invalid_sasl_mech(nval)) { + slapi_log_err(SLAPI_LOG_ERR, "config_set_allowed_sasl_mechs", + "Invalid value/character for sasl mechanism (%s). Use ASCII " + "characters, upto 20 characters, that are upper-case letters, " + "digits, hyphens, or underscores\n", + nval); + slapi_ch_free_string(&nval); return LDAP_UNWILLING_TO_PERFORM; } - CFG_LOCK_WRITE(slapdFrontendConfig); slapi_ch_free_string(&slapdFrontendConfig->allowed_sasl_mechs); slapi_ch_array_free(slapdFrontendConfig->allowed_sasl_mechs_array); - slapdFrontendConfig->allowed_sasl_mechs = slapi_ch_strdup(value); - slapdFrontendConfig->allowed_sasl_mechs_array = slapi_str2charray_ext(value, " ", 0); + slapdFrontendConfig->allowed_sasl_mechs = nval; + slapdFrontendConfig->allowed_sasl_mechs_array = slapi_str2charray_ext(nval, " ", 0); CFG_UNLOCK_WRITE(slapdFrontendConfig); } else { /* If this value is "", we need to set the list to *all* possible mechs */ diff --git a/ldap/servers/slapd/saslbind.c b/ldap/servers/slapd/saslbind.c index 134f5aa..03e2a97 100644 --- a/ldap/servers/slapd/saslbind.c +++ b/ldap/servers/slapd/saslbind.c @@ -169,8 +169,6 @@ static int ids_sasl_getopt( } } else if (strcasecmp(option, "auxprop_plugin") == 0) { *result = "iDS"; - } else if (strcasecmp(option, "mech_list") == 0){ - *result = config_get_allowed_sasl_mechs(); } if (*result) *len = strlen(*result); @@ -572,12 +570,8 @@ static int ids_sasl_userdb_checkpass(sasl_conn_t *conn, void *context, const cha slapi_pblock_set(pb, SLAPI_BIND_METHOD, &method); /* Feed it to pw_verify_be_dn */ bind_result = pw_verify_be_dn(pb, &referral); - /* Now check the result, and unlock be if needed. */ - if (bind_result == SLAPI_BIND_SUCCESS || bind_result == SLAPI_BIND_ANONYMOUS) { - Slapi_Backend *be = NULL; - slapi_pblock_get(pb, SLAPI_BACKEND, &be); - slapi_be_Unlock(be); - } else if (bind_result == SLAPI_BIND_REFERRAL) { + /* Now check the result. */ + if (bind_result == SLAPI_BIND_REFERRAL) { /* If we have a referral do we ignore it for sasl? */ slapi_entry_free(referral); } @@ -760,22 +754,25 @@ char **ids_sasl_listmech(Slapi_PBlock *pb) sup_ret = slapi_get_supported_saslmechanisms_copy(); /* If we have a connection, get the provided list from SASL */ - if (pb->pb_conn != NULL) { - sasl_conn = (sasl_conn_t*)pb->pb_conn->c_sasl_conn; - - /* sasl library mechanisms are connection dependent */ - PR_EnterMonitor(pb->pb_conn->c_mutex); - if (sasl_listmech(sasl_conn, - NULL, /* username */ - "", ",", "", - &str, NULL, NULL) == SASL_OK) { - slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_listmech", "sasl library mechs: %s\n", str); - /* merge into result set */ - dupstr = slapi_ch_strdup(str); - others = slapi_str2charray_ext(dupstr, ",", 0 /* don't list duplicate mechanisms */); - charray_merge(&sup_ret, others, 1); - charray_free(others); - slapi_ch_free((void**)&dupstr); + if (pb_conn != NULL) { + sasl_conn = (sasl_conn_t*)pb_conn->c_sasl_conn; + if (sasl_conn != NULL) { + /* sasl library mechanisms are connection dependent */ + PR_EnterMonitor(pb_conn->c_mutex); + if (sasl_listmech(sasl_conn, + NULL, /* username */ + "", ",", "", + &str, NULL, NULL) == SASL_OK) { + slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_listmech", "sasl library mechs: %s\n", str); + /* merge into result set */ + dupstr = slapi_ch_strdup(str); + others = slapi_str2charray_ext(dupstr, ",", 0 /* don't list duplicate mechanisms */); + + charray_merge(&sup_ret, others, 1); + charray_free(others); + slapi_ch_free((void**)&dupstr); + } + PR_ExitMonitor(pb_conn->c_mutex); } PR_ExitMonitor(pb->pb_conn->c_mutex); } @@ -785,7 +782,7 @@ char **ids_sasl_listmech(Slapi_PBlock *pb) /* Remove any content that isn't in the allowed list */ if (config_ret != NULL) { - /* Get the set of supported mechs in the insection of the two */ + /* Get the set of supported mechs in the intersection of the two */ ret = charray_intersection(sup_ret, config_ret); charray_free(sup_ret); charray_free(config_ret); @@ -816,41 +813,52 @@ char **ids_sasl_listmech(Slapi_PBlock *pb) static int ids_sasl_mech_supported(Slapi_PBlock *pb, const char *mech) { - int i, ret = 0; - char **mechs; - char *dupstr; - const char *str; - int sasl_result = 0; - sasl_conn_t *sasl_conn = (sasl_conn_t *)pb->pb_conn->c_sasl_conn; - - slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_mech_supported", "=>\n"); - - - /* sasl_listmech is not thread-safe - caller must lock pb_conn */ - sasl_result = sasl_listmech(sasl_conn, - NULL, /* username */ - "", ",", "", - &str, NULL, NULL); - if (sasl_result != SASL_OK) { - return 0; - } + int i, ret = 0; + char **mechs; + char **allowed_mechs = NULL; + char *dupstr; + const char *str; + int sasl_result = 0; + Connection *pb_conn = NULL; + + slapi_pblock_get(pb, SLAPI_CONNECTION, &pb_conn); + sasl_conn_t *sasl_conn = (sasl_conn_t *)pb_conn->c_sasl_conn; + slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_mech_supported", "=>\n"); + + /* sasl_listmech is not thread-safe - caller must lock pb_conn */ + sasl_result = sasl_listmech(sasl_conn, + NULL, /* username */ + "", ",", "", + &str, NULL, NULL); + if (sasl_result != SASL_OK) { + return 0; + } - dupstr = slapi_ch_strdup(str); - mechs = slapi_str2charray(dupstr, ","); + dupstr = slapi_ch_strdup(str); + mechs = slapi_str2charray(dupstr, ","); + allowed_mechs = config_get_allowed_sasl_mechs_array(); - for (i = 0; mechs[i] != NULL; i++) { - if (strcasecmp(mech, mechs[i]) == 0) { - ret = 1; - break; + for (i = 0; mechs[i] != NULL; i++) { + if (strcasecmp(mech, mechs[i]) == 0) { + if (allowed_mechs) { + if (charray_inlist(allowed_mechs, (char *)mech) == 0) { + ret = 1; + } + break; + } else { + ret = 1; + break; + } + } } - } - charray_free(mechs); - slapi_ch_free((void**)&dupstr); + charray_free(allowed_mechs); + charray_free(mechs); + slapi_ch_free((void **)&dupstr); - slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_mech_supported", "<=\n"); + slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_mech_supported", "<=\n"); - return ret; + return ret; } /* -- 2.9.5