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!
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
