<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
On 03/03/2015 09:57 PM, Pavel Reichl wrote:<br>
<blockquote cite="mid:54F62032.5000809@redhat.com" type="cite">
<br>
<br>
On 03/03/2015 03:28 PM, Jakub Hrozek wrote:
<br>
<blockquote type="cite">On Mon, Mar 02, 2015 at 11:42:06PM +0100,
Lukas Slebodnik wrote:
<br>
<blockquote type="cite">On (22/01/15 17:06), Pavel Reichl wrote:
<br>
<blockquote type="cite">Second patch now contains unit test.
<br>
</blockquote>
>From f8686fbb988d29309a41064a678c0daf4aa40b6b Mon Sep 17
00:00:00 2001
<br>
<blockquote type="cite">From: Pavel Reichl
<a class="moz-txt-link-rfc2396E" href="mailto:preichl@redhat.com"><preichl@redhat.com></a>
<br>
Date: Mon, 19 Jan 2015 03:24:09 -0500
<br>
Subject: [PATCH 1/3] SDAP: new option - pwdlocking
natural/only by admin
<br>
<br>
This is a follow up to #2364.
<br>
To distinguish user locked out from accessing machine via
SSH if an
<br>
account was administratively locked (pwdAccountLockedTime
set to
<br>
000001010000Z) in the OpenLDAP Password Policy overlay or if
user
<br>
password is locked out from natural reasons (too many
attempts, expired
<br>
password).
<br>
<br>
Part of solution for:
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/sssd/ticket/2534">https://fedorahosted.org/sssd/ticket/2534</a>
<br>
</blockquote>
You wrote in description of this ticket that it it is a follow
up to #2364.
<br>
from my point of view it is an extension.
<br>
<br>
Do we really need an option to enable this feature?
<br>
<br>
The current implementation locks account with
pwdAccountLockedTime set to
<br>
000001010000Z. The other values(dates) are ignored.
<br>
</blockquote>
I would prefer if we didn't add a new option as well, but since
we released
<br>
a version that only supported the lockout and not any other
semantics,
<br>
I don't think we can get away with just changing the
functionality. A
<br>
minor version can break functionality. But a major version can
:-)
<br>
<br>
So I propose the following:
<br>
1) Add a new value for ldap_access_order called "ppolicy" that
would
<br>
evaluate the pwdAccountLockedTime fully, including the new
<br>
functionality in this patchset
<br>
2) In 1.12, deprecate the "lockout" option and log a warning
that it
<br>
will be removed in future relase and users should migrate to
"ppolicy"
<br>
option
<br>
3) In master (1.13), remove the "lockout" ldap_access_order
value
<br>
<br>
That way, we won't add a new option and I think the "ppolicy"
option is
<br>
better anyway.
<br>
<br>
<blockquote type="cite">>From
942f9ab711ac787f10bcc18bd8b638e6f8a540b9 Mon Sep 17 00:00:00
2001
<br>
<blockquote type="cite">From: Pavel Reichl
<a class="moz-txt-link-rfc2396E" href="mailto:preichl@redhat.com"><preichl@redhat.com></a>
<br>
Date: Tue, 20 Jan 2015 16:27:41 -0500
<br>
Subject: [PATCH 2/3] UTIL: convert GeneralizedTime to unix
time
<br>
<br>
New utility function *convert_time* to convert
GeneralizedTime to
<br>
unix time.
<br>
</blockquote>
</blockquote>
According to the slapo-ppolicy man page, the
pwdAccountLockedTime is
<br>
just generalizedTime and according to RFC 4517, it's possible to
use any
<br>
timezone, but the UTC zone is preferred:
<br>
~~~~~~~~~~~~~~~~~~~~~
<br>
The time value represents coordinated universal time (equivalent
to Greenwich
<br>
Mean Time) if the "Z" form of <g-time-zone> is used;
otherwise, the value
<br>
represents a local time in the time zone indicated by
<g-differential>.
<br>
In the latter case, coordinated universal time can be calculated
by
<br>
subtracting the differential from the local time. The "Z" form
of
<br>
<g-time-zone> SHOULD be used in preference to
<g-differential>.
<br>
~~~~~~~~~~~~~~~~~~~~~
<br>
<br>
I would suggest that the utility function would only support the
"Z"
<br>
form and return a special error code if another timezone is
specified.
<br>
In that case, the sssd access code would return ACCESS_DENIED
and log a
<br>
failure saying that the timezone specifier in ppolicy is not
supported.
<br>
<br>
The reason is that when it comes to access control, I strongly
prefer
<br>
using non-ambiguous formats. We'll see if there are any users
out there
<br>
who actually use a non-Zulu time in ppolicy and can improve the
support
<br>
later.
<br>
<br>
Of course, the name of the function must make it clear that only
time
<br>
specification with a trailing "Z" is supported, also the man
page should
<br>
make it clear in description of "ppolicy" ldap_access_order that
only "Z"
<br>
is supported.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">---
<br>
Makefile.am | 3 ++-
<br>
src/tests/util-tests.c | 47
+++++++++++++++++++++++++++++++++++++++++++++++
<br>
src/util/util.c | 39
+++++++++++++++++++++++++++++++++++++++
<br>
src/util/util.h | 3 +++
<br>
4 files changed, 91 insertions(+), 1 deletion(-)
<br>
<br>
diff --git a/Makefile.am b/Makefile.am
<br>
index
ca8921bb132db6b7a57d9436a3219057d08b64c6..2da86572752d0702b8983f4cc647188c8cc81382
100644
<br>
--- a/Makefile.am
<br>
+++ b/Makefile.am
<br>
@@ -1535,7 +1535,8 @@ simple_access_tests_LDADD = \
<br>
libsss_test_common.la
<br>
<br>
util_tests_SOURCES = \
<br>
- src/tests/util-tests.c
<br>
+ src/tests/util-tests.c \
<br>
+ src/util/util.c
<br>
</blockquote>
It's better to link test with internal library
(libsss_util.so) rather
<br>
then add source file to the test. It's not necessary to change
it now.
<br>
Please try to ammend cmocka test in future. (test_utils)
<br>
<br>
It's not necessary to change it now. It's just info for the
future.
<br>
<br>
BTW. You didn't add $(NULL) to the end of aotomake variable.
<br>
<br>
<blockquote type="cite">util_tests_CFLAGS = \
<br>
$(AM_CFLAGS) \
<br>
$(CHECK_CFLAGS)
<br>
diff --git a/src/tests/util-tests.c b/src/tests/util-tests.c
<br>
index
94015d8e1e0efb143a4fea998f1b16db1e63365e..ebfae703ff7962c12b9eddf83d2c2e61db29b6eb
100644
<br>
--- a/src/tests/util-tests.c
<br>
+++ b/src/tests/util-tests.c
<br>
@@ -28,6 +28,8 @@
<br>
#include <sys/types.h>
<br>
#include <sys/stat.h>
<br>
#include <fcntl.h>
<br>
+#include <stdlib.h>
<br>
+
<br>
#include "util/util.h"
<br>
#include "util/sss_utf8.h"
<br>
#include "util/murmurhash3.h"
<br>
@@ -1020,6 +1022,44 @@ START_TEST(test_known_service)
<br>
}
<br>
END_TEST
<br>
<br>
+static void convert_time_tz(void)
<br>
</blockquote>
Why do wee need a static wunction which does not accept any
argument?
<br>
It's better to inline it or add argument timezone and change
timezone(setenv)
<br>
inside function.
<br>
<br>
<blockquote type="cite">+{
<br>
+ errno_t ret;
<br>
+ time_t unix_time;
<br>
+
<br>
+ ret = convert_time("20140801115742Z", "%Y%m%d%H%M%SZ",
&unix_time);
<br>
+ fail_unless(ret == EOK && difftime(1406894262,
unix_time) == 0);
<br>
+}
<br>
+
<br>
+START_TEST(test_convert_time)
<br>
+{
<br>
+ const char *format = "%Y%m%d%H%M%SZ";
<br>
+ time_t unix_time;
<br>
+ errno_t ret;
<br>
+ const char *orig_tz;
<br>
+
<br>
+ ret = convert_time("0", format, &unix_time);
<br>
+ fail_unless(ret == EINVAL);
<br>
+ ret = convert_time("000001010000Z", format,
&unix_time);
<br>
+ fail_unless(ret == EINVAL);
<br>
+
<br>
+ /* test that results are still same no matter what
timezone is set */
<br>
+ convert_time_tz();
<br>
+ orig_tz = getenv("TZ");
<br>
+
<br>
+ ret = setenv("TZ", "GST-1", 1);
<br>
+ fail_if(ret == -1);
<br>
+ convert_time_tz();
<br>
+
<br>
+ ret = setenv("TZ", "GST-2", 1);
<br>
+ fail_if(ret == -1);
<br>
+ convert_time_tz();
<br>
+
<br>
+ ret = setenv("TZ", orig_tz, 1);
<br>
+ fail_if(ret == -1);
<br>
+}
<br>
+END_TEST
<br>
+
<br>
Suite *util_suite(void)
<br>
{
<br>
Suite *s = suite_create("util");
<br>
@@ -1067,10 +1107,17 @@ Suite *util_suite(void)
<br>
tcase_add_test(tc_atomicio,
test_atomicio_read_exact_sized_file);
<br>
tcase_add_test(tc_atomicio,
test_atomicio_read_from_empty_file);
<br>
<br>
+ TCase *tc_convert_time = tcase_create("convert_time");
<br>
+ tcase_add_checked_fixture (tc_convert_time,
<br>
</blockquote>
^
<br>
extra space
<br>
<br>
<blockquote type="cite">+
ck_leak_check_setup,
<br>
+ ck_leak_check_teardown);
<br>
+ tcase_add_test(tc_convert_time, test_convert_time);
<br>
+
<br>
suite_add_tcase (s, tc_util);
<br>
suite_add_tcase (s, tc_utf8);
<br>
suite_add_tcase (s, tc_mh3);
<br>
suite_add_tcase (s, tc_atomicio);
<br>
+ suite_add_tcase (s, tc_convert_time);
<br>
<br>
return s;
<br>
}
<br>
diff --git a/src/util/util.c b/src/util/util.c
<br>
index
613c559bb2002686c7833642d0946e46e5a9b5d6..eb3002fd4dde5b88a6595aee7186bae8d7d773d2
100644
<br>
--- a/src/util/util.c
<br>
+++ b/src/util/util.c
<br>
@@ -18,6 +18,7 @@
<br>
along with this program. If not, see
<a class="moz-txt-link-rfc2396E" href="http://www.gnu.org/licenses/"><http://www.gnu.org/licenses/></a>.
<br>
*/
<br>
<br>
+#include "config.h"
<br>
#include <ctype.h>
<br>
#include <netdb.h>
<br>
#include <poll.h>
<br>
@@ -26,6 +27,7 @@
<br>
#include <arpa/inet.h>
<br>
#include <talloc.h>
<br>
#include <dhash.h>
<br>
+#include <time.h>
<br>
<br>
#include "util/util.h"
<br>
#include "util/sss_utf8.h"
<br>
@@ -904,3 +906,40 @@ errno_t sss_fd_nonblocking(int fd)
<br>
<br>
return EOK;
<br>
}
<br>
+
<br>
+/* Convert GeneralizedTime
(<a class="moz-txt-link-freetext" href="http://en.wikipedia.org/wiki/GeneralizedTime">http://en.wikipedia.org/wiki/GeneralizedTime</a>)
<br>
+ * to unix time (seconds since epoch). Use UTC time zone.
<br>
+ */
<br>
+errno_t convert_time(const char *str, const char *format,
time_t *unix_time)
<br>
+{
<br>
+ char *end;
<br>
+ struct tm tm;
<br>
+
<br>
+ memset(&tm, 0, sizeof(tm));
<br>
+
<br>
+ end = strptime(str, format, &tm);
<br>
+ /* not all characters from format were matched */
<br>
+ if (end == NULL) {
<br>
+ DEBUG(SSSDBG_TRACE_INTERNAL,
<br>
+ "String [%s] failed to match format [%s].\n",
str, format);
<br>
+ return EINVAL;
<br>
+ }
<br>
+
<br>
+ /* str is 'longer' than format */
<br>
+ if (*end != '\0') {
<br>
+ DEBUG(SSSDBG_TRACE_INTERNAL,
<br>
+ "String [%s] is longer than format [%s].\n",
str, format);
<br>
+ return EINVAL;
<br>
+ }
<br>
+
<br>
+ *unix_time = mktime(&tm);
<br>
+ if (*unix_time == -1) {
<br>
+ DEBUG(SSSDBG_TRACE_INTERNAL,
<br>
+ "mktime failed to convert [%s].\n", str);
<br>
+ return EINVAL;
<br>
+ }
<br>
+
<br>
+ tzset();
<br>
+ *unix_time -= timezone;
<br>
</blockquote>
We prefer to touch output argument just once. (just set
output argumetn with
<br>
local variable.
<br>
<br>
The 1st benefit is that function is not poluted with
dereferencing of pointer
<br>
(code is nicer). The second benefit is that you set default
vale just once.
<br>
It is not case in this function.
<br>
<br>
<br>
<blockquote type="cite">+ return EOK;
<br>
+}
<br>
diff --git a/src/util/util.h b/src/util/util.h
<br>
index
60dbf9381c5fe26e7d42d51c6f9c7df6e3ebc4be..bc23d7bc5d6307f63a480c5faaeaa98ca921bd46
100644
<br>
--- a/src/util/util.h
<br>
+++ b/src/util/util.h
<br>
@@ -641,4 +641,7 @@ int set_seuser(const char *login_name,
const char *seuser_name,
<br>
const char *mlsrange);
<br>
int del_seuser(const char *login_name);
<br>
<br>
+/* convert time from generalized form to unix time */
<br>
+errno_t convert_time(const char *str, const char *format,
time_t *unix_time);
<br>
</blockquote>
Please try to prefix this function. I know it's part of
internal library,
<br>
but the name is very general and in theory can conflic with
function in other
<br>
libraries/ (prefix sss_ or s3_ is fine)
<br>
</blockquote>
Prefix and rename, please :-)
<br>
<br>
errno_t sss_utc_to_time_t() ?
<br>
<br>
btw do we need to have this function in src/util? Is it ever
going to be
<br>
used anywhere else?
<br>
<br>
<blockquote type="cite">
<br>
The output argumet shout be prefixed with underscore (_).
<br>
So it is obviout which pointer is input and which is output
argument.
<br>
<br>
<br>
<blockquote type="cite">#endif /* __SSSD_UTIL_H__ */
<br>
-- <br>
2.1.0
<br>
<br>
</blockquote>
>From 9bb77a8cb8501d7827da8457d434dc31321aca50 Mon Sep 17
00:00:00 2001
<br>
<blockquote type="cite">From: Pavel Reichl
<a class="moz-txt-link-rfc2396E" href="mailto:preichl@redhat.com"><preichl@redhat.com></a>
<br>
Date: Tue, 20 Jan 2015 18:34:44 -0500
<br>
Subject: [PATCH 3/3] SDAP: Lock out ssh keys when account
naturally expires
<br>
<br>
Resolves:
<br>
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/sssd/ticket/2534">https://fedorahosted.org/sssd/ticket/2534</a>
<br>
---
<br>
Makefile.am | 3 +-
<br>
src/providers/ldap/sdap_access.c | 106
++++++++++++++++++++++++++++++++++-----
<br>
src/providers/ldap/sdap_access.h | 1 +
<br>
3 files changed, 96 insertions(+), 14 deletions(-)
<br>
<br>
diff --git a/Makefile.am b/Makefile.am
<br>
index
2da86572752d0702b8983f4cc647188c8cc81382..e780b7098cc853dfaaa4ae4d830cc410fa94eb80
100644
<br>
--- a/Makefile.am
<br>
+++ b/Makefile.am
<br>
@@ -2383,7 +2383,8 @@ libsss_ldap_common_la_SOURCES = \
<br>
src/providers/ldap/sdap_domain.c \
<br>
src/providers/ldap/sdap.c \
<br>
src/util/user_info_msg.c \
<br>
- src/util/sss_ldap.c
<br>
+ src/util/sss_ldap.c \
<br>
+ src/util/util.c
<br>
</blockquote>
The file src/util/util.c is part of internal library
libsss_util.so
<br>
It's better to link this module with library rather then add
it to source file.
<br>
<br>
<blockquote type="cite">libsss_ldap_common_la_CFLAGS = \
<br>
$(KRB5_CFLAGS)
<br>
libsss_ldap_common_la_LIBADD = \
<br>
diff --git a/src/providers/ldap/sdap_access.c
b/src/providers/ldap/sdap_access.c
<br>
index
e09e6b8fe55e8e8bae9ff7f46e815595cb938971..39a6f4397f83b2ca560d0fb883002a97caf73c74
100644
<br>
--- a/src/providers/ldap/sdap_access.c
<br>
+++ b/src/providers/ldap/sdap_access.c
<br>
@@ -1568,7 +1568,9 @@ errno_t sdap_access_lock_step(struct
tevent_req *req)
<br>
errno_t ret;
<br>
struct tevent_req *subreq;
<br>
struct sdap_access_lock_req_ctx *state;
<br>
- const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
NULL };
<br>
+ const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKED_TIME,
<br>
+
SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
<br>
+ NULL };
<br>
<br>
state = tevent_req_data(req, struct
sdap_access_lock_req_ctx);
<br>
<br>
@@ -1597,15 +1599,86 @@ done:
<br>
return ret;
<br>
}
<br>
<br>
+static errno_t
<br>
+decide_if_account_is_locked(bool locked_only_by_admin,
<br>
+ const char
*pwdAccountLockedTime,
<br>
+ const char
*pwdAccountLockedDurationTime,
<br>
+ bool *locked)
<br>
</blockquote>
output argumet should be
prefixed with "_"
<br>
</blockquote>
Also is_account_locked() would be a shorter name :-)
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+{
<br>
+ errno_t ret;
<br>
+ time_t lock_time;
<br>
+ time_t duration;
<br>
+ time_t now;
<br>
+
<br>
+ /* Default action is to consider account to be locked.
*/
<br>
+ *locked = true;
<br>
</blockquote>
It's more readable if code does not touch output argument very
often.
<br>
Functions is nicer if stars "*" are not used very often.
<br>
<br>
<br>
<blockquote type="cite">+
<br>
+ /* account is permanently locked */
<br>
+ if (strcasecmp(pwdAccountLockedTime,
<br>
+ PERMANENTLY_LOCKED_ACCOUNT) == 0) {
<br>
+ goto done;
<br>
</blockquote>
return variable ret is not initialized.
<br>
<br>
@see warnigs
<br>
Error: UNINIT (CWE-457): [#def1]
<br>
sssd-1.12.90/src/providers/ldap/sdap_access.c:1620: var_decl:
Declaring variable "ret" without initializer.
<br>
sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:
uninit_use: Using uninitialized value "ret".
<br>
<br>
Error: CLANG_WARNING: [#def2]
<br>
sssd-1.12.90/src/providers/ldap/sdap_access.c:1680:5: warning:
Undefined or garbage value returned to caller
<br>
# return ret;
<br>
# ^~~~~~~~~~
<br>
<br>
Error: COMPILER_WARNING: [#def4]
<br>
sssd-1.12.90/src/providers/ldap/sdap_access.c: scope_hint: In
function 'sdap_access_lock_step_done'
<br>
sssd-1.12.90/src/providers/ldap/sdap_access.c:1758:16:
warning: 'ret' may be used uninitialized in this function
[-Wmaybe-uninitialized]
<br>
# if (ret != EOK) {
<br>
#
<br>
<br>
<blockquote type="cite">+ }
<br>
+
<br>
+ if (locked_only_by_admin) {
<br>
+ /* We do *not* care about exact value of account
locked time, we
<br>
+ * only *do* care if the value is equal to
<br>
+ * PERMANENTLY_LOCKED_ACCOUNT, which means that
account is locked
<br>
+ * permanently.
<br>
+ */
<br>
+ DEBUG(SSSDBG_TRACE_FUNC,
<br>
+ "Account is beeing blocked by password
policy, "
<br>
+ "but value: [%s] value is ignored by
SSSD.\n",
<br>
+ pwdAccountLockedTime);
<br>
+ *locked = false;
<br>
+ } else {
<br>
+ /* Account may be locked out from natural reasons
(too many attempts,
<br>
+ * expired password). In this case,
pwdAccountLockedTime is also set,
<br>
+ * to the time of lock out.
<br>
+ */
<br>
+ ret = convert_time(pwdAccountLockedTime,
"%Y%m%d%H%M%SZ",
<br>
+ &lock_time);
<br>
+ if (ret != EOK) {
<br>
+ DEBUG(SSSDBG_TRACE_FUNC, "convert_time() failed
with %d:%s.\n",
<br>
+ ret, sss_strerror(ret));
<br>
+ goto done;
<br>
+ }
<br>
+
<br>
+ now = time(NULL);
<br>
+
<br>
+ /* Account was NOT locked in past. */
<br>
+ if (difftime(lock_time, now) > 0.0) {
<br>
+ *locked = false;
<br>
+ } else if (pwdAccountLockedDurationTime != NULL) {
<br>
+ errno = 0;
<br>
+ duration = strtol(pwdAccountLockedDurationTime,
NULL, 0);
<br>
</blockquote>
</blockquote>
I think we already have a utility function for this in
<br>
src/util/strtonum.c, please use it.
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">+ if (errno) {
<br>
+ ret = errno;
<br>
+ goto done;
<br>
+ }
<br>
+ /* Lockout has expired */
<br>
+ if (duration != 0 && difftime(now,
lock_time) > duration) {
<br>
+ *locked = false;
<br>
+ }
<br>
+ }
<br>
+ }
<br>
+
<br>
+ ret = EOK;
<br>
+
<br>
+done:
<br>
+ return ret;
<br>
+}
<br>
+
<br>
static void sdap_access_lock_step_done(struct tevent_req
*subreq)
<br>
{
<br>
int ret, tret, dp_error;
<br>
size_t num_results;
<br>
bool locked = false;
<br>
const char *pwdAccountLockedTime;
<br>
+ const char *pwdAccountLockedDurationTime;
<br>
struct sysdb_attrs **results;
<br>
struct tevent_req *req;
<br>
struct sdap_access_lock_req_ctx *state;
<br>
+ bool pwd_admin_locked_only;
<br>
<br>
req = tevent_req_callback_data(subreq, struct
tevent_req);
<br>
state = tevent_req_data(req, struct
sdap_access_lock_req_ctx);
<br>
@@ -1613,6 +1686,9 @@ static void
sdap_access_lock_step_done(struct tevent_req *subreq)
<br>
ret = sdap_get_generic_recv(subreq, state,
&num_results, &results);
<br>
talloc_zfree(subreq);
<br>
<br>
+ pwd_admin_locked_only =
dp_opt_get_bool(state->opts->basic,
<br>
+
SDAP_PWDLOCKOUT_ADMIN_LOCKED_ONLY);
<br>
+
<br>
</blockquote>
I have already commented it in 1st patch that IMHO we do not
need this option
<br>
at all.
<br>
<br>
If other developers agree we need that option then please move
it to the place
<br>
where we DO NEED it. Because it is used just once but
initialized far far away
<br>
from place where it is used.
<br>
<br>
There is another warning from static analyser.
<br>
I think iit's false possitive because we needn't test output
variable of
<br>
dp_opt_get_bool.
<br>
<br>
<br>
Error: CHECKED_RETURN (CWE-252): [#def3]
<br>
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).
<br>
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>)".
<br>
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".
<br>
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>)".
<br>
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>)".
<br>
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>)".
<br>
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.
<br>
<br>
<br>
<blockquote type="cite"> ret =
sdap_id_op_done(state->sdap_op, ret, &dp_error);
<br>
if (ret != EOK) {
<br>
if (dp_error == DP_ERR_OK) {
<br>
@@ -1653,22 +1729,26 @@ static void
sdap_access_lock_step_done(struct tevent_req *subreq)
<br>
ret = ERR_INTERNAL;
<br>
goto done;
<br>
} else { /* Ok, we got a single reply */
<br>
+ ret = sysdb_attrs_get_string(results[0],
SYSDB_LDAP_ACESS_LOCKOUT_DURATION,
<br>
+
&pwdAccountLockedDurationTime);
<br>
+ if (ret != EOK) {
<br>
+ /* This attribute might not be set even if
account is locked */
<br>
+ pwdAccountLockedDurationTime = NULL;
<br>
+ }
<br>
+
<br>
ret = sysdb_attrs_get_string(results[0],
SYSDB_LDAP_ACCESS_LOCKED_TIME,
<br>
&pwdAccountLockedTime);
<br>
if (ret == EOK) {
<br>
- /* We do *not* care about exact value of
account locked time, we
<br>
- * only *do* care if the value is equal to
<br>
- * PERMANENTLY_LOCKED_ACCOUNT, which means that
account is locked
<br>
- * permanently.
<br>
- */
<br>
- if (strcasecmp(pwdAccountLockedTime,
<br>
- PERMANENTLY_LOCKED_ACCOUNT) ==
0) {
<br>
+ ret =
decide_if_account_is_locked(pwd_admin_locked_only,
<br>
+
pwdAccountLockedTime,
<br>
+
pwdAccountLockedDurationTime,
<br>
+ &locked);
<br>
+ if (ret != EOK) {
<br>
+ DEBUG(SSSDBG_MINOR_FAILURE,
<br>
+ "decide_if_account_is_locked failed:
%d:[%s]. "
<br>
+ "Account will be considered to be
locked.\n",
<br>
+ ret, sss_strerror(ret));
<br>
locked = true;
<br>
- } else {
<br>
- DEBUG(SSSDBG_TRACE_FUNC,
<br>
- "Account of: %s is beeing blocked by
password policy, "
<br>
- "but value: [%s] value is ignored by
SSSD.\n",
<br>
- state->username,
pwdAccountLockedTime);
<br>
}
<br>
} else {
<br>
/* Attribute SYSDB_LDAP_ACCESS_LOCKED_TIME in
not be present unless
<br>
diff --git a/src/providers/ldap/sdap_access.h
b/src/providers/ldap/sdap_access.h
<br>
index
f085e619961198b887d65ed5ee0bc5cdd90d1b20..3e59b9b2f4851a87c01f3e353c11b3bdf9bb4ff7
100644
<br>
--- a/src/providers/ldap/sdap_access.h
<br>
+++ b/src/providers/ldap/sdap_access.h
<br>
@@ -35,6 +35,7 @@
<br>
#define SYSDB_LDAP_ACCESS_CACHED_LOCKOUT
"ldap_access_lockout_allow"
<br>
/* names of ppolicy attributes */
<br>
#define SYSDB_LDAP_ACCESS_LOCKED_TIME "pwdAccountLockedTime"
<br>
+#define SYSDB_LDAP_ACESS_LOCKOUT_DURATION
"pwdLockoutDuration"
<br>
#define SYSDB_LDAP_ACCESS_LOCKOUT "pwdLockout"
<br>
<br>
#define LDAP_ACCESS_FILTER_NAME "filter"
<br>
-- <br>
2.1.0
<br>
</blockquote>
Patch works as expexted.
<br>
</blockquote>
Thank you for testing, I only read the patches, didn't do any
testing
<br>
yet.
<br>
<br>
<blockquote type="cite">Tested with 3 different values of
pwdAccountLockedTime
<br>
<br>
dn: uid=testuser1,ou=Users,dc=example,dc=com
<br>
changetype: modify
<br>
replace: pwdAccountLockedTime
<br>
pwdAccountLockedTime: 000001010000Z
<br>
<br>
dn: uid=testuser1,ou=Users,dc=example,dc=com
<br>
changetype: modify
<br>
replace: pwdAccountLockedTime
<br>
pwdAccountLockedTime: 20150310003750Z
<br>
<br>
dn: uid=testuser1,ou=Users,dc=example,dc=com
<br>
changetype: modify
<br>
replace: pwdAccountLockedTime
<br>
pwdAccountLockedTime: 20150301003750Z
<br>
<br>
LS
<br>
</blockquote>
_______________________________________________
<br>
sssd-devel mailing list
<br>
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<br>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
<br>
</blockquote>
Thanks for comments, please see updated patchset.
<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
sssd-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
</blockquote>
Patches needed to be rebased.<br>
<br>
ci passed:<br>
<pre wrap=""><a class="moz-txt-link-freetext" href="http://sssd-ci.duckdns.org/logs/job/8/74/summary.html">http://sssd-ci.duckdns.org/logs/job/8/74/summary.html</a></pre>
<br>
<br>
</body>
</html>