[SSSD] [PATCH] local overrides: don't concact server with overriden name/id

Jakub Hrozek jhrozek at redhat.com
Mon Oct 12 13:46:38 UTC 2015


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 at 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 at 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 at 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)


More information about the sssd-devel mailing list