<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    <div class="moz-cite-prefix">On 08/19/2014 05:55 PM, Pavel Reichl
      wrote:<br>
    </div>
    <blockquote cite="mid:53F37373.1010301@redhat.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <br>
      <div class="moz-cite-prefix">On 08/19/2014 04:31 PM, Pavel Březina
        wrote:<br>
      </div>
      <blockquote cite="mid:53F35FC4.9040900@redhat.com" type="cite">On
        08/19/2014 02:05 PM, Pavel Reichl wrote: <br>
        <blockquote type="cite"> <br>
          On 08/19/2014 12:43 PM, Pavel Březina wrote: <br>
          <blockquote type="cite">On 08/19/2014 10:37 AM, Pavel Reichl
            wrote: <br>
            <blockquote type="cite"> <br>
              On 08/18/2014 04:39 PM, Pavel Březina wrote: <br>
              <blockquote type="cite">On 08/18/2014 04:24 PM, Pavel
                Reichl wrote: <br>
                <blockquote type="cite"> <br>
                  Thank you Pavel for comments, I was mostly able to fix
                  patches as you <br>
                  asked for but I have a little problem with default_dn.
                  Could you <br>
                  please <br>
                  see my comments inline? <br>
                  <br>
                  <br>
                  On 08/18/2014 12:13 PM, Pavel Březina wrote: <br>
                  <blockquote type="cite">On 08/15/2014 03:13 PM, Pavel
                    Reichl wrote: <br>
                    <blockquote type="cite"> <br>
                      On 08/11/2014 04:51 PM, Jakub Hrozek wrote: <br>
                      <blockquote type="cite">On Mon, Aug 11, 2014 at
                        04:19:22PM +0200, Pavel Reichl wrote: <br>
                        <blockquote type="cite">
                          <blockquote type="cite">
                            <blockquote type="cite">Subject: [PATCH 7/9]
                              SDAP: new option - DN to ppolicy on LDAP <br>
                            </blockquote>
                            Do we need to make the DN configurable? When
                            you install the <br>
                            ppolicy, <br>
                            can you actually set the DN? <br>
                          </blockquote>
                          I haven't tested setting different DN, but I
                          believe it's <br>
                          possible. <br>
                          During weekend I was putting my notes for
                          setting ppolicy on <br>
                          OpenLDAP on <br>
                          this wiki page <br>
                          <a moz-do-not-send="true"
                            class="moz-txt-link-freetext"
