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 youasked 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:Could you please elaborate a bit more about this? I'm afraid I don'tOn 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 LDAPDo 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 pagehttps://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 DNsetting, 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 [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-develHello,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 onlysdap_access_decide_offline that will take bool instead of tevent_reqas 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 initializationof the provider and stored in sdap_domain->*search_base. But since there may be more, you need to iterate over them.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 tocreate default dn, i.e. basedn of domain (dc=example,dc=com). This canbe 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 canuse 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 ifthe option is not set. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-develHello, please see updated patches. Thanks, Pavel ReichlHi, 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?
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
