[PATCH] Fix pwdLastSet behaviour in regards to Windows
Douglas Bagnall
douglas.bagnall at catalyst.net.nz
Sun Jan 17 23:28:53 UTC 2016
hi Adrian,
This is good. Some comments below.
On 18/01/16 10:00, adrianc at catalyst.net.nz wrote:
> Here's a patch for fixing https://bugzilla.samba.org/show_bug.cgi?id=9654
>
> 0001-samdb-fix-pwdLastSet-behaviour-in-regards-to-Windows.patch
>
>
> From edd0c0de49c318535653fc9aab27734e1b36e6e7 Mon Sep 17 00:00:00 2001
> From: Adrian Cochrane <adrianc at catalyst.net.nz>
> Date: Thu, 14 Jan 2016 14:13:49 +1300
> Subject: [PATCH] samdb: fix pwdLastSet behaviour in regards to Windows
>
> Windows interprets a -1 in pwdLastSet as the current time, this patch
> emulates that behaviour.
>
> Fixes bug #9654
>
> Signed-off-by: Adrian Cochrane <adrianc at catalyst.net.nz>
> ---
> source4/dsdb/samdb/ldb_modules/operational.c | 51 ++++++++++++++++++++++
> source4/dsdb/tests/python/passwords.py | 64 ++++++++++++++++++++++++++++
> 2 files changed, 115 insertions(+)
>
> diff --git a/source4/dsdb/samdb/ldb_modules/operational.c b/source4/dsdb/samdb/ldb_modules/operational.c
> index c5fd8e2..5a6eff9 100644
> --- a/source4/dsdb/samdb/ldb_modules/operational.c
> +++ b/source4/dsdb/samdb/ldb_modules/operational.c
> @@ -845,6 +845,35 @@ static int construct_msds_user_password_expiry_time_computed(struct ldb_module *
> password_expiry_time);
> }
>
> +static int modify_pwdLastSet(const struct ldb_message *msg) {
This function is returns int, but the value it returns is only ever
LDB_SUCCESS. Which would be annoyingly tautological, but fortunately I
found some error cases below :)
Also the curly bracket should be on a new line, here and below.
> + struct ldb_message_element *pwdLastSet_element = NULL;
> + struct ldb_val *pwdLastSet_value = NULL;
> + int i;
> +
> + for (i = 0; i < msg->num_elements; i++) {
> + if (strcmp(msg->elements[i].name, "pwdLastSet") == 0) {
> + pwdLastSet_element = &msg->elements[i];
> + break;
> + }
> + }
> + if (pwdLastSet_element == NULL || pwdLastSet_element->num_values != 1) {
> + return LDB_SUCCESS;
> + }
Should `pwdLastSet_element->num_values != 1` count as success?
> +
> + pwdLastSet_value = &pwdLastSet_element->values[0];
> + if (ldb_val_string_cmp(pwdLastSet_value, "-1") == 0) {
> + NTTIME now;
> + char *now_s = NULL;
> + unix_to_nt_time(&now, time(NULL));
> + asprintf(&now_s, "%" PRIu64, (uint64_t) now);
This should be talloc_asprintf(). And check for errors.
> +
There's rogue whitespace here.
> + pwdLastSet_value->data = (uint8_t*) now_s;
> + pwdLastSet_value->length = strlen(now_s);
> + }
> +
> + return LDB_SUCCESS;
> +}
> +
>
> struct op_controls_flags {
> bool sd;
> @@ -1301,6 +1330,26 @@ static int operational_search(struct ldb_module *module, struct ldb_request *req
> return ldb_next_request(module, down_req);
> }
>
> +/* These callbacks are here so that a -1 in pwdLastSet can translate to the
> + * time it was modified. */
> +static int operational_add(struct ldb_module *ctx, struct ldb_request *req) {
> + int ret = modify_pwdLastSet(req->op.add.message);
> +
> + if (ret == LDB_SUCCESS) {
> + return ldb_next_request(ctx, req);
> + }
> + return ret;
> +}
> +
> +static int operational_modify(struct ldb_module *ctx, struct ldb_request *req) {
> + int ret = modify_pwdLastSet(req->op.mod.message);
> +
> + if (ret == LDB_SUCCESS) {
> + return ldb_next_request(ctx, req);
> + }
> + return ret;
> +}
> +
> static int operational_init(struct ldb_module *ctx)
> {
> struct operational_data *data;
> @@ -1325,6 +1374,8 @@ static int operational_init(struct ldb_module *ctx)
> static const struct ldb_module_ops ldb_operational_module_ops = {
> .name = "operational",
> .search = operational_search,
> + .add = operational_add,
> + .modify = operational_modify,
> .init_context = operational_init
> };
>
> diff --git a/source4/dsdb/tests/python/passwords.py b/source4/dsdb/tests/python/passwords.py
> index fb3eee5..cb70372 100755
> --- a/source4/dsdb/tests/python/passwords.py
> +++ b/source4/dsdb/tests/python/passwords.py
> @@ -21,6 +21,7 @@ from samba.tests.subunitrun import SubunitOptions, TestProgram
>
> import samba.getopt as options
>
> +from samba import unix2nttime, current_unix_time
> from samba.auth import system_session
> from samba.credentials import Credentials
> from ldb import SCOPE_BASE, LdbError
> @@ -30,6 +31,7 @@ from ldb import ERR_NO_SUCH_ATTRIBUTE
> from ldb import ERR_CONSTRAINT_VIOLATION
> from ldb import Message, MessageElement, Dn
> from ldb import FLAG_MOD_ADD, FLAG_MOD_REPLACE, FLAG_MOD_DELETE
> +import ldb
> from samba import gensec
> from samba.samdb import SamDB
> import samba.tests
> @@ -931,6 +933,68 @@ userPassword: thatsAcomplPASS4
> # Reset the "minPwdLength" as it was before
> self.ldb.set_minPwdLength(minPwdLength)
>
> + def test_pwdLastSet(self):
> + self.ldb.add({
> + "dn" : "cn=user," + self.base_dn,
> + "objectclass" : "user"
> + })
> +
> + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> +
> + now = unix2nttime(current_unix_time())
> + self.ldb.modify(ldb.Message.from_dict(self.ldb, {
> + "dn" : "cn=user," + self.base_dn,
> + "pwdLastSet" : "-1"
> + }))
> +
> + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> + # Be a bit lenient to account for clock differences.
> + self.assertTrue(now - 200000000 <= int(res1[0]["pwdLastSet"][0]) <= now + 200000000)
Watch the line length...
> +
> + self.ldb.modify(ldb.Message.from_dict(self.ldb, {
> + "dn" : "cn=user," + self.base_dn,
> + "pwdLastSet" : "0"
> + }))
> +
> + res1 = self.ldb.search("cn=user," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> + self.assertEqual(res1[0]["pwdLastSet"][0], "0")
> +
> + delete_force(self.ldb, "cn=user," + self.base_dn)
> +
> + # Now using a different object class
> + self.ldb.add({
> + "dn" : "cn=nonuser," + self.base_dn,
> + "objectclass" : "computer"
> + })
> +
> + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> +
> + now = unix2nttime(int(time.time()))
> + self.ldb.modify(ldb.Message.from_dict(self.ldb, {
> + "dn" : "cn=nonuser," + self.base_dn,
> + "pwdLastSet" : "-1"
> + }))
> +
> + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> +
> + self.assertTrue(now - 200000000 <= int(res1[0]["pwdLastSet"][0]) <= now + 200000000)
...and here also.
> +
> + self.ldb.modify(ldb.Message.from_dict(self.ldb, {
> + "dn" : "cn=nonuser," + self.base_dn,
> + "pwdLastSet" : "0"
> + }))
> +
> + res1 = self.ldb.search("cn=nonuser," + self.base_dn, scope=SCOPE_BASE)
> + self.assertEqual(len(res1), 1)
> + self.assertEqual(res1[0]["pwdLastSet"][0], "0")
> +
> + delete_force(self.ldb, "cn=nonuser," + self.base_dn)
> +
> def tearDown(self):
> super(PasswordTests, self).tearDown()
> delete_force(self.ldb, "cn=testuser,cn=users," + self.base_dn)
> --
cheers,
Douglas
More information about the samba-technical
mailing list