On Sun, Oct 11, 2015 at 06:10:50PM +0200, Pavel Březina wrote:
On 10/11/2015 06:03 PM, Pavel Březina wrote:
>https://fedorahosted.org/sssd/ticket/2833
>
>We can't search of overridden name/id in LDAP when using local
>overrides. These patches fix this for nss and sudo responder, please let
>me know if there is any other place.
>
>Similar fix needs to be done in cache_req interface, but I'll send it as
>another patch since I have some work in progress in that area.
Now with patches.
So far I only read the patches..
From e5cbb8c9c384336e25c656e1aebe35c2c43b0079 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Sun, 11 Oct 2015 16:45:19 +0200
Subject: [PATCH 1/3] nss: send original name and id with local views if
possible
Resolves:
https://fedorahosted.org/sssd/ticket/2833
---
src/responder/nss/nsssrv_cmd.c | 121 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 121 insertions(+)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 3e95a3f5a96a91515745a17fad5d20cbc9063d28..eb7f1f549eb8231597370bea02f3ca05492a8bd6
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -600,6 +600,111 @@ is_refreshed_on_bg(enum sss_dp_acct_type req_type,
static void nsssrv_dp_send_acct_req_done(struct tevent_req *req);
+static void get_dp_name_and_id(TALLOC_CTX *mem_ctx,
+ struct sss_domain_info *dom,
+ enum sss_dp_acct_type req_type,
+ const char *opt_name,
+ uint32_t opt_id,
+ const char **_name,
+ uint32_t *_id)
+{
+ TALLOC_CTX *tmp_ctx;
+ struct ldb_result *res = NULL;
+ const char *attr;
+ errno_t ret;
+
+ /* First set the same values to make things easier. */
+ *_name = opt_name;
+ *_id = opt_id;
+
+ if (!DOM_HAS_VIEWS(dom) || !is_local_view(dom->view_name)) {
+ DEBUG(SSSDBG_TRACE_FUNC, "Not a LOCAL view, continuing with "
+ "provided values.\n");
+ return;
+ }
Good, this means the patch is quite safe..
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
+ return;
+ }
+
+ if (opt_name != NULL) {
+ switch (req_type) {
+ case SSS_DP_USER:
+ case SSS_DP_INITGROUPS:
+ ret = sysdb_getpwnam_with_views(tmp_ctx, dom, opt_name, &res);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "sysdb_getpwnam_with_views() failed [%d]: %s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+ break;
+ case SSS_DP_GROUP:
+ ret = sysdb_getgrnam_with_views(tmp_ctx, dom, opt_name, &res);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "sysdb_getgrnam_with_views() failed [%d]: %s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+ break;
+ default:
+ goto done;
+ }
+
+ if (res == NULL || res->count != 1) {
+ /* This should not happen with LOCAL view and overridden value. */
+ DEBUG(SSSDBG_TRACE_FUNC, "Entry is missing?! Continuing with "
+ "provided values.\n");
+ goto done;
+ }
+
+ *_name = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_NAME, NULL);
I would prefer:
name = ldb_msg_find_attr_as_string()
if (name == NULL) {
wtf?!?
}
*_name = talloc_steal(mem_ctx, name);
+ talloc_steal(mem_ctx, *_name);
+ } else if (opt_id != 0) {
+ switch (req_type) {
+ case SSS_DP_USER:
+ ret = sysdb_getpwuid_with_views(tmp_ctx, dom, opt_id, &res);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "sysdb_getpwuid_with_views() failed [%d]: %s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+ attr = SYSDB_UIDNUM;
+ break;
+ case SSS_DP_GROUP:
+ ret = sysdb_getgrgid_with_views(tmp_ctx, dom, opt_id, &res);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "sysdb_getgrgid_with_views() failed [%d]: %s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+ attr = SYSDB_GIDNUM;
+ break;
+ default:
+ goto done;
+ }
+
+ if (res == NULL || res->count != 1) {
+ /* This should not happen with LOCAL view and overridden value. */
+ DEBUG(SSSDBG_TRACE_FUNC, "Entry is missing?! Continuing with "
+ "provided values.\n");
+ goto done;
+ }
+
+ *_id = ldb_msg_find_attr_as_uint(res->msgs[0], attr, 0);
I think it's safer to use ldb_msg_find_attr_as_uint64, because uid is
normally 32bit...so we'd avoid problems with unsigned int on some
strange platforms. I would also prefer a sanity check here, because the
default not-found value is zero which sounds a bit dangerous :-)
+ }
+
+done:
+ talloc_free(tmp_ctx);
+}
+
/* FIXME: do not check res->count, but get in a msgs and check in parent */
errno_t check_cache(struct nss_dom_ctx *dctx,
struct nss_ctx *nctx,
@@ -611,6 +716,7 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
sss_dp_callback_t callback,
void *pvt)
{
+ TALLOC_CTX *tmp_ctx;
errno_t ret;
struct nss_cmd_ctx *cmdctx = dctx->cmdctx;
struct cli_ctx *cctx = cmdctx->cctx;
@@ -628,6 +734,17 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
return ENOENT;
}
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
+ return ENOMEM;
+ }
+
+ /* In case of local view we have to always contant DP with the original
+ * name or id. */
+ get_dp_name_and_id(tmp_ctx, dctx->domain, req_type, opt_name, opt_id,
+ &opt_name, &opt_id);
+
/* if we have any reply let's check cache validity, but ignore netgroups
* if refresh_expired_interval is set (which implies that another method
* is used to refresh netgroups)
@@ -725,12 +842,16 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
tevent_req_set_callback(req, nsssrv_dp_send_acct_req_done, cb_ctx);
+ talloc_free(tmp_ctx);
return EAGAIN;
}
+ talloc_free(tmp_ctx);
return EOK;
error:
+ talloc_free(tmp_ctx);
+
ret = nss_cmd_send_error(cmdctx, ret);
if (ret != EOK) {
NSS_CMD_FATAL_ERROR_CODE(cctx, ret);
--
1.9.3
From c8125859c950ace5b621a98949e5ff211028801c Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Sun, 11 Oct 2015 17:38:34 +0200
Subject: [PATCH 2/3] sudo: search with view even if user is found
LGTM (not tested yet)
From bc1cca81a87bd5b78e5d8406b645dffad97addd4 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
Date: Sun, 11 Oct 2015 17:53:28 +0200
Subject: [PATCH 3/3] sudo: send original name and id with local views if
possible
Resolves:
https://fedorahosted.org/sssd/ticket/2833
LGTM (not tested yet)