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

Pavel Březina pbrezina at redhat.com
Tue Aug 19 10:43:32 UTC 2014


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.








More information about the sssd-devel mailing list