[SSSD] [PATCH] Add ldap_access_filter option

Sumit Bose sbose at redhat.com
Fri May 14 13:57:36 UTC 2010


On Thu, May 13, 2010 at 02:02:57PM -0400, Stephen Gallagher wrote:
> On 05/13/2010 12:07 PM, Simo Sorce wrote:
> >On Thu, 13 May 2010 10:56:35 -0400
> >Stephen Gallagher<sgallagh at redhat.com>  wrote:
> >
> >>+    ret = sysdb_attrs_add_uint32(attrs, SYSDB_LDAP_ACCESS,
> >>+                                 state->pam_status == PAM_SUCCESS ?
> >>+                                                      true :
> >>+                                                      false);
> >
> >Just a nitpick, but normally in LDAP true false values are stored as
> >the strings "TRUE" or "FALSE" (upper case). It makes it clearer this is
> >a boolean attribute than storing 0 vs a random value (usually 1).
> >
> >Simo.
> >
> 
> New patch attached. I added a new sysdb function:
> sysdb_attrs_add_bool() that will handle this in an ldb-compatible
> way.
> 
> I also corrected your other comment about passing an explicit event context.
> 
> Finally, this patch adds the new option to the SSSDConfig API and
> manpages (which I forgot to do originally).
> 
> Please re-review.
> 

Comments are in-line.

bye.
Sumit

> -- 
> Stephen Gallagher
> RHCE 804006346421761
> 
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/

> --- a/src/config/SSSDConfig.py
> +++ b/src/config/SSSDConfig.py
> @@ -149,6 +149,9 @@ option_strings = {
>  
>      # [provider/ldap/auth]
>      'ldap_pwd_policy' : _('Policy to evaluate the password expiration'),
> +    
   ^^^^
unneeded whitespace

> +    # [provider/ldap/access]
> +    'ldap_access_filter' : _('LDAP filter to determine access privileges'),
>  
>      # [provider/simple/access]
>      'simple_allow_users' : _('Comma separated list of allowed users'),



> diff --git a/src/providers/ldap/ldap_init.c b/src/providers/ldap/ldap_init.c
> index 917ece0cbc5f3fbdf3d63603cac2af3a0a16918d..639aba19585ba117a16384d186ff744c25afa65a 100644
>  
> +int sssm_ldap_access_init(struct be_ctx *bectx,
> +                          struct bet_ops **ops,
> +                          void **pvt_data)
> +{
> +    int ret;
> +    struct sdap_access_ctx *access_ctx;
> +
> +    access_ctx = talloc_zero(bectx, struct sdap_access_ctx);
> +    if(access_ctx == NULL) {
> +        ret = ENOMEM;
> +        goto done;
> +    }
> +
> +    ret = sssm_ldap_id_init(bectx, ops, (void **)&access_ctx->id_ctx);
> +    if (ret != EOK) {
> +        DEBUG(1, ("sssm_ldap_id_init failed.\n"));
> +        goto done;
> +    }

I think you need to modify sssm_ldap_id_init() to detect if it was
already called once and return the already allocated sdap_ctx. Currently
a new context is allocated and the global sdap handle is not shared
between the id and the access provider.

> +
> +    access_ctx->filter = dp_opt_get_cstring(access_ctx->id_ctx->opts->basic,
> +                                            SDAP_ACCESS_FILTER);
> +    if (access_ctx->filter == NULL) {
> +        /* It's okay if this is NULL. In that case we will simply act
> +         * like the 'deny' provider.
> +         */
> +        DEBUG(0, ("Warning: access_provider=ldap set, "
> +                  "but no ldap_access_filter configured. "
> +                  "All domain users will be denied access."));

missing \n

> +    }
> +
> +    *ops = &sdap_access_ops;
> +    *pvt_data = access_ctx;
> +
> +done:
> +    if (ret != EOK) {
> +        talloc_free(access_ctx);
> +    }
> +    return ret;
> +}
> +static struct tevent_req *sdap_access_send(TALLOC_CTX *mem_ctx,
> +                                           struct tevent_context *ev,
> +                                           struct be_req *breq,
> +                                           const char *username)

I have made the expirence that it is easier to reuse request if they do
not depend on breq. Instead use be_ctx and access_ctx->filter.

> +    /* Perform online operation */
> +    basedn = ldb_msg_find_attr_as_string(res->msgs[0],
> +                                         SYSDB_ORIG_DN,
> +                                         NULL);
> +    if(basedn == NULL) {
> +        DEBUG(1,("Could not find originalDN for user [%s]\n",
> +                 state->username));
> +        ret = EIO;
> +        state->pam_status = PAM_SYSTEM_ERR;
> +        goto done;
> +    }
> +
> +    state->basedn = talloc_strdup(state, basedn);
> +    if (state->basedn == NULL) {
> +        DEBUG(1, ("Could not allocate memory for originalDN\n"));
> +        ret = ENOMEM;
> +        state->pam_status = PAM_SYSTEM_ERR;
> +        goto done;
> +    }

Wouldn't it be enough to set state->basedn in the first place, because
for the memory hierarchy point of view res is already a child of state?

> +
> +    if (found) {
> +        /* Save "allow" to the cache for future offline
> +         * access checks.
> +         */
> +        DEBUG(6, ("Access granted by online lookup\n"));
> +        state->pam_status = PAM_SUCCESS;
> +    }
> +    else {
> +        /* Save "disallow" to the cache for future offline
> +         * access checks.
> +         */
> +        DEBUG(6, ("Access denied by online lookup\n"));
> +        state->pam_status = PAM_PERM_DENIED;

I think it is not necessary to save "disallow" because if
SYSDB_LDAP_ACCESS is not set 'false' is used as a default.






More information about the sssd-devel mailing list