[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