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.
Why?
- or do you
understand it differently than me?
In SSSD we only deny access if account is locked permanently, other
cases should be handled on ppolicy side.
>>
>> 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. :-)
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel