On 08/19/2014 02:05 PM, Pavel Reichl wrote:
>
> 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],