[SSSD] Access denied for users when using format DOMAIN\user

Jakub Hrozek jhrozek at redhat.com
Fri Jan 10 14:08:53 UTC 2014


On Wed, Jan 08, 2014 at 06:46:55PM +0100, Pavel Reichl wrote:
> Hello,
> 
> please see attached patches.
> 
> PR

Hi,

I haven't tested the patches yet, just read the code.

> From d5fbd3c3ef70f9d890f4eb73a97adb840a64470e Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <pavel.reichl at redhat.com>
> Date: Wed, 8 Jan 2014 15:46:57 +0000
> Subject: [PATCH 1/2] simple access: match objects using flat name

Looks good to me.

> From dc613eca1c7b0ce0821f5d01bb2e5afae8a8b401 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <pavel.reichl at redhat.com>
> Date: Wed, 8 Jan 2014 16:03:08 +0000
> Subject: [PATCH 2/2] simple access: refresh master domain info
> 
> To correctly decide if an object is a member of the main sssd domain, a flat name
> is needed. However, the information may not be available when the module is
> inited so it may be necessary to refresh this data later while processing a
> request.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2189
> ---
>  src/providers/simple/simple_access.c | 75 ++++++++++++++++++++++++++++--------
>  src/providers/simple/simple_access.h |  2 +
>  2 files changed, 60 insertions(+), 17 deletions(-)
> 
> diff --git a/src/providers/simple/simple_access.c b/src/providers/simple/simple_access.c
> index f2bfe755039fd7a370749fd3ce94a47c62c216bc..60179e3e4ac144b265ae7193b7b1b1305108be61 100644
> --- a/src/providers/simple/simple_access.c
> +++ b/src/providers/simple/simple_access.c
> @@ -33,6 +33,7 @@
>  #define CONFDB_SIMPLE_DENY_GROUPS "simple_deny_groups"
>  
>  static void simple_access_check(struct tevent_req *req);
> +static int sssm_simple_access_obtain_filter_lists(struct simple_ctx *ctx);

I don't think the function should be prefixed with sssm_. Only functions
dynamically loaded by sssd_be in the backends should have this prefix.

>  
>  void simple_access_handler(struct be_req *be_req)
>  {
> @@ -40,13 +41,15 @@ void simple_access_handler(struct be_req *be_req)
>      struct pam_data *pd;
>      struct tevent_req *req;
>      struct simple_ctx *ctx;
> +    int ret;
>  
>      pd = talloc_get_type(be_req_get_data(be_req), struct pam_data);
>  
>      pd->pam_status = PAM_SYSTEM_ERR;
>  
>      if (pd->cmd != SSS_PAM_ACCT_MGMT) {
> -        DEBUG(4, ("simple access does not handles pam task %d.\n", pd->cmd));
> +        DEBUG(SSSDBG_CONF_SETTINGS,
> +              ("simple access does not handles pam task %d.\n", pd->cmd));

While you're changing the debug message already, can you also fix the
typo (s/handles/handle/)

>          pd->pam_status = PAM_MODULE_UNKNOWN;
>          goto done;
>      }
> @@ -54,6 +57,15 @@ void simple_access_handler(struct be_req *be_req)
>      ctx = talloc_get_type(be_ctx->bet_info[BET_ACCESS].pvt_bet_data,
>                            struct simple_ctx);
>  
> +    if (ctx->refresh_filter_lists) {

I was actually thinking that becaues the sysdb_update is just a read
from cache, we could do the update either on every request or use some
very low grace period (5 seconds) to avoid unnecessary reads from sysdb
during login bursts.

> +        ret = sssm_simple_access_obtain_filter_lists(ctx);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to refresh filter lists\n"));

I think this can be a MINOR failure, given that the request doesn't
terminate. Also the other place treats EAGAIN as not a failure, would it
make sense to do the same here?

> +        } else {
> +            ctx->refresh_filter_lists = false;
> +        }
> +    }
> +
>      req = simple_access_check_send(be_req, be_ctx->ev, ctx, pd->user);
>      if (!req) {
>          pd->pam_status = PAM_SYSTEM_ERR;

[snip]

> +    ret = sysdb_master_domain_update(bectx->domain);
> +    if (ret != EOK) {
> +        DEBUG(SSSDBG_FUNC_DATA, ("Update of master domain failed [%d]: %s.\n",
> +                                 ret, sss_strerror(ret)));
> +        goto failed;
> +    }
> +
> +    if (bectx->domain->flat_name == NULL) {
> +        /* Cache is probably empty. */
> +        DEBUG(SSSDBG_FUNC_DATA,
> +              ("Flat name of master domain is not supposed to be NULL.\n"));
> +        ctx->refresh_filter_lists = true;
> +    }

This assumption is only valid for AD. Pure LDAP domains or IPA don't have
flat name at all. If you're going to update the list on every request
(or with some short grace period), then I think this check should be
just dropped.

> +
>      for (i = 0; lists[i].name != NULL; i++) {
>          ret = confdb_get_string_as_list(bectx->cdb, ctx, bectx->conf_path,
>                                          lists[i].option, &lists[i].orig_list);



More information about the sssd-devel mailing list