[SSSD] [PATCH] Add ldap_access_filter option

Stephen Gallagher sgallagh at redhat.com
Fri May 14 18:08:48 UTC 2010


On 05/14/2010 09:57 AM, Sumit Bose wrote:
> 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

Fixed

>
>> +    # [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.
>

Good idea. Done.

>> +
>> +    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

Added.

>
>> +    }
>> +
>> +    *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.

I removed the requirement on breq, but I'm still passing in access_ctx, 
since I need to maintain the original filter and the constructed 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?

Thanks, but you actually alerted me that I'd forgotten the 
talloc_free(res) I'd meant to do here. No sense in carrying that memory 
around until the end of the request. I have to talloc_strdup() here, 
because I can't trust that this is actually an allocated string I can 
talloc_steal().

I could do omit the temporary string, but I prefer to be able to tell 
whether an error was from LDB or talloc.
>
>> +
>> +    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.

It's necessary to save "disallow" because it's possible that we have 
previously saved "allow", and this needs to overwrite it. I could do a 
removal, but I prefer the explicit nature of having FALSE written here.

For example, the rule could be "memberOf=<some group>" and the user has 
been removed from that group on the server.


-- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ldap_access_filter-option.patch
Type: text/x-patch
Size: 34312 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100514/5f94a48a/attachment.bin>


More information about the sssd-devel mailing list