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

Pavel Reichl preichl at redhat.com
Wed Mar 4 11:02:03 UTC 2015


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 <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.
>> 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 <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.
>> 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 <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 "_"
>> 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
>> sssd-devel at lists.fedorahosted.org
>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> Thanks for comments, please see updated patchset.
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> 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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150304/a6ca3efe/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-UTIL-convert-GeneralizedTime-to-unix-time.patch
Type: text/x-patch
Size: 6777 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150304/a6ca3efe/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SDAP-Lock-out-ssh-keys-when-account-naturally-expire.patch
Type: text/x-patch
Size: 28105 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150304/a6ca3efe/attachment-0001.bin>


More information about the sssd-devel mailing list