[PATCHES] SDAP: Lock out ssh keys when account naturally expires

[Pavel Reichl]

Hello,

please see attached patches.

Does it make sense to check on cases when pwdAccountLockedTime is some 
future time event?

If agreed to I'll send patch adding test for new utility function 
convert_time().

Thanks!

[Lukas Slebodnik]

On (21/01/15 11:52), Pavel Reichl wrote:
> Hello,
>
> please see attached patches.
>
> Does it make sense to check on cases when pwdAccountLockedTime is some future
> time event?
>
> If agreed to I'll send patch adding test for new utility function
> convert_time().
>
> Thanks!
> From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 19 Jan 2015 03:24:09 -0500
> Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
>
> This is a follow up to #2364.
> To distinguish user locked out from accessing machine via SSH if an
> account was administratively locked (pwdAccountLockedTime set to
> 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> password is locked out from natural reasons (too many attempts, expired
> password).
>
> Part of solution for:
> https://fedorahosted.org/sssd/ticket/2534

@see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
> From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 2/3] UTIL: convert general time to unix time
>
> New utility function *convert_time* to convert 'general time' to
> 'unix time'.
> From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2534

Have you considered to write unit test?

LS

[Pavel Reichl]

On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> On (21/01/15 11:52), Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patches.
> >
> > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > time event?
> >
> > If agreed to I'll send patch adding test for new utility function
> > convert_time().
> >
> > Thanks!
> > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> >
> > This is a follow up to #2364.
> > To distinguish user locked out from accessing machine via SSH if an
> > account was administratively locked (pwdAccountLockedTime set to
> > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > password is locked out from natural reasons (too many attempts, expired
> > password).
> >
> > Part of solution for:
> > https://fedorahosted.org/sssd/ticket/2534
>
> @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html

see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> >
> > New utility function *convert_time* to convert 'general time' to
> > 'unix time'.
> >
> > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2534
>
> Have you considered to write unit test?

For the second patch, yes.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (22/01/15 09:45), Pavel Reichl wrote:
> On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> > On (21/01/15 11:52), Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see attached patches.
> > >
> > > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > > time event?
> > >
> > > If agreed to I'll send patch adding test for new utility function
> > > convert_time().
> > >
> > > Thanks!
> > > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > >
> > > This is a follow up to #2364.
> > > To distinguish user locked out from accessing machine via SSH if an
> > > account was administratively locked (pwdAccountLockedTime set to
> > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > password is locked out from natural reasons (too many attempts, expired
> > > password).
> > >
> > > Part of solution for:
> > > https://fedorahosted.org/sssd/ticket/2534
> >
> > @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
>
> see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> > >
> > > New utility function *convert_time* to convert 'general time' to
> > > 'unix time'.
> > >
> > > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2534
> >
> > Have you considered to write unit test?
>
> For the second patch, yes.

So I'll wait for patchset with unit test(s).

LS

[Pavel Reichl]

On 01/22/2015 09:51 AM, Lukas Slebodnik wrote:
> On (22/01/15 09:45), Pavel Reichl wrote:
> > On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> > > On (21/01/15 11:52), Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see attached patches.
> > > >
> > > > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > > > time event?
> > > >
> > > > If agreed to I'll send patch adding test for new utility function
> > > > convert_time().
> > > >
> > > > Thanks!
> > > > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > > >
> > > > This is a follow up to #2364.
> > > > To distinguish user locked out from accessing machine via SSH if an
> > > > account was administratively locked (pwdAccountLockedTime set to
> > > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > > password is locked out from natural reasons (too many attempts, expired
> > > > password).
> > > >
> > > > Part of solution for:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
> >
> > see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > > > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> > > >
> > > > New utility function *convert_time* to convert 'general time' to
> > > > 'unix time'.
> > > >
> > > > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > Have you considered to write unit test?
> >
> > For the second patch, yes.
>
> So I'll wait for patchset with unit test(s).
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

In my first post I wrote
> If agreed to I'll send patch adding test for new utility function
> convert_time().

So you could just write that you think this utility function is a good 
idea and I would prepare the test then. Never mind, I'll do it asap, as 
it seems really vital for you.

[Pavel Reichl]

On 01/22/2015 09:51 AM, Lukas Slebodnik wrote:
> On (22/01/15 09:45), Pavel Reichl wrote:
> > On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> > > On (21/01/15 11:52), Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see attached patches.
> > > >
> > > > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > > > time event?
> > > >
> > > > If agreed to I'll send patch adding test for new utility function
> > > > convert_time().
> > > >
> > > > Thanks!
> > > > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > > >
> > > > This is a follow up to #2364.
> > > > To distinguish user locked out from accessing machine via SSH if an
> > > > account was administratively locked (pwdAccountLockedTime set to
> > > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > > password is locked out from natural reasons (too many attempts, expired
> > > > password).
> > > >
> > > > Part of solution for:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
> >
> > see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > > > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> > > >
> > > > New utility function *convert_time* to convert 'general time' to
> > > > 'unix time'.
> > > >
> > > > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > Have you considered to write unit test?
> >
> > For the second patch, yes.
>
> So I'll wait for patchset with unit test(s).

BTW do you prefer the unit test to be in the same patch as the tested 
utility function or in a separate patch? Thanks.
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (22/01/15 10:06), Pavel Reichl wrote:
> On 01/22/2015 09:51 AM, Lukas Slebodnik wrote:
> > On (22/01/15 09:45), Pavel Reichl wrote:
> > > On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> > > > On (21/01/15 11:52), Pavel Reichl wrote:
> > > > > Hello,
> > > > >
> > > > > please see attached patches.
> > > > >
> > > > > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > > > > time event?
> > > > >
> > > > > If agreed to I'll send patch adding test for new utility function
> > > > > convert_time().
> > > > >
> > > > > Thanks!
> > > > > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > > > > From: Pavel Reichl 
> > > > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > > > >
> > > > > This is a follow up to #2364.
> > > > > To distinguish user locked out from accessing machine via SSH if an
> > > > > account was administratively locked (pwdAccountLockedTime set to
> > > > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > > > password is locked out from natural reasons (too many attempts, expired
> > > > > password).
> > > > >
> > > > > Part of solution for:
> > > > > https://fedorahosted.org/sssd/ticket/2534
> > > >
> > > > @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
> > >
> > > see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > > > > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > > > > From: Pavel Reichl 
> > > > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > > > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> > > > >
> > > > > New utility function *convert_time* to convert 'general time' to
> > > > > 'unix time'.
> > > > >
> > > > > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > > > > From: Pavel Reichl 
> > > > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > > > >
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/2534
> > > >
> > > > Have you considered to write unit test?
> > >
> > > For the second patch, yes.
> >
> > So I'll wait for patchset with unit test(s).
> In my first post I wrote
> > If agreed to I'll send patch adding test for new utility function
> > convert_time().
>
> So you could just write that you think this utility function is a good idea
> and I would prepare the test then. Never mind, I'll do it asap, as it seems
> really vital for you.

The new utility function is not documented and does not have a unit test.
It isn't about my preferences. It's about best practices and improving our code
coverage.
> BTW do you prefer the unit test to be in the same patch as the tested utility
> function or in a separate patch? Thanks.

New function has just 37 lines. The unit test in separate patch make sense
if functionality is very complicated itself.

You should remember the fact that small utility function can be back ported
to older branches after few months and you can forget to back port test.

LS

[Pavel Reichl]

On 01/22/2015 10:22 AM, Lukas Slebodnik wrote:
> On (22/01/15 10:06), Pavel Reichl wrote:
> > On 01/22/2015 09:51 AM, Lukas Slebodnik wrote:
> > > On (22/01/15 09:45), Pavel Reichl wrote:
> > > > On 01/22/2015 09:27 AM, Lukas Slebodnik wrote:
> > > > > On (21/01/15 11:52), Pavel Reichl wrote:
> > > > > > Hello,
> > > > > >
> > > > > > please see attached patches.
> > > > > >
> > > > > > Does it make sense to check on cases when pwdAccountLockedTime is some future
> > > > > > time event?
> > > > > >
> > > > > > If agreed to I'll send patch adding test for new utility function
> > > > > > convert_time().
> > > > > >
> > > > > > Thanks!
> > > > > > From 4560b5d52e2c816df7b6479908a63cad6bbb8622 Mon Sep 17 00:00:00 2001
> > > > > > From: Pavel Reichl 
> > > > > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > > > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > > > > >
> > > > > > This is a follow up to #2364.
> > > > > > To distinguish user locked out from accessing machine via SSH if an
> > > > > > account was administratively locked (pwdAccountLockedTime set to
> > > > > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > > > > password is locked out from natural reasons (too many attempts, expired
> > > > > > password).
> > > > > >
> > > > > > Part of solution for:
> > > > > > https://fedorahosted.org/sssd/ticket/2534
> > > > >
> > > > > @see https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html
> > > >
> > > > see commit in master: 022456e93c9b175ce3774afe524e3926f41ba80f
> > > > > > From 84385a53068c4832d1c66eb3a767c8e553b24b4f Mon Sep 17 00:00:00 2001
> > > > > > From: Pavel Reichl 
> > > > > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > > > > Subject: [PATCH 2/3] UTIL: convert general time to unix time
> > > > > >
> > > > > > New utility function *convert_time* to convert 'general time' to
> > > > > > 'unix time'.
> > > > > >
> > > > > > From c5fc4ea54bab0a9933148f4ef22a2d28cd5c71a7 Mon Sep 17 00:00:00 2001
> > > > > > From: Pavel Reichl 
> > > > > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > > > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > > > > >
> > > > > > Resolves:
> > > > > > https://fedorahosted.org/sssd/ticket/2534
> > > > >
> > > > > Have you considered to write unit test?
> > > >
> > > > For the second patch, yes.
> > >
> > > So I'll wait for patchset with unit test(s).
> >
> > In my first post I wrote
> > > If agreed to I'll send patch adding test for new utility function
> > > convert_time().
> >
> > So you could just write that you think this utility function is a good idea
> > and I would prepare the test then. Never mind, I'll do it asap, as it seems
> > really vital for you.
>
> The new utility function is not documented and does not have a unit test.
> It isn't about my preferences. It's about best practices and improving our code
> coverage.
> > BTW do you prefer the unit test to be in the same patch as the tested utility
> > function or in a separate patch? Thanks.
>
> New function has just 37 lines. The unit test in separate patch make sense
> if functionality is very complicated itself.
>
> You should remember the fact that small utility function can be back ported
> to older branches after few months and you can forget to back port test.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Second patch now contains unit test.

[Lukas Slebodnik]

On (22/01/15 17:06), Pavel Reichl wrote:
> Second patch now contains unit test.
> From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 19 Jan 2015 03:24:09 -0500
> Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
>
> This is a follow up to #2364.
> To distinguish user locked out from accessing machine via SSH if an
> account was administratively locked (pwdAccountLockedTime set to
> 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> password is locked out from natural reasons (too many attempts, expired
> password).
>
> Part of solution for:
> https://fedorahosted.org/sssd/ticket/2534

I'm sorry you missed a note in the 1st mail.
Try to read following mail one more time.
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/020467.html

The most problematic part is that man page is updated before feature
is implemented. Opposite order of patches could be acceptable
but I prefer to have change in man page for small features in the same patch.

It is not necessary to send patches one more time. You can incorporate such
change in next round of review. So please wait.

LS

[Lukas Slebodnik]

On (22/01/15 17:06), Pavel Reichl wrote:
> Second patch now contains unit test.
> From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Mon, 19 Jan 2015 03:24:09 -0500
> Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
>
> This is a follow up to #2364.
> To distinguish user locked out from accessing machine via SSH if an
> account was administratively locked (pwdAccountLockedTime set to
> 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> password is locked out from natural reasons (too many attempts, expired
> password).
>
> Part of solution for:
> https://fedorahosted.org/sssd/ticket/2534

You wrote in description of this ticket that it it is a follow up to #2364.
from my point of view it is an extension.

Do we really need an option to enable this feature?

The current implementation locks account with pwdAccountLockedTime set to
000001010000Z. The other values(dates) are ignored.

The new version implemented in 3rd patch is stricter and will lock account
if value in pwdAccountLockedTime is lower than current date.

I would prefer to get rid of this new option. We try to add new options only
if it is necessary.

Jakub do you have an other opinion?

If we really want to add new option then man page should be changed after
after the feature is implemented not before.
> src/config/SSSDConfig/__init__.py.in     |  1 +
> src/config/etc/sssd.api.d/sssd-ad.conf   |  1 +
> src/config/etc/sssd.api.d/sssd-ipa.conf  |  1 +
> src/config/etc/sssd.api.d/sssd-ldap.conf |  1 +
> src/man/sssd-ldap.5.xml                  | 31 +++++++++++++++++++++++++++++++
> src/providers/ad/ad_opts.h               |  1 +
> src/providers/ipa/ipa_opts.h             |  1 +
> src/providers/ldap/ldap_opts.h           |  1 +
> src/providers/ldap/sdap.h                |  1 +
> 9 files changed, 39 insertions(+)
>
> diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
> index 500bd717fec7abcaafd5153ccca7847b91e208ad..90d5aa2d49454e6f8664349fe3fe3d22943dd037 100644
> --- a/src/config/SSSDConfig/__init__.py.in
> +++ b/src/config/SSSDConfig/__init__.py.in
> @@ -344,6 +344,7 @@ option_strings = {
>     'ldap_min_id' : _('Set lower boundary for allowed IDs from the LDAP server'),
>     'ldap_max_id' : _('Set upper boundary for allowed IDs from the LDAP server'),
>     'ldap_pwdlockout_dn' : _('DN for ppolicy queries'),
>
> 'ldap_pwdlockout_admin_locked_only' : _('Account is locked only if administrator sets specific value (000001010000Z)'),
>
> # [provider/ldap/auth]
> 'ldap_pwd_policy' : _('Policy to evaluate the password expiration'),
>
> diff --git a/src/config/etc/sssd.api.d/sssd-ad.conf b/src/config/etc/sssd.api.d/sssd-ad.conf
> index 3496fb4006697d380f7c9729ed9997272cbce2ea..714ff5ab03601cf73b0356ab9a58a3a2af234d75 100644
> --- a/src/config/etc/sssd.api.d/sssd-ad.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ad.conf
> @@ -124,6 +124,7 @@ ldap_initgroups_use_matching_rule_in_chain = bool, None, false
> ldap_use_tokengroups = bool, None, false
> ldap_rfc2307_fallback_to_local_users = bool, None, false
> ldap_pwdlockout_dn = str, None, false
> +ldap_pwdlockout_admin_locked_only = bool, None, false
>
> [provider/ad/auth]
> krb5_ccachedir = str, None, false
> diff --git a/src/config/etc/sssd.api.d/sssd-ipa.conf b/src/config/etc/sssd.api.d/sssd-ipa.conf
> index 2a3b7ef1519e3476cb4b432336da0c359b1844ba..c02e6f0ba8eeaed322939653658bb43719d67474 100644
> --- a/src/config/etc/sssd.api.d/sssd-ipa.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ipa.conf
> @@ -134,6 +134,7 @@ ldap_use_tokengroups = bool, None, false
> ldap_rfc2307_fallback_to_local_users = bool, None, false
> ipa_server_mode = bool, None, false
> ldap_pwdlockout_dn = str, None, false
> +ldap_pwdlockout_admin_locked_only = bool, None, false
> ipa_views_search_base = str, None, false
> ipa_view_class = str, None, false
> ipa_view_name = str, None, false
> diff --git a/src/config/etc/sssd.api.d/sssd-ldap.conf b/src/config/etc/sssd.api.d/sssd-ldap.conf
> index ba5f56f1942da552fc6ab8f82851714756683a8f..4beb50fc8cbeea18ce6948bfa0a72da650fbb934 100644
> --- a/src/config/etc/sssd.api.d/sssd-ldap.conf
> +++ b/src/config/etc/sssd.api.d/sssd-ldap.conf
> @@ -122,6 +122,7 @@ ldap_rfc2307_fallback_to_local_users = bool, None, false
> ldap_min_id = int, None, false
> ldap_max_id = int, None, false
> ldap_pwdlockout_dn = str, None, false
> +ldap_pwdlockout_admin_locked_only = bool, None, false
>
> [provider/ldap/auth]
> ldap_pwd_policy = str, None, false
> diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
> index 9f2e9ac34add13e40d316374094024afdcc4ae31..3e6520995fb4af4e1b9eca7a4535616b2e1ccef6 100644
> --- a/src/man/sssd-ldap.5.xml
> +++ b/src/man/sssd-ldap.5.xml
> @@ -1998,6 +1998,37 @@ ldap_access_filter = (employeeType=admin)
>                 </varlistentry>
>             <varlistentry>
>                <term>ldap_pwdlockout_admin_locked_only (string)</term>
>                <listitem>
>                    <para>
>                      If set to true then account is considered locked only if
>                      attribute pwdAccountLockedTime is equal to
>                      000001010000Z.
>                      Otherwise account is considered locked if:
>                      <itemizedlist>
>                          <listitem>
>                              <para>
>                                value of pwdAccountLockedTime is time event
>                                in past
>                              </para>
>                          </listitem>
>                          <listitem>
>                              <para>
>                                and if pwdLockoutDuration attribute is
>                                present and is not equal to zero then
>                                (pwdAccountLockedTime + pwdLockoutDuration)
>                                is time event in future.
>                              </para>
>                          </listitem>
>                      </itemizedlist>
>                    </para>
>                    <para>
>                      Default: true
>                    </para>
>                </listitem>
>            </varlistentry>
>            <varlistentry>
>               <term>ldap_deref (string)</term>
>               <listitem>
>                   <para>
>
> diff --git a/src/providers/ad/ad_opts.h b/src/providers/ad/ad_opts.h
> index d9405e5020ca724a0f7caa752ac10fb07d8aa397..87f3e5d10817923123c5f51425cc5ddeb930f28d 100644
> --- a/src/providers/ad/ad_opts.h
> +++ b/src/providers/ad/ad_opts.h
> @@ -144,6 +144,7 @@ struct dp_option ad_def_ldap_opts[] = {
>     { "ldap_min_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_max_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_pwdlockout_dn", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> { "ldap_pwdlockout_admin_locked_only", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> DP_OPTION_TERMINATOR
>
> };
>
> diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
> index 66af648583e552d7edd932f6bb5a2c3bef107e51..59a4bcba4b8afd1194a0f16a28c14473364253d1 100644
> --- a/src/providers/ipa/ipa_opts.h
> +++ b/src/providers/ipa/ipa_opts.h
> @@ -158,6 +158,7 @@ struct dp_option ipa_def_ldap_opts[] = {
>     { "ldap_min_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_max_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_pwdlockout_dn", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> { "ldap_pwdlockout_admin_locked_only", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> DP_OPTION_TERMINATOR
>
> };
>
> diff --git a/src/providers/ldap/ldap_opts.h b/src/providers/ldap/ldap_opts.h
> index 7c9ed3e01f726f2ba6ecb2a7268867abd3baa37d..a3307305cbeeb2c13a2b8ac44c54e5c56ff2893b 100644
> --- a/src/providers/ldap/ldap_opts.h
> +++ b/src/providers/ldap/ldap_opts.h
> @@ -122,6 +122,7 @@ struct dp_option default_basic_opts[] = {
>     { "ldap_min_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_max_id", DP_OPT_NUMBER, NULL_NUMBER, NULL_NUMBER},
>     { "ldap_pwdlockout_dn", DP_OPT_STRING, NULL_STRING, NULL_STRING },
> { "ldap_pwdlockout_admin_locked_only", DP_OPT_BOOL, BOOL_TRUE, BOOL_TRUE },
> DP_OPTION_TERMINATOR
>
> };
>
> diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
> index 921051b41a911a2d1117672a8e9c2697b679f24e..20020a62f288fcf3bd9f8f9060cdcfc3b3043a47 100644
> --- a/src/providers/ldap/sdap.h
> +++ b/src/providers/ldap/sdap.h
> @@ -231,6 +231,7 @@ enum sdap_basic_opt {
>     SDAP_MIN_ID,
>     SDAP_MAX_ID,
>     SDAP_PWDLOCKOUT_DN,
>
> SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY,
>
> SDAP_OPTS_BASIC /* opts counter */
> };
>
> 2.1.0
> From 942f9ab711ac787f10bcc18bd8b638e6f8a540b9 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 2/3] UTIL: convert GeneralizedTime to unix time
>
> New utility function *convert_time* to convert GeneralizedTime to
> unix time.
>
> Makefile.am            |  3 ++-
> src/tests/util-tests.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> src/util/util.c        | 39 +++++++++++++++++++++++++++++++++++++++
> src/util/util.h        |  3 +++
> 4 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index ca8921bb132db6b7a57d9436a3219057d08b64c6..2da86572752d0702b8983f4cc647188c8cc81382 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1535,7 +1535,8 @@ simple_access_tests_LDADD = \
>     libsss_test_common.la
>
> util_tests_SOURCES = \
> src/tests/util-tests.c
> src/tests/util-tests.c \
> src/util/util.c

It's better to link test with internal library (libsss_util.so) rather
then add source file to the test. It's not necessary to change it now.
Please try to ammend cmocka test in future. (test_utils)

It's not necessary to change it now. It's just info for the future.

BTW. You didn't add $(NULL) to the end of aotomake variable.
> util_tests_CFLAGS = \
>     $(AM_CFLAGS) \
>     $(CHECK_CFLAGS)
> diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
> index 94015d8e1e0efb143a4fea998f1b16db1e63365e..ebfae703ff7962c12b9eddf83d2c2e61db29b6eb 100644
> --- a/src/tests/util-tests.c
> +++ b/src/tests/util-tests.c
> @@ -28,6 +28,8 @@
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> +#include <stdlib.h>
>
> #include "util/util.h"
> #include "util/sss_utf8.h"
> #include "util/murmurhash3.h"
> @@ -1020,6 +1022,44 @@ START_TEST(test_known_service)
> }
> END_TEST
>
> +static void convert_time_tz(void)

Why do wee need a static wunction which does not accept any argument?
It's better to inline it or add argument timezone and change timezone(setenv)
inside function.
> +{
> errno_t ret;
> time_t unix_time;
> ret = convert_time("20140801115742Z", "%Y%m%d%H%M%SZ", &unix_time);
> fail_unless(ret == EOK && difftime(1406894262, unix_time) == 0);
>
> +}
>
> +START_TEST(test_convert_time)
> +{
> const char *format = "%Y%m%d%H%M%SZ";
> time_t unix_time;
> errno_t ret;
> const char *orig_tz;
> ret = convert_time("0", format, &unix_time);
> fail_unless(ret == EINVAL);
> ret = convert_time("000001010000Z", format, &unix_time);
> fail_unless(ret == EINVAL);
> /* test that results are still same no matter what timezone is set */
> convert_time_tz();
> orig_tz = getenv("TZ");
> ret = setenv("TZ", "GST-1", 1);
> fail_if(ret == -1);
> convert_time_tz();
> ret = setenv("TZ", "GST-2", 1);
> fail_if(ret == -1);
> convert_time_tz();
> ret = setenv("TZ", orig_tz, 1);
> fail_if(ret == -1);
>
> +}
> +END_TEST
>
> Suite *util_suite(void)
> {
>     Suite *s = suite_create("util");
> @@ -1067,10 +1107,17 @@ Suite *util_suite(void)
>     tcase_add_test(tc_atomicio, test_atomicio_read_exact_sized_file);
>     tcase_add_test(tc_atomicio, test_atomicio_read_from_empty_file);
> TCase *tc_convert_time = tcase_create("convert_time");
> tcase_add_checked_fixture (tc_convert_time,

^
                               extra space
>                           ck_leak_check_setup,
>                           ck_leak_check_teardown);
>
> tcase_add_test(tc_convert_time, test_convert_time);
>
> suite_add_tcase (s, tc_util);
>   suite_add_tcase (s, tc_utf8);
>   suite_add_tcase (s, tc_mh3);
>   suite_add_tcase (s, tc_atomicio);
>
> suite_add_tcase (s, tc_convert_time);
>
> return s;
>
> }
> diff --git a/src/util/util.c b/src/util/util.c
> index 613c559bb2002686c7833642d0946e46e5a9b5d6..eb3002fd4dde5b88a6595aee7186bae8d7d773d2 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see http://www.gnu.org/licenses/.
> */
>
> +#include "config.h"
> #include <ctype.h>
> #include <netdb.h>
> #include <poll.h>
> @@ -26,6 +27,7 @@
> #include <arpa/inet.h>
> #include <talloc.h>
> #include <dhash.h>
> +#include <time.h>
>
> #include "util/util.h"
> #include "util/sss_utf8.h"
> @@ -904,3 +906,40 @@ errno_t sss_fd_nonblocking(int fd)
> return EOK;
>
> }
>
> +/* Convert GeneralizedTime (http://en.wikipedia.org/wiki/GeneralizedTime)
> to unix time (seconds since epoch). Use UTC time zone.
> */
>
> +errno_t convert_time(const char *str, const char *format, time_t *unix_time)
> +{
> char *end;
> struct tm tm;
> memset(&tm, 0, sizeof(tm));
> end = strptime(str, format, &tm);
> /* not all characters from format were matched */
> if (end == NULL) {
>    DEBUG(SSSDBG_TRACE_INTERNAL,
>          "String [%s] failed to match format [%s].\n", str, format);
>    return EINVAL;
> }
> /* str is 'longer' than format */
> if (*end != '\0') {
>    DEBUG(SSSDBG_TRACE_INTERNAL,
>          "String [%s] is longer than format [%s].\n", str, format);
>    return EINVAL;
> }
> *unix_time = mktime(&tm);
> if (*unix_time == -1) {
>    DEBUG(SSSDBG_TRACE_INTERNAL,
>          "mktime failed to convert [%s].\n", str);
>    return EINVAL;
> }
> tzset();
> *unix_time -= timezone;

We prefer to touch output argument just once. (just set output argumetn with
  local variable.

The 1st benefit is that function is not poluted with dereferencing of pointer
  (code is nicer). The second benefit is that you set default vale just once.
  It is not case in this function.
> return EOK;
>
> +}
> diff --git a/src/util/util.h b/src/util/util.h
> index 60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be..bc23d7bc5d6307f63a480c5faaeaa98ca921bd46 100644
> --- a/src/util/util.h
> +++ b/src/util/util.h
> @@ -641,4 +641,7 @@ int set_seuser(const char *login_name, const char *seuser_name,
>                const char *mlsrange);
> int del_seuser(const char *login_name);
>
> +/* convert time from generalized form to unix time */
> +errno_t convert_time(const char *str, const char *format, time_t *unix_time);

Please try to prefix this function. I know it's part of internal library,
 but the name is very general and in theory can conflic with function in other
 libraries/ (prefix sss_ or s3_ is fine)

The output argumet shout be prefixed with underscore (_).
So it is obviout which pointer is input and which is output argument.
> #endif /* __SSSD_UTIL_H__ */
>
> 2.1.0
> From 9bb77a8cb8501d7827da8457d434dc31321aca50 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2534
>
> Makefile.am                      |   3 +-
> src/providers/ldap/sdap_access.c | 106 ++++++++++++++++++++++++++++++++++-----
> src/providers/ldap/sdap_access.h |   1 +
> 3 files changed, 96 insertions(+), 14 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 2da86572752d0702b8983f4cc647188c8cc81382..e780b7098cc853dfaaa4ae4d830cc410fa94eb80 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2383,7 +2383,8 @@ libsss_ldap_common_la_SOURCES = \
>     src/providers/ldap/sdap_domain.c \
>     src/providers/ldap/sdap.c \
>     src/util/user_info_msg.c \
> src/util/sss_ldap.c
> src/util/sss_ldap.c \
> src/util/util.c

The file src/util/util.c is part of internal library libsss_util.so
It's better to link this module with library rather then add it to source file.
> libsss_ldap_common_la_CFLAGS = \
>     $(KRB5_CFLAGS)
> libsss_ldap_common_la_LIBADD = \
> diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> index e09e6b8fe55e8e8bae9ff7f46e815595cb938971..39a6f4397f83b2ca560d0fb883002a97caf73c74 100644
> --- a/src/providers/ldap/sdap_access.c
> +++ b/src/providers/ldap/sdap_access.c
> @@ -1568,7 +1568,9 @@ errno_t sdap_access_lock_step(struct tevent_req *req)
>     errno_t ret;
>     struct tevent_req *subreq;
>     struct sdap_access_lock_req_ctx *state;
> const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME, NULL };
>
> const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
>                        SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
>                        NULL };
>
> state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
>
> @@ -1597,15 +1599,86 @@ done:
>     return ret;
> }
>
> +static errno_t
> +decide_if_account_is_locked(bool locked_only_by_admin,
>                        const char *pwdAccountLockedTime,
>                        const char *pwdAccountLockedDurationTime,
>                        bool *locked)

output argumet should be prefixed with "_"
> +{
> errno_t ret;
> time_t lock_time;
> time_t duration;
> time_t now;
> /* Default action is to consider account to be locked. */
> *locked = true;

It's more readable if code does not touch output argument very often.
Functions is nicer if stars "*" are not used very often.
> /* account is permanently locked */
> if (strcasecmp(pwdAccountLockedTime,
>               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>    goto done;

return variable ret is not initialized.

@see warnigs
Error: UNINIT (CWE-457): [#def1]
sssd-1.12.90/src/providers/ldap/sdap_access.c:1620: var_decl: Declaring variable "ret" without initializer.
sssd-1.12.90/src/providers/ldap/sdap_access.c:1680: uninit_use: Using uninitialized value "ret".

Error: CLANG_WARNING: [#def2]
sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:5: warning: Undefined or garbage value returned to caller
#    return ret;
#    ^~~~~~~~~~

Error: COMPILER_WARNING: [#def4]
sssd-1.12.90/src/providers/ldap/sdap_access.c: scope_hint: In function 'sdap_access_lock_step_done'
sssd-1.12.90/src/providers/ldap/sdap_access.c:1758:16: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#             if (ret != EOK) {
#
> }
> if (locked_only_by_admin) {
>    /* We do *not* care about exact value of account locked time, we
>     * only *do* care if the value is equal to
>     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
>     * permanently.
>     */
>    DEBUG(SSSDBG_TRACE_FUNC,
>          "Account is beeing blocked by password policy, "
>          "but value: [%s] value is ignored by SSSD.\n",
>          pwdAccountLockedTime);
>    *locked = false;
> } else {
>    /* Account may be locked out from natural reasons (too many attempts,
>     * expired password). In this case, pwdAccountLockedTime is also set,
>     * to the time of lock out.
>     */
>    ret = convert_time(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
>                       &lock_time);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_TRACE_FUNC, "convert_time() failed with %d:%s.\n",
>              ret, sss_strerror(ret));
>        goto done;
>    }
>    now = time(NULL);
>    /* Account was NOT locked in past. */
>    if (difftime(lock_time, now) > 0.0) {
>        *locked = false;
>    } else if (pwdAccountLockedDurationTime != NULL) {
>        errno = 0;
>        duration = strtol(pwdAccountLockedDurationTime, NULL, 0);
>        if (errno) {
>            ret = errno;
>            goto done;
>        }
>        /* Lockout has expired */
>        if (duration != 0 && difftime(now, lock_time) > duration) {
>            *locked = false;
>        }
>    }
> }
> ret = EOK;
>
> +done:
> return ret;
>
> +}
>
> static void sdap_access_lock_step_done(struct tevent_req *subreq)
> {
>     int ret, tret, dp_error;
>     size_t num_results;
>     bool locked = false;
>     const char *pwdAccountLockedTime;
>
> const char *pwdAccountLockedDurationTime;
> struct sysdb_attrs **results;
> struct tevent_req *req;
> struct sdap_access_lock_req_ctx *state;
>
> bool pwd_admin_locked_only;
>
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
>
> @@ -1613,6 +1686,9 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
>     ret = sdap_get_generic_recv(subreq, state, &num_results, &results);
>     talloc_zfree(subreq);
> pwd_admin_locked_only = dp_opt_get_bool(state->opts->basic,
>                                        SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY);

I have already commented it in 1st patch that IMHO we do not need this option
at all.

If other developers agree we need that option then please move it to the place
where we DO NEED it. Because it is used just once but initialized far far away
from place where it is used.

There is another warning from static analyser.
I think iit's false possitive because we needn't test output variable of
dp_opt_get_bool.

Error: CHECKED_RETURN (CWE-252): [#def3]
sssd-1.12.90/src/providers/ldap/sdap_access.c:1701: check_return: Calling "_dp_opt_get_bool" without checking return value (as is done elsewhere 48 out of 67 times).
sssd-1.12.90/src/providers/ad/ad_common.c:1170: example_checked: Example 1: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
sssd-1.12.90/src/providers/ad/ad_dyndns.c:48: example_checked: Example 2: "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>) == 0".
sssd-1.12.90/src/providers/ad/ad_id.c:230: example_checked: Example 3: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
sssd-1.12.90/src/providers/ad/ad_init.c:235: example_checked: Example 4: "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)".
sssd-1.12.90/src/providers/ipa/ipa_init.c:332: example_checked: Example 5: "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)".
sssd-1.12.90/src/providers/ldap/sdap_access.c:1754: unchecked_value: Using value "pwd_admin_locked_only" as a function argument without checking appropriately.
> ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> if (ret != EOK) {
>     if (dp_error == DP_ERR_OK) {
>
> @@ -1653,22 +1729,26 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
>         ret = ERR_INTERNAL;
>         goto done;
>     } else { /* Ok, we got a single reply */
>    ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
>                                 &pwdAccountLockedDurationTime);
>    if (ret != EOK) {
>        /* This attribute might not be set even if account is locked */
>        pwdAccountLockedDurationTime = NULL;
>    }
>   ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACCESS_LOCKED_TIME,
>                                &pwdAccountLockedTime);
>   if (ret == EOK) {
>        /* We do *not* care about exact value of account locked time, we
>         * only *do* care if the value is equal to
>         * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
>         * permanently.
>         */
>        if (strcasecmp(pwdAccountLockedTime,
>                       PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>        ret = decide_if_account_is_locked(pwd_admin_locked_only,
>                                          pwdAccountLockedTime,
>                                          pwdAccountLockedDurationTime,
>                                          &locked);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_MINOR_FAILURE,
>                  "decide_if_account_is_locked failed: %d:[%s]. "
>                  "Account will be considered to be locked.\n",
>                  ret, sss_strerror(ret));
>           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);
>       }
>   } else {
>       /* Attribute SYSDB_LDAP_ACCESS_LOCKED_TIME in not be present unless
>
> diff --git a/src/providers/ldap/sdap_access.h b/src/providers/ldap/sdap_access.h
> index f085e619961198b887d65ed5ee0bc5cdd90d1b20..3e59b9b2f4851a87c01f3e353c11b3bdf9bb4ff7 100644
> --- a/src/providers/ldap/sdap_access.h
> +++ b/src/providers/ldap/sdap_access.h
> @@ -35,6 +35,7 @@
> #define SYSDB_LDAP_ACCESS_CACHED_LOCKOUT "ldap_access_lockout_allow"
> /* names of ppolicy attributes */
> #define SYSDB_LDAP_ACCESS_LOCKED_TIME "pwdAccountLockedTime"
> +#define SYSDB_LDAP_ACESS_LOCKOUT_DURATION "pwdLockoutDuration"
> #define SYSDB_LDAP_ACCESS_LOCKOUT "pwdLockout"
> #define LDAP_ACCESS_FILTER_NAME "filter"
>
> 2.1.0

Patch works as expexted.

Tested with 3 different values of pwdAccountLockedTime

dn: uid=testuser1,ou=Users,dc=example,dc=com
changetype: modify
replace: pwdAccountLockedTime
pwdAccountLockedTime: 000001010000Z

dn: uid=testuser1,ou=Users,dc=example,dc=com
changetype: modify
replace: pwdAccountLockedTime
pwdAccountLockedTime: 20150310003750Z

dn: uid=testuser1,ou=Users,dc=example,dc=com
changetype: modify
replace: pwdAccountLockedTime
pwdAccountLockedTime: 20150301003750Z

LS

[Jakub Hrozek]

On Mon, Mar 02, 2015 at 11:42:06PM +0100, Lukas Slebodnik wrote:
> On (22/01/15 17:06), Pavel Reichl wrote:
> > Second patch now contains unit test.
> > From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> >
> > This is a follow up to #2364.
> > To distinguish user locked out from accessing machine via SSH if an
> > account was administratively locked (pwdAccountLockedTime set to
> > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > password is locked out from natural reasons (too many attempts, expired
> > password).
> >
> > Part of solution for:
> > https://fedorahosted.org/sssd/ticket/2534
>
> You wrote in description of this ticket that it it is a follow up to #2364.
> from my point of view it is an extension.
>
> Do we really need an option to enable this feature?
>
> The current implementation locks account with pwdAccountLockedTime set to
> 000001010000Z. The other values(dates) are ignored.

I would prefer if we didn't add a new option as well, but since we released
a version that only supported the lockout and not any other semantics,
I don't think we can get away with just changing the functionality. A
minor version can break functionality. But a major version can :-)

So I propose the following:
1) Add a new value for ldap_access_order called "ppolicy" that would
evaluate the pwdAccountLockedTime fully, including the new
functionality in this patchset
2) In 1.12, deprecate the "lockout" option and log a warning that it
will be removed in future relase and users should migrate to "ppolicy"
option
3) In master (1.13), remove the "lockout" ldap_access_order value

That way, we won't add a new option and I think the "ppolicy" option is
better anyway.
> > From 942f9ab711ac787f10bcc18bd8b638e6f8a540b9 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > Subject: [PATCH 2/3] UTIL: convert GeneralizedTime to unix time
> >
> > New utility function *convert_time* to convert GeneralizedTime to
> > unix time.

According to the slapo-ppolicy man page, the pwdAccountLockedTime is
just generalizedTime and according to RFC 4517, it's possible to use any
timezone, but the UTC zone is preferred:
~~~~~~~~~~~~~~~~~~~~~
The time value represents coordinated universal time (equivalent to Greenwich
Mean Time) if the "Z" form of <g-time-zone> is used; otherwise, the value
represents a local time in the time zone indicated by <g-differential>.
In the latter case, coordinated universal time can be calculated by
subtracting the differential from the local time.  The "Z" form of
<g-time-zone> SHOULD be used in preference to <g-differential>.
~~~~~~~~~~~~~~~~~~~~~

I would suggest that the utility function would only support the "Z"
form and return a special error code if another timezone is specified.
In that case, the sssd access code would return ACCESS_DENIED and log a
failure saying that the timezone specifier in ppolicy is not supported.

The reason is that when it comes to access control, I strongly prefer
using non-ambiguous formats. We'll see if there are any users out there
who actually use a non-Zulu time in ppolicy and can improve the support
later.

Of course, the name of the function must make it clear that only time
specification with a trailing "Z" is supported, also the man page should
make it clear in description of "ppolicy" ldap_access_order that only "Z"
is supported.
> > Makefile.am            |  3 ++-
> > src/tests/util-tests.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > src/util/util.c        | 39 +++++++++++++++++++++++++++++++++++++++
> > src/util/util.h        |  3 +++
> > 4 files changed, 91 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index ca8921bb132db6b7a57d9436a3219057d08b64c6..2da86572752d0702b8983f4cc647188c8cc81382 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1535,7 +1535,8 @@ simple_access_tests_LDADD = \
> >     libsss_test_common.la
> >
> > util_tests_SOURCES = \
> > src/tests/util-tests.c
> > src/tests/util-tests.c \
> > src/util/util.c
>
> It's better to link test with internal library (libsss_util.so) rather
> then add source file to the test. It's not necessary to change it now.
> Please try to ammend cmocka test in future. (test_utils)
>
> It's not necessary to change it now. It's just info for the future.
>
> BTW. You didn't add $(NULL) to the end of aotomake variable.
> > util_tests_CFLAGS = \
> >     $(AM_CFLAGS) \
> >     $(CHECK_CFLAGS)
> > diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
> > index 94015d8e1e0efb143a4fea998f1b16db1e63365e..ebfae703ff7962c12b9eddf83d2c2e61db29b6eb 100644
> > --- a/src/tests/util-tests.c
> > +++ b/src/tests/util-tests.c
> > @@ -28,6 +28,8 @@
> > #include <sys/types.h>
> > #include <sys/stat.h>
> > #include <fcntl.h>
> > +#include <stdlib.h>
> >
> > #include "util/util.h"
> > #include "util/sss_utf8.h"
> > #include "util/murmurhash3.h"
> > @@ -1020,6 +1022,44 @@ START_TEST(test_known_service)
> > }
> > END_TEST
> >
> > +static void convert_time_tz(void)
>
> Why do wee need a static wunction which does not accept any argument?
> It's better to inline it or add argument timezone and change timezone(setenv)
> inside function.
> > +{
> > errno_t ret;
> > time_t unix_time;
> > ret = convert_time("20140801115742Z", "%Y%m%d%H%M%SZ", &unix_time);
> > fail_unless(ret == EOK && difftime(1406894262, unix_time) == 0);
> >
> > +}
> >
> > +START_TEST(test_convert_time)
> > +{
> > const char *format = "%Y%m%d%H%M%SZ";
> > time_t unix_time;
> > errno_t ret;
> > const char *orig_tz;
> > ret = convert_time("0", format, &unix_time);
> > fail_unless(ret == EINVAL);
> > ret = convert_time("000001010000Z", format, &unix_time);
> > fail_unless(ret == EINVAL);
> > /* test that results are still same no matter what timezone is set */
> > convert_time_tz();
> > orig_tz = getenv("TZ");
> > ret = setenv("TZ", "GST-1", 1);
> > fail_if(ret == -1);
> > convert_time_tz();
> > ret = setenv("TZ", "GST-2", 1);
> > fail_if(ret == -1);
> > convert_time_tz();
> > ret = setenv("TZ", orig_tz, 1);
> > fail_if(ret == -1);
> >
> > +}
> > +END_TEST
> >
> > Suite *util_suite(void)
> > {
> >     Suite *s = suite_create("util");
> > @@ -1067,10 +1107,17 @@ Suite *util_suite(void)
> >     tcase_add_test(tc_atomicio, test_atomicio_read_exact_sized_file);
> >     tcase_add_test(tc_atomicio, test_atomicio_read_from_empty_file);
> > TCase *tc_convert_time = tcase_create("convert_time");
> > tcase_add_checked_fixture (tc_convert_time,
>                            ^
>                            extra space
>
> >                           ck_leak_check_setup,
> >                           ck_leak_check_teardown);
> >
> > tcase_add_test(tc_convert_time, test_convert_time);
> >
> > suite_add_tcase (s, tc_util);
> >   suite_add_tcase (s, tc_utf8);
> >   suite_add_tcase (s, tc_mh3);
> >   suite_add_tcase (s, tc_atomicio);
> >
> > suite_add_tcase (s, tc_convert_time);
> >
> > return s;
> >
> > }
> > diff --git a/src/util/util.c b/src/util/util.c
> > index 613c559bb2002686c7833642d0946e46e5a9b5d6..eb3002fd4dde5b88a6595aee7186bae8d7d773d2 100644
> > --- a/src/util/util.c
> > +++ b/src/util/util.c
> > @@ -18,6 +18,7 @@
> >     along with this program.  If not, see http://www.gnu.org/licenses/.
> > */
> >
> > +#include "config.h"
> > #include <ctype.h>
> > #include <netdb.h>
> > #include <poll.h>
> > @@ -26,6 +27,7 @@
> > #include <arpa/inet.h>
> > #include <talloc.h>
> > #include <dhash.h>
> > +#include <time.h>
> >
> > #include "util/util.h"
> > #include "util/sss_utf8.h"
> > @@ -904,3 +906,40 @@ errno_t sss_fd_nonblocking(int fd)
> > return EOK;
> >
> > }
> >
> > +/* Convert GeneralizedTime (http://en.wikipedia.org/wiki/GeneralizedTime)
> > to unix time (seconds since epoch). Use UTC time zone.
> > */
> >
> > +errno_t convert_time(const char *str, const char *format, time_t *unix_time)
> > +{
> > char *end;
> > struct tm tm;
> > memset(&tm, 0, sizeof(tm));
> > end = strptime(str, format, &tm);
> > /* not all characters from format were matched */
> > if (end == NULL) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL,
> >          "String [%s] failed to match format [%s].\n", str, format);
> >    return EINVAL;
> > }
> > /* str is 'longer' than format */
> > if (*end != '\0') {
> >    DEBUG(SSSDBG_TRACE_INTERNAL,
> >          "String [%s] is longer than format [%s].\n", str, format);
> >    return EINVAL;
> > }
> > *unix_time = mktime(&tm);
> > if (*unix_time == -1) {
> >    DEBUG(SSSDBG_TRACE_INTERNAL,
> >          "mktime failed to convert [%s].\n", str);
> >    return EINVAL;
> > }
> > tzset();
> > *unix_time -= timezone;
>
> We prefer to touch output argument just once. (just set output argumetn with
>   local variable.
>
> The 1st benefit is that function is not poluted with dereferencing of pointer
>   (code is nicer). The second benefit is that you set default vale just once.
>   It is not case in this function.
> > return EOK;
> >
> > +}
> > diff --git a/src/util/util.h b/src/util/util.h
> > index 60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be..bc23d7bc5d6307f63a480c5faaeaa98ca921bd46 100644
> > --- a/src/util/util.h
> > +++ b/src/util/util.h
> > @@ -641,4 +641,7 @@ int set_seuser(const char *login_name, const char *seuser_name,
> >                const char *mlsrange);
> > int del_seuser(const char *login_name);
> >
> > +/* convert time from generalized form to unix time */
> > +errno_t convert_time(const char *str, const char *format, time_t *unix_time);
>
> Please try to prefix this function. I know it's part of internal library,
>  but the name is very general and in theory can conflic with function in other
>  libraries/ (prefix sss_ or s3_ is fine)

Prefix and rename, please :-)

errno_t sss_utc_to_time_t() ?

btw do we need to have this function in src/util? Is it ever going to be
used anywhere else?
> The output argumet shout be prefixed with underscore (_).
> So it is obviout which pointer is input and which is output argument.
> > #endif /* __SSSD_UTIL_H__ */
> >
> > 2.1.0
> > From 9bb77a8cb8501d7827da8457d434dc31321aca50 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2534
> >
> > Makefile.am                      |   3 +-
> > src/providers/ldap/sdap_access.c | 106 ++++++++++++++++++++++++++++++++++-----
> > src/providers/ldap/sdap_access.h |   1 +
> > 3 files changed, 96 insertions(+), 14 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 2da86572752d0702b8983f4cc647188c8cc81382..e780b7098cc853dfaaa4ae4d830cc410fa94eb80 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -2383,7 +2383,8 @@ libsss_ldap_common_la_SOURCES = \
> >     src/providers/ldap/sdap_domain.c \
> >     src/providers/ldap/sdap.c \
> >     src/util/user_info_msg.c \
> > src/util/sss_ldap.c
> > src/util/sss_ldap.c \
> > src/util/util.c
>
> The file src/util/util.c is part of internal library libsss_util.so
> It's better to link this module with library rather then add it to source file.
> > libsss_ldap_common_la_CFLAGS = \
> >     $(KRB5_CFLAGS)
> > libsss_ldap_common_la_LIBADD = \
> > diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> > index e09e6b8fe55e8e8bae9ff7f46e815595cb938971..39a6f4397f83b2ca560d0fb883002a97caf73c74 100644
> > --- a/src/providers/ldap/sdap_access.c
> > +++ b/src/providers/ldap/sdap_access.c
> > @@ -1568,7 +1568,9 @@ errno_t sdap_access_lock_step(struct tevent_req *req)
> >     errno_t ret;
> >     struct tevent_req *subreq;
> >     struct sdap_access_lock_req_ctx *state;
> > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME, NULL };
> >
> > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
> >                        SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> >                        NULL };
> >
> > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> >
> > @@ -1597,15 +1599,86 @@ done:
> >     return ret;
> > }
> >
> > +static errno_t
> > +decide_if_account_is_locked(bool locked_only_by_admin,
> >                        const char *pwdAccountLockedTime,
> >                        const char *pwdAccountLockedDurationTime,
> >                        bool *locked)
>                           output argumet should be prefixed with "_"

Also is_account_locked() would be a shorter name :-)
> > +{
> > errno_t ret;
> > time_t lock_time;
> > time_t duration;
> > time_t now;
> > /* Default action is to consider account to be locked. */
> > *locked = true;
>
> It's more readable if code does not touch output argument very often.
> Functions is nicer if stars "*" are not used very often.
> > /* account is permanently locked */
> > if (strcasecmp(pwdAccountLockedTime,
> >               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> >    goto done;
>
> return variable ret is not initialized.
>
> @see warnigs
> Error: UNINIT (CWE-457): [#def1]
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1620: var_decl: Declaring variable "ret" without initializer.
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1680: uninit_use: Using uninitialized value "ret".
>
> Error: CLANG_WARNING: [#def2]
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:5: warning: Undefined or garbage value returned to caller
> #    return ret;
> #    ^~~~~~~~~~
>
> Error: COMPILER_WARNING: [#def4]
> sssd-1.12.90/src/providers/ldap/sdap_access.c: scope_hint: In function 'sdap_access_lock_step_done'
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1758:16: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> #             if (ret != EOK) {
> #
> > }
> > if (locked_only_by_admin) {
> >    /* We do *not* care about exact value of account locked time, we
> >     * only *do* care if the value is equal to
> >     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> >     * permanently.
> >     */
> >    DEBUG(SSSDBG_TRACE_FUNC,
> >          "Account is beeing blocked by password policy, "
> >          "but value: [%s] value is ignored by SSSD.\n",
> >          pwdAccountLockedTime);
> >    *locked = false;
> > } else {
> >    /* Account may be locked out from natural reasons (too many attempts,
> >     * expired password). In this case, pwdAccountLockedTime is also set,
> >     * to the time of lock out.
> >     */
> >    ret = convert_time(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
> >                       &lock_time);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_TRACE_FUNC, "convert_time() failed with %d:%s.\n",
> >              ret, sss_strerror(ret));
> >        goto done;
> >    }
> >    now = time(NULL);
> >    /* Account was NOT locked in past. */
> >    if (difftime(lock_time, now) > 0.0) {
> >        *locked = false;
> >    } else if (pwdAccountLockedDurationTime != NULL) {
> >        errno = 0;
> >        duration = strtol(pwdAccountLockedDurationTime, NULL, 0);

I think we already have a utility function for this in
src/util/strtonum.c, please use it.
> >        if (errno) {
> >            ret = errno;
> >            goto done;
> >        }
> >        /* Lockout has expired */
> >        if (duration != 0 && difftime(now, lock_time) > duration) {
> >            *locked = false;
> >        }
> >    }
> > }
> > ret = EOK;
> >
> > +done:
> > return ret;
> >
> > +}
> >
> > static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > {
> >     int ret, tret, dp_error;
> >     size_t num_results;
> >     bool locked = false;
> >     const char *pwdAccountLockedTime;
> >
> > const char *pwdAccountLockedDurationTime;
> > struct sysdb_attrs **results;
> > struct tevent_req *req;
> > struct sdap_access_lock_req_ctx *state;
> >
> > bool pwd_admin_locked_only;
> >
> > req = tevent_req_callback_data(subreq, struct tevent_req);
> > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> >
> > @@ -1613,6 +1686,9 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
> >     ret = sdap_get_generic_recv(subreq, state, &num_results, &results);
> >     talloc_zfree(subreq);
> > pwd_admin_locked_only = dp_opt_get_bool(state->opts->basic,
> >                                        SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY);
>
> I have already commented it in 1st patch that IMHO we do not need this option
> at all.
>
> If other developers agree we need that option then please move it to the place
> where we DO NEED it. Because it is used just once but initialized far far away
> from place where it is used.
>
> There is another warning from static analyser.
> I think iit's false possitive because we needn't test output variable of
> dp_opt_get_bool.
>
> Error: CHECKED_RETURN (CWE-252): [#def3]
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1701: check_return: Calling "_dp_opt_get_bool" without checking return value (as is done elsewhere 48 out of 67 times).
> sssd-1.12.90/src/providers/ad/ad_common.c:1170: example_checked: Example 1: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
> sssd-1.12.90/src/providers/ad/ad_dyndns.c:48: example_checked: Example 2: "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>) == 0".
> sssd-1.12.90/src/providers/ad/ad_id.c:230: example_checked: Example 3: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
> sssd-1.12.90/src/providers/ad/ad_init.c:235: example_checked: Example 4: "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)".
> sssd-1.12.90/src/providers/ipa/ipa_init.c:332: example_checked: Example 5: "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)".
> sssd-1.12.90/src/providers/ldap/sdap_access.c:1754: unchecked_value: Using value "pwd_admin_locked_only" as a function argument without checking appropriately.
> > ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> > if (ret != EOK) {
> >     if (dp_error == DP_ERR_OK) {
> >
> > @@ -1653,22 +1729,26 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
> >         ret = ERR_INTERNAL;
> >         goto done;
> >     } else { /* Ok, we got a single reply */
> >    ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> >                                 &pwdAccountLockedDurationTime);
> >    if (ret != EOK) {
> >        /* This attribute might not be set even if account is locked */
> >        pwdAccountLockedDurationTime = NULL;
> >    }
> >   ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACCESS_LOCKED_TIME,
> >                                &pwdAccountLockedTime);
> >   if (ret == EOK) {
> >        /* We do *not* care about exact value of account locked time, we
> >         * only *do* care if the value is equal to
> >         * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> >         * permanently.
> >         */
> >        if (strcasecmp(pwdAccountLockedTime,
> >                       PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> >        ret = decide_if_account_is_locked(pwd_admin_locked_only,
> >                                          pwdAccountLockedTime,
> >                                          pwdAccountLockedDurationTime,
> >                                          &locked);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_MINOR_FAILURE,
> >                  "decide_if_account_is_locked failed: %d:[%s]. "
> >                  "Account will be considered to be locked.\n",
> >                  ret, sss_strerror(ret));
> >           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);
> >       }
> >   } else {
> >       /* Attribute SYSDB_LDAP_ACCESS_LOCKED_TIME in not be present unless
> >
> > diff --git a/src/providers/ldap/sdap_access.h b/src/providers/ldap/sdap_access.h
> > index f085e619961198b887d65ed5ee0bc5cdd90d1b20..3e59b9b2f4851a87c01f3e353c11b3bdf9bb4ff7 100644
> > --- a/src/providers/ldap/sdap_access.h
> > +++ b/src/providers/ldap/sdap_access.h
> > @@ -35,6 +35,7 @@
> > #define SYSDB_LDAP_ACCESS_CACHED_LOCKOUT "ldap_access_lockout_allow"
> > /* names of ppolicy attributes */
> > #define SYSDB_LDAP_ACCESS_LOCKED_TIME "pwdAccountLockedTime"
> > +#define SYSDB_LDAP_ACESS_LOCKOUT_DURATION "pwdLockoutDuration"
> > #define SYSDB_LDAP_ACCESS_LOCKOUT "pwdLockout"
> > #define LDAP_ACCESS_FILTER_NAME "filter"
> >
> > 2.1.0
>
> Patch works as expexted.

Thank you for testing, I only read the patches, didn't do any testing
yet.
> Tested with 3 different values of pwdAccountLockedTime
>
> dn: uid=testuser1,ou=Users,dc=example,dc=com
> changetype: modify
> replace: pwdAccountLockedTime
> pwdAccountLockedTime: 000001010000Z
>
> dn: uid=testuser1,ou=Users,dc=example,dc=com
> changetype: modify
> replace: pwdAccountLockedTime
> pwdAccountLockedTime: 20150310003750Z
>
> dn: uid=testuser1,ou=Users,dc=example,dc=com
> changetype: modify
> replace: pwdAccountLockedTime
> pwdAccountLockedTime: 20150301003750Z
>
> LS

[Pavel Reichl]

On 03/03/2015 03:28 PM, Jakub Hrozek wrote:
> On Mon, Mar 02, 2015 at 11:42:06PM +0100, Lukas Slebodnik wrote:
> > On (22/01/15 17:06), Pavel Reichl wrote:
> > > Second patch now contains unit test.
> > > From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by admin
> > >
> > > This is a follow up to #2364.
> > > To distinguish user locked out from accessing machine via SSH if an
> > > account was administratively locked (pwdAccountLockedTime set to
> > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > password is locked out from natural reasons (too many attempts, expired
> > > password).
> > >
> > > Part of solution for:
> > > https://fedorahosted.org/sssd/ticket/2534
> >
> > You wrote in description of this ticket that it it is a follow up to #2364.
> > from my point of view it is an extension.
> >
> > Do we really need an option to enable this feature?
> >
> > The current implementation locks account with pwdAccountLockedTime set to
> > 000001010000Z. The other values(dates) are ignored.
>
> I would prefer if we didn't add a new option as well, but since we released
> a version that only supported the lockout and not any other semantics,
> I don't think we can get away with just changing the functionality. A
> minor version can break functionality. But a major version can :-)
>
> So I propose the following:
> Add a new value for ldap_access_order called "ppolicy" that would
>
> evaluate the pwdAccountLockedTime fully, including the new
> functionality in this patchset
> 2) In 1.12, deprecate the "lockout" option and log a warning that it
> will be removed in future relase and users should migrate to "ppolicy"
> option
> 3) In master (1.13), remove the "lockout" ldap_access_order value
>
> That way, we won't add a new option and I think the "ppolicy" option is
> better anyway.
> > > From 942f9ab711ac787f10bcc18bd8b638e6f8a540b9 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > Subject: [PATCH 2/3] UTIL: convert GeneralizedTime to unix time
> > >
> > > New utility function *convert_time* to convert GeneralizedTime to
> > > unix time.
>
> According to the slapo-ppolicy man page, the pwdAccountLockedTime is
> just generalizedTime and according to RFC 4517, it's possible to use any
> timezone, but the UTC zone is preferred:
> The time value represents coordinated universal time (equivalent to Greenwich
> Mean Time) if the "Z" form of <g-time-zone> is used; otherwise, the value
> represents a local time in the time zone indicated by <g-differential>.
> In the latter case, coordinated universal time can be calculated by
> subtracting the differential from the local time.  The "Z" form of
> <g-time-zone> SHOULD be used in preference to <g-differential>.
>
> I would suggest that the utility function would only support the "Z"
> form and return a special error code if another timezone is specified.
> In that case, the sssd access code would return ACCESS_DENIED and log a
> failure saying that the timezone specifier in ppolicy is not supported.
>
> The reason is that when it comes to access control, I strongly prefer
> using non-ambiguous formats. We'll see if there are any users out there
> who actually use a non-Zulu time in ppolicy and can improve the support
> later.
>
> Of course, the name of the function must make it clear that only time
> specification with a trailing "Z" is supported, also the man page should
> make it clear in description of "ppolicy" ldap_access_order that only "Z"
> is supported.
> > > Makefile.am            |  3 ++-
> > > src/tests/util-tests.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > > src/util/util.c        | 39 +++++++++++++++++++++++++++++++++++++++
> > > src/util/util.h        |  3 +++
> > > 4 files changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index ca8921bb132db6b7a57d9436a3219057d08b64c6..2da86572752d0702b8983f4cc647188c8cc81382 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -1535,7 +1535,8 @@ simple_access_tests_LDADD = \
> > >      libsss_test_common.la
> > >
> > > util_tests_SOURCES = \
> > > src/tests/util-tests.c
> > > src/tests/util-tests.c \
> > > src/util/util.c
> >
> > It's better to link test with internal library (libsss_util.so) rather
> > then add source file to the test. It's not necessary to change it now.
> > Please try to ammend cmocka test in future. (test_utils)
> >
> > It's not necessary to change it now. It's just info for the future.
> >
> > BTW. You didn't add $(NULL) to the end of aotomake variable.
> > > util_tests_CFLAGS = \
> > >      $(AM_CFLAGS) \
> > >      $(CHECK_CFLAGS)
> > > diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
> > > index 94015d8e1e0efb143a4fea998f1b16db1e63365e..ebfae703ff7962c12b9eddf83d2c2e61db29b6eb 100644
> > > --- a/src/tests/util-tests.c
> > > +++ b/src/tests/util-tests.c
> > > @@ -28,6 +28,8 @@
> > > #include <sys/types.h>
> > > #include <sys/stat.h>
> > > #include <fcntl.h>
> > > +#include <stdlib.h>
> > >
> > > #include "util/util.h"
> > > #include "util/sss_utf8.h"
> > > #include "util/murmurhash3.h"
> > > @@ -1020,6 +1022,44 @@ START_TEST(test_known_service)
> > > }
> > > END_TEST
> > >
> > > +static void convert_time_tz(void)
> >
> > Why do wee need a static wunction which does not accept any argument?
> > It's better to inline it or add argument timezone and change timezone(setenv)
> > inside function.
> > > +{
> > > errno_t ret;
> > > time_t unix_time;
> > > ret = convert_time("20140801115742Z", "%Y%m%d%H%M%SZ", &unix_time);
> > > fail_unless(ret == EOK && difftime(1406894262, unix_time) == 0);
> > >
> > > +}
> > >
> > > +START_TEST(test_convert_time)
> > > +{
> > > const char *format = "%Y%m%d%H%M%SZ";
> > > time_t unix_time;
> > > errno_t ret;
> > > const char *orig_tz;
> > > ret = convert_time("0", format, &unix_time);
> > > fail_unless(ret == EINVAL);
> > > ret = convert_time("000001010000Z", format, &unix_time);
> > > fail_unless(ret == EINVAL);
> > > /* test that results are still same no matter what timezone is set */
> > > convert_time_tz();
> > > orig_tz = getenv("TZ");
> > > ret = setenv("TZ", "GST-1", 1);
> > > fail_if(ret == -1);
> > > convert_time_tz();
> > > ret = setenv("TZ", "GST-2", 1);
> > > fail_if(ret == -1);
> > > convert_time_tz();
> > > ret = setenv("TZ", orig_tz, 1);
> > > fail_if(ret == -1);
> > >
> > > +}
> > > +END_TEST
> > >
> > > Suite *util_suite(void)
> > > {
> > >      Suite *s = suite_create("util");
> > > @@ -1067,10 +1107,17 @@ Suite *util_suite(void)
> > >      tcase_add_test(tc_atomicio, test_atomicio_read_exact_sized_file);
> > >      tcase_add_test(tc_atomicio, test_atomicio_read_from_empty_file);
> > > TCase *tc_convert_time = tcase_create("convert_time");
> > > tcase_add_checked_fixture (tc_convert_time,
> >                             ^
> >                             extra space
> >
> > >                           ck_leak_check_setup,
> > >                           ck_leak_check_teardown);
> > >
> > > tcase_add_test(tc_convert_time, test_convert_time);
> > >
> > > suite_add_tcase (s, tc_util);
> > >    suite_add_tcase (s, tc_utf8);
> > >    suite_add_tcase (s, tc_mh3);
> > >    suite_add_tcase (s, tc_atomicio);
> > >
> > > suite_add_tcase (s, tc_convert_time);
> > >
> > > return s;
> > >
> > > }
> > > diff --git a/src/util/util.c b/src/util/util.c
> > > index 613c559bb2002686c7833642d0946e46e5a9b5d6..eb3002fd4dde5b88a6595aee7186bae8d7d773d2 100644
> > > --- a/src/util/util.c
> > > +++ b/src/util/util.c
> > > @@ -18,6 +18,7 @@
> > >      along with this program.  If not, see http://www.gnu.org/licenses/.
> > > */
> > >
> > > +#include "config.h"
> > > #include <ctype.h>
> > > #include <netdb.h>
> > > #include <poll.h>
> > > @@ -26,6 +27,7 @@
> > > #include <arpa/inet.h>
> > > #include <talloc.h>
> > > #include <dhash.h>
> > > +#include <time.h>
> > >
> > > #include "util/util.h"
> > > #include "util/sss_utf8.h"
> > > @@ -904,3 +906,40 @@ errno_t sss_fd_nonblocking(int fd)
> > >  return EOK;
> > >
> > > }
> > >
> > > +/* Convert GeneralizedTime (http://en.wikipedia.org/wiki/GeneralizedTime)
> > > to unix time (seconds since epoch). Use UTC time zone.
> > > */
> > >
> > > +errno_t convert_time(const char *str, const char *format, time_t *unix_time)
> > > +{
> > > char *end;
> > > struct tm tm;
> > > memset(&tm, 0, sizeof(tm));
> > > end = strptime(str, format, &tm);
> > > /* not all characters from format were matched */
> > > if (end == NULL) {
> > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > >          "String [%s] failed to match format [%s].\n", str, format);
> > >    return EINVAL;
> > > }
> > > /* str is 'longer' than format */
> > > if (*end != '\0') {
> > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > >          "String [%s] is longer than format [%s].\n", str, format);
> > >    return EINVAL;
> > > }
> > > *unix_time = mktime(&tm);
> > > if (*unix_time == -1) {
> > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > >          "mktime failed to convert [%s].\n", str);
> > >    return EINVAL;
> > > }
> > > tzset();
> > > *unix_time -= timezone;
> >
> > We prefer to touch output argument just once. (just set output argumetn with
> >    local variable.
> >
> > The 1st benefit is that function is not poluted with dereferencing of pointer
> >    (code is nicer). The second benefit is that you set default vale just once.
> >    It is not case in this function.
> > > return EOK;
> > >
> > > +}
> > > diff --git a/src/util/util.h b/src/util/util.h
> > > index 60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be..bc23d7bc5d6307f63a480c5faaeaa98ca921bd46 100644
> > > --- a/src/util/util.h
> > > +++ b/src/util/util.h
> > > @@ -641,4 +641,7 @@ int set_seuser(const char *login_name, const char *seuser_name,
> > >                 const char *mlsrange);
> > > int del_seuser(const char *login_name);
> > >
> > > +/* convert time from generalized form to unix time */
> > > +errno_t convert_time(const char *str, const char *format, time_t *unix_time);
> >
> > Please try to prefix this function. I know it's part of internal library,
> >   but the name is very general and in theory can conflic with function in other
> >   libraries/ (prefix sss_ or s3_ is fine)
>
> Prefix and rename, please :-)
>
> errno_t sss_utc_to_time_t() ?
>
> btw do we need to have this function in src/util? Is it ever going to be
> used anywhere else?
> > The output argumet shout be prefixed with underscore (_).
> > So it is obviout which pointer is input and which is output argument.
> > > #endif /* __SSSD_UTIL_H__ */
> > >
> > > 2.1.0
> > >
> > > From 9bb77a8cb8501d7827da8457d434dc31321aca50 Mon Sep 17 00:00:00 2001
> > > From: Pavel Reichl 
> > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally expires
> > >
> > > Resolves:
> > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > Makefile.am                      |   3 +-
> > > src/providers/ldap/sdap_access.c | 106 ++++++++++++++++++++++++++++++++++-----
> > > src/providers/ldap/sdap_access.h |   1 +
> > > 3 files changed, 96 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 2da86572752d0702b8983f4cc647188c8cc81382..e780b7098cc853dfaaa4ae4d830cc410fa94eb80 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -2383,7 +2383,8 @@ libsss_ldap_common_la_SOURCES = \
> > >      src/providers/ldap/sdap_domain.c \
> > >      src/providers/ldap/sdap.c \
> > >      src/util/user_info_msg.c \
> > > src/util/sss_ldap.c
> > > src/util/sss_ldap.c \
> > > src/util/util.c
> >
> > The file src/util/util.c is part of internal library libsss_util.so
> > It's better to link this module with library rather then add it to source file.
> > > libsss_ldap_common_la_CFLAGS = \
> > >      $(KRB5_CFLAGS)
> > > libsss_ldap_common_la_LIBADD = \
> > > diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> > > index e09e6b8fe55e8e8bae9ff7f46e815595cb938971..39a6f4397f83b2ca560d0fb883002a97caf73c74 100644
> > > --- a/src/providers/ldap/sdap_access.c
> > > +++ b/src/providers/ldap/sdap_access.c
> > > @@ -1568,7 +1568,9 @@ errno_t sdap_access_lock_step(struct tevent_req *req)
> > >      errno_t ret;
> > >      struct tevent_req *subreq;
> > >      struct sdap_access_lock_req_ctx *state;
> > > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME, NULL };
> > >
> > > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
> > >                        SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> > >                        NULL };
> > >
> > > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> > >
> > > @@ -1597,15 +1599,86 @@ done:
> > >      return ret;
> > > }
> > >
> > > +static errno_t
> > > +decide_if_account_is_locked(bool locked_only_by_admin,
> > >                        const char *pwdAccountLockedTime,
> > >                        const char *pwdAccountLockedDurationTime,
> > >                        bool *locked)
> >                            output argumet should be prefixed with "_"
>
> Also is_account_locked() would be a shorter name :-)
> > > +{
> > > errno_t ret;
> > > time_t lock_time;
> > > time_t duration;
> > > time_t now;
> > > /* Default action is to consider account to be locked. */
> > > *locked = true;
> >
> > It's more readable if code does not touch output argument very often.
> > Functions is nicer if stars "*" are not used very often.
> > > /* account is permanently locked */
> > > if (strcasecmp(pwdAccountLockedTime,
> > >               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> > >    goto done;
> >
> > return variable ret is not initialized.
> >
> > @see warnigs
> > Error: UNINIT (CWE-457): [#def1]
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1620: var_decl: Declaring variable "ret" without initializer.
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1680: uninit_use: Using uninitialized value "ret".
> >
> > Error: CLANG_WARNING: [#def2]
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:5: warning: Undefined or garbage value returned to caller
> > #    return ret;
> > #    ^~~~~~~~~~
> >
> > Error: COMPILER_WARNING: [#def4]
> > sssd-1.12.90/src/providers/ldap/sdap_access.c: scope_hint: In function 'sdap_access_lock_step_done'
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1758:16: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > #             if (ret != EOK) {
> > #
> > > }
> > > if (locked_only_by_admin) {
> > >    /* We do *not* care about exact value of account locked time, we
> > >     * only *do* care if the value is equal to
> > >     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> > >     * permanently.
> > >     */
> > >    DEBUG(SSSDBG_TRACE_FUNC,
> > >          "Account is beeing blocked by password policy, "
> > >          "but value: [%s] value is ignored by SSSD.\n",
> > >          pwdAccountLockedTime);
> > >    *locked = false;
> > > } else {
> > >    /* Account may be locked out from natural reasons (too many attempts,
> > >     * expired password). In this case, pwdAccountLockedTime is also set,
> > >     * to the time of lock out.
> > >     */
> > >    ret = convert_time(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
> > >                       &lock_time);
> > >    if (ret != EOK) {
> > >        DEBUG(SSSDBG_TRACE_FUNC, "convert_time() failed with %d:%s.\n",
> > >              ret, sss_strerror(ret));
> > >        goto done;
> > >    }
> > >    now = time(NULL);
> > >    /* Account was NOT locked in past. */
> > >    if (difftime(lock_time, now) > 0.0) {
> > >        *locked = false;
> > >    } else if (pwdAccountLockedDurationTime != NULL) {
> > >        errno = 0;
> > >        duration = strtol(pwdAccountLockedDurationTime, NULL, 0);
>
> I think we already have a utility function for this in
> src/util/strtonum.c, please use it.
> > >        if (errno) {
> > >            ret = errno;
> > >            goto done;
> > >        }
> > >        /* Lockout has expired */
> > >        if (duration != 0 && difftime(now, lock_time) > duration) {
> > >            *locked = false;
> > >        }
> > >    }
> > > }
> > > ret = EOK;
> > >
> > > +done:
> > > return ret;
> > >
> > > +}
> > >
> > > static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > > {
> > >      int ret, tret, dp_error;
> > >      size_t num_results;
> > >      bool locked = false;
> > >      const char *pwdAccountLockedTime;
> > >
> > > const char *pwdAccountLockedDurationTime;
> > > struct sysdb_attrs **results;
> > > struct tevent_req *req;
> > > struct sdap_access_lock_req_ctx *state;
> > >
> > > bool pwd_admin_locked_only;
> > >
> > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> > >
> > > @@ -1613,6 +1686,9 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > >      ret = sdap_get_generic_recv(subreq, state, &num_results, &results);
> > >      talloc_zfree(subreq);
> > > pwd_admin_locked_only = dp_opt_get_bool(state->opts->basic,
> > >                                        SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY);
> >
> > I have already commented it in 1st patch that IMHO we do not need this option
> > at all.
> >
> > If other developers agree we need that option then please move it to the place
> > where we DO NEED it. Because it is used just once but initialized far far away
> > from place where it is used.
> >
> > There is another warning from static analyser.
> > I think iit's false possitive because we needn't test output variable of
> > dp_opt_get_bool.
> >
> > Error: CHECKED_RETURN (CWE-252): [#def3]
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1701: check_return: Calling "_dp_opt_get_bool" without checking return value (as is done elsewhere 48 out of 67 times).
> > sssd-1.12.90/src/providers/ad/ad_common.c:1170: example_checked: Example 1: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
> > sssd-1.12.90/src/providers/ad/ad_dyndns.c:48: example_checked: Example 2: "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, <anonymous>) == 0".
> > sssd-1.12.90/src/providers/ad/ad_id.c:230: example_checked: Example 3: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, <anonymous>)".
> > sssd-1.12.90/src/providers/ad/ad_init.c:235: example_checked: Example 4: "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, <anonymous>)".
> > sssd-1.12.90/src/providers/ipa/ipa_init.c:332: example_checked: Example 5: "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)" has its value checked in "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, <anonymous>)".
> > sssd-1.12.90/src/providers/ldap/sdap_access.c:1754: unchecked_value: Using value "pwd_admin_locked_only" as a function argument without checking appropriately.
> > >  ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> > >  if (ret != EOK) {
> > >      if (dp_error == DP_ERR_OK) {
> > >
> > > @@ -1653,22 +1729,26 @@ static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > >          ret = ERR_INTERNAL;
> > >          goto done;
> > >      } else { /* Ok, we got a single reply */
> > >    ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> > >                                 &pwdAccountLockedDurationTime);
> > >    if (ret != EOK) {
> > >        /* This attribute might not be set even if account is locked */
> > >        pwdAccountLockedDurationTime = NULL;
> > >    }
> > >    ret = sysdb_attrs_get_string(results[0], SYSDB_LDAP_ACCESS_LOCKED_TIME,
> > >                                 &pwdAccountLockedTime);
> > >    if (ret == EOK) {
> > >        /* We do *not* care about exact value of account locked time, we
> > >         * only *do* care if the value is equal to
> > >         * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> > >         * permanently.
> > >         */
> > >        if (strcasecmp(pwdAccountLockedTime,
> > >                       PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> > >        ret = decide_if_account_is_locked(pwd_admin_locked_only,
> > >                                          pwdAccountLockedTime,
> > >                                          pwdAccountLockedDurationTime,
> > >                                          &locked);
> > >        if (ret != EOK) {
> > >            DEBUG(SSSDBG_MINOR_FAILURE,
> > >                  "decide_if_account_is_locked failed: %d:[%s]. "
> > >                  "Account will be considered to be locked.\n",
> > >                  ret, sss_strerror(ret));
> > >            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);
> > >        }
> > >    } else {
> > >        /* Attribute SYSDB_LDAP_ACCESS_LOCKED_TIME in not be present unless
> > >
> > > diff --git a/src/providers/ldap/sdap_access.h b/src/providers/ldap/sdap_access.h
> > > index f085e619961198b887d65ed5ee0bc5cdd90d1b20..3e59b9b2f4851a87c01f3e353c11b3bdf9bb4ff7 100644
> > > --- a/src/providers/ldap/sdap_access.h
> > > +++ b/src/providers/ldap/sdap_access.h
> > > @@ -35,6 +35,7 @@
> > > #define SYSDB_LDAP_ACCESS_CACHED_LOCKOUT "ldap_access_lockout_allow"
> > > /* names of ppolicy attributes */
> > > #define SYSDB_LDAP_ACCESS_LOCKED_TIME "pwdAccountLockedTime"
> > > +#define SYSDB_LDAP_ACESS_LOCKOUT_DURATION "pwdLockoutDuration"
> > > #define SYSDB_LDAP_ACCESS_LOCKOUT "pwdLockout"
> > > #define LDAP_ACCESS_FILTER_NAME "filter"
> > >
> > > 2.1.0
> >
> > Patch works as expexted.
>
> Thank you for testing, I only read the patches, didn't do any testing
> yet.
> > Tested with 3 different values of pwdAccountLockedTime
> >
> > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > changetype: modify
> > replace: pwdAccountLockedTime
> > pwdAccountLockedTime: 000001010000Z
> >
> > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > changetype: modify
> > replace: pwdAccountLockedTime
> > pwdAccountLockedTime: 20150310003750Z
> >
> > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > changetype: modify
> > replace: pwdAccountLockedTime
> > pwdAccountLockedTime: 20150301003750Z
> >
> > LS
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Thanks for comments, please see updated patchset.

[Pavel Reichl]

On 03/03/2015 09:57 PM, Pavel Reichl wrote:
> On 03/03/2015 03:28 PM, Jakub Hrozek wrote:
> > On Mon, Mar 02, 2015 at 11:42:06PM +0100, Lukas Slebodnik wrote:
> > > On (22/01/15 17:06), Pavel Reichl wrote:
> > > > Second patch now contains unit test.
> > > > From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Mon, 19 Jan 2015 03:24:09 -0500
> > > > Subject: [PATCH 1/3] SDAP: new option - pwdlocking natural/only by 
> > > > admin
> > > >
> > > > This is a follow up to #2364.
> > > > To distinguish user locked out from accessing machine via SSH if an
> > > > account was administratively locked (pwdAccountLockedTime set to
> > > > 000001010000Z) in the OpenLDAP Password Policy overlay or if user
> > > > password is locked out from natural reasons (too many attempts, 
> > > > expired
> > > > password).
> > > >
> > > > Part of solution for:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > >
> > > You wrote in description of this ticket that it it is a follow up to 
> > > #2364.
> > > from my point of view it is an extension.
> > >
> > > Do we really need an option to enable this feature?
> > >
> > > The current implementation locks account with pwdAccountLockedTime 
> > > set to
> > > 000001010000Z. The other values(dates) are ignored.
> >
> > I would prefer if we didn't add a new option as well, but since we 
> > released
> > a version that only supported the lockout and not any other semantics,
> > I don't think we can get away with just changing the functionality. A
> > minor version can break functionality. But a major version can :-)
> >
> > So I propose the following:
> > Add a new value for ldap_access_order called "ppolicy" that would
> >
> > evaluate the pwdAccountLockedTime fully, including the new
> > functionality in this patchset
> > 2) In 1.12, deprecate the "lockout" option and log a warning that it
> > will be removed in future relase and users should migrate to "ppolicy"
> > option
> > 3) In master (1.13), remove the "lockout" ldap_access_order value
> >
> > That way, we won't add a new option and I think the "ppolicy" option is
> > better anyway.
> > > > From 942f9ab711ac787f10bcc18bd8b638e6f8a540b9 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > > > Subject: [PATCH 2/3] UTIL: convert GeneralizedTime to unix time
> > > >
> > > > New utility function *convert_time* to convert GeneralizedTime to
> > > > unix time.
> >
> > According to the slapo-ppolicy man page, the pwdAccountLockedTime is
> > just generalizedTime and according to RFC 4517, it's possible to use any
> > timezone, but the UTC zone is preferred:
> > The time value represents coordinated universal time (equivalent to 
> > Greenwich
> > Mean Time) if the "Z" form of <g-time-zone> is used; otherwise, the 
> > value
> > represents a local time in the time zone indicated by <g-differential>.
> > In the latter case, coordinated universal time can be calculated by
> > subtracting the differential from the local time.  The "Z" form of
> > <g-time-zone> SHOULD be used in preference to <g-differential>.
> >
> > I would suggest that the utility function would only support the "Z"
> > form and return a special error code if another timezone is specified.
> > In that case, the sssd access code would return ACCESS_DENIED and log a
> > failure saying that the timezone specifier in ppolicy is not supported.
> >
> > The reason is that when it comes to access control, I strongly prefer
> > using non-ambiguous formats. We'll see if there are any users out there
> > who actually use a non-Zulu time in ppolicy and can improve the support
> > later.
> >
> > Of course, the name of the function must make it clear that only time
> > specification with a trailing "Z" is supported, also the man page should
> > make it clear in description of "ppolicy" ldap_access_order that only 
> > "Z"
> > is supported.
> > > > Makefile.am            |  3 ++-
> > > > src/tests/util-tests.c | 47 
> > > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > > src/util/util.c        | 39 +++++++++++++++++++++++++++++++++++++++
> > > > src/util/util.h        |  3 +++
> > > > 4 files changed, 91 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 
> > > > ca8921bb132db6b7a57d9436a3219057d08b64c6..2da86572752d0702b8983f4cc647188c8cc81382 
> > > > 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -1535,7 +1535,8 @@ simple_access_tests_LDADD = \
> > > >      libsss_test_common.la
> > > >
> > > > util_tests_SOURCES = \
> > > > src/tests/util-tests.c
> > > > src/tests/util-tests.c \
> > > > src/util/util.c
> > >
> > > It's better to link test with internal library (libsss_util.so) rather
> > > then add source file to the test. It's not necessary to change it now.
> > > Please try to ammend cmocka test in future. (test_utils)
> > >
> > > It's not necessary to change it now. It's just info for the future.
> > >
> > > BTW. You didn't add $(NULL) to the end of aotomake variable.
> > > > util_tests_CFLAGS = \
> > > >      $(AM_CFLAGS) \
> > > >      $(CHECK_CFLAGS)
> > > > diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
> > > > index 
> > > > 94015d8e1e0efb143a4fea998f1b16db1e63365e..ebfae703ff7962c12b9eddf83d2c2e61db29b6eb 
> > > > 100644
> > > > --- a/src/tests/util-tests.c
> > > > +++ b/src/tests/util-tests.c
> > > > @@ -28,6 +28,8 @@
> > > > #include <sys/types.h>
> > > > #include <sys/stat.h>
> > > > #include <fcntl.h>
> > > > +#include <stdlib.h>
> > > >
> > > > #include "util/util.h"
> > > > #include "util/sss_utf8.h"
> > > > #include "util/murmurhash3.h"
> > > > @@ -1020,6 +1022,44 @@ START_TEST(test_known_service)
> > > > }
> > > > END_TEST
> > > >
> > > > +static void convert_time_tz(void)
> > >
> > > Why do wee need a static wunction which does not accept any argument?
> > > It's better to inline it or add argument timezone and change 
> > > timezone(setenv)
> > > inside function.
> > > > +{
> > > > errno_t ret;
> > > > time_t unix_time;
> > > > ret = convert_time("20140801115742Z", "%Y%m%d%H%M%SZ",
> > > >
> > > > &unix_time);
> > > > fail_unless(ret == EOK && difftime(1406894262, unix_time) == 0);
> > > >
> > > > +}
> > > >
> > > > +START_TEST(test_convert_time)
> > > > +{
> > > > const char *format = "%Y%m%d%H%M%SZ";
> > > > time_t unix_time;
> > > > errno_t ret;
> > > > const char *orig_tz;
> > > > ret = convert_time("0", format, &unix_time);
> > > > fail_unless(ret == EINVAL);
> > > > ret = convert_time("000001010000Z", format, &unix_time);
> > > > fail_unless(ret == EINVAL);
> > > > /* test that results are still same no matter what timezone is
> > > >
> > > > set */
> > > > convert_time_tz();
> > > > orig_tz = getenv("TZ");
> > > > ret = setenv("TZ", "GST-1", 1);
> > > > fail_if(ret == -1);
> > > > convert_time_tz();
> > > > ret = setenv("TZ", "GST-2", 1);
> > > > fail_if(ret == -1);
> > > > convert_time_tz();
> > > > ret = setenv("TZ", orig_tz, 1);
> > > > fail_if(ret == -1);
> > > >
> > > > +}
> > > > +END_TEST
> > > >
> > > > Suite *util_suite(void)
> > > > {
> > > >      Suite *s = suite_create("util");
> > > > @@ -1067,10 +1107,17 @@ Suite *util_suite(void)
> > > >      tcase_add_test(tc_atomicio, test_atomicio_read_exact_sized_file);
> > > >      tcase_add_test(tc_atomicio, test_atomicio_read_from_empty_file);
> > > > TCase *tc_convert_time = tcase_create("convert_time");
> > > > tcase_add_checked_fixture (tc_convert_time,
> > >                             ^
> > >                             extra space
> > >
> > > > ck_leak_check_setup,
> > > >                           ck_leak_check_teardown);
> > > >
> > > > tcase_add_test(tc_convert_time, test_convert_time);
> > > >
> > > > suite_add_tcase (s, tc_util);
> > > >    suite_add_tcase (s, tc_utf8);
> > > >    suite_add_tcase (s, tc_mh3);
> > > >    suite_add_tcase (s, tc_atomicio);
> > > >
> > > > suite_add_tcase (s, tc_convert_time);
> > > >
> > > > return s;
> > > >
> > > > }
> > > > diff --git a/src/util/util.c b/src/util/util.c
> > > > index 
> > > > 613c559bb2002686c7833642d0946e46e5a9b5d6..eb3002fd4dde5b88a6595aee7186bae8d7d773d2 
> > > > 100644
> > > > --- a/src/util/util.c
> > > > +++ b/src/util/util.c
> > > > @@ -18,6 +18,7 @@
> > > >      along with this program.  If not, see 
> > > > http://www.gnu.org/licenses/.
> > > > */
> > > >
> > > > +#include "config.h"
> > > > #include <ctype.h>
> > > > #include <netdb.h>
> > > > #include <poll.h>
> > > > @@ -26,6 +27,7 @@
> > > > #include <arpa/inet.h>
> > > > #include <talloc.h>
> > > > #include <dhash.h>
> > > > +#include <time.h>
> > > >
> > > > #include "util/util.h"
> > > > #include "util/sss_utf8.h"
> > > > @@ -904,3 +906,40 @@ errno_t sss_fd_nonblocking(int fd)
> > > >  return EOK;
> > > >
> > > > }
> > > >
> > > > +/* Convert GeneralizedTime 
> > > > (http://en.wikipedia.org/wiki/GeneralizedTime)
> > > > to unix time (seconds since epoch). Use UTC time zone.
> > > > */
> > > >
> > > > +errno_t convert_time(const char *str, const char *format, time_t 
> > > > *unix_time)
> > > > +{
> > > > char *end;
> > > > struct tm tm;
> > > > memset(&tm, 0, sizeof(tm));
> > > > end = strptime(str, format, &tm);
> > > > /* not all characters from format were matched */
> > > > if (end == NULL) {
> > > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > > >          "String [%s] failed to match format [%s].\n", str, 
> > > >
> > > > format);
> > > >    return EINVAL;
> > > > }
> > > > /* str is 'longer' than format */
> > > > if (*end != '\0') {
> > > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > > >          "String [%s] is longer than format [%s].\n", str, 
> > > >
> > > > format);
> > > >    return EINVAL;
> > > > }
> > > > *unix_time = mktime(&tm);
> > > > if (*unix_time == -1) {
> > > >    DEBUG(SSSDBG_TRACE_INTERNAL,
> > > >          "mktime failed to convert [%s].\n", str);
> > > >    return EINVAL;
> > > > }
> > > > tzset();
> > > > *unix_time -= timezone;
> > >
> > > We prefer to touch output argument just once. (just set output 
> > > argumetn with
> > >    local variable.
> > >
> > > The 1st benefit is that function is not poluted with 
> > > dereferencing of pointer
> > >    (code is nicer). The second benefit is that you set default vale 
> > > just once.
> > >    It is not case in this function.
> > > > return EOK;
> > > >
> > > > +}
> > > > diff --git a/src/util/util.h b/src/util/util.h
> > > > index 
> > > > 60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be..bc23d7bc5d6307f63a480c5faaeaa98ca921bd46 
> > > > 100644
> > > > --- a/src/util/util.h
> > > > +++ b/src/util/util.h
> > > > @@ -641,4 +641,7 @@ int set_seuser(const char *login_name, const 
> > > > char *seuser_name,
> > > >                 const char *mlsrange);
> > > > int del_seuser(const char *login_name);
> > > >
> > > > +/* convert time from generalized form to unix time */
> > > > +errno_t convert_time(const char *str, const char *format, time_t 
> > > > *unix_time);
> > >
> > > Please try to prefix this function. I know it's part of internal 
> > > library,
> > >   but the name is very general and in theory can conflic with 
> > > function in other
> > >   libraries/ (prefix sss_ or s3_ is fine)
> >
> > Prefix and rename, please :-)
> >
> > errno_t sss_utc_to_time_t() ?
> >
> > btw do we need to have this function in src/util? Is it ever going to be
> > used anywhere else?
> > > The output argumet shout be prefixed with underscore (_).
> > > So it is obviout which pointer is input and which is output argument.
> > > > #endif /* __SSSD_UTIL_H__ */
> > > >
> > > > 2.1.0
> > > >
> > > > From 9bb77a8cb8501d7827da8457d434dc31321aca50 Mon Sep 17 00:00:00 2001
> > > > From: Pavel Reichl 
> > > > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > > > Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account naturally 
> > > > expires
> > > >
> > > > Resolves:
> > > > https://fedorahosted.org/sssd/ticket/2534
> > > >
> > > > Makefile.am                      |   3 +-
> > > > src/providers/ldap/sdap_access.c | 106 
> > > > ++++++++++++++++++++++++++++++++++-----
> > > > src/providers/ldap/sdap_access.h |   1 +
> > > > 3 files changed, 96 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 
> > > > 2da86572752d0702b8983f4cc647188c8cc81382..e780b7098cc853dfaaa4ae4d830cc410fa94eb80 
> > > > 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -2383,7 +2383,8 @@ libsss_ldap_common_la_SOURCES = \
> > > >      src/providers/ldap/sdap_domain.c \
> > > >      src/providers/ldap/sdap.c \
> > > >      src/util/user_info_msg.c \
> > > > src/util/sss_ldap.c
> > > > src/util/sss_ldap.c \
> > > > src/util/util.c
> > >
> > > The file src/util/util.c is part of internal library libsss_util.so
> > > It's better to link this module with library rather then add it to 
> > > source file.
> > > > libsss_ldap_common_la_CFLAGS = \
> > > >      $(KRB5_CFLAGS)
> > > > libsss_ldap_common_la_LIBADD = \
> > > > diff --git a/src/providers/ldap/sdap_access.c 
> > > > b/src/providers/ldap/sdap_access.c
> > > > index 
> > > > e09e6b8fe55e8e8bae9ff7f46e815595cb938971..39a6f4397f83b2ca560d0fb883002a97caf73c74 
> > > > 100644
> > > > --- a/src/providers/ldap/sdap_access.c
> > > > +++ b/src/providers/ldap/sdap_access.c
> > > > @@ -1568,7 +1568,9 @@ errno_t sdap_access_lock_step(struct 
> > > > tevent_req *req)
> > > >      errno_t ret;
> > > >      struct tevent_req *subreq;
> > > >      struct sdap_access_lock_req_ctx *state;
> > > > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME, NULL };
> > > >
> > > > const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
> > > >
> > > > SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> > > >                        NULL };
> > > >
> > > > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> > > >
> > > > @@ -1597,15 +1599,86 @@ done:
> > > >      return ret;
> > > > }
> > > >
> > > > +static errno_t
> > > > +decide_if_account_is_locked(bool locked_only_by_admin,
> > > >                        const char *pwdAccountLockedTime,
> > > >                        const char *pwdAccountLockedDurationTime,
> > > >                        bool *locked)
> > >                            output argumet should be prefixed 
> > >
> > > with "_"
> >
> > Also is_account_locked() would be a shorter name :-)
> > > > +{
> > > > errno_t ret;
> > > > time_t lock_time;
> > > > time_t duration;
> > > > time_t now;
> > > > /* Default action is to consider account to be locked. */
> > > > *locked = true;
> > >
> > > It's more readable if code does not touch output argument very often.
> > > Functions is nicer if stars "*" are not used very often.
> > > > /* account is permanently locked */
> > > > if (strcasecmp(pwdAccountLockedTime,
> > > >               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> > > >    goto done;
> > >
> > > return variable ret is not initialized.
> > >
> > > @see warnigs
> > > Error: UNINIT (CWE-457): [#def1]
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1620: var_decl: 
> > > Declaring variable "ret" without initializer.
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1680: uninit_use: 
> > > Using uninitialized value "ret".
> > >
> > > Error: CLANG_WARNING: [#def2]
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:5: warning: 
> > > Undefined or garbage value returned to caller
> > > #    return ret;
> > > #    ^~~~~~~~~~
> > >
> > > Error: COMPILER_WARNING: [#def4]
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c: scope_hint: In 
> > > function 'sdap_access_lock_step_done'
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1758:16: warning: 
> > > 'ret' may be used uninitialized in this function 
> > > [-Wmaybe-uninitialized]
> > > #             if (ret != EOK) {
> > > #
> > > > }
> > > > if (locked_only_by_admin) {
> > > >    /* We do *not* care about exact value of account locked 
> > > >
> > > > time, we
> > > >     * only *do* care if the value is equal to
> > > >     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is 
> > > >
> > > > locked
> > > >     * permanently.
> > > >     */
> > > >    DEBUG(SSSDBG_TRACE_FUNC,
> > > >          "Account is beeing blocked by password policy, "
> > > >          "but value: [%s] value is ignored by SSSD.\n",
> > > >          pwdAccountLockedTime);
> > > >    *locked = false;
> > > > } else {
> > > >    /* Account may be locked out from natural reasons (too 
> > > >
> > > > many attempts,
> > > >     * expired password). In this case, pwdAccountLockedTime 
> > > >
> > > > is also set,
> > > >     * to the time of lock out.
> > > >     */
> > > >    ret = convert_time(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
> > > >                       &lock_time);
> > > >    if (ret != EOK) {
> > > >        DEBUG(SSSDBG_TRACE_FUNC, "convert_time() failed with 
> > > >
> > > > %d:%s.\n",
> > > >              ret, sss_strerror(ret));
> > > >        goto done;
> > > >    }
> > > >    now = time(NULL);
> > > >    /* Account was NOT locked in past. */
> > > >    if (difftime(lock_time, now) > 0.0) {
> > > >        *locked = false;
> > > >    } else if (pwdAccountLockedDurationTime != NULL) {
> > > >        errno = 0;
> > > >        duration = strtol(pwdAccountLockedDurationTime, NULL, 0);
> >
> > I think we already have a utility function for this in
> > src/util/strtonum.c, please use it.
> > > >        if (errno) {
> > > >            ret = errno;
> > > >            goto done;
> > > >        }
> > > >        /* Lockout has expired */
> > > >        if (duration != 0 && difftime(now, lock_time) > 
> > > >
> > > > duration) {
> > > >            *locked = false;
> > > >        }
> > > >    }
> > > > }
> > > > ret = EOK;
> > > >
> > > > +done:
> > > > return ret;
> > > >
> > > > +}
> > > >
> > > > static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > > > {
> > > >      int ret, tret, dp_error;
> > > >      size_t num_results;
> > > >      bool locked = false;
> > > >      const char *pwdAccountLockedTime;
> > > >
> > > > const char *pwdAccountLockedDurationTime;
> > > > struct sysdb_attrs **results;
> > > > struct tevent_req *req;
> > > > struct sdap_access_lock_req_ctx *state;
> > > >
> > > > bool pwd_admin_locked_only;
> > > >
> > > > req = tevent_req_callback_data(subreq, struct tevent_req);
> > > > state = tevent_req_data(req, struct sdap_access_lock_req_ctx);
> > > >
> > > > @@ -1613,6 +1686,9 @@ static void sdap_access_lock_step_done(struct 
> > > > tevent_req *subreq)
> > > >      ret = sdap_get_generic_recv(subreq, state, &num_results, 
> > > > &results);
> > > >      talloc_zfree(subreq);
> > > > pwd_admin_locked_only = dp_opt_get_bool(state->opts->basic,
> > > > SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY);
> > >
> > > I have already commented it in 1st patch that IMHO we do not need 
> > > this option
> > > at all.
> > >
> > > If other developers agree we need that option then please move it to 
> > > the place
> > > where we DO NEED it. Because it is used just once but initialized 
> > > far far away
> > > from place where it is used.
> > >
> > > There is another warning from static analyser.
> > > I think iit's false possitive because we needn't test output 
> > > variable of
> > > dp_opt_get_bool.
> > >
> > > Error: CHECKED_RETURN (CWE-252): [#def3]
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1701: check_return: 
> > > Calling "_dp_opt_get_bool" without checking return value (as is done 
> > > elsewhere 48 out of 67 times).
> > > sssd-1.12.90/src/providers/ad/ad_common.c:1170: example_checked: 
> > > Example 1: "_dp_opt_get_bool(ad_ctx->ad_options->basic, 
> > > AD_ENABLE_GC, <anonymous>)" has its value checked in 
> > > "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, 
> > > <anonymous>)".
> > > sssd-1.12.90/src/providers/ad/ad_dyndns.c:48: example_checked: 
> > > Example 2: "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, 
> > > DP_OPT_DYNDNS_UPDATE, <anonymous>)" has its value checked in 
> > > "_dp_opt_get_bool(ad_opts->dyndns_ctx->opts, DP_OPT_DYNDNS_UPDATE, 
> > > <anonymous>) == 0".
> > > sssd-1.12.90/src/providers/ad/ad_id.c:230: example_checked: Example 
> > > 3: "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, 
> > > <anonymous>)" has its value checked in 
> > > "_dp_opt_get_bool(ad_ctx->ad_options->basic, AD_ENABLE_GC, 
> > > <anonymous>)".
> > > sssd-1.12.90/src/providers/ad/ad_init.c:235: example_checked: 
> > > Example 4: "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, 
> > > <anonymous>)" has its value checked in 
> > > "_dp_opt_get_bool(ad_options->basic, AD_ENABLE_DNS_SITES, 
> > > <anonymous>)".
> > > sssd-1.12.90/src/providers/ipa/ipa_init.c:332: example_checked: 
> > > Example 5: "_dp_opt_get_bool(ipa_options->basic, 
> > > IPA_ENABLE_DNS_SITES, <anonymous>)" has its value checked in 
> > > "_dp_opt_get_bool(ipa_options->basic, IPA_ENABLE_DNS_SITES, 
> > > <anonymous>)".
> > > sssd-1.12.90/src/providers/ldap/sdap_access.c:1754: unchecked_value: 
> > > Using value "pwd_admin_locked_only" as a function argument without 
> > > checking appropriately.
> > > >  ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> > > >  if (ret != EOK) {
> > > >      if (dp_error == DP_ERR_OK) {
> > > >
> > > > @@ -1653,22 +1729,26 @@ static void 
> > > > sdap_access_lock_step_done(struct tevent_req *subreq)
> > > >          ret = ERR_INTERNAL;
> > > >          goto done;
> > > >      } else { /* Ok, we got a single reply */
> > > >    ret = sysdb_attrs_get_string(results[0], 
> > > >
> > > > SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
> > > > &pwdAccountLockedDurationTime);
> > > >    if (ret != EOK) {
> > > >        /* This attribute might not be set even if account is 
> > > >
> > > > locked */
> > > >        pwdAccountLockedDurationTime = NULL;
> > > >    }
> > > >    ret = sysdb_attrs_get_string(results[0], 
> > > >
> > > > SYSDB_LDAP_ACCESS_LOCKED_TIME,
> > > > &pwdAccountLockedTime);
> > > >          if (ret == EOK) {
> > > >        /* We do *not* care about exact value of account 
> > > >
> > > > locked time, we
> > > >         * only *do* care if the value is equal to
> > > >         * PERMANENTLY_LOCKED_ACCOUNT, which means that 
> > > >
> > > > account is locked
> > > >         * permanently.
> > > >         */
> > > >        if (strcasecmp(pwdAccountLockedTime,
> > > >                       PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> > > >        ret = decide_if_account_is_locked(pwd_admin_locked_only,
> > > > pwdAccountLockedTime,
> > > > pwdAccountLockedDurationTime,
> > > >                                          &locked);
> > > >        if (ret != EOK) {
> > > >            DEBUG(SSSDBG_MINOR_FAILURE,
> > > >                  "decide_if_account_is_locked failed: %d:[%s]. "
> > > >                  "Account will be considered to be locked.\n",
> > > >                  ret, sss_strerror(ret));
> > > >            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);
> > > >        }
> > > >    } else {
> > > >        /* Attribute SYSDB_LDAP_ACCESS_LOCKED_TIME in not be 
> > > >
> > > > present unless
> > > > diff --git a/src/providers/ldap/sdap_access.h 
> > > > b/src/providers/ldap/sdap_access.h
> > > > index 
> > > > f085e619961198b887d65ed5ee0bc5cdd90d1b20..3e59b9b2f4851a87c01f3e353c11b3bdf9bb4ff7 
> > > > 100644
> > > > --- a/src/providers/ldap/sdap_access.h
> > > > +++ b/src/providers/ldap/sdap_access.h
> > > > @@ -35,6 +35,7 @@
> > > > #define SYSDB_LDAP_ACCESS_CACHED_LOCKOUT "ldap_access_lockout_allow"
> > > > /* names of ppolicy attributes */
> > > > #define SYSDB_LDAP_ACCESS_LOCKED_TIME "pwdAccountLockedTime"
> > > > +#define SYSDB_LDAP_ACESS_LOCKOUT_DURATION "pwdLockoutDuration"
> > > > #define SYSDB_LDAP_ACCESS_LOCKOUT "pwdLockout"
> > > > #define LDAP_ACCESS_FILTER_NAME "filter"
> > > >
> > > > 2.1.0
> > >
> > > Patch works as expexted.
> >
> > Thank you for testing, I only read the patches, didn't do any testing
> > yet.
> > > Tested with 3 different values of pwdAccountLockedTime
> > >
> > > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > > changetype: modify
> > > replace: pwdAccountLockedTime
> > > pwdAccountLockedTime: 000001010000Z
> > >
> > > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > > changetype: modify
> > > replace: pwdAccountLockedTime
> > > pwdAccountLockedTime: 20150310003750Z
> > >
> > > dn: uid=testuser1,ou=Users,dc=example,dc=com
> > > changetype: modify
> > > replace: pwdAccountLockedTime
> > > pwdAccountLockedTime: 20150301003750Z
> > >
> > > LS
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Thanks for comments, please see updated patchset.
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Patches needed to be rebased.

ci passed:

http://sssd-ci.duckdns.org/logs/job/8/74/summary.html

[Jakub Hrozek]

On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> Patches needed to be rebased.
>
> ci passed:
>
> http://sssd-ci.duckdns.org/logs/job/8/74/summary.html
> From f2801324e2d184e1bf7092f22e8509ffc78a5bfc Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time
>
> New utility function *sss_utc_to_time_t* to convert GeneralizedTime to
> unix time.

Almost ack, see the attached patch for suggestions. It's mostly renaming
the new error code (sorry, the name was my fault, I hope sending the
correcting patch makes that up)

I also moved changing the TZ environment variable into the test function
to avoid side-effects.

Can you also (in another patch) use sss_utc_to_time_t on other
places in the code where pretty much the same code is used? I
guess check_pwexpire_kerberos(), nds_check_expired() and maybe even
sysdb_sudo_convert_time() but I'm not sure about the last one.
> From 5be740031c33ea76aa2c79fd859591ab347ab521 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2534

Here I also have a small fix for the Makefile and man page attached. But
more importantly, we shouldn't copy the whole lockout request, but
rename it to ppolicy and add an enum:

enum spap_pwpolicy_mode {
    PWP_LOCKOUT_ONLY,
    PWP_LOCKOUT_EXPIRE,
    PWP_SENTINEL,
};

to the _send function. If the value would be >= than SENTINEL, error
out. Then just modify the is_account_locked() function.

The request is too big to be duplicated, sorry. We also duplicated some
bugs, see inline.
> +static struct tevent_req *
> +sdap_access_ppolicy_send(TALLOC_CTX *mem_ctx,
>                  struct tevent_context *ev,
>                  struct be_ctx *be_ctx,
>                  struct sss_domain_info *domain,
>                  struct sdap_access_ctx *access_ctx,
>                  struct sdap_id_conn_ctx *conn,
>                  const char *username,
>                  struct ldb_message *user_entry)
>
> +{
> struct sdap_access_ppolicy_req_ctx *state;
> struct tevent_req *req;
> errno_t ret;
> req = tevent_req_create(mem_ctx,
>                        &state, struct sdap_access_ppolicy_req_ctx);
> if (req == NULL) {
>    return NULL;
> }
> state->filter = NULL;
> state->username = username;
> state->opts = access_ctx->id_ctx->opts;
> state->conn = conn;
> state->ev = ev;
> state->access_ctx = access_ctx;
> state->domain = domain;
> state->ppolicy_dns_index = 0;
> DEBUG(SSSDBG_TRACE_FUNC,
>      "Performing access ppolicy check for user [%s]\n", username);
> state->cached_access = ldb_msg_find_attr_as_bool(
>    user_entry, SYSDB_LDAP_ACCESS_CACHED_LOCKOUT, false);
> /* Ok, we have one result, check if we are online or offline */
> if (be_is_offline(be_ctx)) {
>    /* Ok, we're offline. Return from the cache */
>    ret = sdap_access_decide_offline(state->cached_access);
>    goto done;
> }
> ret = sdap_get_basedn_user_entry(user_entry, state->username,
>                                 &state->basedn);
> if (ret != EOK) {
>    goto done;
> }
> DEBUG(SSSDBG_TRACE_FUNC, "Checking ppolicy against LDAP\n");
> state->sdap_op = sdap_id_op_create(state,
>                                   state->conn->conn_cache);
> if (!state->sdap_op) {
>    DEBUG(SSSDBG_OP_FAILURE, "sdap_id_op_create failed\n");
>    ret = ENOMEM;
>    goto done;
> }
> ret = sdap_access_ppolicy_retry(req);
> if (ret != EOK) {
>    goto done;
> }
> return req;
>
> +done:
> if (ret == EOK) {
>    tevent_req_done(req);
> } else {
>    tevent_req_error(req, ret);
> }
> tevent_req_post(req, ev);
> return req;
>
> +}
>
> +static int sdap_access_ppolicy_retry(struct tevent_req *req)
> +{
> struct sdap_access_ppolicy_req_ctx *state;
> struct tevent_req *subreq;
> int ret;
> state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> subreq = sdap_id_op_connect_send(state->sdap_op, state, &ret);
> if (!subreq) {
>    DEBUG(SSSDBG_OP_FAILURE,
>          "sdap_id_op_connect_send failed: %d (%s)\n", ret, strerror(ret));
>    return ret;
> }
> tevent_req_set_callback(subreq, sdap_access_ppolicy_connect_done, req);
> return EOK;
>
> +}
>
> +static void sdap_access_ppolicy_connect_done(struct tevent_req *subreq)
> +{
> struct tevent_req *req;
> struct sdap_access_ppolicy_req_ctx *state;
> int ret, dp_error;
> const char *ppolicy_dn;
> req = tevent_req_callback_data(subreq, struct tevent_req);
> state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> ret = sdap_id_op_connect_recv(subreq, &dp_error);
> talloc_zfree(subreq);
> if (ret != EOK) {
>    if (dp_error == DP_ERR_OFFLINE) {
>        ret = sdap_access_decide_offline(state->cached_access);
>        if (ret == EOK) {
>            tevent_req_done(req);
>            return;
>        }
>    }
>    tevent_req_error(req, ret);
>    return;
> }
> ppolicy_dn = dp_opt_get_string(state->opts->basic,
>                               SDAP_PWDLOCKOUT_DN);
> /* option was configured */
> if (ppolicy_dn != NULL) {
>    state->ppolicy_dns = talloc_array(state, const char*, 2);
>    if (state->ppolicy_dns == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "Could not allocate ppolicy_dns.\n");
>        tevent_req_error(req, ERR_ACCESS_DENIED);

Why access denied and not internal error? Wouldn't the user be tricked
into thinking everything worked fine but he was denied access?
>        return;
>    }
>    state->ppolicy_dns[0] = ppolicy_dn;
>    state->ppolicy_dns[1] = NULL;
> } else {
>    /* try to determine default value */
>    DEBUG(SSSDBG_CONF_SETTINGS,
>          "ldap_pwdlockout_dn was not defined in configuration file.\n");
>    state->ppolicy_dns = get_default_ppolicy_dns(state, state->opts->sdom);
>    if (state->ppolicy_dns == NULL) {
>        tevent_req_error(req, ERR_ACCESS_DENIED);

Same comment about access denied.
>        return;
>    }
> }
> /* Connection to LDAP succeeded
> * Send 'pwdLockout' request
> */
> ret = sdap_access_ppolicy_get_lockout_step(req);
> if (ret != EOK && ret != EAGAIN) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "sdap_access_ppolicy_get_lockout_step failed: [%d][%s]\n",
>          ret, strerror(ret));
>    tevent_req_error(req, ERR_ACCESS_DENIED);

Same comment about access denied.
>    return;
> }

You don't handle EOK here, so the request would never finish.
> +}
>
> +static errno_t
> +sdap_access_ppolicy_get_lockout_step(struct tevent_req *req)
> +{
> const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKOUT, NULL };
> struct sdap_access_ppolicy_req_ctx *state;
> struct tevent_req *subreq;
> errno_t ret;
> state = tevent_req_data(req, struct sdap_access_ppolicy_req_ctx);
> /* no more DNs to try */
> if (state->ppolicy_dns[state->ppolicy_dns_index] == NULL) {

Please add a debug message here.
>    ret = EOK;
>    goto done;
> }
> DEBUG(SSSDBG_CONF_SETTINGS,
>      "Trying to find out if ppolicy is enabled using the DN: %s\n",
>      state->ppolicy_dns[state->ppolicy_dns_index]);
> subreq = sdap_get_generic_send(state,
>                               state->ev,
>                               state->opts,
>                               sdap_id_op_handle(state->sdap_op),
>                               state->ppolicy_dns[state->ppolicy_dns_index],
>                               LDAP_SCOPE_BASE,
>                               NULL, attrs,
>                               NULL, 0,
>                               dp_opt_get_int(state->opts->basic,
>                                              SDAP_SEARCH_TIMEOUT),
>                               false);
> if (subreq == NULL) {
>    DEBUG(SSSDBG_CRIT_FAILURE, "Could not start LDAP communication\n");
>    tevent_req_error(req, EIO);

The request is marked as faile by the caller in other cases, which would
result in a double-free.
>    ret = EIO;
>    goto done;
> }
> /* try next basedn */
> state->ppolicy_dns_index++;
> tevent_req_set_callback(subreq, sdap_access_ppolicy_get_lockout_done, req);
> ret = EAGAIN;
>
> +done:
> return ret;
>
> +}

