[SSSD] [PATCHES] SDAP: Be able to configure sssd to honor openldap account lock to restrict access via ssh key

Jakub Hrozek jhrozek at redhat.com
Wed Aug 27 13:53:09 UTC 2014


On Wed, Aug 27, 2014 at 11:47:49AM +0200, Pavel Reichl wrote:
> 
> 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 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 at 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 at lists.fedorahosted.org
> >>>>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>>>>
> >>>>>>>_______________________________________________
> >>>>>>>sssd-devel mailing list
> >>>>>>>sssd-devel at lists.fedorahosted.org
> >>>>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>>>>>
> >>>>>>_______________________________________________
> >>>>>>sssd-devel mailing list
> >>>>>>sssd-devel at 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],
> >>
> >>
> >
> >Thanks attached patches include this fix.
> >
> May we push these patches to the master?

Yes, sorry for the delay, for some reason I thought you were going to
fix up these patches a bit.

master:
    e87f92f04f297fbdb0ae916945513a67b8a63044
    2a91d3dd0ce4387332db27bd1a0c0005c74f870e
    5668d294a39326f7024cbf24333e33ee970caf2d
    1a357c873baa79c0b82ab1d084f942cfcc8ba1c0



More information about the sssd-devel mailing list