On 08/19/2014 12:43 PM, Pavel Březina wrote:
> On 08/19/2014 10:37 AM, Pavel Reichl wrote:
>>
>> On 08/18/2014 04:39 PM, Pavel Březina wrote:
>>> On 08/18/2014 04:24 PM, Pavel Reichl wrote:
>>>>
>>>> Thank you Pavel for comments, I was mostly able to fix patches as you
>>>> asked for but I have a little problem with default_dn. Could you
>>>> please
>>>> see my comments inline?
>>>>
>>>>
>>>> On 08/18/2014 12:13 PM, Pavel Březina wrote:
>>>>> On 08/15/2014 03:13 PM, Pavel Reichl wrote:
>>>>>>
>>>>>> On 08/11/2014 04:51 PM, Jakub Hrozek wrote:
>>>>>>> On Mon, Aug 11, 2014 at 04:19:22PM +0200, Pavel Reichl
wrote:
>>>>>>>>>> Subject: [PATCH 7/9] SDAP: new option - DN to
ppolicy on LDAP
>>>>>>>>> Do we need to make the DN configurable? When you
install the
>>>>>>>>> ppolicy,
>>>>>>>>> can you actually set the DN?
>>>>>>>> I haven't tested setting different DN, but I believe
it's
>>>>>>>> possible.
>>>>>>>> During weekend I was putting my notes for setting ppolicy
on
>>>>>>>> OpenLDAP on
>>>>>>>> this wiki page
>>>>>>>>
https://fedorahosted.org/sssd/wiki/openldap_ppolicy#Loadingppolicyoverlay
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> (this is work in progress and I'm not even sure we
care to have
>>>>>>>> such a
>>>>>>>> how-to on our wiki), but I believe you can see from there
that
>>>>>>>> DN is
>>>>>>>> configurable.
>>>>>>> Any howto is good as long as it's up to date.
>>>>>>>
>>>>>>> And from the howto it seems clear that olcPPolicyDefault is
the DN
>>>>>>> setting, so yes, this should be configurable.
>>>>>>>
>>>>>>>> Still we could use a default DN if user didn't set DN
himself:
>>>>>>>>
>>>>>>>> "cn=ppolicy,ou=policies,$search_base"
>>>>>>> That sounds like a good idea.
>>>>>>> _______________________________________________
>>>>>>> sssd-devel mailing list
>>>>>>> sssd-devel(a)lists.fedorahosted.org
>>>>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>> Hello,
>>>>>>
>>>>>> please note that attached patches requires to be patches from
"SDAP:
>>>>>> refactor sdap_access" thread to pushed first.
>>>>>
>>>>> Patch 7:
>>>>> Nack.
>>>>>
>>>>> + /* cached results of access control checks */
>>>>> ^ cached_access contain only one result
>>>>> bool cached_access;
>>>>> const char *basedn;
>>>>> };
>>>>>
>>>>> Please remove sdap_access_{filter|lockout}_decide_offline and use
>>>>> only
>>>>> sdap_access_decide_offline that will take bool instead of tevent_req
>>>>> as parameter.
>>>>>
>>>>> Patch 8:
>>>>> Ack.
>>>>>
>>>>> Patch 9:
>>>>>
>>>>> + case LDAP_ACCESS_LOCKOUT:
>>>>> + ret = EOK;
>>>>> +
>>>>> + subreq = sdap_access_lock_send(state, state->ev,
>>>>> state->be_ctx,
>>>>> + state->domain,
>>>>> + state->access_ctx,
>>>>> + state->conn,
>>>>> + state->pd->user,
>>>>> + state->user_entry);
>>>>> + if (subreq == NULL) {
>>>>> + DEBUG(SSSDBG_CRIT_FAILURE,
"sdap_access_lock_send
>>>>> failed.\n");
>>>>> + return ENOMEM;
>>>>> + }
>>>>> +
>>>>> + state->ac_type = SDAP_ACCESS_CONTROL_PPOLICY_LOCK;
>>>>> +
>>>>> + tevent_req_set_callback(subreq, sdap_access_done, req);
>>>>> + return EAGAIN;
>>>>> + break;
>>>>> +
>>>>>
>>>>> There is no need for ret = EOK and break.
>>>>>
>>>>> +static char*
>>>>> +get_default_ppolicy_dn(TALLOC_CTX *mem_ctx, struct sdap_options
>>>>> *opts)
>>>>> +{
>>>>> + char *search_base;
>>>>> +
>>>>> + search_base = dp_opt_get_string(opts->basic,
SDAP_SEARCH_BASE);
>>>>> +
>>>>> + return talloc_asprintf(mem_ctx,
"cn=ppolicy,ou=policies,%s",
>>>>> search_base);
>>>>> +}
>>>>>
>>>>> ldap_search_base option may contain more than one search base and it
>>>>> may also contain filter. This option is parsed during initialization
>>>>> of the provider and stored in sdap_domain->*search_base. But
since
>>>>> there may be more, you need to iterate over them.
>>>> Could you please elaborate a bit more about this? I'm afraid I
don't
>>>> fully understand.
>>>>
>>>> Would you like me to create an array of possible default DN's and
>>>> query
>>>> ldap server for every one? Or is there a way how to use multiple dn
>>>> in a
>>>> single ldap query?
>>>
>>> Yes, you have to iterate over all of them. There is no way how to
>>> search all of them in one query AFAIK.
>>>
>>> However, I think it is sufficient to use only domain components to
>>> create default dn, i.e. basedn of domain (dc=example,dc=com). This can
>>> be obtained from sdap_domain structure. Then you wouldn't have to
>>> iterate over all search bases.
>>>
>>>>>
>>>>> However, I think it is sufficient to use only dc part here so you
can
>>>>> use basedn from sdap_domain structure.
>>>>>
>>>>> Also, I think it would be better if this function is removed and the
>>>>> default value is set in ldap_get_options() with dp_opt_set_string if
>>>>> the option is not set.
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> sssd-devel mailing list
>>>>> sssd-devel(a)lists.fedorahosted.org
>>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel(a)lists.fedorahosted.org
>>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel(a)lists.fedorahosted.org
>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> Hello, please see updated patches.
>>
>> Thanks,
>>
>> Pavel Reichl
>
> Hi,
> I have just last few nitpicks.
>
> + /* option was configured */
> + if (ppolicy_dn != NULL) {
> + state->ppolicy_dns = talloc_array(state, const char*, 2);
> + if (state->ppolicy_dns == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate
> ppolicy_dns.\n");
> + tevent_req_error(req, ENOMEM);
> + return;
> + }
> +
> + state->ppolicy_dns[0] = ppolicy_dn;
> + state->ppolicy_dns[1] = NULL;
> +
> + } else {
> + /* try to determine default value */
> + DEBUG(SSSDBG_CONF_SETTINGS,
> + "ldap_pwdlockout_dn was not defined in configuration
> file.\n");
> +
> + state->ppolicy_dns = get_default_ppolicy_dns(state,
> state->opts->sdom);
> + if (state->ppolicy_dns == NULL) {
> + tevent_req_error(req, ERR_ACCESS_DENIED);
> + return;
> + }
> + }
>
> Why do you return ENOMEM on the top and ERR_ACCESS_DENIED on the
> bottom? Shouldn't it be the same?
>
> errno_t sdap_access_lock_step(struct tevent_req *req)
>
> When we iterate over an array, the convention is that the step
> function will return EOK if we are at the end, EAGAIN if it created
> another tevent_req and error on error.
>
> Also, I would prefer to use an index to access state->ppolicy_dns
> instead of dereferencing it and using pointer arithmetic.
>
>
OK, I hope that attached patches address all your concerns!
Ack.
I just squash into the 3rd patch the following change:
- *state->ppolicy_dns,
+
state->ppolicy_dns[state->ppolicy_dns_index],