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

Pavel Reichl preichl at redhat.com
Wed Aug 27 09:47:49 UTC 2014


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?
>>
>>
>>
>>
>> _______________________________________________
>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140827/4ca4dddc/attachment.html>


More information about the sssd-devel mailing list