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. :-)