On Wed, Aug 27, 2014 at 11:47:49AM +0200, Pavel Reichl wrote:
On 08/19/2014 05:55 PM, Pavel Reichl wrote:
>
>On 08/19/2014 04:31 PM, Pavel Březina wrote:
>>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],
>>
>>
>
>Thanks attached patches include this fix.
>
May we push these patches to the master?
Yes, sorry for the delay, for some reason I thought you were going to
fix up these patches a bit.
master:
e87f92f04f297fbdb0ae916945513a67b8a63044
2a91d3dd0ce4387332db27bd1a0c0005c74f870e
5668d294a39326f7024cbf24333e33ee970caf2d
1a357c873baa79c0b82ab1d084f942cfcc8ba1c0