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@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@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],
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patches for 1.11 (includes Pavel's mini patch)
Can we push this to 1.11?