[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
Fri Aug 8 10:19:16 UTC 2014


On 08/07/2014 02:22 PM, Pavel Březina wrote:
> On 08/06/2014 06:36 PM, Pavel Reichl wrote:
>>
>> On 08/04/2014 02:37 PM, Pavel Březina wrote:
>>> On 08/01/2014 07:48 PM, Pavel Reichl wrote:
>>>> Hello,
>>>>
>>>> please see attached patches. Every patch is documented in its commit
>>>> message.
>>>>
>>>> Generally speaking the first 6 patches are preparation for patch #8.
>>>>
>>>> Thanks,
>>>>
>>>> Pavel Reichl
>>>>
>>>> PS: I also attached output of my testing to make it more obvious 
>>>> how the
>>>> patches are supposed to work.
>>>>
>>>> ------------------------------------------------------------------------ 
>>>>
>>>> # john, people, example.com
>>>> dn: uid=john,ou=people,dc=example,dc=com
>>>> pwdAccountLockedTime: 000001010000Z
>>>>
>>>> # max, people, example.com
>>>> dn: uid=max,ou=people,dc=example,dc=com
>>>>
>>>> # dick, people, example.com
>>>> dn: uid=dick,ou=people,dc=example,dc=com
>>>> pwdAccountLockedTime: 20140801115742Z
>>>> -------------------------------------------------------------------------- 
>>>>
>>>>
>>>> $ ssh -l john at openldap `hostname`
>>>> Connection closed by UNKNOWN
>>>>
>>>> $ ssh -l max at openldap `hostname`
>>>> Last login: Fri Aug  1 15:16:21 2014 from sssd.dev.work
>>>>
>>>> $ ssh -l dick at openldap `hostname`
>>>> Last login: Fri Aug  1 12:57:33 2014
>>>
>>> Patch #1:
>>> Tentative ack, although you don't have to use tmp_ctx in
>>> sdap_save_user_cache_bool(). You can just allocate attrs on NULL since
>>> it is the only talloc context you use there.
>>>
>>> Patch #2:
>>> Nack. I see it is safe here but please avoid initializing ret to EOK.
>>> Set it to EOK at the end of the function instead.
>>>
>>> Patch #3:
>>> Ack.
>>>
>>> Patch #4:
>>> Typo in commit message "Also remove *sdap_access_filter_recv()* as it
>>> is replaced by *sdap_access_filter_recv()*."
>>>
>>> But it's nack from me anyway.
>>>
>>> Tevent naming convention seems to be broken all around this code and
>>> this patch actually makes it a little bit better. But you shouldn't
>>> use function named "sdap_access_recv" inside "sdap_access_done".
>>>
>>> I see that sdap_access_send can invoke two different requests
>>> depending on the configuration. Ideally both should have custom _recv
>>> and you should use some kind of switch to determine which one should
>>> be called.
>>>
>>> I would be willing to tentatively ack a patch with let's say
>>> "sdap_access_check_recv" or "sdap_access_subreq_recv" or similar used
>>> in sdap_access_done.
>>>
>>> Patch #5:
>>> Ack.
>>>
>>> Patch #6:
>>> Ack.
>>>
>>> Patch #7:
>>> Ack. But please swap this ticket with #8. The feature should be first
>>> implemented then documented.
>>>
>>> Patch #8:
>>> I believe the following comment is copy & paste? It shouldn't say
>>> filter request :)
>>> +    /* Connection to LDAP succeeded
>>> +     * Send filter request
>>> +     */
>>>
>>> +            if (strcasecmp(pwdAccountLockedTime,
>>> +                           PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>>> +                locked = true;
>>> +            } else {
>>> +                DEBUG(SSSDBG_TRACE_FUNC,
>>> +                      "Account of: %s is beeing blocked by password
>>> policy, "
>>> +                      "but value: [%s] value is ignored by SSSD.\n",
>>> +                      state->username, pwdAccountLockedTime);
>>> +            }
>>>
>>> Why do we ignore the value? Isn't the value a time? Shouldn't we check
>>> if lock_time < machine_time?
>> Yes, it's time, but we care only if its the special value
>> '000001010000Z' otherwise we grant the access.
>>>
>>> Also the ticket says that this policy is only enabled if pwdLockout is
>>> true. Is it possible to acquire the attribute if pwdLockout is false?
>>> I see you don't check it.
>> Yes it's possible to acquire, I have updated the patches.
>>>
>>> Those are questions that should be answered in the code if they are
>>> not bugs.
>>>
>>> I haven't test the code yet - will it work also with 389ds or do you
>>> have an OpenLDAP server I could use handy?
>>>
>> I'll try to grant you access to my VM as I find configuring ppolicy as
>> very painful experience.
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> I hope I have addressed all you concerns and didn't produce many new
>> bugs. :-)
>
> Patch 1: ACK
> Patch 2: ACK
> Patch 3: ACK
> Patch 4: ACK
> Patch 5: ACK
> Patch 6:
> +    /* Used for both filter and locked use-cases. */
> nitpick: use case doesn't have a hyphen inside
>
> But more importantly, you changed a sysdb attribute name here. It will 
> require sysdb update to convert the old attribute name to the new one. 
> Maybe it would be better to keep the existing attribute and also 
> create a new one for lock? Opinions?
Hmm thanks for noticing that.  I personally dislike the idea of having 
two separate sysdb attributes for 'filter' and 'lockout' option and with 
that connected code which would be doubled. I would prefer to have a 
single sysdb attribute representing the result of whole access control 
stack process.

Now tell me please sysdb update - maybe I'll change my mind then. :-)
>
> Patch 7:
> The option is suppose to configure dn of an object that contains 
> pwdLockout attribute so I believe it should not be named 
> ldap_ppolicy_search_base - you do not treat it as a search base 
> anyway, the usage is different from other similar options.
>
> I would suggest ldap_pwdlockout_dn?
You are right, I'll do it as you proposed.
>
> If I understand it [1] correctly the attribute may be defined in any 
> object of class pwdPolicy and dn of the policy is configured in 
> OpenLDAP configuration. Different users may have different policy 
> configured but we don't have to deal with this now IMHO so the current 
> approach is fine.
>
> Patch 8:
> sdap_access_lock_send() initializing ret to an error is certainly 
> better then EOK but please, try to avoid it unless you take some 
> advantage from it. It is completely unused here. We want to let gcc to 
> catch cases where 'ret' remains uninitialized.
OK, will fix it there and I'll try to remember next time.
>
> Tevent style:
> - please, keep _send, _done and _recv function together (i.e. move 
> lock_recv after lock_done)
> - _send function should create a new tevent_req with a new state, so 
> either discard or rename sdap_access_locked_time_send
>
> Patch 9:
> LGTM besides the option name. Can a native speaker check it please?
>
>
> In general, please add more comments (mostly about the questions 
> asked) around patches #7 and #8 (either in the code directly or in the 
> commit message). I think this code contains a lot of premisses that 
> are not known to us and need to be understood so we can effectively 
> maintain the code.
>
> [1] http://www.zytrax.com/books/ldap/ch6/ppolicy.html
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list