[...]
> +static errno_t
> +is_account_locked(const char *pwdAccountLockedTime,
>              const char *pwdAccountLockedDurationTime,
>              bool *_locked)
>
> +{
> errno_t ret;
> time_t lock_time;
> time_t duration;
> time_t now;
> bool locked;
> /* Default action is to consider account to be locked. */
> locked = true;
> /* account is permanently locked */
> if (strcasecmp(pwdAccountLockedTime,
>               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>    ret = EOK;
>    goto done;
> }

Could we add code like:
    if (lockout_only) {
        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);
        ret = EOK;
        locked = false;
        goto done;
    }
> /* Account may be locked out from natural reasons (too many attempts,
> * expired password). In this case, pwdAccountLockedTime is also set,
> * to the time of lock out.
> */
> ret = sss_utc_to_time_t(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
>                   &lock_time);

Strange indent

[Jakub Hrozek]

On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> Patches needed to be rebased.

here are the fixups I mentioned in the other mail

[Pavel Reichl]

On 03/04/2015 04:37 PM, Jakub Hrozek wrote:
> On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> > Patches needed to be rebased.
>
> here are the fixups I mentioned in the other mail

Thanks, new patch set attached.

[Jakub Hrozek]

On Wed, Mar 04, 2015 at 09:45:19PM +0100, Pavel Reichl wrote:
> On 03/04/2015 04:37 PM, Jakub Hrozek wrote:
> > On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> > > Patches needed to be rebased.
> >
> > here are the fixups I mentioned in the other mail
>
> Thanks, new patch set attached.

Thank you for the patience.

Almost ack :-) The code now reads good to me, thanks for removing the
duplication. I couldn't figure out what's up with the tests, my brain is
too tired, we might need to ask nick what could be the issue.
> From a8826633a944e92b7248e446fd37c349c1c72292 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time

