[PATCH] Fixes for bug 13873 (Py 3 bug on 4.10)
Tim Beale
timbeale at catalyst.net.nz
Wed Apr 3 03:05:03 UTC 2019
Someone on the samba mailing-list found an interesting problem with
Python3 on 4.10 this week.
https://bugzilla.samba.org/show_bug.cgi?id=13873
We were taking a parameter that defaulted to None and comparing it to an
integer value. On Python 3 this now throws an exception. The code is
still incorrect, even with Python 2 (None is always less than any
integer, so the comparison would never do anything sensible). The
difference is we now get an exception, rather than silently skipping
that particular block of code.
Attached is a fix for the problem, along with a test to highlight the
buggy 'samba-tool domain passwordsettings set' command.
CI pass*: https://gitlab.com/catalyst-samba/samba/pipelines/54912258
*Note I had to apply a separate patch (also attached) to get around the
flappy samba3.smb2.notify.valid-req test. I think Gary is just going to
mark the test as flapping, which is probably a better option.
-------------- next part --------------
From 394ace9ee1b07aff3d9348e2b83e6e2472c65bfe Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 1 Apr 2019 16:32:27 +1300
Subject: [PATCH 1/4] tests: Add test for setting min/maxPwdAge
Currently setting maxPwdAge doesn't work at all.
While we're adding a test, we might as well assert that minPwdAge
can't be greater than maxPwdAge as well.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13873
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/tests/samba_tool/passwordsettings.py | 38 +++++++++++++++++++++++
selftest/knownfail.d/passwordsettings | 1 +
2 files changed, 39 insertions(+)
create mode 100644 selftest/knownfail.d/passwordsettings
diff --git a/python/samba/tests/samba_tool/passwordsettings.py b/python/samba/tests/samba_tool/passwordsettings.py
index e29c76c..43264b6 100644
--- a/python/samba/tests/samba_tool/passwordsettings.py
+++ b/python/samba/tests/samba_tool/passwordsettings.py
@@ -444,3 +444,41 @@ class PwdSettingsCmdTestCase(SambaToolCmdTest):
self.assertCmdSuccess(result, out, err)
self.assertEquals(err, "", "Shouldn't be any error messages")
self.assertIn("Minimum password length: %u" % new_len, out)
+
+ def test_domain_passwordsettings_pwdage(self):
+ """Checks the 'set' command for the domain password age (non-PSO)"""
+
+ # check we can set the domain max password age
+ max_pwd_age = self.ldb.get_maxPwdAge()
+ self.addCleanup(self.ldb.set_maxPwdAge, max_pwd_age)
+ max_pwd_args = "--max-pwd-age=270"
+ (result, out, err) = self.runsublevelcmd("domain", ("passwordsettings",
+ "set"), max_pwd_args,
+ "-H", self.server,
+ self.user_auth)
+ self.assertCmdSuccess(result, out, err)
+ self.assertEquals(err, "", "Shouldn't be any error messages")
+ self.assertIn("successful", out)
+ self.assertNotEquals(max_pwd_age, self.ldb.get_maxPwdAge())
+
+ # check we can't set the domain min password age to more than the max
+ min_pwd_age = self.ldb.get_minPwdAge()
+ self.addCleanup(self.ldb.set_minPwdAge, min_pwd_age)
+ min_pwd_args = "--min-pwd-age=271"
+ (result, out, err) = self.runsublevelcmd("domain", ("passwordsettings",
+ "set"), min_pwd_args,
+ "-H", self.server,
+ self.user_auth)
+ self.assertCmdFail(result, "minPwdAge > maxPwdAge should be rejected")
+ self.assertIn("Maximum password age", err)
+
+ # check we can set the domain min password age to less than the max
+ min_pwd_args = "--min-pwd-age=269"
+ (result, out, err) = self.runsublevelcmd("domain", ("passwordsettings",
+ "set"), min_pwd_args,
+ "-H", self.server,
+ self.user_auth)
+ self.assertCmdSuccess(result, out, err)
+ self.assertEquals(err, "", "Shouldn't be any error messages")
+ self.assertIn("successful", out)
+ self.assertNotEquals(min_pwd_age, self.ldb.get_minPwdAge())
diff --git a/selftest/knownfail.d/passwordsettings b/selftest/knownfail.d/passwordsettings
new file mode 100644
index 0000000..8e94cef
--- /dev/null
+++ b/selftest/knownfail.d/passwordsettings
@@ -0,0 +1 @@
+samba.tests.samba_tool.passwordsettings.samba.tests.samba_tool.passwordsettings.PwdSettingsCmdTestCase.test_domain_passwordsettings_pwdage\(ad_dc_default:local\)
--
2.7.4
From c7ac5c725af0b7dfd4cca54a500e2822e702d6f1 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Mon, 1 Apr 2019 16:42:32 +1300
Subject: [PATCH 2/4] netcmd: Use python constant for -0x8000000000000000
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13873
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/netcmd/domain.py | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 851e724..76c3b6e 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -1255,6 +1255,10 @@ class cmd_domain_level(Command):
raise CommandError("invalid argument: '%s' (choose from 'show', 'raise')" % subcommand)
+# In MS AD, setting a timeout to '(never)' corresponds to this value
+NEVER_TIMESTAMP = int(-0x8000000000000000)
+
+
class cmd_domain_passwordsettings_show(Command):
"""Display current password settings for the domain."""
@@ -1290,13 +1294,13 @@ class cmd_domain_passwordsettings_show(Command):
cur_min_pwd_len = int(res[0]["minPwdLength"][0])
# ticks -> days
cur_min_pwd_age = int(abs(int(res[0]["minPwdAge"][0])) / (1e7 * 60 * 60 * 24))
- if int(res[0]["maxPwdAge"][0]) == -0x8000000000000000:
+ if int(res[0]["maxPwdAge"][0]) == NEVER_TIMESTAMP:
cur_max_pwd_age = 0
else:
cur_max_pwd_age = int(abs(int(res[0]["maxPwdAge"][0])) / (1e7 * 60 * 60 * 24))
cur_account_lockout_threshold = int(res[0]["lockoutThreshold"][0])
# ticks -> mins
- if int(res[0]["lockoutDuration"][0]) == -0x8000000000000000:
+ if int(res[0]["lockoutDuration"][0]) == NEVER_TIMESTAMP:
cur_account_lockout_duration = 0
else:
cur_account_lockout_duration = abs(int(res[0]["lockoutDuration"][0])) / (1e7 * 60)
@@ -1455,7 +1459,7 @@ class cmd_domain_passwordsettings_set(Command):
# days -> ticks
if max_pwd_age == 0:
- max_pwd_age_ticks = -0x8000000000000000
+ max_pwd_age_ticks = NEVER_TIMESTAMP
else:
max_pwd_age_ticks = -int(max_pwd_age * (24 * 60 * 60 * 1e7))
@@ -1474,7 +1478,7 @@ class cmd_domain_passwordsettings_set(Command):
# minutes -> ticks
if account_lockout_duration == 0:
- account_lockout_duration_ticks = -0x8000000000000000
+ account_lockout_duration_ticks = NEVER_TIMESTAMP
else:
account_lockout_duration_ticks = -int(account_lockout_duration * (60 * 1e7))
@@ -1503,7 +1507,7 @@ class cmd_domain_passwordsettings_set(Command):
# minutes -> ticks
if reset_account_lockout_after == 0:
- reset_account_lockout_after_ticks = -0x8000000000000000
+ reset_account_lockout_after_ticks = NEVER_TIMESTAMP
else:
reset_account_lockout_after_ticks = -int(reset_account_lockout_after * (60 * 1e7))
--
2.7.4
From 9b05e3fd937a361dbcdc22fde8a51d020e8a53e5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 2 Apr 2019 11:10:41 +1300
Subject: [PATCH 3/4] netcmd: Add some timestamp conversion helper functions
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13873
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/netcmd/domain.py | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 76c3b6e..2477030 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -1259,6 +1259,22 @@ class cmd_domain_level(Command):
NEVER_TIMESTAMP = int(-0x8000000000000000)
+def timestamp_to_mins(timestamp_str):
+ """Converts a timestamp in -100 nanosecond units to minutes"""
+ # treat a timestamp of 'never' the same as zero (this should work OK for
+ # most settings, and it displays better than trying to convert
+ # -0x8000000000000000 to minutes)
+ if int(timestamp_str) == NEVER_TIMESTAMP:
+ return 0
+ else:
+ return abs(int(timestamp_str)) / (1e7 * 60)
+
+
+def timestamp_to_days(timestamp_str):
+ """Converts a timestamp in -100 nanosecond units to days"""
+ return timestamp_to_mins(timestamp_str) / (60 * 24)
+
+
class cmd_domain_passwordsettings_show(Command):
"""Display current password settings for the domain."""
@@ -1293,18 +1309,14 @@ class cmd_domain_passwordsettings_show(Command):
pwd_hist_len = int(res[0]["pwdHistoryLength"][0])
cur_min_pwd_len = int(res[0]["minPwdLength"][0])
# ticks -> days
- cur_min_pwd_age = int(abs(int(res[0]["minPwdAge"][0])) / (1e7 * 60 * 60 * 24))
- if int(res[0]["maxPwdAge"][0]) == NEVER_TIMESTAMP:
- cur_max_pwd_age = 0
- else:
- cur_max_pwd_age = int(abs(int(res[0]["maxPwdAge"][0])) / (1e7 * 60 * 60 * 24))
+ cur_min_pwd_age = timestamp_to_days(res[0]["minPwdAge"][0])
+ cur_max_pwd_age = timestamp_to_days(res[0]["maxPwdAge"][0])
+
cur_account_lockout_threshold = int(res[0]["lockoutThreshold"][0])
+
# ticks -> mins
- if int(res[0]["lockoutDuration"][0]) == NEVER_TIMESTAMP:
- cur_account_lockout_duration = 0
- else:
- cur_account_lockout_duration = abs(int(res[0]["lockoutDuration"][0])) / (1e7 * 60)
- cur_reset_account_lockout_after = abs(int(res[0]["lockOutObservationWindow"][0])) / (1e7 * 60)
+ cur_account_lockout_duration = timestamp_to_mins(res[0]["lockoutDuration"][0])
+ cur_reset_account_lockout_after = timestamp_to_mins(res[0]["lockOutObservationWindow"][0])
except Exception as e:
raise CommandError("Could not retrieve password properties!", e)
--
2.7.4
From 5d4cd11ef5c3aa824cf7a59c2b7d26b7ec9bfcc5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 3 Apr 2019 09:10:55 +1300
Subject: [PATCH 4/4] netcmd: Fix passwordsettings --max-pwd-age command
The min_pwd_age and max_pwd_age parameters are both optional and default
to None. However, if we just set the max-pwd-age, then the check
'min_pwd_age >= max_pwd_age' will throw a Python exception because it's
trying to compare an int to NoneType (min_pwd_age). This works on Python 2
but is a problem on Python 3.
We could just add a check that min_pwd_age is not None, but that defeats
the point of having the check if you're only setting either the min or
max age indepedently.
This patch gets the current min/max password age from the DB (in ticks).
If either setting is changed, the ticks will be updated. Then at the end
we check the min is still less than the max (to do this, we convert the
ticks back to days in the interests of readability).
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13873
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/netcmd/domain.py | 14 ++++++++++++--
selftest/knownfail.d/passwordsettings | 1 -
2 files changed, 12 insertions(+), 3 deletions(-)
delete mode 100644 selftest/knownfail.d/passwordsettings
diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 2477030..b616dba 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -1398,6 +1398,10 @@ class cmd_domain_passwordsettings_set(Command):
m.dn = ldb.Dn(samdb, domain_dn)
pwd_props = int(samdb.get_pwdProperties())
+ # get the current password age settings
+ max_pwd_age_ticks = samdb.get_maxPwdAge()
+ min_pwd_age_ticks = samdb.get_minPwdAge()
+
if complexity is not None:
if complexity == "on" or complexity == "default":
pwd_props = pwd_props | DOMAIN_PASSWORD_COMPLEX
@@ -1527,8 +1531,14 @@ class cmd_domain_passwordsettings_set(Command):
ldb.FLAG_MOD_REPLACE, "lockOutObservationWindow")
msgs.append("Duration to reset account lockout after changed!")
- if max_pwd_age and max_pwd_age > 0 and min_pwd_age >= max_pwd_age:
- raise CommandError("Maximum password age (%d) must be greater than minimum password age (%d)!" % (max_pwd_age, min_pwd_age))
+ if max_pwd_age or min_pwd_age:
+ # If we're setting either min or max password, make sure the max is
+ # still greater overall. As either setting could be None, we use the
+ # ticks here (which are always set) and work backwards.
+ max_pwd_age = timestamp_to_days(max_pwd_age_ticks)
+ min_pwd_age = timestamp_to_days(min_pwd_age_ticks)
+ if max_pwd_age != 0 and min_pwd_age >= max_pwd_age:
+ raise CommandError("Maximum password age (%d) must be greater than minimum password age (%d)!" % (max_pwd_age, min_pwd_age))
if len(m) == 0:
raise CommandError("You must specify at least one option to set. Try --help")
diff --git a/selftest/knownfail.d/passwordsettings b/selftest/knownfail.d/passwordsettings
deleted file mode 100644
index 8e94cef..0000000
--- a/selftest/knownfail.d/passwordsettings
+++ /dev/null
@@ -1 +0,0 @@
-samba.tests.samba_tool.passwordsettings.samba.tests.samba_tool.passwordsettings.PwdSettingsCmdTestCase.test_domain_passwordsettings_pwdage\(ad_dc_default:local\)
--
2.7.4
-------------- next part --------------
From 0542fcc56360cdb32a95706c598bcd5cd6c757ff Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 3 Apr 2019 13:29:57 +1300
Subject: [PATCH] s4/torture: Relax assert to avoid notify test flapping
CI seems to consistently get '2' here instead of the expected '3'.
Relax the assert so either constitutes a pass in the meantime.
(Metze intends to look into this problem properly next week).
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
source4/torture/smb2/notify.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/source4/torture/smb2/notify.c b/source4/torture/smb2/notify.c
index 9b8a44f..975219d 100644
--- a/source4/torture/smb2/notify.c
+++ b/source4/torture/smb2/notify.c
@@ -60,6 +60,15 @@
goto done; \
}} while (0)
+#define CHECK_COND(v, cond) do { \
+ if (!(cond)) { \
+ torture_result(torture, TORTURE_FAIL, \
+ "(%s) wrong value for %s 0x%x. Should be: %s\n", \
+ __location__, #v, (int)v, #cond); \
+ ret = false; \
+ goto done; \
+ }} while (0)
+
#define CHECK_WIRE_STR(field, value) do { \
if (!field.s || strcmp(field.s, value)) { \
torture_result(torture, TORTURE_FAIL, \
@@ -160,13 +169,19 @@ static bool test_valid_request(struct torture_context *torture,
status = smb2_notify_recv(req, torture, &n);
CHECK_STATUS(status, NT_STATUS_OK);
- CHECK_VAL(n.out.num_changes, 3);
+ // TODO: sometimes we get 2 instead of 3 here.
+ // For now, accept either as a pass
+ //CHECK_VAL(n.out.num_changes, 3);
+ CHECK_COND(n.out.num_changes,
+ n.out.num_changes == 2 || n.out.num_changes == 3);
CHECK_VAL(n.out.changes[0].action, NOTIFY_ACTION_REMOVED);
CHECK_WIRE_STR(n.out.changes[0].name, FNAME);
CHECK_VAL(n.out.changes[1].action, NOTIFY_ACTION_ADDED);
CHECK_WIRE_STR(n.out.changes[1].name, FNAME);
- CHECK_VAL(n.out.changes[2].action, NOTIFY_ACTION_MODIFIED);
- CHECK_WIRE_STR(n.out.changes[2].name, FNAME);
+ if (n.out.num_changes == 3) {
+ CHECK_VAL(n.out.changes[2].action, NOTIFY_ACTION_MODIFIED);
+ CHECK_WIRE_STR(n.out.changes[2].name, FNAME);
+ }
/* if the first notify returns NOTIFY_ENUM_DIR, all do */
status = smb2_util_close(tree, dh);
--
2.7.4
More information about the samba-technical
mailing list