<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 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 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 class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                      <br>
<a 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 class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                  <br>
<a 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 class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
                <br>
<a 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 class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
              <br>
              <a 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>
      <br>
      <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>
    Rebased patches for 1.11 (includes Pavel's mini patch)<br>
  </body>
</html>