ack sans the test failure in CI. We need to solve it before pushing, but
the patch looks good to me.
> From 4b3a7b2e196c6c19639b30e6be3fe7cb57ec7b7b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires

Mostly good, I only have one place where we might not have
understood each other on IRC and one request.

If you are going to respin the patch once again, can you chance
strerror() for sss_strerror()? It's not a big deal since we'd see the
error value either way, it would just make the error output nicer since
we're using custom codes.
> @@ -1462,28 +1493,30 @@ static void sdap_access_lock_connect_done(struct tevent_req *subreq)
>      /* Connection to LDAP succeeded
>       * Send 'pwdLockout' request
>       */
> ret = sdap_access_lock_get_lockout_step(req);
> ret = sdap_access_ppolicy_get_lockout_step(req);
> if (ret != EOK && ret != EAGAIN) {
>     DEBUG(SSSDBG_CRIT_FAILURE,
>          "sdap_access_lock_get_lockout_step failed: [%d][%s]\n",
>          "sdap_access_ppolicy_get_lockout_step failed: [%d][%s]\n",
>          ret, strerror(ret));
>    tevent_req_error(req, ERR_ACCESS_DENIED);
>    tevent_req_error(req, ERR_INTERNAL);
>    return;
> }

Here is the place where we might have not understand one another. The
code continues with either EOK or EAGAIN. I think it's correct to
continue with EAGAIN, but do you see issues with marking the request as
done if the function returns EOK? I think EOK should actually never
happen here, but we should handle it nonetheless, because the way I read
the code, the request would never finish otherwise.

