On 08/07/2014 11:17 AM, Jakub Hrozek wrote:
On Thu, Aug 07, 2014 at 11:13:34AM +0200, Pavel Reichl wrote:
On 08/07/2014 11:00 AM, Jakub Hrozek wrote:
On Thu, Aug 07, 2014 at 10:52:37AM +0200, Pavel Reichl wrote:
On 08/07/2014 10:45 AM, 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@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?
Because that's how it's described in BZ
https://bugzilla.redhat.com/show_bug.cgi?id=1099290 - or do you understand
it differently than me?
Can you please check with the openldap maintainer if that value indeed
has some special meaning?
Sure, but I'm not sure what should I be asking exactly?
This is from man slapo-ppolicy:
pwdAccountLockedTime
This attribute contains the time that the user's account was locked.
If the account has been locked, the password may no longer be used to
authenticate the user to the directory. If pwdAccount‐
LockedTime is set to 000001010000Z, the user's account has been
permanently locked and may only be unlocked by an administrator. Note that
account locking only takes effect when the pwdLockout pass‐
word policy attribute is set to "TRUE".
Then you're right :-) I haven't read the man page TBH. This would be a
nice addition to the commit message, too!
OK, I'll do that, pbrezina had similar request. I'll just wait for more
comments to address them all at once.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
