[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