btw -- this might be a good place to start using custom return codes
also in future patches. The code that iterates over search bases and
returns EAGAIN on search or EOK when it's done is quite common and I
wonder if using ERR_SEARCH_IN_PROGRESS and ERR_NO_MORE_SEARCH_BASES
would make the callers more readable. Just a thought.
> -static void sdap_access_lock_step_done(struct tevent_req *subreq)
> +static errno_t
> +is_account_locked(const char *pwdAccountLockedTime,
>              const char *pwdAccountLockedDurationTime,
>              enum sdap_pwpolicy_mode pwpol_mode,
>              const char *username,
>              bool *_locked)
>
> +{
> errno_t ret;
> time_t lock_time;
> time_t duration;
> time_t now;
> bool locked;
> /* Default action is to consider account to be locked. */
> locked = true;
> /* account is permanently locked */
> if (strcasecmp(pwdAccountLockedTime,
>               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
>    ret = EOK;
>    goto done;
> }
> if (pwpol_mode == PWP_LOCKOUT_ONLY) {
>    /* We do *not* care about exact value of account locked time, we
>     * only *do* care if the value is equal to
>     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
>     * permanently.
>     */
>    DEBUG(SSSDBG_TRACE_FUNC,
>          "Account of: %s is beeing blocked by password policy, "
>          "but value: [%s] value is ignored by SSSD.\n",
>          username, pwdAccountLockedTime);
>    locked = false;
> } else {
>    /* Account may be locked out from natural reasons (too many attempts,
>     * expired password). In this case, pwdAccountLockedTime is also set,
>     * to the time of lock out.
>     */
>    ret = sss_utc_to_time_t(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
>                            &lock_time);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_TRACE_FUNC, "sss_utc_to_time_t failed with %d:%s.\n",
>              ret, sss_strerror(ret));
>        goto done;
>    }
>    now = time(NULL);
>    /* Account was NOT locked in past. */
>    if (difftime(lock_time, now) > 0.0) {
>        locked = false;
>    } else if (pwdAccountLockedDurationTime != NULL) {
>        errno = 0;
>        duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);
>        if (errno) {
>            ret = errno;
>            goto done;
>        }
>        /* Lockout has expired */
>        if (duration != 0 && difftime(now, lock_time) > duration) {
>            locked = false;
>        }
>    }
> }

