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@openldap `hostname`
>>> Connection closed by UNKNOWN
>>>
>>> $ ssh -l max@openldap `hostname`
>>> Last login: Fri Aug 1 15:16:21 2014 from sssd.dev.work
>>>
>>> $ ssh -l dick@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(a)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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks Pavel,
please see updated patches.
Stephen,
I took the liberty of CCing you because I was wondering if you would
mind to review a man page change (patch #9).
Regards,
Pavel Reichl