href="https://fedorahosted.org/sssd/wiki/openldap_ppolicy#Loadingppolicyoverlay">https://fedorahosted.org/sssd/wiki/openldap_ppolicy#Loadingppolicyoverlay</a>
                          <br>
                          <br>
                          <br>
                          <br>
                          <br>
                          (this is work in progress and I'm not even
                          sure we care to have <br>
                          such a <br>
                          how-to on our wiki), but I believe you can see
                          from there that <br>
                          DN is <br>
                          configurable. <br>
                        </blockquote>
                        Any howto is good as long as it's up to date. <br>
                        <br>
                        And from the howto it seems clear that
                        olcPPolicyDefault is the DN <br>
                        setting, so yes, this should be configurable. <br>
                        <br>
                        <blockquote type="cite">Still we could use a
                          default DN if user didn't set DN himself: <br>
                          <br>
                          "cn=ppolicy,ou=policies,$search_base" <br>
                        </blockquote>
                        That sounds like a good idea. <br>
                        _______________________________________________
                        <br>
                        sssd-devel mailing list <br>
                        <a moz-do-not-send="true"
                          class="moz-txt-link-abbreviated"
                          href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                        <br>
                        <a moz-do-not-send="true"
                          class="moz-txt-link-freetext"
                          href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
                        <br>
                      </blockquote>
                      Hello, <br>
                      <br>
                      please note that attached patches requires to be
                      patches from "SDAP: <br>
                      refactor sdap_access" thread to pushed first. <br>
                    </blockquote>
                    <br>
                    Patch 7: <br>
                    Nack. <br>
                    <br>
                    +    /* cached results of access control checks */ <br>
                                         ^ cached_access contain only
                    one result <br>
                         bool cached_access; <br>
                         const char *basedn; <br>
                      }; <br>
                    <br>
                    Please remove
                    sdap_access_{filter|lockout}_decide_offline and use
                    <br>
                    only <br>
                    sdap_access_decide_offline that will take bool
                    instead of tevent_req <br>
                    as parameter. <br>
                    <br>
                    Patch 8: <br>
                    Ack. <br>
                    <br>
                    Patch 9: <br>
                    <br>
                    +        case LDAP_ACCESS_LOCKOUT: <br>
                    +            ret = EOK; <br>
                    + <br>
                    +            subreq = sdap_access_lock_send(state,
                    state-&gt;ev, <br>
                    state-&gt;be_ctx, <br>
                    + state-&gt;domain, <br>
                    + state-&gt;access_ctx, <br>
                    + state-&gt;conn, <br>
                    + state-&gt;pd-&gt;user, <br>
                    + state-&gt;user_entry); <br>
                    +            if (subreq == NULL) { <br>
                    +                DEBUG(SSSDBG_CRIT_FAILURE,
                    "sdap_access_lock_send <br>
                    failed.\n"); <br>
                    +                return ENOMEM; <br>
                    +            } <br>
                    + <br>
                    +            state-&gt;ac_type =
                    SDAP_ACCESS_CONTROL_PPOLICY_LOCK; <br>
                    + <br>
                    +            tevent_req_set_callback(subreq,
                    sdap_access_done, req); <br>
                    +            return EAGAIN; <br>
                    +            break; <br>
                    + <br>
                    <br>
                    There is no need for ret = EOK and break. <br>
                    <br>
                    +static char* <br>
                    +get_default_ppolicy_dn(TALLOC_CTX *mem_ctx, struct
                    sdap_options <br>
                    *opts) <br>
                    +{ <br>
                    +    char *search_base; <br>
                    + <br>
                    +    search_base = dp_opt_get_string(opts-&gt;basic,
                    SDAP_SEARCH_BASE); <br>
                    + <br>
                    +    return talloc_asprintf(mem_ctx,
                    "cn=ppolicy,ou=policies,%s", <br>
                    search_base); <br>
                    +} <br>
                    <br>
                    ldap_search_base option may contain more than one
                    search base and it <br>
                    may also contain filter. This option is parsed
                    during initialization <br>
                    of the provider and stored in
                    sdap_domain-&gt;*search_base. But since <br>
                    there may be more, you need to iterate over them. <br>
                  </blockquote>
                  Could you please elaborate a bit more about this? I'm
                  afraid I don't <br>
                  fully understand. <br>
                  <br>
                  Would you like me to create an array of possible
                  default DN's and <br>
                  query <br>
                  ldap server for every one? Or is there a way how to
                  use multiple dn <br>
                  in a <br>
                  single ldap query? <br>
                </blockquote>
                <br>
                Yes, you have to iterate over all of them. There is no
                way how to <br>
                search all of them in one query AFAIK. <br>
                <br>
                However, I think it is sufficient to use only domain
                components to <br>
                create default dn, i.e. basedn of domain
                (dc=example,dc=com). This can <br>
                be obtained from sdap_domain structure. Then you
                wouldn't have to <br>
                iterate over all search bases. <br>
                <br>
                <blockquote type="cite">
                  <blockquote type="cite"> <br>
                    However, I think it is sufficient to use only dc
                    part here so you can <br>
                    use basedn from sdap_domain structure. <br>
                    <br>
                    Also, I think it would be better if this function is
                    removed and the <br>
                    default value is set in ldap_get_options() with
                    dp_opt_set_string if <br>
                    the option is not set. <br>
                    <br>
                    <br>
                    <br>
                    _______________________________________________ <br>
                    sssd-devel mailing list <br>
                    <a moz-do-not-send="true"
                      class="moz-txt-link-abbreviated"
                      href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                    <br>
                    <a moz-do-not-send="true"
                      class="moz-txt-link-freetext"
                      href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
                    <br>
                  </blockquote>
                  <br>
                  _______________________________________________ <br>
                  sssd-devel mailing list <br>
                  <a moz-do-not-send="true"
                    class="moz-txt-link-abbreviated"
                    href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                  <br>
                  <a moz-do-not-send="true"
                    class="moz-txt-link-freetext"
                    href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
                  <br>
                </blockquote>
                <br>
                _______________________________________________ <br>
                sssd-devel mailing list <br>
                <a moz-do-not-send="true"
                  class="moz-txt-link-abbreviated"
                  href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                <br>
                <a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
                <br>
              </blockquote>
              <br>
              Hello, please see updated patches. <br>
              <br>
              Thanks, <br>
              <br>
              Pavel Reichl <br>
            </blockquote>
            <br>
            Hi, <br>
            I have just last few nitpicks. <br>
            <br>
            +    /* option was configured */ <br>
            +    if (ppolicy_dn != NULL) { <br>
            +        state-&gt;ppolicy_dns = talloc_array(state, const
            char*, 2); <br>
            +        if (state-&gt;ppolicy_dns == NULL) { <br>
            +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate
            <br>
            ppolicy_dns.\n"); <br>
            +            tevent_req_error(req, ENOMEM); <br>
            +            return; <br>
            +        } <br>
            + <br>
            +        state-&gt;ppolicy_dns[0] = ppolicy_dn; <br>
            +        state-&gt;ppolicy_dns[1] = NULL; <br>
            + <br>
            +    } else { <br>
            +        /* try to determine default value */ <br>
            +        DEBUG(SSSDBG_CONF_SETTINGS, <br>
            +              "ldap_pwdlockout_dn was not defined in
            configuration <br>
            file.\n"); <br>
            + <br>
            +        state-&gt;ppolicy_dns =
            get_default_ppolicy_dns(state, <br>
            state-&gt;opts-&gt;sdom); <br>
            +        if (state-&gt;ppolicy_dns == NULL) { <br>
            +            tevent_req_error(req, ERR_ACCESS_DENIED); <br>
            +            return; <br>
            +        } <br>
            +    } <br>
            <br>
            Why do you return ENOMEM on the top and ERR_ACCESS_DENIED on
            the <br>
            bottom? Shouldn't it be the same? <br>
            <br>
            errno_t sdap_access_lock_step(struct tevent_req *req) <br>
            <br>
            When we iterate over an array, the convention is that the
            step <br>
            function will return EOK if we are at the end, EAGAIN if it
            created <br>
            another tevent_req and error on error. <br>
            <br>
            Also, I would prefer to use an index to access
            state-&gt;ppolicy_dns <br>
            instead of dereferencing it and using pointer arithmetic. <br>
            <br>
            <br>
          </blockquote>
          <br>
          OK, I hope that attached patches address all your concerns! <br>
        </blockquote>
        <br>
        Ack. <br>
        <br>
        I just squash into the 3rd patch the following change: <br>
        <br>
        -                                   *state-&gt;ppolicy_dns, <br>
        + state-&gt;ppolicy_dns[state-&gt;ppolicy_dns_index], <br>
        <br>
        <br>
      </blockquote>
      <br>
      Thanks attached patches include this fix.<br>
      <br>
    </blockquote>
    May we push these patches to the master?<br>
    <blockquote cite="mid:53F37373.1010301@redhat.com" type="cite">
      <blockquote cite="mid:53F35FC4.9040900@redhat.com" type="cite"> <br>
        <br>
        <br>
        <fieldset class="mimeAttachmentHeader"></fieldset>
        <br>
        <pre wrap="">_______________________________________________
sssd-devel mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
      </blockquote>
      <br>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">_______________________________________________
sssd-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
    </blockquote>
    <br>
  </body>
</html>