I would prefer a switch-case here, for one reason - we would need to
have all values of the enum and we would either get a compilation
warning if all values are not handled or an error if we use a default
handled. I prefer the first option, but IIRC Sumit prefers the second
and I'm not too sold on either of them.

Thanks again for the patience. I know there's been several respins, but
this is access control code and should be reviewed really carefully.

[Pavel Reichl]

On 03/04/2015 10:38 PM, Jakub Hrozek wrote:
> On Wed, Mar 04, 2015 at 09:45:19PM +0100, Pavel Reichl wrote:
> > On 03/04/2015 04:37 PM, Jakub Hrozek wrote:
> > > On Wed, Mar 04, 2015 at 12:02:03PM +0100, Pavel Reichl wrote:
> > > > Patches needed to be rebased.
> > >
> > > here are the fixups I mentioned in the other mail
> >
> > Thanks, new patch set attached.
>
> Thank you for the patience.

Thanks, for yours. Patch with duplication was no good.
> Almost ack :-) The code now reads good to me, thanks for removing the
> duplication. I couldn't figure out what's up with the tests, my brain is
> too tired, we might need to ask nick what could be the issue.

hopefully fixed now:
ci:

http://sssd-ci.duckdns.org/logs/job/8/86/summary.html
> > From a8826633a944e92b7248e446fd37c349c1c72292 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time
>
> ack sans the test failure in CI. We need to solve it before pushing, but
> the patch looks good to me.
> > From 4b3a7b2e196c6c19639b30e6be3fe7cb57ec7b7b Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires
>
> Mostly good, I only have one place where we might not have
> understood each other on IRC and one request.
>
> If you are going to respin the patch once again, can you chance
> strerror() for sss_strerror()? It's not a big deal since we'd see the
> error value either way, it would just make the error output nicer since
> we're using custom codes.

