[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