Attachment 14645 Details for Bug 13683 – Proposed patch for master

Proposed patch for master

bug13683-patch.txt (text/plain), 10.70 KB, created by Tim Beale on 2018-11-13 01:26:34 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Beale

Created: 2018-11-13 01:26:34 UTC

Size: 10.70 KB

patch

obsolete

>From 29763b0fa428e58938f6c8644ecf6f7dd4401383 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 13 Nov 2018 11:49:56 +1300
>Subject: [PATCH 1/4] tests: Sanity-check password lockout works with default
> values
>
>Sanity-check that when we use the default lockOutObservationWindow that
>user lockout actually works.
>
>The easiest way to do this is to reuse the _test_login_lockout()
>test-case, but stop at the point where we wait for the lockout duration
>to expire (because we don't want the test to wait 30 mins).
>
>This highlights a problem currently where the default values don't work.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> selftest/knownfail.d/password_lockout              |  4 +++
> source4/dsdb/tests/python/password_lockout.py      | 30 ++++++++++++++++++++++
> source4/dsdb/tests/python/password_lockout_base.py |  6 ++++-
> 3 files changed, 39 insertions(+), 1 deletion(-)
> create mode 100644 selftest/knownfail.d/password_lockout
>
>diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout
>new file mode 100644
>index 0000000..305bcbd
>--- /dev/null
>+++ b/selftest/knownfail.d/password_lockout
>@@ -0,0 +1,4 @@
>+samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\)
>+samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\)
>+samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_ntlm\(ad_dc_ntvfs\)
>+samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_krb5\(ad_dc_ntvfs\)
>diff --git a/source4/dsdb/tests/python/password_lockout.py b/source4/dsdb/tests/python/password_lockout.py
>index 14cf00a..bc0613b 100755
>--- a/source4/dsdb/tests/python/password_lockout.py
>+++ b/source4/dsdb/tests/python/password_lockout.py
>@@ -1371,6 +1371,36 @@ userPassword: """ + userpass + """
>         self._testing_add_user(lockout4ntlm_creds,
>                                lockOutObservationWindow=self.lockout_observation_window)
> 
>+class PasswordTestsWithDefaults(PasswordTests):
>+    def setUp(self):
>+        # The tests in this class do not sleep, so we can use the default
>+        # timeout windows here
>+        self.account_lockout_duration = 30 * 60
>+        self.lockout_observation_window = 30 * 60
>+        super(PasswordTestsWithDefaults, self).setUp()
>+
>+    # sanity-check that user lockout works with the default settings (we just
>+    # check the user is locked out - we don't wait for the lockout to expire)
>+    def test_login_lockout_krb5(self):
>+        self._test_login_lockout(self.lockout1krb5_creds,
>+                                 wait_lockout_duration=False)
>+
>+    def test_login_lockout_ntlm(self):
>+        self._test_login_lockout(self.lockout1ntlm_creds,
>+                                 wait_lockout_duration=False)
>+
>+    # Repeat the login lockout tests using PSOs
>+    def test_pso_login_lockout_krb5(self):
>+        """Check the PSO lockout settings get applied to the user correctly"""
>+        self.use_pso_lockout_settings(self.lockout1krb5_creds)
>+        self._test_login_lockout(self.lockout1krb5_creds,
>+                                 wait_lockout_duration=False)
>+
>+    def test_pso_login_lockout_ntlm(self):
>+        """Check the PSO lockout settings get applied to the user correctly"""
>+        self.use_pso_lockout_settings(self.lockout1ntlm_creds)
>+        self._test_login_lockout(self.lockout1ntlm_creds,
>+                                 wait_lockout_duration=False)
> 
> host_url = "ldap://%s" % host
> 
>diff --git a/source4/dsdb/tests/python/password_lockout_base.py b/source4/dsdb/tests/python/password_lockout_base.py
>index c2664e9..24b066c 100644
>--- a/source4/dsdb/tests/python/password_lockout_base.py
>+++ b/source4/dsdb/tests/python/password_lockout_base.py
>@@ -364,7 +364,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """
>     def tearDown(self):
>         super(BasePasswordTestCase, self).tearDown()
> 
>-    def _test_login_lockout(self, creds):
>+    def _test_login_lockout(self, creds, wait_lockout_duration=True):
>         username = creds.get_username()
>         userpass = creds.get_password()
>         userdn = "cn=%s,cn=users,%s" % (username, self.base_dn)
>@@ -561,6 +561,10 @@ lockoutThreshold: """ + str(lockoutThreshold) + """
>                                   userAccountControl=dsdb.UF_NORMAL_ACCOUNT,
>                                   msDSUserAccountControlComputed=dsdb.UF_LOCKOUT)
> 
>+        # if we're just checking the user gets locked out, we can stop here
>+        if not wait_lockout_duration:
>+            return
>+
>         # wait for the lockout to end
>         time.sleep(self.account_lockout_duration + 1)
>         print(self.account_lockout_duration + 1)
>-- 
>2.7.4
>
>
>From 5a167429d14d0a5af17b313fbdccbd3bd24db5d8 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 13 Nov 2018 12:24:16 +1300
>Subject: [PATCH 2/4] dsdb/util: Correctly treat lockOutObservationWindow as
> 64-bit int
>
>Commit 442a38c918ae1666b35 refactored some code into a new
>get_lockout_observation_window() function. However, in moving the code,
>an ldb_msg_find_attr_as_int64() inadvertently got converted to a
>ldb_msg_find_attr_as_int().
>
>ldb_msg_find_attr_as_int() will only work for values up to -2147483648
>(about 3.5 minutes in MS timestamp form). Unfortunately, the automated
>tests used a low enough timeout that they still worked, however,
>password lockout would not work with the Samba default settings.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> selftest/knownfail.d/password_lockout |  2 --
> source4/dsdb/common/util.c            | 10 +++++-----
> 2 files changed, 5 insertions(+), 7 deletions(-)
>
>diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout
>index 305bcbd..a4e37a8 100644
>--- a/selftest/knownfail.d/password_lockout
>+++ b/selftest/knownfail.d/password_lockout
>@@ -1,4 +1,2 @@
> samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\)
> samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\)
>-samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_ntlm\(ad_dc_ntvfs\)
>-samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_krb5\(ad_dc_ntvfs\)
>diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
>index e7b860d..a0bbf48 100644
>--- a/source4/dsdb/common/util.c
>+++ b/source4/dsdb/common/util.c
>@@ -5408,12 +5408,12 @@ static int64_t get_lockout_observation_window(struct ldb_message *domain_msg,
> 					      struct ldb_message *pso_msg)
> {
> 	if (pso_msg != NULL) {
>-		return ldb_msg_find_attr_as_int(pso_msg,
>-						"msDS-LockoutObservationWindow",
>-						 0);
>+		return ldb_msg_find_attr_as_int64(pso_msg,
>+						  "msDS-LockoutObservationWindow",
>+						   0);
> 	} else {
>-		return ldb_msg_find_attr_as_int(domain_msg,
>-						"lockOutObservationWindow", 0);
>+		return ldb_msg_find_attr_as_int64(domain_msg,
>+						  "lockOutObservationWindow", 0);
> 	}
> }
> 
>-- 
>2.7.4
>
>
>From 9247ef00f7498d46c4dbefa1867d9b8174d27f61 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 13 Nov 2018 13:19:04 +1300
>Subject: [PATCH 3/4] dsdb/util: Fix lockOutObservationWindow for PSOs
>
>Fix a remaining place where we were trying to read the
>msDS-LockoutObservationWindow as an int instead of an int64.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> selftest/knownfail.d/password_lockout | 2 --
> source4/dsdb/common/util.c            | 6 +++---
> 2 files changed, 3 insertions(+), 5 deletions(-)
> delete mode 100644 selftest/knownfail.d/password_lockout
>
>diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout
>deleted file mode 100644
>index a4e37a8..0000000
>--- a/selftest/knownfail.d/password_lockout
>+++ /dev/null
>@@ -1,2 +0,0 @@
>-samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\)
>-samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\)
>diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
>index a0bbf48..9ce2e02 100644
>--- a/source4/dsdb/common/util.c
>+++ b/source4/dsdb/common/util.c
>@@ -5369,9 +5369,9 @@ int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb,
> 
> 	if (res != NULL) {
> 		lockOutObservationWindow =
>-			ldb_msg_find_attr_as_int(res->msgs[0],
>-						 "msDS-LockoutObservationWindow",
>-						  0);
>+			ldb_msg_find_attr_as_int64(res->msgs[0],
>+						   "msDS-LockoutObservationWindow",
>+						    0);
> 		talloc_free(res);
> 	} else {
> 
>-- 
>2.7.4
>
>
>From 61e172fd18eeb1fdbc5cd4c64c84674b6b41db77 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 13 Nov 2018 13:22:41 +1300
>Subject: [PATCH 4/4] dsdb/util: Add better default lockOutObservationWindow
>
>Clearly the lockOutObservationWindow value is important, and using a
>default value of zero doesn't work very well.
>
>This patch adds a better default value (the domain default setting of 30
>minutes).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>---
> source4/dsdb/common/util.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
>index 9ce2e02..36e4b26 100644
>--- a/source4/dsdb/common/util.c
>+++ b/source4/dsdb/common/util.c
>@@ -56,6 +56,9 @@
>  */
> #include "dsdb/samdb/ldb_modules/util.h"
> 
>+/* default is 30 minutes: -1e7 * 30 * 60 */
>+#define DEFAULT_OBSERVATION_WINDOW              -18000000000
>+
> /*
>   search the sam for the specified attributes in a specific domain, filter on
>   objectSid being in domain_sid.
>@@ -5371,7 +5374,7 @@ int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb,
> 		lockOutObservationWindow =
> 			ldb_msg_find_attr_as_int64(res->msgs[0],
> 						   "msDS-LockoutObservationWindow",
>-						    0);
>+						    DEFAULT_OBSERVATION_WINDOW);
> 		talloc_free(res);
> 	} else {
> 
>@@ -5410,10 +5413,11 @@ static int64_t get_lockout_observation_window(struct ldb_message *domain_msg,
> 	if (pso_msg != NULL) {
> 		return ldb_msg_find_attr_as_int64(pso_msg,
> 						  "msDS-LockoutObservationWindow",
>-						   0);
>+						   DEFAULT_OBSERVATION_WINDOW);
> 	} else {
> 		return ldb_msg_find_attr_as_int64(domain_msg,
>-						  "lockOutObservationWindow", 0);
>+						  "lockOutObservationWindow",
>+						   DEFAULT_OBSERVATION_WINDOW);
> 	}
> }
> 
>-- 
>2.7.4
>

Flags:

abartlet: review+

Actions: View

Attachments on bug 13683: 14644 | 14645 | 14646 | 14647 | 14654 | 14655 | 14656 | 14657 | 14686