[PATCH] Fix pwdLastSet behaviour in regards to Windows

adrianc at catalyst.net.nz adrianc at catalyst.net.nz
Mon Jan 18 00:56:45 UTC 2016


On 2016-01-18 12:28, Douglas Bagnall wrote:
> 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

Thanks for your feedback, I've now addressed your concerns.

It's worth noting that the reason I left a possibility to return an 
error is because when I started I was trying to emulate Windows errors, 
but (as noted in the commit message) I took that out because it break 
provisioning. But, as you noted, I didn't remove the possibility to 
through errors, and thanks for pointing that out.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-samdb-fix-pwdLastSet-behaviour-in-regards-to-Windows.patch
Type: text/x-diff
Size: 6627 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160118/ce072528/0001-samdb-fix-pwdLastSet-behaviour-in-regards-to-Windows.diff>


More information about the samba-technical mailing list