[PATCH] fromServer pointing to dead server causes DSDB headaches
Tim Beale
timbeale at catalyst.net.nz
Mon Oct 1 00:28:27 UTC 2018
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.
-------------- next part --------------
From 94000e1a355034dd215acba879b933ce25765ba3 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/3] 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>
---
selftest/knownfail.d/attr_from_server | 5 +
source4/dsdb/tests/python/attr_from_server.py | 149 ++++++++++++++++++++++++++
source4/selftest/tests.py | 5 +
3 files changed, 159 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..7243042
--- /dev/null
+++ b/source4/dsdb/tests/python/attr_from_server.py
@@ -0,0 +1,149 @@
+# -*- 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,{}".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={},{}".format(os.environ["SERVER"], servers_dn)
+ ntds_conn = "CN=Test-NTDS-Conn,CN=NTDS Settings,{}".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,{}".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 = long(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 af6733f1773f38309ae564d4d9300eca270acdb5 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/3] 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>
---
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 3158c94949d48523bfdc73e889ad2e8e4c1f38ed 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/3] 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>
---
source4/dsdb/samdb/ldb_modules/extended_dn_out.c | 9 +++------
1 file changed, 3 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..eb76218 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,8 @@ 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
More information about the samba-technical
mailing list