> On Sun, 2012-02-05 at 21:18 +0100, Jan Zeleny wrote: > > Stephen Gallagher <[email protected]> wrote: > > > On Fri, 2012-02-03 at 15:45 +0100, Jan Zelený wrote: > > > > Please note that I haven't fully tested this yet, the LDAP server > > > > configuration needed for this is a little bit twisted ;-) I will > > > > perform more testing during the weekend. Consider this patch being > > > > preliminary and don't push it until it's tested. > > > > > > Nack. > > > > > > You need to update the IPA LDAP options as well: > > > Running suite(s): ipa_ldap_opt > > > 50%: Checks: 2, Failures: 1, Errors: 0 > > > /home/sgallagh/workspace/sssd/src/tests/ipa_ldap_opt-tests.c:79:F:ipa_l > > > dap_ opt:test_check_num_opts:0: Failure 'IPA_OPTS_BASIC_TEST != > > > SDAP_OPTS_BASIC' occured > > > > Done > > Nack. Just updating the IPA_OPTS_BASIC_TEST value isn't sufficient. You > also need to add the equivalent line in ipa_common.c for > ipa_def_ldap_opts. I made this same mistake in a patch last week, don't > copy my bad behaviors :)
Good catch, fixed > > > > Just for aesthetic reasons, please don't use the prefix sdap_ldap_*. > > > "sdap" is already shorthand for SSSD LDAP. sdap_ldap_result() is a > > > special case, since it's specifically a handler for "ldap_result" > > > structures. In general, I'd prefer that you call it something like > > > sdap_modify_shadow_lastchange_send(). > > > > Done > > You didn't change sdap_ldap_passwd_change_done(). But on further > reflection, this should be named differently anyway (since we're not > changing the password with this function, just the lastChanged time). > Maybe call it sdap_lastchange_done() or similar? Yes, I meant to rename that as well. I guess Sunday evening is not the best time to do patches for me ;-) Fixed > > > Why are you changing the user's password? That's not only out of scope, > > > but you're setting their password in plaintext on the server, if I'm > > > reading this correctly (you're modifying the entry with the raw text of > > > the new password). We ONLY support password modification via > > > password-change extended operation. > > > > > > The scope of https://fedorahosted.org/sssd/ticket/1019 is ONLY to > > > update the last change time. We absolutely don't want people using > > > shadow passwords; they're extremely insecure (highly vulnerable to > > > offline dictionary attack). > > > > As I wrote before, I'd like to consult this with you because I have no > > idea how to test this and all my attempts were unsuccessful - I didn't > > even manage to reproduce the original behavior. > > Yeah, I'm beginning to get the sense that the original behavior was > actually the customer using the password hash directly through the > shadow map (and verified using pam_unix.so, not pam_ldap.so). I don't > think we want to support that at all. > > Our reasonable nod to this atrocity will be to modify the lastChange > time (which you're doing correctly, assuming that the LDAP server ACIs > are sufficiently stupid to allow this). Frankly, the more I was diving into this, the less I liked it. I am truly curious if this change will do any good for the original bug reporter. I still had no luck reproducing it the way it was described in the bug. > > > Please modify the manpage for ldap_pwd_policy in sssd-ldap(5) as "Note > > > that the current version of sssd cannot update this attribute during a > > > password change." is no longer accurate. > > > > Done > > > > > Please don't conflate LDAP errors and errno errors in the "ret" > > > variable in sdap_ldap_modify_passwd_done(). If for some reason > > > openldap changed the value of LDAP_SUCCESS, this would break under our > > > noses. > > > > Done > > ldap_parse_result() doesn't return an errno_t. It's an unspecified > 'int'. (Yes, errno_t happens to be internally defined as an int, but > let's play it safe here.) My bad. Fixed > > > The logic of the functions looks good. > > > > Thanks for the review > > Jan > > One more pass, then we should be good to go. Hopefully this is it. Just a note that I intentionally left out description in man page, I don't want to encourage this setup by any means. Jan
From 28c508e69bdea855a14156710d281b555f2a3dcd Mon Sep 17 00:00:00 2001 From: Jan Zeleny <[email protected]> Date: Fri, 3 Feb 2012 09:33:08 -0500 Subject: [PATCH] Update shadowLastChanged attribute during LDAP password change https://fedorahosted.org/sssd/ticket/1019 --- src/man/sssd-ldap.5.xml | 2 - src/providers/ipa/ipa_common.c | 1 + src/providers/ipa/ipa_common.h | 2 +- src/providers/ldap/ldap_auth.c | 46 +++++++++++++ src/providers/ldap/ldap_common.c | 1 + src/providers/ldap/sdap.h | 1 + src/providers/ldap/sdap_async.c | 132 ++++++++++++++++++++++++++++++++++++++ src/providers/ldap/sdap_async.h | 9 +++ 8 files changed, 191 insertions(+), 3 deletions(-) diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index 0d4885d24be4983d54d2270731e0e4bbf46e0098..a145e388fb770001ddf57ccc1ac5770d366ab7e4 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -1407,8 +1407,6 @@ <citerefentry><refentrytitle>shadow</refentrytitle> <manvolnum>5</manvolnum></citerefentry> style attributes to evaluate if the password has expired. - Note that the current version of sssd cannot - update this attribute during a password change. </para> <para> <emphasis>mit_kerberos</emphasis> - Use the attributes diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c index 2e6dad8ae4e5ac238cb953e2390ffa879ed19f17..bdf2e9a542b2843bfafb84e87aa89decf8bde89b 100644 --- a/src/providers/ipa/ipa_common.c +++ b/src/providers/ipa/ipa_common.c @@ -100,6 +100,7 @@ struct dp_option ipa_def_ldap_opts[] = { { "ldap_access_order", DP_OPT_STRING, NULL_STRING, NULL_STRING }, { "ldap_chpass_uri", DP_OPT_STRING, NULL_STRING, NULL_STRING }, { "ldap_chpass_dns_service_name", DP_OPT_STRING, NULL_STRING, NULL_STRING }, + { "ldap_chpass_update_last_change", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "ldap_enumeration_search_timeout", DP_OPT_NUMBER, { .number = 60 }, NULL_NUMBER }, /* Do not include ldap_auth_disable_tls_never_use_in_production in the * manpages or SSSDConfig API diff --git a/src/providers/ipa/ipa_common.h b/src/providers/ipa/ipa_common.h index 9cbd993f547e76d53e6d46747c7181fe6592061a..7fac1b2f9da30271c603dce294dbd7dcd6f020f5 100644 --- a/src/providers/ipa/ipa_common.h +++ b/src/providers/ipa/ipa_common.h @@ -35,7 +35,7 @@ struct ipa_service { /* the following defines are used to keep track of the options in the ldap * module, so that if they change and ipa is not updated correspondingly * this will trigger a runtime abort error */ -#define IPA_OPTS_BASIC_TEST 60 +#define IPA_OPTS_BASIC_TEST 61 #define IPA_OPTS_SVC_TEST 5 diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c index f58d52feed5c28b63864e42a2f6e04223c57afe1..170a34b9414eaf2885fb5b09bae5730557179759 100644 --- a/src/providers/ldap/ldap_auth.c +++ b/src/providers/ldap/ldap_auth.c @@ -45,6 +45,7 @@ #include "db/sysdb.h" #include "providers/ldap/ldap_common.h" #include "providers/ldap/sdap_async.h" +#include "providers/ldap/sdap_async_private.h" /* MIT Kerberos has the same hardcoded warning interval of 7 days. Due to the * fact that using the expiration time of a Kerberos password with LDAP @@ -705,6 +706,8 @@ struct sdap_pam_chpass_state { char *password; char *new_password; struct sdap_handle *sh; + + struct sdap_auth_ctx *ctx; }; static void sdap_auth4chpass_done(struct tevent_req *req); @@ -754,6 +757,7 @@ void sdap_pam_chpass_handler(struct be_req *breq) state->breq = breq; state->pd = pd; state->username = pd->user; + state->ctx = ctx; state->password = talloc_strndup(state, (char *)pd->authtok, pd->authtok_size); if (!state->password) goto done; @@ -782,6 +786,7 @@ done: sdap_pam_auth_reply(breq, dp_err, pd->pam_status); } +static void sdap_lastchange_done(struct tevent_req *req); static void sdap_auth4chpass_done(struct tevent_req *req) { struct sdap_pam_chpass_state *state = @@ -898,6 +903,8 @@ static void sdap_pam_chpass_done(struct tevent_req *req) int dp_err = DP_ERR_FATAL; int ret; char *user_error_message = NULL; + char *lastchanged_name; + struct tevent_req *subreq; size_t msg_len; uint8_t *msg; @@ -935,9 +942,48 @@ static void sdap_pam_chpass_done(struct tevent_req *req) } } + if (dp_opt_get_bool(state->ctx->opts->basic, + SDAP_CHPASS_UPDATE_LAST_CHANGE)) { + lastchanged_name = state->ctx->opts->user_map[SDAP_AT_SP_LSTCHG].name; + + subreq = sdap_modify_shadow_lastchange_send(state, + state->breq->be_ctx->ev, + state->sh, + state->dn, + lastchanged_name); + if (subreq == NULL) { + state->pd->pam_status = PAM_SYSTEM_ERR; + goto done; + } + + tevent_req_set_callback(subreq, sdap_lastchange_done, state); + return; + } + done: sdap_pam_auth_reply(state->breq, dp_err, state->pd->pam_status); } + +static void sdap_lastchange_done(struct tevent_req *req) +{ + struct sdap_pam_chpass_state *state = + tevent_req_callback_data(req, struct sdap_pam_chpass_state); + int dp_err = DP_ERR_FATAL; + errno_t ret; + + ret = sdap_modify_shadow_lastchange_recv(req); + if (ret != EOK) { + state->pd->pam_status = PAM_SYSTEM_ERR; + goto done; + } + + dp_err = DP_ERR_OK; + state->pd->pam_status = PAM_SUCCESS; + +done: + sdap_pam_auth_reply(state->breq, dp_err, state->pd->pam_status); +} + /* ==Perform-User-Authentication-and-Password-Caching===================== */ struct sdap_pam_auth_state { diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c index 3b8e0eeb5b96e2b5a3e780dc5ad31df859e33304..ce8848384be92a35e0404bd9c90816f14ec9f6b9 100644 --- a/src/providers/ldap/ldap_common.c +++ b/src/providers/ldap/ldap_common.c @@ -91,6 +91,7 @@ struct dp_option default_basic_opts[] = { { "ldap_access_order", DP_OPT_STRING, { "filter" }, NULL_STRING }, { "ldap_chpass_uri", DP_OPT_STRING, NULL_STRING, NULL_STRING }, { "ldap_chpass_dns_service_name", DP_OPT_STRING, NULL_STRING, NULL_STRING }, + { "ldap_chpass_update_last_change", DP_OPT_BOOL, BOOL_FALSE, BOOL_FALSE }, { "ldap_enumeration_search_timeout", DP_OPT_NUMBER, { .number = 60 }, NULL_NUMBER }, /* Do not include ldap_auth_disable_tls_never_use_in_production in the * manpages or SSSDConfig API diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h index 0a0f996d11fc22dd801c534e821c7b06a4f7fe82..4cc8408c73743e4b077f0be52287722e977c5914 100644 --- a/src/providers/ldap/sdap.h +++ b/src/providers/ldap/sdap.h @@ -202,6 +202,7 @@ enum sdap_basic_opt { SDAP_ACCESS_ORDER, SDAP_CHPASS_URI, SDAP_CHPASS_DNS_SERVICE_NAME, + SDAP_CHPASS_UPDATE_LAST_CHANGE, SDAP_ENUM_SEARCH_TIMEOUT, SDAP_DISABLE_AUTH_TLS, SDAP_PAGE_SIZE, diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c index c2f616bef13744916d2b0c5bab92524566959af9..8ea51050560a0f34477c903520bbfec39eae023e 100644 --- a/src/providers/ldap/sdap_async.c +++ b/src/providers/ldap/sdap_async.c @@ -691,6 +691,138 @@ int sdap_exop_modify_passwd_recv(struct tevent_req *req, return EOK; } +/* ==Update-passwordLastChanged-attribute====================== */ +struct update_last_changed_state { + struct tevent_context *ev; + struct sdap_handle *sh; + struct sdap_op *op; + + const char *dn; + LDAPMod **mods; +}; + +static void sdap_modify_shadow_lastchange_done(struct sdap_op *op, + struct sdap_msg *reply, + int error, void *pvt); + +struct tevent_req * +sdap_modify_shadow_lastchange_send(TALLOC_CTX *mem_ctx, + struct tevent_context *ev, + struct sdap_handle *sh, + const char *dn, + char *lastchanged_name) +{ + struct tevent_req *req; + struct update_last_changed_state *state; + char **values; + errno_t ret; + int msgid; + + req = tevent_req_create(mem_ctx, &state, struct update_last_changed_state); + if (req == NULL) { + return NULL; + } + + state->ev = ev; + state->sh = sh; + state->dn = dn; + state->mods = talloc_zero_array(state, LDAPMod *, 2); + if (state->mods == NULL) { + ret = ENOMEM; + goto done; + } + state->mods[0] = talloc_zero(state->mods, LDAPMod); + state->mods[1] = talloc_zero(state->mods, LDAPMod); + if (!state->mods[0] || !state->mods[1]) { + ret = ENOMEM; + goto done; + } + values = talloc_zero_array(state->mods[0], char *, 2); + if (values == NULL) { + ret = ENOMEM; + goto done; + } + values[0] = talloc_asprintf(values, "%ld", (long)time(NULL)); + if (values[0] == NULL) { + ret = ENOMEM; + goto done; + } + state->mods[0]->mod_op = LDAP_MOD_REPLACE; + state->mods[0]->mod_type = lastchanged_name; + state->mods[0]->mod_vals.modv_strvals = values; + state->mods[1] = NULL; + + ret = ldap_modify_ext(state->sh->ldap, state->dn, state->mods, + NULL, NULL, &msgid); + if (ret) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send operation!\n")); + goto done; + } + + ret = sdap_op_add(state, state->ev, state->sh, msgid, + sdap_modify_shadow_lastchange_done, req, 5, &state->op); + if (ret) { + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to set up operation!\n")); + goto done; + } + +done: + if (ret != EOK) { + tevent_req_error(req, ret); + tevent_req_post(req, ev); + } + return req; +} + +static void sdap_modify_shadow_lastchange_done(struct sdap_op *op, + struct sdap_msg *reply, + int error, void *pvt) +{ + struct tevent_req *req = talloc_get_type(pvt, struct tevent_req); + struct update_last_changed_state *state; + state = tevent_req_data(req, struct update_last_changed_state); + char *errmsg; + int result; + errno_t ret = EOK; + int lret; + + if (error) { + tevent_req_error(req, error); + return; + } + + lret = ldap_parse_result(state->sh->ldap, reply->msg, + &result, NULL, &errmsg, NULL, + NULL, 0); + if (lret != LDAP_SUCCESS) { + DEBUG(SSSDBG_OP_FAILURE, ("ldap_parse_result failed (%d)\n", + state->op->msgid)); + ret = EIO; + goto done; + } + + DEBUG(SSSDBG_OP_FAILURE, ("Updating lastPwdChange result: %s(%d), %s\n", + sss_ldap_err2string(result), + result, errmsg)); + +done: + ldap_memfree(errmsg); + + if (ret == EOK) { + tevent_req_done(req); + } else { + tevent_req_error(req, ret); + } +} + +errno_t sdap_modify_shadow_lastchange_recv(struct tevent_req *req) +{ + TEVENT_REQ_RETURN_ON_ERROR(req); + + return EOK; +} + + /* ==Fetch-RootDSE============================================= */ struct sdap_get_rootdse_state { diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h index 8f8af47d5b17792325eb504d0b25f3658bb766ba..47d1014925932ae93c5c9d4c45b78d3babe394dd 100644 --- a/src/providers/ldap/sdap_async.h +++ b/src/providers/ldap/sdap_async.h @@ -132,6 +132,15 @@ int sdap_exop_modify_passwd_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx, enum sdap_result *result, char **user_error_msg); +struct tevent_req * +sdap_modify_shadow_lastchange_send(TALLOC_CTX *mem_ctx, + struct tevent_context *ev, + struct sdap_handle *sh, + const char *dn, + char *lastchanged_name); + +errno_t sdap_modify_shadow_lastchange_recv(struct tevent_req *req); + enum connect_tls { CON_TLS_DFL, CON_TLS_ON, -- 1.7.6.4
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
