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(a)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/