On 08/19/2014 05:43 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@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?

Thanks!


_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel