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@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@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@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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for comments, please see updated patchset.


_______________________________________________
sssd-devel mailing list
sssd-devel@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