done
> > @@ -1462,28 +1493,30 @@ static void sdap_access_lock_connect_done(struct tevent_req *subreq)
> >       /* Connection to LDAP succeeded
> >        * Send 'pwdLockout' request
> >        */
> > ret = sdap_access_lock_get_lockout_step(req);
> > ret = sdap_access_ppolicy_get_lockout_step(req);
> >  if (ret != EOK && ret != EAGAIN) {
> >      DEBUG(SSSDBG_CRIT_FAILURE,
> >          "sdap_access_lock_get_lockout_step failed: [%d][%s]\n",
> >          "sdap_access_ppolicy_get_lockout_step failed: [%d][%s]\n",
> >           ret, strerror(ret));
> >    tevent_req_error(req, ERR_ACCESS_DENIED);
> >    tevent_req_error(req, ERR_INTERNAL);
> >     return;
> > }
>
> Here is the place where we might have not understand one another. The
> code continues with either EOK or EAGAIN. I think it's correct to
> continue with EAGAIN, but do you see issues with marking the request as
> done if the function returns EOK? I think EOK should actually never
> happen here, but we should handle it nonetheless, because the way I read
> the code, the request would never finish otherwise.

done
> btw -- this might be a good place to start using custom return codes
> also in future patches. The code that iterates over search bases and
> returns EAGAIN on search or EOK when it's done is quite common and I
> wonder if using ERR_SEARCH_IN_PROGRESS and ERR_NO_MORE_SEARCH_BASES
> would make the callers more readable. Just a thought.

