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