From 8d32fb49e44e1cca4698759737e5c3dc9b49ea86 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Oct 15 2019 20:52:05 +0000 Subject: [PATCH 1/2] cache_req: introduce cache_behavior enumeration Instead of using individual booleans for controlling the behavior of the nss reponder with regard to cache usage, use a single enumeration that can be extended to add new behaviors as needed. Related: https://pagure.io/SSSD/sssd/issue/4098 Signed-off-by: Simo Sorce --- diff --git a/src/responder/common/cache_req/cache_req.c b/src/responder/common/cache_req/cache_req.c index 303cee3..18df2e7 100644 --- a/src/responder/common/cache_req/cache_req.c +++ b/src/responder/common/cache_req/cache_req.c @@ -121,6 +121,7 @@ cache_req_create(TALLOC_CTX *mem_ctx, enum cache_req_dom_type req_dom_type) { struct cache_req *cr; + bool bypass_cache; errno_t ret; cr = talloc_zero(mem_ctx, struct cache_req); @@ -144,15 +145,22 @@ cache_req_create(TALLOC_CTX *mem_ctx, return NULL; } - cr->cache_first = rctx->cache_first; - cr->bypass_cache = cr->plugin->bypass_cache || cr->data->bypass_cache; - cr->bypass_dp = cr->data->bypass_dp; - if (cr->bypass_cache && cr->bypass_dp) { + bypass_cache = cr->plugin->bypass_cache || cr->data->bypass_cache; + if (bypass_cache && cr->data->bypass_dp) { CACHE_REQ_DEBUG(SSSDBG_CRIT_FAILURE, cr, "Cannot bypass cache and dp at the same time!"); talloc_free(cr); return NULL; } + if (rctx->cache_first) { + cr->cache_behavior = CACHE_REQ_CACHE_FIRST; + } + /* it is ok to override cache_first here */ + if (bypass_cache) { + cr->cache_behavior = CACHE_REQ_BYPASS_CACHE; + } else if (cr->data->bypass_dp) { + cr->cache_behavior = CACHE_REQ_BYPASS_PROVIDER; + } return cr; } @@ -493,8 +501,8 @@ static struct tevent_req *cache_req_locate_dom_send(TALLOC_CTX *mem_ctx, subreq = cache_req_search_send(state, state->ev, state->cr, - false, /* Don't bypass cache */ - true); /* Do bypass DP */ + false, + true); if (subreq == NULL) { ret = ENOMEM; goto immediately; @@ -677,8 +685,8 @@ struct cache_req_search_domains_state { size_t num_results; bool check_next; bool dp_success; - bool bypass_cache; - bool bypass_dp; + bool first_iteration; + enum cache_req_behavior cache_behavior; }; static errno_t cache_req_search_domains_next(struct tevent_req *req); @@ -695,9 +703,7 @@ cache_req_search_domains_send(TALLOC_CTX *mem_ctx, struct cache_req *cr, struct cache_req_domain *cr_domain, bool check_next, - bool first_iteration, - bool bypass_cache, - bool bypass_dp) + bool first_iteration) { struct tevent_req *req; struct cache_req_search_domains_state *state = NULL; @@ -717,12 +723,12 @@ cache_req_search_domains_send(TALLOC_CTX *mem_ctx, state->req_domains = cr_domain; state->check_next = check_next; state->dp_success = true; - state->bypass_cache = bypass_cache; - state->bypass_dp = bypass_dp; + state->first_iteration = first_iteration; if (cr->plugin->dp_get_domain_send_fn != NULL && ((state->check_next && cr_domain->next != NULL) - || (state->bypass_cache && !first_iteration))) { + || ((state->cr->cache_behavior == CACHE_REQ_CACHE_FIRST) + && !first_iteration))) { /* If the request is not qualified with a domain name AND * there are multiple domains to search OR if this is the second * pass during the "check-cache-first" schema, it makes sense @@ -814,7 +820,8 @@ static errno_t cache_req_search_domains_next(struct tevent_req *req) } subreq = cache_req_search_send(state, state->ev, cr, - state->bypass_cache, state->bypass_dp); + state->first_iteration, + false); if (subreq == NULL) { return ENOMEM; } @@ -1018,66 +1025,6 @@ cache_req_search_domains_recv(TALLOC_CTX *mem_ctx, return EOK; } -/** - * Return true if we should issue another search. - */ -static bool cache_req_search_schema(struct cache_req *cr, - const char *input_domain, - bool first_iteration, - bool *_bypass_cache, - bool *_bypass_dp) -{ - bool bypass_cache; - bool bypass_dp; - - if (cr->bypass_cache) { - /* The caller wants to contact Data Provider first - * or it is inferred by cache_req plug-in. */ - bypass_cache = true; - bypass_dp = false; - - if (!first_iteration) { - return false; - } - } else if (cr->bypass_dp) { - /* The caller wants to lookup only in the cache */ - bypass_cache = false; - bypass_dp = true; - - if (!first_iteration) { - return false; - } - } else if (input_domain != NULL) { - /* We will search only one domain. */ - bypass_cache = false; - bypass_dp = false; - - if (!first_iteration) { - return false; - } - } else if (!cr->cache_first) { - /* We will search cache and on cache-miss - * contact domain provider sequentially. */ - bypass_cache = false; - bypass_dp = false; - - if (!first_iteration) { - return false; - } - } else { - /* We will first search the cache in all domains. If we don't get - * any match we will then contact Data Provider starting with the - * first domain again. */ - bypass_cache = first_iteration ? false : true; - bypass_dp = first_iteration ? true : false; - } - - *_bypass_cache = bypass_cache; - *_bypass_dp = bypass_dp; - - return true; -} - struct cache_req_state { /* input data */ struct tevent_context *ev; @@ -1111,9 +1058,7 @@ static errno_t cache_req_select_domains(struct tevent_req *req, static errno_t cache_req_search_domains(struct tevent_req *req, struct cache_req_domain *oredered_domain, - bool check_next, - bool bypass_cache, - bool bypass_dp); + bool check_next); static void cache_req_process_result(struct tevent_req *subreq); @@ -1337,19 +1282,17 @@ static errno_t cache_req_select_domains(struct tevent_req *req, struct cache_req_state *state = NULL; struct cache_req_domain *cr_domain; bool check_next; - bool bypass_cache; - bool bypass_dp; - bool search; errno_t ret; state = tevent_req_data(req, struct cache_req_state); - search = cache_req_search_schema(state->cr, domain_name, - state->first_iteration, - &bypass_cache, &bypass_dp); - if (!search) { - /* We're done here. */ - return EOK; + if ((state->cr->cache_behavior != CACHE_REQ_CACHE_FIRST) + || (domain_name != NULL)) { + + if (!state->first_iteration) { + /* We're done here. */ + return EOK; + } } ret = cache_req_domain_copy_cr_domains(state, @@ -1378,31 +1321,46 @@ static errno_t cache_req_select_domains(struct tevent_req *req, check_next = true; } - return cache_req_search_domains(req, cr_domain, check_next, - bypass_cache, bypass_dp); + return cache_req_search_domains(req, cr_domain, check_next); } static errno_t cache_req_search_domains(struct tevent_req *req, struct cache_req_domain *cr_domain, - bool check_next, - bool bypass_cache, - bool bypass_dp) + bool check_next) { struct tevent_req *subreq; struct cache_req_state *state = NULL; + const char *cache_action; + const char *provider_action; state = tevent_req_data(req, struct cache_req_state); + switch (state->cr->cache_behavior) { + case CACHE_REQ_CACHE_FIRST: + cache_action = (state->first_iteration) ? "check" : "bypass"; + provider_action = (state->first_iteration) ? "bypass" : "check"; + break; + case CACHE_REQ_BYPASS_CACHE: + cache_action = "bypass"; + provider_action = "check"; + break; + case CACHE_REQ_BYPASS_PROVIDER: + cache_action = "check"; + provider_action = "bypass"; + break; + default: + cache_action = "check"; + provider_action = "check"; + break; + } CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, state->cr, "Search will %s the cache and %s the data provider\n", - bypass_cache ? "bypass" : "check", - bypass_dp ? "bypass" : "check"); + cache_action, provider_action); subreq = cache_req_search_domains_send(state, state->ev, state->cr, cr_domain, check_next, - state->first_iteration, - bypass_cache, bypass_dp); + state->first_iteration); if (subreq == NULL) { return ENOMEM; } diff --git a/src/responder/common/cache_req/cache_req.h b/src/responder/common/cache_req/cache_req.h index 5ce1d6a..886a017 100644 --- a/src/responder/common/cache_req/cache_req.h +++ b/src/responder/common/cache_req/cache_req.h @@ -73,6 +73,17 @@ enum cache_req_dom_type { CACHE_REQ_ANY_DOM }; +/* Controls behavior about how to use cached information during + * a lookup, this is to fine tune some behaviors for specific + * situations + */ +enum cache_req_behavior { + CACHE_REQ_NORMAL, + CACHE_REQ_CACHE_FIRST, + CACHE_REQ_BYPASS_CACHE, + CACHE_REQ_BYPASS_PROVIDER, +}; + /* Input data. */ struct cache_req_data; diff --git a/src/responder/common/cache_req/cache_req_private.h b/src/responder/common/cache_req/cache_req_private.h index af4712f..4b4525c 100644 --- a/src/responder/common/cache_req/cache_req_private.h +++ b/src/responder/common/cache_req/cache_req_private.h @@ -40,9 +40,10 @@ struct cache_req { /* Domain related information. */ struct sss_domain_info *domain; - bool cache_first; - bool bypass_cache; - bool bypass_dp; + + /* wanted cache behavior */ + enum cache_req_behavior cache_behavior; + /* Only contact domains with this type */ enum cache_req_dom_type req_dom_type; @@ -99,8 +100,8 @@ struct tevent_req * cache_req_search_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct cache_req *cr, - bool bypass_cache, - bool bypass_dp); + bool first_iteration, + bool cache_only_override); errno_t cache_req_search_recv(TALLOC_CTX *mem_ctx, struct tevent_req *req, diff --git a/src/responder/common/cache_req/cache_req_search.c b/src/responder/common/cache_req/cache_req_search.c index 8732145..7b86d71 100644 --- a/src/responder/common/cache_req/cache_req_search.c +++ b/src/responder/common/cache_req/cache_req_search.c @@ -301,12 +301,14 @@ struct tevent_req * cache_req_search_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev, struct cache_req *cr, - bool bypass_cache, - bool bypass_dp) + bool first_iteration, + bool cache_only_override) { struct cache_req_search_state *state; enum cache_object_status status; struct tevent_req *req; + bool bypass_cache = false; + bool bypass_dp = false; errno_t ret; req = tevent_req_create(mem_ctx, &state, struct cache_req_search_state); @@ -325,6 +327,25 @@ cache_req_search_send(TALLOC_CTX *mem_ctx, goto done; } + if (cache_only_override) { + bypass_dp = true; + } else { + switch (cr->cache_behavior) { + case CACHE_REQ_CACHE_FIRST: + bypass_cache = first_iteration ? false : true; + bypass_dp = first_iteration ? true : false; + break; + case CACHE_REQ_BYPASS_CACHE: + bypass_cache = true; + break; + case CACHE_REQ_BYPASS_PROVIDER: + bypass_dp = true; + break; + default: + break; + } + } + /* If bypass_cache is enabled we always contact data provider before * searching the cache. Thus we set expiration status to missing, * which will trigger data provider request later. From a795dd141050584acb54eb0596f0691381d30e60 Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Oct 17 2019 22:04:12 +0000 Subject: [PATCH 2/2] pam: Use cache for users with existing session Users that have an existing session do the bulk of their authentication to unlock services that do not make use of initgroups (used only to create a new login session). Forcing online initgroups calls for these users leads mostly to delays in providing those services and do not provide any useful data. Resolves: https://pagure.io/SSSD/sssd/issue/4098 Signed-off-by: Simo Sorce --- diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index 7241220..f177298 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -23,6 +23,7 @@ #include #include "util/util.h" #include "util/auth_utils.h" +#include "util/find_uid.h" #include "db/sysdb.h" #include "confdb/confdb.h" #include "responder/common/responder_packet.h" @@ -1846,12 +1847,16 @@ done: pam_check_user_done(preq, ret); } -static void pam_dp_send_acct_req_done(struct tevent_req *req); +static void pam_check_user_search_next(struct tevent_req *req); +static void pam_check_user_search_lookup(struct tevent_req *req); +static void pam_check_user_search_done(struct pam_auth_req *preq, int ret, + struct cache_req_result *result); + +/* lookup the user uid from the cache first, + * then we'll refresh initgroups if needed */ static int pam_check_user_search(struct pam_auth_req *preq) { - int ret; struct tevent_req *dpreq; - struct pam_ctx *pctx; struct cache_req_data *data; data = cache_req_data_name(preq, @@ -1861,23 +1866,8 @@ static int pam_check_user_search(struct pam_auth_req *preq) return ENOMEM; } - pctx = talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); - - /* The initgr cache is used to make sure that during a single PAM session - * (auth, acct_mgtm, ....) the backend is contacted only once. logon_name - * is the name provided by the PAM client and will not be modified during - * the request, so it makes sense to use it here instead od the pd->user. */ - ret = pam_initgr_check_timeout(pctx->id_table, preq->pd->logon_name); - if (ret == EOK) { - /* Entry is still valid, force to lookup in the cache first */ - cache_req_data_set_bypass_cache(data, false); - } else if (ret == ENOENT) { - /* Call the data provider first */ - cache_req_data_set_bypass_cache(data, true); - } else { - DEBUG(SSSDBG_OP_FAILURE, "Could not look up initgroup timeout\n"); - return EIO; - } + cache_req_data_set_bypass_cache(data, false); + cache_req_data_set_bypass_dp(data, true); dpreq = cache_req_send(preq, preq->cctx->rctx->ev, @@ -1893,17 +1883,19 @@ static int pam_check_user_search(struct pam_auth_req *preq) return ENOMEM; } - tevent_req_set_callback(dpreq, pam_dp_send_acct_req_done, preq); + tevent_req_set_callback(dpreq, pam_check_user_search_next, preq); /* tell caller we are in an async call */ return EAGAIN; } -static void pam_dp_send_acct_req_done(struct tevent_req *req) +static void pam_check_user_search_next(struct tevent_req *req) { - struct cache_req_result *result; struct pam_auth_req *preq; struct pam_ctx *pctx; + struct cache_req_result *result; + struct cache_req_data *data; + struct tevent_req *dpreq; int ret; preq = tevent_req_callback_data(req, struct pam_auth_req); @@ -1919,6 +1911,100 @@ static void pam_dp_send_acct_req_done(struct tevent_req *req) } if (ret == EOK) { + bool user_has_session = false; + uid_t uid = ldb_msg_find_attr_as_uint64(result->msgs[0], SYSDB_UIDNUM, 0); + if (!uid) { + DEBUG(SSSDBG_CRIT_FAILURE, "A user with no UID?\n"); + talloc_zfree(preq->cctx); + return; + } + + /* If a user already has a session on the system, we take the + * cache for granted and do not force an online lookup. This is + * because in most cases the user is just trying to authenticate + * but not create a new session (sudo, lockscreen, polkit, etc.) + * An online refresh in this situation would just delay operations + * without providing any useful additional information. + */ + (void)check_if_uid_is_active(uid, &user_has_session); + + /* The initgr cache is used to make sure that during a single PAM + * session (auth, acct_mgtm, ....) the backend is contacted only + * once. logon_name is the name provided by the PAM client and + * will not be modified during the request, so it makes sense to + * use it here instead od the pd->user. + */ + ret = pam_initgr_check_timeout(pctx->id_table, preq->pd->logon_name); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_OP_FAILURE, "Could not look up initgroup timeout\n"); + } + + if ((ret == EOK) || user_has_session) { + pam_check_user_search_done(preq, EOK, result); + return; + } + } + + /* If we get here it means the user was not found or does not have a + * session, or initgr has not been cached before, so we force a new + * online lookup */ + data = cache_req_data_name(preq, + CACHE_REQ_INITGROUPS, + preq->pd->logon_name); + if (data == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory\n"); + talloc_zfree(preq->cctx); + return; + } + cache_req_data_set_bypass_cache(data, true); + cache_req_data_set_bypass_dp(data, false); + + dpreq = cache_req_send(preq, + preq->cctx->rctx->ev, + preq->cctx->rctx, + preq->cctx->rctx->ncache, + 0, + preq->req_dom_type, + NULL, + data); + if (!dpreq) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Out of memory sending data provider request\n"); + talloc_zfree(preq->cctx); + return; + } + + tevent_req_set_callback(dpreq, pam_check_user_search_lookup, preq); +} + +static void pam_check_user_search_lookup(struct tevent_req *req) +{ + struct cache_req_result *result; + struct pam_auth_req *preq; + int ret; + + preq = tevent_req_callback_data(req, struct pam_auth_req); + + ret = cache_req_single_domain_recv(preq, req, &result); + talloc_zfree(req); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Fatal error, killing connection!\n"); + talloc_zfree(preq->cctx); + return; + } + + pam_check_user_search_done(preq, ret, result); +} + +static void pam_check_user_search_done(struct pam_auth_req *preq, int ret, + struct cache_req_result *result) +{ + struct pam_ctx *pctx; + + pctx = talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); + + if (ret == EOK) { preq->user_obj = result->msgs[0]; pd_set_primary_name(preq->user_obj, preq->pd); preq->domain = result->domain; diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index 6a92622..296c5a7 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -1950,8 +1950,9 @@ void test_pam_auth_no_upn_logon_name(void **state) assert_int_equal(ret, EOK); mock_input_pam_ex(pam_test_ctx, "upn@"TEST_DOM_NAME, "12345", NULL, NULL, - true); + false); mock_account_recv_simple(); + mock_parse_inp("upn@"TEST_DOM_NAME, NULL, EOK); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1991,6 +1992,8 @@ void test_pam_auth_upn_logon_name(void **state) true); mock_account_recv_simple(); + mock_parse_inp("pamuser", NULL, EOK); + will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -2270,6 +2273,8 @@ void test_pam_preauth_cert_no_logon_name(void **state) test_lookup_by_cert_cb, SSSD_TEST_CERT_0001, false); mock_account_recv_simple(); mock_parse_inp("pamuser", NULL, EOK); + mock_account_recv_simple(); + mock_parse_inp("pamuser", NULL, EOK); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -2496,6 +2501,8 @@ void test_pam_cert_auth_no_logon_name(void **state) mock_account_recv_simple(); mock_parse_inp("pamuser", NULL, EOK); + mock_account_recv_simple(); + mock_parse_inp("pamuser", NULL, EOK); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -2928,6 +2935,7 @@ void test_appsvc_posix_dom(void **state) /* The domain is POSIX, the request will skip over it */ mock_input_pam_ex(pam_test_ctx, "pamuser", NULL, NULL, "app_svc", false); + mock_parse_inp("pamuser", NULL, EOK); pam_test_ctx->exp_pam_status = PAM_USER_UNKNOWN; will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); @@ -3012,6 +3020,7 @@ void test_not_appsvc_app_dom(void **state) /* A different service than the app one can authenticate against a POSIX domain */ mock_input_pam_ex(pam_test_ctx, "pamuser", NULL, NULL, "not_app_svc", false); + mock_parse_inp("pamuser", NULL, EOK); pam_test_ctx->exp_pam_status = PAM_USER_UNKNOWN;