[PATCH] Fix pwdLastSet behaviour in regards to Windows
adrianc at catalyst.net.nz
adrianc at catalyst.net.nz
Mon Jan 18 03:40:36 UTC 2016
On 2016-01-18 13:56, adrianc at catalyst.net.nz wrote:
> 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.
This just in, the patches fail to pass the autobuild. I'll notify when
the problem's found and fixed.
More information about the samba-technical
mailing list