nice but later
> > -static void sdap_access_lock_step_done(struct tevent_req *subreq)
> > +static errno_t
> > +is_account_locked(const char *pwdAccountLockedTime,
> >              const char *pwdAccountLockedDurationTime,
> >              enum sdap_pwpolicy_mode pwpol_mode,
> >              const char *username,
> >              bool *_locked)
> >
> > +{
> > errno_t ret;
> > time_t lock_time;
> > time_t duration;
> > time_t now;
> > bool locked;
> > /* Default action is to consider account to be locked. */
> > locked = true;
> > /* account is permanently locked */
> > if (strcasecmp(pwdAccountLockedTime,
> >               PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> >    ret = EOK;
> >    goto done;
> > }
> > if (pwpol_mode == PWP_LOCKOUT_ONLY) {
> >    /* We do *not* care about exact value of account locked time, we
> >     * only *do* care if the value is equal to
> >     * PERMANENTLY_LOCKED_ACCOUNT, which means that account is locked
> >     * permanently.
> >     */
> >    DEBUG(SSSDBG_TRACE_FUNC,
> >          "Account of: %s is beeing blocked by password policy, "
> >          "but value: [%s] value is ignored by SSSD.\n",
> >          username, pwdAccountLockedTime);
> >    locked = false;
> > } else {
> >    /* Account may be locked out from natural reasons (too many attempts,
> >     * expired password). In this case, pwdAccountLockedTime is also set,
> >     * to the time of lock out.
> >     */
> >    ret = sss_utc_to_time_t(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
> >                            &lock_time);
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_TRACE_FUNC, "sss_utc_to_time_t failed with %d:%s.\n",
> >              ret, sss_strerror(ret));
> >        goto done;
> >    }
> >    now = time(NULL);
> >    /* Account was NOT locked in past. */
> >    if (difftime(lock_time, now) > 0.0) {
> >        locked = false;
> >    } else if (pwdAccountLockedDurationTime != NULL) {
> >        errno = 0;
> >        duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);
> >        if (errno) {
> >            ret = errno;
> >            goto done;
> >        }
> >        /* Lockout has expired */
> >        if (duration != 0 && difftime(now, lock_time) > duration) {
> >            locked = false;
> >        }
> >    }
> > }
>
> I would prefer a switch-case here, for one reason - we would need to
> have all values of the enum and we would either get a compilation
> warning if all values are not handled or an error if we use a default
> handled. I prefer the first option, but IIRC Sumit prefers the second
> and I'm not too sold on either of them.

