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

Lukas Slebodnik lslebodn at redhat.com
Mon Mar 2 22:42:06 UTC 2015


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 <preichl at redhat.com>
>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 <preichl at redhat.com>
>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 <preichl at redhat.com>
>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



More information about the sssd-devel mailing list