URL: https://github.com/SSSD/sssd/pull/141 Author: fidencio Title: #141: PAM: Use cache_req to perform initgroups lookups Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/141/head:pr141 git checkout pr141
From 143d84ac936f38648f0cd603c54891f9d8554cd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Thu, 2 Feb 2017 13:06:30 +0100 Subject: [PATCH 1/4] CACHE_REQ: Add cache_req_data_set_bypass_cache() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This new cache_req_data method has been added because of the upcoming changes in the PAM responder. For deciding whether to contact the cache, or just query the data provider directly, PAM responder calls pam_initgr_check_timeout() which will return whether the cache entry may still be valid. The cache will be contacted only in case the cache entry is still valid, otherwise the data provider will be called. pam_initgr_check_timeout() basically checks whether the user (being looked up) is still a part of an in-memory hash table. Because the entry is a part of the hash table for really short period of time, and is automatically removed, the communication with the data provider is forced to happen quite often. As the follow-up changes should not modify this behaviour, this function was introduced so we can still call pam_initgr_check_timeout() and pass its result to the cache_req call that will perform the lookup. Related: https://fedorahosted.org/sssd/ticket/1126 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/common/cache_req/cache_req.h | 3 +++ src/responder/common/cache_req/cache_req_data.c | 7 +++++++ src/responder/common/cache_req/cache_req_private.h | 2 ++ src/responder/common/cache_req/cache_req_search.c | 2 +- 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/responder/common/cache_req/cache_req.h b/src/responder/common/cache_req/cache_req.h index 185558d..d0e5ff4 100644 --- a/src/responder/common/cache_req/cache_req.h +++ b/src/responder/common/cache_req/cache_req.h @@ -111,6 +111,9 @@ cache_req_data_host(TALLOC_CTX *mem_ctx, const char *name, const char *alias, const char **attrs); +void +cache_req_data_set_bypass_cache(struct cache_req_data *data, + bool bypass_cache); /* Output data. */ diff --git a/src/responder/common/cache_req/cache_req_data.c b/src/responder/common/cache_req/cache_req_data.c index b2e22ec..729e187 100644 --- a/src/responder/common/cache_req/cache_req_data.c +++ b/src/responder/common/cache_req/cache_req_data.c @@ -357,3 +357,10 @@ cache_req_data_host(TALLOC_CTX *mem_ctx, return cache_req_data_create(mem_ctx, type, &input); } + +void +cache_req_data_set_bypass_cache(struct cache_req_data *data, + bool bypass_cache) +{ + data->bypass_cache = bypass_cache; +} diff --git a/src/responder/common/cache_req/cache_req_private.h b/src/responder/common/cache_req/cache_req_private.h index cc47375..3839f5a 100644 --- a/src/responder/common/cache_req/cache_req_private.h +++ b/src/responder/common/cache_req/cache_req_private.h @@ -84,6 +84,8 @@ struct cache_req_data { struct cache_req_cased_name protocol; uint16_t port; } svc; + + bool bypass_cache; }; struct tevent_req * diff --git a/src/responder/common/cache_req/cache_req_search.c b/src/responder/common/cache_req/cache_req_search.c index eed82cf..ebbc2c7 100644 --- a/src/responder/common/cache_req/cache_req_search.c +++ b/src/responder/common/cache_req/cache_req_search.c @@ -214,7 +214,7 @@ cache_req_search_send(TALLOC_CTX *mem_ctx, */ state->result = NULL; status = CACHE_OBJECT_MISSING; - if (!cr->plugin->bypass_cache) { + if (!cr->plugin->bypass_cache && !cr->data->bypass_cache) { ret = cache_req_search_cache(state, cr, &state->result); if (ret != EOK && ret != ENOENT) { goto done; From cae34b4cf3232db75009698488de0a3d7849c261 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Thu, 2 Feb 2017 13:19:18 +0100 Subject: [PATCH 2/4] PAM: Use cache_req to perform initgroups lookups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PAM responder has been already taking advantage of the cache_req interface, so this patch is just replacing some code that performs initgroups lookups by using cache_req to do so. Resolves: https://fedorahosted.org/sssd/ticket/1126 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/providers/data_provider.h | 1 - src/responder/pam/pamsrv.h | 1 - src/responder/pam/pamsrv_cmd.c | 468 +++++++---------------------------------- 3 files changed, 76 insertions(+), 394 deletions(-) diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h index 5ccc0ad..3c6b5ae 100644 --- a/src/providers/data_provider.h +++ b/src/providers/data_provider.h @@ -169,7 +169,6 @@ struct pam_data { struct sss_auth_token *newauthtok; uint32_t cli_pid; char *logon_name; - bool name_is_upn; int pam_status; int response_delay; diff --git a/src/responder/pam/pamsrv.h b/src/responder/pam/pamsrv.h index 7860a99..4f58d6e 100644 --- a/src/responder/pam/pamsrv.h +++ b/src/responder/pam/pamsrv.h @@ -60,7 +60,6 @@ struct pam_auth_req { pam_dp_callback_t *callback; bool is_uid_trusted; - bool check_provider; void *data; bool use_cached_auth; /* whether cached authentication was tried and failed */ diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index e788a75..c74525d 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -1041,42 +1041,8 @@ static void pam_handle_cached_login(struct pam_auth_req *preq, int ret, static void pam_forwarder_cb(struct tevent_req *req); static void pam_forwarder_cert_cb(struct tevent_req *req); -static void pam_check_user_dp_callback(uint16_t err_maj, uint32_t err_min, - const char *err_msg, void *ptr); static int pam_check_user_search(struct pam_auth_req *preq); -static errno_t pam_cmd_assume_upn(struct pam_auth_req *preq) -{ - int ret; - - if (!preq->pd->name_is_upn - && preq->pd->logon_name != NULL - && strchr(preq->pd->logon_name, '@') != NULL) { - DEBUG(SSSDBG_TRACE_ALL, - "No entry found so far, trying UPN/email lookup with [%s].\n", - preq->pd->logon_name); - /* Assuming Kerberos principal */ - preq->domain = preq->cctx->rctx->domains; - preq->check_provider = - NEED_CHECK_PROVIDER(preq->domain->provider); - preq->pd->user = talloc_strdup(preq->pd, preq->pd->logon_name); - if (preq->pd->user == NULL) { - DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n"); - return ENOMEM; - } - preq->pd->name_is_upn = true; - preq->pd->domain = NULL; - - ret = pam_check_user_search(preq); - if (ret == EOK) { - pam_dom_forwarder(preq); - } - return EOK; - } - - return ENOENT; -} - /* TODO: we should probably return some sort of cookie that is set in the * PAM_ENVIRONMENT, so that we can save performing some calls and cache @@ -1242,15 +1208,12 @@ static errno_t check_cert(TALLOC_CTX *mctx, static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) { - struct sss_domain_info *dom; struct pam_auth_req *preq; struct pam_data *pd; int ret; - errno_t ncret; struct pam_ctx *pctx = talloc_get_type(cctx->rctx->pvt_ctx, struct pam_ctx); struct tevent_req *req; - char *name = NULL; preq = talloc_zero(cctx, struct pam_auth_req); if (!preq) { @@ -1294,67 +1257,6 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) goto done; } - if (pd->user != NULL) { - /* now check user is valid */ - if (pd->domain) { - preq->domain = responder_get_domain(cctx->rctx, pd->domain); - if (!preq->domain) { - ret = ENOENT; - goto done; - } - - name = sss_resp_create_fqname(preq, pctx->rctx, preq->domain, - preq->pd->name_is_upn, - preq->pd->user); - if (name == NULL) { - return ENOMEM; - } - - ncret = sss_ncache_check_user(pctx->rctx->ncache, - preq->domain, name); - talloc_free(name); - if (ncret == EEXIST) { - /* User found in the negative cache */ - ret = ENOENT; - goto done; - } - } else { - for (dom = preq->cctx->rctx->domains; - dom; - dom = get_next_domain(dom, 0)) { - if (dom->fqnames) continue; - - name = sss_resp_create_fqname(preq, pctx->rctx, dom, - preq->pd->name_is_upn, - preq->pd->user); - if (name == NULL) { - return ENOMEM; - } - - ncret = sss_ncache_check_user(pctx->rctx->ncache, - dom, name); - talloc_free(name); - if (ncret == ENOENT) { - /* User not found in the negative cache - * Proceed with PAM actions - */ - break; - } - - /* Try the next domain */ - DEBUG(SSSDBG_TRACE_FUNC, - "User [%s@%s] filtered out (negative cache). " - "Trying next domain.\n", pd->user, dom->name); - } - - if (!dom) { - ret = ENOENT; - goto done; - } - preq->domain = dom; - } - } - /* try backend first for authentication before doing local Smartcard * authentication */ if (pd->cmd != SSS_PAM_AUTHENTICATE && may_do_cert_auth(pctx, pd)) { @@ -1363,22 +1265,7 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd) goto done; } - - if (preq->domain->provider == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Domain [%s] has no auth provider.\n", preq->domain->name); - ret = EINVAL; - goto done; - } - - preq->check_provider = NEED_CHECK_PROVIDER(preq->domain->provider); - ret = pam_check_user_search(preq); - if (ret == EOK) { - pam_dom_forwarder(preq); - } else if (ret == ENOENT) { - ret = pam_cmd_assume_upn(preq); - } done: return pam_check_user_done(preq, ret); @@ -1420,9 +1307,6 @@ static void pam_forwarder_cert_cb(struct tevent_req *req) ret = ENOENT; } else { ret = pam_check_user_search(preq); - if (ret == EOK) { - pam_dom_forwarder(preq); - } } } @@ -1496,6 +1380,15 @@ static void pam_forwarder_lookup_by_cert_done(struct tevent_req *req) "sss_parse_name_for_domains failed.\n"); goto done; } + + /* cert_user will be returned to the PAM client as user name, so + * we can use it here already e.g. to set in initgroups timeout */ + preq->pd->logon_name = talloc_strdup(preq->pd, cert_user); + if (preq->pd->logon_name == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n"); + ret = ENOMEM; + goto done; + } } } else { if (preq->pd->logon_name == NULL) { @@ -1539,34 +1432,11 @@ static void pam_forwarder_cb(struct tevent_req *req) pd = preq->pd; ret = pam_forwarder_parse_data(cctx, pd); - if (ret == EAGAIN) { - if (strchr(preq->pd->logon_name, '@') == NULL) { - goto done; - } - /* Assuming Kerberos principal */ - preq->domain = preq->cctx->rctx->domains; - preq->check_provider = NEED_CHECK_PROVIDER(preq->domain->provider); - preq->pd->user = talloc_strdup(preq->pd, preq->pd->logon_name); - if (preq->pd->user == NULL) { - DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n"); - ret = ENOMEM; - goto done; - } - preq->pd->name_is_upn = true; - preq->pd->domain = NULL; - } else if (ret != EOK) { + if (ret != EOK) { ret = EINVAL; goto done; } - if (preq->pd->domain) { - preq->domain = responder_get_domain(cctx->rctx, preq->pd->domain); - if (preq->domain == NULL) { - ret = ENOENT; - goto done; - } - } - /* try backend first for authentication before doing local Smartcard * authentication */ if (pd->cmd != SSS_PAM_AUTHENTICATE && may_do_cert_auth(pctx, pd)) { @@ -1576,11 +1446,6 @@ static void pam_forwarder_cb(struct tevent_req *req) } ret = pam_check_user_search(preq); - if (ret == EOK) { - pam_dom_forwarder(preq); - } else if (ret == ENOENT) { - ret = pam_cmd_assume_upn(preq); - } done: pam_check_user_done(preq, ret); @@ -1589,233 +1454,94 @@ static void pam_forwarder_cb(struct tevent_req *req) static void pam_dp_send_acct_req_done(struct tevent_req *req); static int pam_check_user_search(struct pam_auth_req *preq) { - struct sss_domain_info *dom = preq->domain; - char *name = NULL; - time_t cacheExpire; int ret; struct tevent_req *dpreq; - struct dp_callback_ctx *cb_ctx; - struct pam_ctx *pctx = - talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); - static const char *user_attrs[] = SYSDB_PW_ATTRS; - struct ldb_result *res; - const char *sysdb_name; - - while (dom) { - /* if it is a domainless search, skip domains that require fully - * qualified names instead */ - while (dom && !preq->pd->domain && !preq->pd->name_is_upn - && dom->fqnames) { - dom = get_next_domain(dom, 0); - } - - if (!dom) break; - - if (dom != preq->domain) { - /* make sure we reset the check_provider flag when we check - * a new domain */ - preq->check_provider = NEED_CHECK_PROVIDER(dom->provider); - } - - /* make sure to update the preq if we changed domain */ - preq->domain = dom; - - talloc_free(name); - - name = sss_resp_create_fqname(preq, pctx->rctx, dom, - preq->pd->name_is_upn, - preq->pd->user); - if (name == NULL) { - return ENOMEM; - } - - /* Refresh the user's cache entry on any PAM query - * We put a timeout in the client context so that we limit - * the number of updates within a reasonable timeout - */ - if (preq->check_provider) { - 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 timout\n"); - return EIO; - } else if (ret == ENOENT) { - /* Call provider first */ - break; - } - /* Entry is still valid, get it from the sysdb */ - } - - DEBUG(SSSDBG_CONF_SETTINGS, "Requesting info for [%s]\n", name); - - if (dom->sysdb == NULL) { - DEBUG(SSSDBG_FATAL_FAILURE, - "Fatal: Sysdb CTX not found for this domain!\n"); - preq->pd->pam_status = PAM_SYSTEM_ERR; - return EFAULT; - } - - if (preq->pd->name_is_upn) { - ret = sysdb_search_user_by_upn(preq, dom, name, user_attrs, - &preq->user_obj); - if (ret == EOK) { - /* Since sysdb_search_user_by_upn() searches the whole cache we - * have to set the domain so that it matches the result. */ - sysdb_name = ldb_msg_find_attr_as_string(preq->user_obj, - SYSDB_NAME, NULL); - if (sysdb_name == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "Cached entry has no name.\n"); - return EINVAL; - } - preq->domain = find_domain_by_object_name( - get_domains_head(dom), - sysdb_name); - if (preq->domain == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Cannot find matching domain for [%s].\n", - sysdb_name); - return EINVAL; - } - } - } else { - ret = sysdb_getpwnam_with_views(preq, dom, name, &res); - if (res->count > 1) { - DEBUG(SSSDBG_FATAL_FAILURE, - "getpwnam call returned more than one result !?!\n"); - sss_log(SSS_LOG_ERR, - "More users have the same name [%s@%s] in SSSD cache. " - "SSSD will not work correctly.\n", - name, dom->name); - return ENOENT; - } else if (res->count == 0) { - ret = ENOENT; - } else { - preq->user_obj = res->msgs[0]; - } - } - if (ret != EOK && ret != ENOENT) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Failed to make request to our cache!\n"); - return EIO; - } - - if (ret == ENOENT) { - if (preq->check_provider == false) { - /* set negative cache only if not result of cache check */ - ret = sss_ncache_set_user(pctx->rctx->ncache, - false, dom, preq->pd->user); - if (ret != EOK) { - /* Should not be fatal, just slower next time */ - DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot set ncache for [%s@%s]\n", name, - dom->name); - } - } - - /* if a multidomain search, try with next */ - if (!preq->pd->domain) { - dom = get_next_domain(dom, 0); - continue; - } - - DEBUG(SSSDBG_OP_FAILURE, "No results for getpwnam call\n"); - - /* TODO: store negative cache ? */ - - return ENOENT; - } - - /* One result found */ - - /* if we need to check the remote account go on */ - if (preq->check_provider) { - cacheExpire = ldb_msg_find_attr_as_uint64(preq->user_obj, - SYSDB_CACHE_EXPIRE, 0); - if (cacheExpire < time(NULL)) { - break; - } - } + struct pam_ctx *pctx; + struct cache_req_data *data; - DEBUG(SSSDBG_TRACE_FUNC, - "Returning info for user [%s@%s]\n", name, dom->name); + data = cache_req_data_name(preq, + CACHE_REQ_INITGROUPS, + preq->pd->user); - /* We might have searched by alias. Pass on the primary name */ - ret = pd_set_primary_name(preq->user_obj, preq->pd); - if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "Could not canonicalize username\n"); - return ret; - } + pctx = talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); - return EOK; + /* 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; } - if (!dom) { - /* Ensure that we don't try to check a provider without a domain, - * since this will cause a NULL-dereference below. - */ - preq->check_provider = false; + dpreq = cache_req_send(preq, + preq->cctx->rctx->ev, + preq->cctx->rctx, + preq->cctx->rctx->ncache, + 0, + preq->pd->domain, + data); + if (!dpreq) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Out of memory sending data provider request\n"); + return ENOMEM; } - if (preq->check_provider) { - - /* dont loop forever :-) */ - preq->check_provider = false; + tevent_req_set_callback(dpreq, pam_dp_send_acct_req_done, preq); - dpreq = sss_dp_get_account_send(preq, preq->cctx->rctx, - dom, false, SSS_DP_INITGROUPS, name, 0, - preq->pd->name_is_upn ? EXTRA_NAME_IS_UPN : NULL); - if (!dpreq) { - DEBUG(SSSDBG_CRIT_FAILURE, - "Out of memory sending data provider request\n"); - return ENOMEM; - } - - cb_ctx = talloc_zero(preq, struct dp_callback_ctx); - if(!cb_ctx) { - talloc_zfree(dpreq); - return ENOMEM; - } - - cb_ctx->callback = pam_check_user_dp_callback; - cb_ctx->ptr = preq; - cb_ctx->cctx = preq->cctx; - cb_ctx->mem_ctx = preq; - - tevent_req_set_callback(dpreq, pam_dp_send_acct_req_done, cb_ctx); - - /* tell caller we are in an async call */ - return EAGAIN; - } - - DEBUG(SSSDBG_MINOR_FAILURE, - "No matching domain found for [%s], fail!\n", preq->pd->user); - return ENOENT; + /* tell caller we are in an async call */ + return EAGAIN; } static void pam_dp_send_acct_req_done(struct tevent_req *req) { - struct dp_callback_ctx *cb_ctx = - tevent_req_callback_data(req, struct dp_callback_ctx); + struct cache_req_result *result; + struct pam_auth_req *preq; + struct pam_ctx *pctx; + int ret; - errno_t ret; - dbus_uint16_t err_maj; - dbus_uint32_t err_min; - char *err_msg; + preq = tevent_req_callback_data(req, struct pam_auth_req); + pctx = talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); - ret = sss_dp_get_account_recv(cb_ctx->mem_ctx, req, - &err_maj, &err_min, - &err_msg); + ret = cache_req_single_domain_recv(preq, req, &result); talloc_zfree(req); - if (ret != EOK) { + if (ret != EOK && ret != ENOENT) { DEBUG(SSSDBG_CRIT_FAILURE, "Fatal error, killing connection!\n"); - talloc_free(cb_ctx->cctx); + talloc_zfree(preq->cctx); return; } - cb_ctx->callback(err_maj, err_min, err_msg, cb_ctx->ptr); + if (ret == EOK) { + preq->user_obj = result->msgs[0]; + pd_set_primary_name(preq->user_obj, preq->pd); + preq->domain = result->domain; + + ret = pam_initgr_cache_set(pctx->rctx->ev, + pctx->id_table, + preq->pd->logon_name, + pctx->id_timeout); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "Could not save initgr timestamp." + "Proceeding with PAM actions\n"); + } + + pam_dom_forwarder(preq); + } + + ret = pam_check_user_done(preq, ret); + if (ret != EOK) { + preq->pd->pam_status = PAM_SYSTEM_ERR; + pam_reply(preq); + } } static int pam_check_user_done(struct pam_auth_req *preq, int ret) @@ -1847,48 +1573,6 @@ static int pam_check_user_done(struct pam_auth_req *preq, int ret) return EOK; } -static void pam_check_user_dp_callback(uint16_t err_maj, uint32_t err_min, - const char *err_msg, void *ptr) -{ - struct pam_auth_req *preq = talloc_get_type(ptr, struct pam_auth_req); - int ret; - struct pam_ctx *pctx = - talloc_get_type(preq->cctx->rctx->pvt_ctx, struct pam_ctx); - - if (err_maj) { - DEBUG(SSSDBG_OP_FAILURE, - "Unable to get information from Data Provider\n" - "Error: %u, %u, %s\n", - (unsigned int)err_maj, (unsigned int)err_min, err_msg); - } - - ret = pam_check_user_search(preq); - if (ret == EOK) { - /* Make sure we don't go to the ID provider too often */ - ret = pam_initgr_cache_set(pctx->rctx->ev, pctx->id_table, - preq->pd->logon_name, pctx->id_timeout); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "Could not save initgr timestamp. " - "Proceeding with PAM actions\n"); - /* This is non-fatal, we'll just end up going to the - * data provider again next time. - */ - } - - pam_dom_forwarder(preq); - } else if (ret == ENOENT) { - ret = pam_cmd_assume_upn(preq); - } - - ret = pam_check_user_done(preq, ret); - - if (ret) { - preq->pd->pam_status = PAM_SYSTEM_ERR; - pam_reply(preq); - } -} - static errno_t pam_is_last_online_login_fresh(struct sss_domain_info *domain, const char* user, int cached_auth_timeout, From 3ba866122ea39bf97873496a08d529cf5d73c0d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <fiden...@redhat.com> Date: Mon, 6 Feb 2017 11:05:16 +0100 Subject: [PATCH 3/4] TESTS: Adapt pam-srv-tests to deal with cache_req related changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similar to what happened for nss-srv-tests, there were a few kind of changes required to fix the tests breakage caused by the last commit. 1) For tests including no user, no changes was required. 2) As we call an equivalent to "get by name" command, a name is parsed with sss_parse_inp and the returned value is now mocked. 3) For the "cache_auth_success*" tests we set pam_test_ctx->tctx->done to false after adding the password to the cache, since the code now contains tevent calls and without it only the first request proceeds into tevent_loop in test_ev_loop(), as the first finished request sets done to true. 4) As the user certificate is added as a result of calling sss_dp_account_recv and the certificate value is read by the certificate lookup, we have to, in case a certificate lookup callback is set, call mock_account_recv() for the certificate before going through the mock_account_recv() for the initgroup. 5) If no logon name is given, then the user is looked by certificates first. Since there's a matching user, the upcoming lookup by name will find the user entry. However, since the looked ip data is up to date the dp response has to be mocked and the second argument of mock_input_pam_cert() cannot be NULL but must match the user name. 6) Add a new attribute to mock_input_pam_cert() that represents whether the backend is contacted only once. It's needed because in test_pam_cert_auth() the backend is contacted first to check whether it can handle smartcard authenticatiom, but before that there's a lookup. Since the first mocked reply already adds the certificate to the user entry, the lookup by certificate will already find the user in the cache and no second lookup is needed. Co-Author: Pavel Březina <pbrez...@redhat.com> Co-Author: Sumit Bose <sb...@redhat.com> Resolves: https://fedorahosted.org/sssd/ticket/1126 Signed-off-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/tests/cmocka/test_pam_srv.c | 76 ++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 23 deletions(-) diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index cbc1d03..074eabf 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -557,16 +557,22 @@ static void mock_input_pam(TALLOC_CTX *mem_ctx, const char *name, will_return(__wrap_sss_packet_get_body, WRAP_CALL_WRAPPER); will_return(__wrap_sss_packet_get_body, buf); will_return(__wrap_sss_packet_get_body, buf_size); + + mock_parse_inp(name, NULL, EOK); + mock_account_recv(0, 0, NULL, NULL, NULL); } static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, - const char *pin, const char *service) + const char *pin, const char *service, + acct_cb_t acct_cb, const char *cert, + bool only_one_provider_call) { size_t buf_size; uint8_t *m_buf; uint8_t *buf; struct pam_items pi = { 0 }; int ret; + bool already_mocked = false; if (name != NULL) { pi.pam_user = name; @@ -603,6 +609,18 @@ static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, will_return(__wrap_sss_packet_get_body, WRAP_CALL_WRAPPER); will_return(__wrap_sss_packet_get_body, buf); will_return(__wrap_sss_packet_get_body, buf_size); + + if (acct_cb != NULL) { + mock_account_recv(0, 0, NULL, acct_cb, discard_const(cert)); + already_mocked = true; + } + + if (name != NULL) { + mock_parse_inp(name, NULL, EOK); + if (!(only_one_provider_call && already_mocked)) { + mock_account_recv(0, 0, NULL, NULL, NULL); + } + } } static int test_pam_simple_check(uint32_t status, uint8_t *body, size_t blen) @@ -1051,6 +1069,8 @@ void test_pam_cached_auth_success(void **state) /* Reset before next call */ pam_test_ctx->provider_contacted = false; + pam_test_ctx->tctx->done = false; + common_test_pam_cached_auth("12345"); /* Back end should not be contacted */ @@ -1137,6 +1157,8 @@ void test_pam_cached_auth_success_combined_pw_with_cached_2fa(void **state) /* Reset before next call */ pam_test_ctx->provider_contacted = false; + pam_test_ctx->tctx->done = false; + common_test_pam_cached_auth("12345678"); assert_false(pam_test_ctx->provider_contacted); @@ -1519,7 +1541,7 @@ void test_pam_preauth_no_logon_name(void **state) { int ret; - mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL, NULL, NULL, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1546,7 +1568,7 @@ void test_pam_preauth_cert_nocert(void **state) set_cert_auth_param(pam_test_ctx->pctx, "/no/path"); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL, NULL, NULL, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1625,11 +1647,11 @@ void test_pam_preauth_cert_nomatch(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL, + test_lookup_by_cert_cb, NULL, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, NULL); set_cmd_cb(test_pam_simple_check); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1647,12 +1669,11 @@ void test_pam_preauth_cert_match(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL, + test_lookup_by_cert_cb, TEST_TOKEN_CERT, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, - discard_const(TEST_TOKEN_CERT)); set_cmd_cb(test_pam_cert_check); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1671,12 +1692,11 @@ void test_pam_preauth_cert_match_gdm_smartcard(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, "gdm-smartcard"); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, "gdm-smartcard", + test_lookup_by_cert_cb, TEST_TOKEN_CERT, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, - discard_const(TEST_TOKEN_CERT)); set_cmd_cb(test_pam_cert_check_gdm_smartcard); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1694,12 +1714,12 @@ void test_pam_preauth_cert_match_wrong_user(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL); + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL, + test_lookup_by_cert_wrong_user_cb, + TEST_TOKEN_CERT, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_wrong_user_cb, - discard_const(TEST_TOKEN_CERT)); set_cmd_cb(test_pam_simple_check); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1718,12 +1738,17 @@ void test_pam_preauth_cert_no_logon_name(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); + /* If no logon name is given the user is looked by certificate first. + * Since there is a matching user the upcoming lookup by name will find + * the user entry. But since we force the lookup by name to go to the + * backend to make sure the group-membership data is up to date the + * backend response has to be mocked twice and the second argument of + * mock_input_pam_cert cannot be NULL but must match the user name. */ + mock_input_pam_cert(pam_test_ctx, "pamuser", NULL, NULL, + test_lookup_by_cert_cb, TEST_TOKEN_CERT, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, - discard_const(TEST_TOKEN_CERT)); set_cmd_cb(test_pam_cert_check); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1741,7 +1766,7 @@ void test_pam_preauth_no_cert_no_logon_name(void **state) set_cert_auth_param(pam_test_ctx->pctx, "/no/path"); - mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL, NULL, NULL, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); @@ -1762,11 +1787,11 @@ void test_pam_preauth_cert_no_logon_name_no_match(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL); + mock_input_pam_cert(pam_test_ctx, NULL, NULL, NULL, + test_lookup_by_cert_cb, NULL, false); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_PREAUTH); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, NULL); set_cmd_cb(test_pam_user_unknown_check); ret = sss_cmd_execute(pam_test_ctx->cctx, SSS_PAM_PREAUTH, @@ -1784,12 +1809,17 @@ void test_pam_cert_auth(void **state) set_cert_auth_param(pam_test_ctx->pctx, NSS_DB); - mock_input_pam_cert(pam_test_ctx, "pamuser", "123456", NULL); + /* Here the last option must be set to true because the backend is only + * connected once. During authentication the backend is connected first to + * see if it can handle Smartcard authentication, but before that the user + * is looked up. Since the first mocked reply already adds the certificate + * to the user entry the lookup by certificate will already find the user + * in the cache and no second request to the backend is needed. */ + mock_input_pam_cert(pam_test_ctx, "pamuser", "123456", NULL, + test_lookup_by_cert_cb, TEST_TOKEN_CERT, true); will_return(__wrap_sss_packet_get_cmd, SSS_PAM_AUTHENTICATE); will_return(__wrap_sss_packet_get_body, WRAP_CALL_REAL); - mock_account_recv(0, 0, NULL, test_lookup_by_cert_cb, - discard_const(TEST_TOKEN_CERT)); /* Assume backend cannot handle Smartcard credentials */ pam_test_ctx->exp_pam_status = PAM_BAD_ITEM; From 5f10d26307ee311267f05efd8c52be2e24d01b3c Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 23 Feb 2017 22:42:45 +0100 Subject: [PATCH 4/4] PAM: Improve debugging on smartcard creds forward MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewed-by: Fabiano Fidêncio <fiden...@redhat.com> --- src/responder/pam/pamsrv_cmd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c index c74525d..5d3d05e 100644 --- a/src/responder/pam/pamsrv_cmd.c +++ b/src/responder/pam/pamsrv_cmd.c @@ -1717,6 +1717,7 @@ static void pam_dom_forwarder(struct pam_auth_req *preq) * was successful, so pd->user contains the canonical sysdb name * as well */ if (ldb_dn_compare(preq->cert_user_obj->dn, preq->user_obj->dn) == 0) { + DEBUG(SSSDBG_TRACE_ALL, "User and cert user DN match.\n"); if (preq->pd->cmd == SSS_PAM_PREAUTH) { ret = sss_authtok_set_sc(preq->pd->authtok,
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
