[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
Thu Aug 7 12:22:46 UTC 2014


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?

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?

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.

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



More information about the sssd-devel mailing list