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.