done, but difficult to parse - please check
> Thanks again for the patience. I know there's been several respins, but
> this is access control code and should be reviewed really carefully.

OK, I agree, I just hate to do this things in time press, but I know 
it's my mistake.
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On 03/05/2015 02:15 PM, Pavel Reichl wrote:
> case PWP_LOCKOUT_EXPIRE:
>    /* Account may be locked out from natural reasons (too many attempts,
>     * expired password). In this case, pwdAccountLockedTime is also set,
>     * to the time of lock out.
>     */
>    ret = sss_utc_to_time_t(pwdAccountLockedTime, "%Y%m%d%H%M%SZ",
>                            &lock_time);
>    if (ret != EOK) {
>        DEBUG(SSSDBG_TRACE_FUNC, "sss_utc_to_time_t failed with %d:%s.\n",
>              ret, sss_strerror(ret));
>        goto done;
>    }
>    now = time(NULL);
>    /* Account was NOT locked in past. */
>    if (difftime(lock_time, now) > 0.0) {
>        locked = false;
>    } else if (pwdAccountLockedDurationTime != NULL) {
>        errno = 0;
>        duration = strtouint32(pwdAccountLockedDurationTime, NULL, 0);
>        if (errno) {
>            ret = errno;
>            goto done;
>        }
>        /* Lockout has expired */
>        if (duration != 0 && difftime(now, lock_time) > duration) {
>            locked = false;
>        }
>    }
>    break;
> case PWP_SENTINEL:
> default:
>    DEBUG(SSSDBG_MINOR_FAILURE,
>          "Unexpected value of password policy mode: %d.\n", pwpol_mode);

I guess it would be better to set 'ret = EINVAL;' here
> }
> ret = EOK

SSSD GPO Inheritance

[Dan Lavu]

I found something very awkward about this feature. It took me a little bit of time to figure out what it is but finally I'm able to reproduce it. The policies cannot overlap, it will prioritize one policy and ignore the others. For example;

When the site GPO th e only policy applied, domain users (allow_dm_u) are not allowed to login, which is working fine. 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0)

Note, that user 'allow_u' is *not* denied in the domain policy and he is denied when both GPOs are applied. 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0)

So inheritance is working but the domain policy is overriding the site policy entirely. The windows gpo behavior is Explicit Deny > Implicit Grant > Explicit Grant > Implicit Deny, and the user allow_u is not being denied anywhere so should still be permitted to login.

Is this the desired design? or should I file a bug?

Dan

SSSD GPO Inheritance

[Dan Lavu]

Spoke to Stephen today about this and it's working as design.

----- Original Message -----

From: "Dan Lavu"  
To: "Development of the System Security Services Daemon"  
Sent: Thursday, March 5, 2015 7:46:56 PM 
Subject: SSSD GPO Inheritance

I found something very awkward about this feature. It took me a little bit of time to figure out what it is but finally I'm able to reproduce it. The policies cannot overlap, it will prioritize one policy and ignore the others. For example;

When the site GPO th e only policy applied, domain users (allow_dm_u) are not allowed to login, which is working fine. 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0)

Note, that user 'allow_u' is *not* denied in the domain policy and he is denied when both GPOs are applied. 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ FAIL ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 1) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_success '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0) 
:: [ PASS ] :: Command 'su_fail '[e-mail]' Secret123' (Expected 0, got 0)

So inheritance is working but the domain policy is overriding the site policy entirely. The windows gpo behavior is Explicit Deny > Implicit Grant > Explicit Grant > Implicit Deny, and the user allow_u is not being denied anywhere so should still be permitted to login.

Is this the desired design? or should I file a bug?

Dan

[Jakub Hrozek]

On Thu, Mar 05, 2015 at 02:34:15PM +0100, Pavel Reichl wrote:
> > case PWP_SENTINEL:
> > default:
> >    DEBUG(SSSDBG_MINOR_FAILURE,
> >          "Unexpected value of password policy mode: %d.\n", pwpol_mode);
>
> I guess it would be better to set 'ret = EINVAL;' here

Oops, yes. Can you send an additional patch for master along with the
sssd-1-12 rebase?

[Pavel Reichl]

On 03/06/2015 10:24 AM, Jakub Hrozek wrote:
> On Thu, Mar 05, 2015 at 02:34:15PM +0100, Pavel Reichl wrote:
> > > case PWP_SENTINEL:
> > > default:
> > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > >          "Unexpected value of password policy mode: %d.\n", pwpol_mode);
> >
> > I guess it would be better to set 'ret = EINVAL;' here
>
> Oops, yes. Can you send an additional patch for master along with the
> sssd-1-12 rebase?
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Sure, I'll do it now.

BTW the list delay is starting to be a real annoyance.

[Pavel Reichl]

On 03/06/2015 10:27 AM, Pavel Reichl wrote:
> On 03/06/2015 10:24 AM, Jakub Hrozek wrote:
> > On Thu, Mar 05, 2015 at 02:34:15PM +0100, Pavel Reichl wrote:
> > > > case PWP_SENTINEL:
> > > > default:
> > > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > > >          "Unexpected value of password policy mode: %d.\n", 
> > > >
> > > > pwpol_mode);
> > >
> > > I guess it would be better to set 'ret = EINVAL;' here
> >
> > Oops, yes. Can you send an additional patch for master along with the
> > sssd-1-12 rebase?
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Sure, I'll do it now.
>
> BTW the list delay is starting to be a real annoyance.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Master ci:

http://sssd-ci.duckdns.org/logs/job/8/91/summary.html

1-12 ci:

http://sssd-ci.duckdns.org/logs/job/8/92/summary.html

[Jakub Hrozek]

On Fri, Mar 06, 2015 at 12:50:28PM +0100, Pavel Reichl wrote:
> On 03/06/2015 10:27 AM, Pavel Reichl wrote:
> > On 03/06/2015 10:24 AM, Jakub Hrozek wrote:
> > > On Thu, Mar 05, 2015 at 02:34:15PM +0100, Pavel Reichl wrote:
> > > > > case PWP_SENTINEL:
> > > > > default:
> > > > >    DEBUG(SSSDBG_MINOR_FAILURE,
> > > > >          "Unexpected value of password policy mode: %d.\n",
> > > > >
> > > > > pwpol_mode);
> > > >
> > > > I guess it would be better to set 'ret = EINVAL;' here
> > >
> > > Oops, yes. Can you send an additional patch for master along with the
> > > sssd-1-12 rebase?
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Sure, I'll do it now.
> >
> > BTW the list delay is starting to be a real annoyance.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Master ci:
>
> http://sssd-ci.duckdns.org/logs/job/8/91/summary.html
>
> 1-12 ci:
>
> http://sssd-ci.duckdns.org/logs/job/8/92/summary.html
> From 36adbcf7bfcb3dd38d4288f74c4c31ff98dc3b51 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Fri, 6 Mar 2015 04:38:05 -0500
> Subject: [PATCH] SDAP: fix minor neglect in is_account_locked()

ACK for master. Pushed as 79ee5fbacd6ee4153fa59edf5b1ae55b4f020211
> From d0676bee3a8a67321e81870ff142efb5d119c014 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 1/3] UTIL: convert GeneralizedTime to unix time

ACK, I just removed util.c from Makefile.am, just like I did in master.
> From 105239ddc6cd2f5d0c50ec00d5cb05c9058dab0b Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 2/3] SDAP: Lock out ssh keys when account naturally expires
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2534

ACK, I just removed util.c from Makefile.am, just like I did in master.
> From 1f7d5182d6c6744e93ec47db6825a18da5e4d935 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Fri, 6 Mar 2015 04:38:05 -0500
> Subject: [PATCH 3/3] SDAP: fix minor neglect in is_account_locked()
>
> It would be better to return explicit error code, although access is
> still denied and error message printed.
>
> Relates:
> https://fedorahosted.org/sssd/ticket/2534

ACK

sssd-1-12 commits are:
 * 371c5f40199b6389bd3cbfd05654b2213caecfc1
 * 8ebc05498460ce28eff012649c892b248c53632f
 * 3cace03ac7a2c4ff6d3469a3d3128c79a1882e43

[Jakub Hrozek]

On Thu, Mar 05, 2015 at 02:15:15PM +0100, Pavel Reichl wrote:
> From 3d55ab9bb607ba6cae11199f9701d90aa326dcac Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 16:27:41 -0500
> Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time

ACK.
> From 10a7a667ba897fad62620b0fc981a0f87b7da956 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 20 Jan 2015 18:34:44 -0500
> Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires

Code-wise ACK, I will run some tests before pushing.

[Jakub Hrozek]

On Thu, Mar 05, 2015 at 04:41:14PM +0100, Jakub Hrozek wrote:
> On Thu, Mar 05, 2015 at 02:15:15PM +0100, Pavel Reichl wrote:
> > From 3d55ab9bb607ba6cae11199f9701d90aa326dcac Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time
>
> ACK.
> > From 10a7a667ba897fad62620b0fc981a0f87b7da956 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires
>
> Code-wise ACK, I will run some tests before pushing.

After Lukas' recommendation, I did two small changes to Makefile.am in
the patches, ran CI again:
    http://sssd-ci.duckdns.org/logs/job/8/88/summary.html

Both me and Lukas tested the functionality in various ways and everythig
works fine.

ACK
Reviewed-by: Lukáš Slebodník 
Reviewed-by: Jakub Hrozek

[Jakub Hrozek]

On Thu, Mar 05, 2015 at 04:41:14PM +0100, Jakub Hrozek wrote:
> On Thu, Mar 05, 2015 at 02:15:15PM +0100, Pavel Reichl wrote:
> > From 3d55ab9bb607ba6cae11199f9701d90aa326dcac Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 16:27:41 -0500
> > Subject: [PATCH 1/2] UTIL: convert GeneralizedTime to unix time
>
> ACK.
> > From 10a7a667ba897fad62620b0fc981a0f87b7da956 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 20 Jan 2015 18:34:44 -0500
> > Subject: [PATCH 2/2] SDAP: Lock out ssh keys when account naturally expires
>
> Code-wise ACK, I will run some tests before pushing.

(sorry to respond to another mail than the one with acks, but mailman is
slow again)

I've pushed the patches to master:
    * 13ec767e6ca3e435e119f1f07bda10eb213383f6
    * 5a5c5cdeb92f4012fc75fd717bfea06598f68f12

Can you rebase them atop sssd-1-12 as well?