[PATCH] fromServer pointing to dead server causes DSDB headaches

Tim Beale timbeale at catalyst.net.nz
Thu Oct 11 20:02:46 UTC 2018


Rebased, reworded Py3 commit description. Fixed 80+ char line. Added new
patch to make the long-line slightly tidier.

CI pass: https://gitlab.com/catalyst-samba/samba/pipelines/32625701


On 04/10/18 15:23, Douglas Bagnall wrote:
> On 04/10/18 14:55, Tim Beale via samba-technical wrote:
>> I fixed up a couple of python compatibility issues in my original patch.
>> Updated patch-set attached.
>>
>> New CI link: https://gitlab.com/catalyst-samba/samba/pipelines/31952840
>>
>> On 01/10/18 13:28, Tim Beale via samba-technical wrote:
>>> We found a problem where if you demote a DC, and the deleted DC object
>>> is later expunged, then a fromServer attribute can get left behind that
>>> still references the dead server. The interesting thing is that
>>> fromServer is a mandatory attribute in the schema - because the
>>> extended-DN module detects the DN reference is bad, it tries to hide the
>>> attribute from the search result. This causes the objectclass_attr
>>> module to think the mandatory attribute is missing, and therefore the
>>> object modification breaks the schema. This means that any attempts to
>>> modify the object fail.
>>>
>>> This patch doesn't fix up the bad fromServer reference completely, but
>>> it takes the first step and makes it possible to actually modify the
>>> object once more.
>>>
>>> This was found running the backup-rename tool (i.e. creating a lab
>>> domain). The rename tries to modify the attributes for a bunch of
>>> objects (i.e. objectCategory, as the Schema DNs have now changed).
>>> However, this fails with an error when it hits a nTDSConnection object
>>> with a bad fromServer link.
>>>
>>> The fix was kindly provided by Andrew.
>>>
>>> CI link: https://gitlab.com/catalyst-samba/samba/pipelines/31443186
>>>
>>> Review appreciated. Thanks.
>>>
> Looks good to me. RB+. Another reviewer please!
>
> However, on this last one:
>
>> From 776db91a42b2e271cf59935fbdf890d30af8819b Mon Sep 17 00:00:00 2001
>> From: Tim Beale <timbeale at catalyst.net.nz>
>> Date: Thu, 4 Oct 2018 14:37:44 +1300
>> Subject: [PATCH 4/4] netcmd: Change Py3 incompatible long() for tombstone
>>  expunge
>>
>> The code to expunge tombstones uses long(), which is not Python3
>> compatible. Python3 uses int() instead, and works out how big it needs
>> to be.
>>
>> Python 2.6 appears to work differently for int(), but as long as we
>> stop running 'make test' on Python 2.6 by 2038, then we should be OK
>> (the integer won't overflow).
> It is not just 'make test' in this case, because this is samba-tool.
>
> On the other hand, using Python 2.6 in 2038 is not sufficient to cause
> a problem -- only a 32 bit machine will suffer. What is more the
> function this is used in (py_dsdb_garbage_collect_tombstones(),
> source4/dsdb/pydsdb.c:1225) takes a C long, which is the underlying
> type of a Python 2.6 int, so it is better to use the Python int in any
> case. (Python 3 ints and 2.6 longs are of indefinite length, which
> will end up being truncated at the C layer to the sizes of long thence
> time_t, undefined-ly in the C fashion, making the Python 2.6 int
> semantics look better and better).
>
> cheers,
> Douglas
>
>> Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
>> ---
>>  python/samba/netcmd/domain.py | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
>> index ce4f36a..ef6833e 100644
>> --- a/python/samba/netcmd/domain.py
>> +++ b/python/samba/netcmd/domain.py
>> @@ -3919,10 +3919,10 @@ This command expunges tombstones from the database."""
>>  
>>          if current_time_string is not None:
>>              current_time_obj = time.strptime(current_time_string, "%Y-%m-%d")
>> -            current_time = long(time.mktime(current_time_obj))
>> +            current_time = int(time.mktime(current_time_obj))
>>  
>>          else:
>> -            current_time = long(time.time())
>> +            current_time = int(time.time())
>>  
>>          if len(ncs) == 0:
>>              res = samdb.search(expression="", base="", scope=ldb.SCOPE_BASE,
>

-------------- next part --------------
From 54c395908b8232ed1bfc30f2aab5df3b5c83ac38 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Fri, 28 Sep 2018 12:35:35 +1200
Subject: [PATCH 1/5] tests: Add corner-case test: fromServer points to dead
 server

The fromServer attribute is slightly unique, in that it's a DN (similar
to a one-way link), but it is also a mandatory attribute.

Currently, if fromServer gets a bad value (i.e. a dead server that has
been expunged), the DSDB rejects any attempts to modify the associated
nTDSConnection object (regardless of whether or not you're actually
changing the fromServer attribute).

This patch adds a test-case that demonstrates how the DB can get into
such a state.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/attr_from_server         |   5 +
 source4/dsdb/tests/python/attr_from_server.py | 150 ++++++++++++++++++++++++++
 source4/selftest/tests.py                     |   5 +
 3 files changed, 160 insertions(+)
 create mode 100644 selftest/knownfail.d/attr_from_server
 create mode 100644 source4/dsdb/tests/python/attr_from_server.py

diff --git a/selftest/knownfail.d/attr_from_server b/selftest/knownfail.d/attr_from_server
new file mode 100644
index 0000000..fd4f6b9
--- /dev/null
+++ b/selftest/knownfail.d/attr_from_server
@@ -0,0 +1,5 @@
+# test currently fails because once we have a fromServer attribute that points
+# to a non-existent object, the extended_dn DSDB module then suppresses that
+# attribute, which means the object is missing a mandatory attribute, thus
+# invalidating the schema 
+^samba4.tests.attr_from_server.python\(ad_dc_ntvfs\).__main__.FromServerAttrTest.test_dangling_server_attr\(ad_dc_ntvfs:local\)
diff --git a/source4/dsdb/tests/python/attr_from_server.py b/source4/dsdb/tests/python/attr_from_server.py
new file mode 100644
index 0000000..1a0112d
--- /dev/null
+++ b/source4/dsdb/tests/python/attr_from_server.py
@@ -0,0 +1,150 @@
+# -*- coding: utf-8 -*-
+#
+# Tests a corner-case involving the fromServer attribute, which is slightly
+# unique: it's an Object(DS-DN) (like a one-way link), but it is also a
+# mandatory attribute (for nTDSConnection). The corner-case is that the
+# fromServer can potentially end up pointing to a non-existent object.
+# This can happen with other one-way links, but these other one-way links
+# are not mandatory attributes.
+#
+# Copyright (C) Andrew Bartlett 2018
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+from __future__ import print_function
+import optparse
+import sys
+sys.path.insert(0, "bin/python")
+import samba
+import os
+import time
+import ldb
+import samba.tests
+from samba.tests.subunitrun import TestProgram, SubunitOptions
+from samba.dcerpc import misc
+from samba.provision import DEFAULTSITE
+
+# note we must connect to the local ldb file on disk, in order to
+# add system-only nTDSDSA objects
+parser = optparse.OptionParser("attr_from_server.py <LDB-filepath>")
+subunitopts = SubunitOptions(parser)
+parser.add_option_group(subunitopts)
+opts, args = parser.parse_args()
+
+if len(args) < 1:
+    parser.print_usage()
+    sys.exit(1)
+
+ldb_path = args[0]
+
+
+class FromServerAttrTest(samba.tests.TestCase):
+    def setUp(self):
+        super(FromServerAttrTest, self).setUp()
+        self.ldb = samba.tests.connect_samdb(ldb_path)
+
+    def tearDown(self):
+        super(FromServerAttrTest, self).tearDown()
+
+    def set_attribute(self, dn, attr, value, operation=ldb.FLAG_MOD_ADD):
+        """Modifies an attribute for an object"""
+        m = ldb.Message()
+        m.dn = ldb.Dn(self.ldb, dn)
+        m[attr] = ldb.MessageElement(value, operation, attr)
+        self.ldb.modify(m)
+
+    def get_object_guid(self, dn):
+        res = self.ldb.search(base=dn, attrs=["objectGUID"],
+                              scope=ldb.SCOPE_BASE)
+        self.assertTrue(len(res) == 1)
+        return str(misc.GUID(res[0]['objectGUID'][0]))
+
+    def test_dangling_server_attr(self):
+        """
+        Tests a scenario where an object has a fromServer attribute that points
+        to an object that no longer exists.
+        """
+
+        # add a temporary server and its associated NTDS Settings object
+        config_dn = self.ldb.get_config_basedn()
+        sites_dn = "CN=Sites,{0}".format(config_dn)
+        servers_dn = "CN=Servers,CN={0},{1}".format(DEFAULTSITE, sites_dn)
+        tmp_server = "CN=TMPSERVER,{0}".format(servers_dn)
+        self.ldb.add({"dn": tmp_server, "objectclass": "server"})
+        server_guid = self.get_object_guid(tmp_server)
+        tmp_ntds_settings = "CN=NTDS Settings,{0}".format(tmp_server)
+        self.ldb.add({"dn": tmp_ntds_settings, "objectClass": "nTDSDSA"},
+                     ["relax:0"])
+
+        # add an NTDS connection under the testenv DC that points to the tmp DC
+        testenv_dc = "CN={0},{1}".format(os.environ["SERVER"], servers_dn)
+        ntds_conn = "CN=Test-NTDS-Conn,CN=NTDS Settings,{0}".format(testenv_dc)
+        ldif = """
+dn: {dn}
+objectClass: nTDSConnection
+fromServer: CN=NTDS Settings,{fromServer}
+options: 1
+enabledConnection: TRUE
+""".format(dn=ntds_conn, fromServer=tmp_server)
+        self.ldb.add_ldif(ldif)
+        self.addCleanup(self.ldb.delete, ntds_conn)
+
+        # sanity-check we can modify the NTDS Connection object
+        self.set_attribute(ntds_conn, 'description', 'Test-value')
+
+        # sanity-check we can't modify the fromServer to point to a bad DN
+        try:
+            bad_dn = "CN=NTDS Settings,CN=BAD-DC,{0}".format(servers_dn)
+            self.set_attribute(ntds_conn, 'fromServer', bad_dn,
+                               operation=ldb.FLAG_MOD_REPLACE)
+            self.fail("Successfully set fromServer to bad DN")
+        except ldb.LdbError as err:
+            enum = err.args[0]
+            self.assertEqual(enum, ldb.ERR_CONSTRAINT_VIOLATION)
+
+        # delete the tmp server, i.e. pretend we demoted it
+        self.ldb.delete(tmp_server, ["tree_delete:1"])
+
+        # check we can still see the deleted server object
+        search_expr = '(objectGUID={0})'.format(server_guid)
+        res = self.ldb.search(config_dn, scope=ldb.SCOPE_SUBTREE,
+                              expression=search_expr,
+                              controls=["show_deleted:1"])
+        self.assertTrue(len(res) == 1, "Could not find deleted server entry")
+
+        # now pretend some time has passed and the deleted server object
+        # has been tombstone-expunged from the DB
+        time.sleep(1)
+        current_time = int(time.time())
+        self.ldb.garbage_collect_tombstones([str(config_dn)], current_time,
+                                            tombstone_lifetime=0)
+
+        # repeat the search to sanity-check the deleted object is really gone
+        res = self.ldb.search(config_dn, scope=ldb.SCOPE_SUBTREE,
+                              expression=search_expr,
+                              controls=["show_deleted:1"])
+        self.assertTrue(len(res) == 0, "Did not expunge deleted server")
+
+        # the nTDSConnection now has a (mandatory) fromServer attribute that
+        # points to an object that no longer exists. Now try to modify an
+        # unrelated attribute on the nTDSConnection
+        try:
+            self.set_attribute(ntds_conn, 'description', 'Test-value-2',
+                               operation=ldb.FLAG_MOD_REPLACE)
+        except ldb.LdbError as err:
+            print(err)
+            self.fail("Could not modify NTDS connection")
+
+
+TestProgram(module=__name__, opts=subunitopts)
diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
index c841131..1a56630 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -147,6 +147,11 @@ if os.path.exists(os.path.join(samba4bindir, "ldbtest")):
 else:
     skiptestsuite("ldb.base", "Using system LDB, ldbtest not available")
 
+plantestsuite_loadlist("samba4.tests.attr_from_server.python(ad_dc_ntvfs)",
+                       "ad_dc_ntvfs:local",
+                       [python, os.path.join(samba4srcdir, "dsdb/tests/python/attr_from_server.py"),
+                        '$PREFIX_ABS/ad_dc_ntvfs/private/sam.ldb', '$LOADLIST', '$LISTOPT'])
+
 # Tests for RPC
 
 # add tests to this list as they start passing, so we test
-- 
2.7.4


From 20c4a95488ba8974410d846d16b09d41b05d5ced Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet at samba.org>
Date: Wed, 12 Sep 2018 14:48:04 -0500
Subject: [PATCH 2/5] dsdb: Ensure that a DN (now) pointing at a deleted object
 counts for objectclass-based MUST

Add the 'reveal_internals' controls when performing objectclass-based
checks of mandatory attributes. This prevents the extended_dn DSDB
module from suppressing attributes that point to deleted (i.e.
non-existent/expunged) objects.

This ensures that, when modifying an object (and often not even
touching the mandatory attribute) that the fact that an attribute is a
DN, and the DN target is deleted, that the schema check will still pass.

Otherwise a fromServer pointing at a dead server can cause failures,
i.e. you can't modify the affected object at all, because the DSDB
thinks a mandatory attribute is missing.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621

Signed-off-by: Andrew Bartlett <abartlet at samba.org>
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
---
 selftest/knownfail.d/attr_from_server              |  5 -----
 source4/dsdb/samdb/ldb_modules/objectclass_attrs.c | 11 +++++++++++
 2 files changed, 11 insertions(+), 5 deletions(-)
 delete mode 100644 selftest/knownfail.d/attr_from_server

diff --git a/selftest/knownfail.d/attr_from_server b/selftest/knownfail.d/attr_from_server
deleted file mode 100644
index fd4f6b9..0000000
--- a/selftest/knownfail.d/attr_from_server
+++ /dev/null
@@ -1,5 +0,0 @@
-# test currently fails because once we have a fromServer attribute that points
-# to a non-existent object, the extended_dn DSDB module then suppresses that
-# attribute, which means the object is missing a mandatory attribute, thus
-# invalidating the schema 
-^samba4.tests.attr_from_server.python\(ad_dc_ntvfs\).__main__.FromServerAttrTest.test_dangling_server_attr\(ad_dc_ntvfs:local\)
diff --git a/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c b/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
index cfacaf5..67c93ca 100644
--- a/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
+++ b/source4/dsdb/samdb/ldb_modules/objectclass_attrs.c
@@ -617,6 +617,17 @@ static int oc_op_callback(struct ldb_request *req, struct ldb_reply *ares)
 		return ldb_module_done(ac->req, NULL, NULL, ret);
 	}
 
+	/*
+	 * This ensures we see if there was a DN, that pointed at an
+	 * object that is now deleted, that we still consider the
+	 * schema check to have passed
+	 */
+	ret = ldb_request_add_control(search_req, LDB_CONTROL_REVEAL_INTERNALS,
+				      false, NULL);
+	if (ret != LDB_SUCCESS) {
+		return ldb_module_done(ac->req, NULL, NULL, ret);
+	}
+
 	ret = ldb_next_request(ac->module, search_req);
 	if (ret != LDB_SUCCESS) {
 		return ldb_module_done(ac->req, NULL, NULL, ret);
-- 
2.7.4


From 3c8e59ffbfe87efe7efbfc4e8c4bec12b00a1339 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Fri, 28 Sep 2018 14:55:14 +1200
Subject: [PATCH 3/5] dsdb: Remove redundant variable/check

Previously, this code used to live inside the loop, so the
checked_reveal_control was needed to save ourselves unnecessary work.

However, now that the code has been moved outside the loop, the
checked_reveal_control variable is just unnecessary complication.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
---
 source4/dsdb/samdb/ldb_modules/extended_dn_out.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
index 6a869d0..feb0bf4 100644
--- a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
+++ b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
@@ -402,7 +402,7 @@ static int extended_callback(struct ldb_request *req, struct ldb_reply *ares,
 	struct ldb_message *msg;
 	struct extended_dn_out_private *p;
 	struct ldb_context *ldb;
-	bool have_reveal_control=false, checked_reveal_control=false;
+	bool have_reveal_control=false;
 
 	ac = talloc_get_type(req->context, struct extended_search_context);
 	p = talloc_get_type(ldb_module_get_private(ac->module), struct extended_dn_out_private);
@@ -473,11 +473,9 @@ static int extended_callback(struct ldb_request *req, struct ldb_reply *ares,
 		}
 	}
 
-	if (!checked_reveal_control) {
-		have_reveal_control =
-			ldb_request_get_control(req, LDB_CONTROL_REVEAL_INTERNALS) != NULL;
-		checked_reveal_control = true;
-	}
+	have_reveal_control =
+		ldb_request_get_control(req,
+					LDB_CONTROL_REVEAL_INTERNALS) != NULL;
 
 	/* 
 	 * Shortcut for repl_meta_data.  We asked for the data
-- 
2.7.4


From 6f8892a1e0e47066bd9ed6718f9f23da848b6e3c Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 4 Oct 2018 14:37:44 +1300
Subject: [PATCH 4/5] netcmd: Change Py3 incompatible long() for tombstone
 expunge

The code to expunge tombstones uses long(), which is not Python3
compatible. Python3 uses int() instead, and works out how big it needs
to be.

As long as we don't run the samba-tool command on a 32-bit machine
after the year 2038, then we should avoid any integer overflow on
Python 2.x.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Gary Lockyer <gary at catalyst.net.nz>
---
 python/samba/netcmd/domain.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index ce4f36a..ef6833e 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -3919,10 +3919,10 @@ This command expunges tombstones from the database."""
 
         if current_time_string is not None:
             current_time_obj = time.strptime(current_time_string, "%Y-%m-%d")
-            current_time = long(time.mktime(current_time_obj))
+            current_time = int(time.mktime(current_time_obj))
 
         else:
-            current_time = long(time.time())
+            current_time = int(time.time())
 
         if len(ncs) == 0:
             res = samdb.search(expression="", base="", scope=ldb.SCOPE_BASE,
-- 
2.7.4


From fd0479044c9757328ca5d67a86f627f47ebb0535 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 11 Oct 2018 17:50:52 +1300
Subject: [PATCH 5/5] dsdb: Add dsdb_request_has_control() helper function

Most of the DSDB modules only want to check the existence of a control,
rather than access the control itself. Adding a helper function allows
the code to ask more natural-sounding yes/no questions, and tidies up
an ugly-looking long-line in extended_dn_out.c.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
 source4/dsdb/common/util.c                       | 8 ++++++++
 source4/dsdb/samdb/ldb_modules/extended_dn_out.c | 3 +--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index 193fa2a..e7b860d 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -4439,6 +4439,14 @@ int dsdb_request_add_controls(struct ldb_request *req, uint32_t dsdb_flags)
 }
 
 /*
+   returns true if a control with the specified "oid" exists
+*/
+bool dsdb_request_has_control(struct ldb_request *req, const char *oid)
+{
+	return (ldb_request_get_control(req, oid) != NULL);
+}
+
+/*
   an add with a set of controls
 */
 int dsdb_add(struct ldb_context *ldb, const struct ldb_message *message,
diff --git a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
index feb0bf4..014ae0a 100644
--- a/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
+++ b/source4/dsdb/samdb/ldb_modules/extended_dn_out.c
@@ -474,8 +474,7 @@ static int extended_callback(struct ldb_request *req, struct ldb_reply *ares,
 	}
 
 	have_reveal_control =
-		ldb_request_get_control(req,
-					LDB_CONTROL_REVEAL_INTERNALS) != NULL;
+		dsdb_request_has_control(req, LDB_CONTROL_REVEAL_INTERNALS);
 
 	/* 
 	 * Shortcut for repl_meta_data.  We asked for the data
-- 
2.7.4



More information about the samba-technical mailing list