From d09d5df963116b3ce50bcc6022ef0ac2a1c07ae1 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 18 Nov 2020 12:11:10 +1300 Subject: [PATCH 01/12] samba-tool domain backup: Confirm the sidForRestore we will put into the backup is free Otherwise the administrator might only find there is a problem once they attempt to restore the domain! BUG: https://bugzilla.samba.org/show_bug.cgi?id=14575 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer (cherry picked from commit 15609cb91986b3e29c5b1a3b6c69c04829f43eb4) --- python/samba/netcmd/domain_backup.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index a3dc7fb454f..25dba78fa03 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -107,6 +107,32 @@ def get_sid_for_restore(samdb, logger): # Construct full SID sid = dom_sid(samdb.get_domain_sid()) + sid_for_restore = str(sid) + '-' + str(rid) + + # Confirm the SID is not already in use + try: + res = samdb.search(scope=ldb.SCOPE_BASE, + base='' % sid_for_restore, + attrs=[], + controls=['show_deleted:1', + 'show_recycled:1']) + if len(res) != 1: + # This case makes no sense, but neither does a corrupt RID set + raise CommandError("Cannot create backup - " + "this DC's RID pool is corrupt, " + "the next SID (%s) appears to be in use." % + sid_for_restore) + raise CommandError("Cannot create backup - " + "this DC's RID pool is corrupt, " + "the next SID %s points to existing object %s. " + "Please run samba-tool dbcheck on the source DC." % + (sid_for_restore, res[0].dn)) + except ldb.LdbError as e: + (enum, emsg) = e.args + if enum != ldb.ERR_NO_SUCH_OBJECT: + # We want NO_SUCH_OBJECT, anything else is a serious issue + raise + return str(sid) + '-' + str(rid) -- 2.31.1.362.g311531c9de From ba3fd299d03d734a24585fd9dba679ec2994397d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 13 Nov 2020 15:26:07 +1300 Subject: [PATCH 02/12] samba-tool: Give better error information when the 'domain backup restore' fails with a duplicate SID BUG: https://bugzilla.samba.org/show_bug.cgi?id=14575 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer Autobuild-User(master): Gary Lockyer Autobuild-Date(master): Thu Nov 26 21:15:40 UTC 2020 on sn-devel-184 (cherry picked from commit 8ad82ae66157c893a2b84d353ec4d9feb4815ede) --- python/samba/netcmd/domain_backup.py | 45 +++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index 25dba78fa03..fff39fcbc2a 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -27,6 +27,7 @@ import tdb import samba.getopt as options from samba.samdb import SamDB, get_default_backend_store import ldb +from ldb import LdbError from samba.samba3 import libsmb_samba_internal as libsmb from samba.samba3 import param as s3param from samba.ntacls import backup_online, backup_restore, backup_offline @@ -576,7 +577,49 @@ class cmd_domain_backup_restore(cmd_fsmo_seize): attrs=['sidForRestore']) sid = res[0].get('sidForRestore')[0] logger.info('Creating account with SID: ' + str(sid)) - ctx.join_add_objects(specified_sid=dom_sid(str(sid))) + try: + ctx.join_add_objects(specified_sid=dom_sid(str(sid))) + except LdbError as e: + (enum, emsg) = e.args + if enum != ldb.ERR_CONSTRAINT_VIOLATION: + raise + + dup_res = [] + try: + dup_res = samdb.search(base=ldb.Dn(samdb, "" % sid), + scope=ldb.SCOPE_BASE, + attrs=['objectGUID'], + controls=["show_deleted:0", + "show_recycled:0"]) + except LdbError as dup_e: + if enum != ldb.ERR_NO_SUCH_OBJECT: + raise e + + if (len(dup_res) != 1): + raise e + + objectguid = samdb.schema_format_value("objectGUID", + dup_res[0]["objectGUID"][0]) + objectguid = objectguid.decode('utf-8') + logger.error("The RID Pool on the source DC for the backup in %s " + "may be corrupt " + "or in conflict with SIDs already allocated " + "in the domain. " % backup_file) + logger.error("Running 'samba-tool dbcheck' on the source " + "DC (and obtaining a new backup) may correct the issue.") + logger.error("Alternatively please obtain a new backup " + "against a different DC.") + logger.error("The SID we wish to use (%s) is recorded in " + "@SAMBA_DSDB as the sidForRestore attribute." + % sid) + + raise CommandError("Domain restore failed because there " + "is already an existing object (%s) " + "with SID %s and objectGUID %s. " + "This conflicts with " + "the new DC account we want to add " + "for the restored domain. " % ( + dup_res[0].dn, sid, objectguid)) m = ldb.Message() m.dn = ldb.Dn(samdb, '@ROOTDSE') -- 2.31.1.362.g311531c9de From f263c8b3c07cac803cea8a721442946c9679a112 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 16:13:05 +1300 Subject: [PATCH 03/12] netcmd: Add test for an offline backup of a directory containing hardlinks This test verifies that when performing an offline backup of a domain where the directories to be backed up contain hardlinks, only one instance of each file is backed up, and that files in the private directory take precedence. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14027 Signed-off-by: Joseph Sutton (cherry picked from commit 0e5738887524b467bfebcf657bcb00ed71827784) --- python/samba/tests/domain_backup_offline.py | 74 +++++++++++++++------ selftest/knownfail.d/bug-14027 | 1 + 2 files changed, 56 insertions(+), 19 deletions(-) create mode 100644 selftest/knownfail.d/bug-14027 diff --git a/python/samba/tests/domain_backup_offline.py b/python/samba/tests/domain_backup_offline.py index 8b7209ec24d..80ccf3bb93b 100644 --- a/python/samba/tests/domain_backup_offline.py +++ b/python/samba/tests/domain_backup_offline.py @@ -31,6 +31,34 @@ from samba.netcmd import CommandError # so that we never inadvertently use .runcmd() by accident. class DomainBackupOfflineCmp(BlackboxTestCase): + def test_domain_backup_offline_hard_link_tdb(self): + self.hard_link_testcase('tdb') + + def test_domain_backup_offline_hard_link_mdb(self): + self.hard_link_testcase('mdb') + + def hard_link_testcase(self, backend): + self.prov_dir = self.provision(backend) + self.extract_dir = None + + # Create hard links in the private and state directories + os.link(os.path.join(self.prov_dir, "private", "krb5.conf"), + os.path.join(self.prov_dir, "state", "krb5.conf")) + + backup_file = self.backup(self.prov_dir) + + # Extract the backup + self.extract_dir = tempfile.mkdtemp(dir=self.tempdir) + tf = tarfile.open(backup_file) + tf.extractall(self.extract_dir) + + # Ensure that the hard link in the private directory was backed up, + # while the one in the state directory was not. + self.assertTrue(os.path.exists(os.path.join(self.extract_dir, + "private", "krb5.conf"))) + self.assertFalse(os.path.exists(os.path.join(self.extract_dir, + "statedir", "krb5.conf"))) + def test_domain_backup_offline_untar_tdb(self): self.untar_testcase('tdb') @@ -44,12 +72,15 @@ class DomainBackupOfflineCmp(BlackboxTestCase): self.restore_testcase('mdb') def restore_testcase(self, backend): - prov_dir, backup_file = self.provision_and_backup(backend) + self.prov_dir = self.provision(backend) + self.extract_dir = None + backup_file = self.backup(self.prov_dir) - extract_dir = tempfile.mkdtemp(dir=self.tempdir) + self.extract_dir = tempfile.mkdtemp(dir=self.tempdir) cmd = ("samba-tool domain backup restore --backup-file={f}" " --targetdir={d} " - "--newservername=NEWSERVER").format(f=backup_file, d=extract_dir) + "--newservername=NEWSERVER").format(f=backup_file, + d=self.extract_dir) self.check_output(cmd) # attrs that are altered by the restore process @@ -61,22 +92,18 @@ class DomainBackupOfflineCmp(BlackboxTestCase): "interSiteTopologyGenerator"] filter_arg = "--filter=" + ",".join(ignore_attrs) args = ["--two", filter_arg] - self.ldapcmp(prov_dir, extract_dir, args) - - shutil.rmtree(prov_dir) - shutil.rmtree(extract_dir) + self.ldapcmp(self.prov_dir, self.extract_dir, args) def untar_testcase(self, backend): - prov_dir, backup_file = self.provision_and_backup(backend) + self.prov_dir = self.provision(backend) + self.extract_dir = None + backup_file = self.backup(self.prov_dir) - extract_dir = tempfile.mkdtemp(dir=self.tempdir) + self.extract_dir = tempfile.mkdtemp(dir=self.tempdir) tf = tarfile.open(backup_file) - tf.extractall(extract_dir) + tf.extractall(self.extract_dir) - self.ldapcmp(prov_dir, extract_dir) - - shutil.rmtree(prov_dir) - shutil.rmtree(extract_dir) + self.ldapcmp(self.prov_dir, self.extract_dir) def ldapcmp(self, prov_dir, ex_dir, args=[]): sam_fn = os.path.join("private", "sam.ldb") @@ -90,8 +117,8 @@ class DomainBackupOfflineCmp(BlackboxTestCase): self.check_output(cmd) # Test the "samba-tool domain backup" command with ldapcmp - def provision_and_backup(self, backend): - prov_dir = tempfile.mkdtemp(dir=self.tempdir) + def provision(self, backend): + target = tempfile.mkdtemp(dir=self.tempdir) # Provision domain. Use fake ACLs and store xattrs in tdbs so that # NTACL backup will work inside the testenv. @@ -100,13 +127,16 @@ class DomainBackupOfflineCmp(BlackboxTestCase): # circumstances, causing the ldapcmp to fail. prov_cmd = "samba-tool domain provision " +\ "--domain FOO --realm foo.example.com " +\ - "--targetdir {prov_dir} " +\ + "--targetdir {target} " +\ "--backend-store {backend} " +\ "--host-name OLDSERVER "+\ "--option=\"vfs objects=fake_acls xattr_tdb\"" - prov_cmd = prov_cmd.format(prov_dir=prov_dir, backend=backend) + prov_cmd = prov_cmd.format(target=target, backend=backend) self.check_output(prov_cmd) + return target + + def backup(self, prov_dir): # Run the backup and check we got one backup tar file cmd = ("samba-tool domain backup offline --targetdir={prov_dir} " "-s {prov_dir}/etc/smb.conf").format(prov_dir=prov_dir) @@ -120,4 +150,10 @@ class DomainBackupOfflineCmp(BlackboxTestCase): " file but got {0}".format(len(tar_files))) backup_file = os.path.join(prov_dir, tar_files[0]) - return prov_dir, backup_file + return backup_file + + def tearDown(self): + # Remove temporary directories + shutil.rmtree(self.prov_dir) + if self.extract_dir: + shutil.rmtree(self.extract_dir) diff --git a/selftest/knownfail.d/bug-14027 b/selftest/knownfail.d/bug-14027 new file mode 100644 index 00000000000..b1bb5270b3e --- /dev/null +++ b/selftest/knownfail.d/bug-14027 @@ -0,0 +1 @@ +^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_hard_link -- 2.31.1.362.g311531c9de From b3f14c9e8a95436458e69510bb53ba436121c71e Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 18 Mar 2021 10:52:52 +1300 Subject: [PATCH 04/12] netcmd: Add test for an offline backup of nested directories This test verifies that when performing an offline backup of a domain where one of the directories to be backed up is nested inside another, the contained files are only included once in the backup. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14027 Signed-off-by: Joseph Sutton (cherry picked from commit f994783f4279884ec4d2ee3e7db80fb7af267d1c) --- python/samba/tests/domain_backup_offline.py | 31 +++++++++++++++++++++ selftest/knownfail.d/bug-14027 | 1 + 2 files changed, 32 insertions(+) diff --git a/python/samba/tests/domain_backup_offline.py b/python/samba/tests/domain_backup_offline.py index 80ccf3bb93b..16d3e7c36f4 100644 --- a/python/samba/tests/domain_backup_offline.py +++ b/python/samba/tests/domain_backup_offline.py @@ -21,6 +21,7 @@ import shutil import tempfile from samba.tests import BlackboxTestCase from samba.netcmd import CommandError +from samba.param import LoadParm # The backup tests require that a completely clean LoadParm object gets used # for the restore. Otherwise the same global LP gets re-used, and the LP @@ -31,6 +32,36 @@ from samba.netcmd import CommandError # so that we never inadvertently use .runcmd() by accident. class DomainBackupOfflineCmp(BlackboxTestCase): + def test_domain_backup_offline_nested_tdb(self): + self.nested_testcase('tdb') + + def test_domain_backup_offline_nested_mdb(self): + self.nested_testcase('mdb') + + def nested_testcase(self, backend): + self.prov_dir = self.provision(backend) + self.extract_dir = None + + src = os.path.join(self.prov_dir, "private") + dst = os.path.join(self.prov_dir, "state", "private") + + # Move private directory inside state directory + shutil.move(src, dst) + + smbconf = os.path.join(self.prov_dir, "etc", "smb.conf") + + # Update the conf file + lp = LoadParm(filename_for_non_global_lp=smbconf) + lp.set("private dir", dst) + lp.dump(False, smbconf) + + backup_file = self.backup(self.prov_dir) + + # Ensure each file is only present once in the tar file + tf = tarfile.open(backup_file) + names = tf.getnames() + self.assertEqual(len(names), len(set(names))) + def test_domain_backup_offline_hard_link_tdb(self): self.hard_link_testcase('tdb') diff --git a/selftest/knownfail.d/bug-14027 b/selftest/knownfail.d/bug-14027 index b1bb5270b3e..f0746474be2 100644 --- a/selftest/knownfail.d/bug-14027 +++ b/selftest/knownfail.d/bug-14027 @@ -1 +1,2 @@ ^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_hard_link +^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_nested -- 2.31.1.362.g311531c9de From 645b629350a15e26d661bf95e7f608d17de06219 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 16:22:40 +1300 Subject: [PATCH 05/12] netcmd: Determine which files are to be copied for an offline domain backup The old behaviour attempted to check for and remove files with duplicate names, but did not do so due to a bug, and would have left undetermined which files were given priority when duplicate filenames were present. Now when hardlinks are present, only one instance of each file is chosen, with files in the private directory having priority. If one backup dir is nested inside another, the files contained in the nested directory are only added once. Additionally, the BIND DNS database is omitted from the backup. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14027 Signed-off-by: Joseph Sutton (cherry picked from commit 3723148e7aa7e6d4a48a1a38112f121f52b6ee6f) --- python/samba/netcmd/domain_backup.py | 19 ++++++++++++++++--- selftest/knownfail.d/bug-14027 | 2 -- 2 files changed, 16 insertions(+), 5 deletions(-) delete mode 100644 selftest/knownfail.d/bug-14027 diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index fff39fcbc2a..c5749a5c8e2 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -1097,6 +1097,10 @@ class cmd_domain_backup_offline(samba.netcmd.Command): samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp) sid = get_sid_for_restore(samdb, logger) + # Iterating over the directories in this specific order ensures that + # when the private directory contains hardlinks that are also contained + # in other directories to be backed up (such as in paths.binddns_dir), + # the hardlinks in the private directory take precedence. backup_dirs = [paths.private_dir, paths.state_dir, os.path.dirname(paths.smbconf)] # etc dir logger.info('running backup on dirs: {0}'.format(' '.join(backup_dirs))) @@ -1109,22 +1113,31 @@ class cmd_domain_backup_offline(samba.netcmd.Command): continue if working_dir.endswith('.sock') or '.sock/' in working_dir: continue + # The BIND DNS database can be regenerated, so it doesn't need + # to be backed up. + if working_dir.startswith(os.path.join(paths.binddns_dir, 'dns')): + continue for filename in filenames: - if filename in all_files: + full_path = os.path.join(working_dir, filename) + + # Ignore files that have already been added. This prevents + # duplicates if one backup dir is a subdirectory of another, + # or if backup dirs contain hardlinks. + if any(os.path.samefile(full_path, file) for file in all_files): continue # Assume existing backup files are from a previous backup. # Delete and ignore. if filename.endswith(self.backup_ext): - os.remove(os.path.join(working_dir, filename)) + os.remove(full_path) continue # Sock files are autogenerated at runtime, ignore. if filename.endswith('.sock'): continue - all_files.append(os.path.join(working_dir, filename)) + all_files.append(full_path) # Backup secrets, sam.ldb and their downstream files self.backup_secrets(paths.private_dir, lp, logger) diff --git a/selftest/knownfail.d/bug-14027 b/selftest/knownfail.d/bug-14027 deleted file mode 100644 index f0746474be2..00000000000 --- a/selftest/knownfail.d/bug-14027 +++ /dev/null @@ -1,2 +0,0 @@ -^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_hard_link -^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_nested -- 2.31.1.362.g311531c9de From 9febaea96ee7553c1bb83d9322ee062e1c4e0b78 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 22:20:21 +1300 Subject: [PATCH 06/12] netcmd: Avoid database corruption by opting not to create database files during an offline domain backup If backup dirs contain hardlinks, the backup process could previously attempt to open an LMDB database already opened during the backup, causing it to be recreated as a new TDB database. This commit ensures that new database files are not created during this operation, and that the main SamDB database is not modified. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14027 Signed-off-by: Joseph Sutton (cherry picked from commit 4cf773591d49166b8c7ef8d637d7edfe755d48aa) --- python/samba/netcmd/domain_backup.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index c5749a5c8e2..c5a7083f536 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -305,7 +305,8 @@ class cmd_domain_backup_online(samba.netcmd.Command): shutil.rmtree(paths.sysvol) # Edit the downloaded sam.ldb to mark it as a backup - samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp) + samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) time_str = get_timestamp() add_backup_marker(samdb, "backupDate", time_str) add_backup_marker(samdb, "sidForRestore", new_sid) @@ -529,7 +530,8 @@ class cmd_domain_backup_restore(cmd_fsmo_seize): # open a DB connection to the restored DB private_dir = os.path.join(targetdir, 'private') samdb_path = os.path.join(private_dir, 'sam.ldb') - samdb = SamDB(url=samdb_path, session_info=system_session(), lp=lp) + samdb = SamDB(url=samdb_path, session_info=system_session(), lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) backup_type = self.get_backup_type(samdb) if site is None: @@ -637,7 +639,8 @@ class cmd_domain_backup_restore(cmd_fsmo_seize): host_ip, host_ip6, site) secrets_path = os.path.join(private_dir, 'secrets.ldb') - secrets_ldb = Ldb(secrets_path, session_info=system_session(), lp=lp) + secrets_ldb = Ldb(secrets_path, session_info=system_session(), lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) secretsdb_self_join(secrets_ldb, domain=ctx.domain_name, realm=ctx.realm, dnsdomain=ctx.dnsdomain, netbiosname=ctx.myname, domainsid=ctx.domsid, @@ -929,7 +932,8 @@ class cmd_domain_backup_rename(samba.netcmd.Command): # connect to the local DB (making sure we use the new/renamed config) lp.load(paths.smbconf) - samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp) + samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) # Edit the cloned sam.ldb to mark it as a backup time_str = get_timestamp() @@ -1017,7 +1021,8 @@ class cmd_domain_backup_offline(samba.netcmd.Command): # on the secrets.ldb file before backing up that file and secrets.tdb def backup_secrets(self, private_dir, lp, logger): secrets_path = os.path.join(private_dir, 'secrets') - secrets_obj = Ldb(secrets_path + '.ldb', lp=lp) + secrets_obj = Ldb(secrets_path + '.ldb', lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) logger.info('Starting transaction on ' + secrets_path) secrets_obj.transaction_start() self.offline_tdb_copy(secrets_path + '.ldb') @@ -1042,7 +1047,7 @@ class cmd_domain_backup_offline(samba.netcmd.Command): else: logger.info('Starting transaction on ' + sam_ldb_path) copy_function = self.offline_tdb_copy - sam_obj = Ldb(sam_ldb_path, lp=lp) + sam_obj = Ldb(sam_ldb_path, lp=lp, flags=ldb.FLG_DONT_CREATE_DB) sam_obj.transaction_start() logger.info(' backing up ' + sam_ldb_path) @@ -1094,7 +1099,8 @@ class cmd_domain_backup_offline(samba.netcmd.Command): check_targetdir(logger, targetdir) - samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp) + samdb = SamDB(url=paths.samdb, session_info=system_session(), lp=lp, + flags=ldb.FLG_RDONLY) sid = get_sid_for_restore(samdb, logger) # Iterating over the directories in this specific order ensures that @@ -1149,7 +1155,8 @@ class cmd_domain_backup_offline(samba.netcmd.Command): # Writing to a .bak file only works because the DN being # written to happens to be top level. samdb = SamDB(url=paths.samdb + self.backup_ext, - session_info=system_session(), lp=lp) + session_info=system_session(), lp=lp, + flags=ldb.FLG_DONT_CREATE_DB) time_str = get_timestamp() add_backup_marker(samdb, "backupDate", time_str) add_backup_marker(samdb, "sidForRestore", sid) @@ -1161,7 +1168,7 @@ class cmd_domain_backup_offline(samba.netcmd.Command): if not os.path.exists(path + self.backup_ext): if path.endswith('.ldb'): logger.info('Starting transaction on solo db: ' + path) - ldb_obj = Ldb(path, lp=lp) + ldb_obj = Ldb(path, lp=lp, flags=ldb.FLG_DONT_CREATE_DB) ldb_obj.transaction_start() logger.info(' running tdbbackup on the same file') self.offline_tdb_copy(path) -- 2.31.1.362.g311531c9de From 81aabe8461e33ded0073c93d9cdfb1dd856e8f70 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 24 May 2021 16:40:55 +1200 Subject: [PATCH 07/12] netcmd: Fix error-checking condition This condition probably meant to check the argument of the most recently thrown exception, rather than the previous one again. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit e8c242bed19432d96e78dc345ab5f06422c5b104) --- python/samba/netcmd/domain_backup.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index c5a7083f536..3a1bb924a9a 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -594,11 +594,12 @@ class cmd_domain_backup_restore(cmd_fsmo_seize): controls=["show_deleted:0", "show_recycled:0"]) except LdbError as dup_e: - if enum != ldb.ERR_NO_SUCH_OBJECT: - raise e + (dup_enum, _) = dup_e.args + if dup_enum != ldb.ERR_NO_SUCH_OBJECT: + raise if (len(dup_res) != 1): - raise e + raise objectguid = samdb.schema_format_value("objectGUID", dup_res[0]["objectGUID"][0]) -- 2.31.1.362.g311531c9de From 320f2affb7996f24d9ef3cf086eb157d24d20b43 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 26 May 2021 13:40:30 +1200 Subject: [PATCH 08/12] netcmd: Ignore rIDUsedPool attribute in offline domain backup test The RID Set of the newly created DC account has all its values initialised to zero. If the rIDUsedPool attribute was previously non-zero, then the restore process will cause its value to change. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 658e5a6cc20b57f48477affd370fe25458178b92) --- python/samba/tests/domain_backup_offline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/samba/tests/domain_backup_offline.py b/python/samba/tests/domain_backup_offline.py index 16d3e7c36f4..83cd6cfa57f 100644 --- a/python/samba/tests/domain_backup_offline.py +++ b/python/samba/tests/domain_backup_offline.py @@ -116,7 +116,7 @@ class DomainBackupOfflineCmp(BlackboxTestCase): # attrs that are altered by the restore process ignore_attrs = ["servicePrincipalName", "lastLogonTimestamp", - "rIDAllocationPool", "rIDAvailablePool", + "rIDAllocationPool", "rIDAvailablePool", "rIDUsedPool", "localPolicyFlags", "operatingSystem", "displayName", "dnsRecord", "dNSTombstoned", "msDS-NC-Replica-Locations", "msDS-HasInstantiatedNCs", -- 2.31.1.362.g311531c9de From 3cc3bdaaed05d1c092d3c860afa2fe8f897e577b Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 24 May 2021 14:58:40 +1200 Subject: [PATCH 09/12] netcmd: Add tests for performing an offline backup immediately after joining a domain This currently fails due to the DC not having a rIDNextRID attribute, which is required for the restore process. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit b7e6a1c5da7283c49586dc29f85ab19e0e57b0f6) --- python/samba/tests/domain_backup_offline.py | 61 ++++++++++++++++++--- selftest/knownfail.d/bug-14669 | 1 + source4/selftest/tests.py | 2 +- 3 files changed, 56 insertions(+), 8 deletions(-) create mode 100644 selftest/knownfail.d/bug-14669 diff --git a/python/samba/tests/domain_backup_offline.py b/python/samba/tests/domain_backup_offline.py index 83cd6cfa57f..21f42c6dab8 100644 --- a/python/samba/tests/domain_backup_offline.py +++ b/python/samba/tests/domain_backup_offline.py @@ -19,9 +19,12 @@ import tarfile import os import shutil import tempfile -from samba.tests import BlackboxTestCase +from samba.tests import BlackboxTestCase, BlackboxProcessError from samba.netcmd import CommandError from samba.param import LoadParm +from samba.join import join_DC +from samba.credentials import Credentials +from samba.logger import get_samba_logger # The backup tests require that a completely clean LoadParm object gets used # for the restore. Otherwise the same global LP gets re-used, and the LP @@ -62,6 +65,23 @@ class DomainBackupOfflineCmp(BlackboxTestCase): names = tf.getnames() self.assertEqual(len(names), len(set(names))) + def test_domain_backup_offline_join_restore_tdb(self): + self.join_restore_testcase('tdb') + + def test_domain_backup_offline_join_restore_mdb(self): + self.join_restore_testcase('mdb') + + def join_restore_testcase(self, backend): + self.prov_dir = self.join(backend) + self.extract_dir = None + + try: + backup_file = self.backup(self.prov_dir) + except BlackboxProcessError as e: + self.fail(e) + + self.extract_dir = self.restore(backup_file) + def test_domain_backup_offline_hard_link_tdb(self): self.hard_link_testcase('tdb') @@ -107,12 +127,7 @@ class DomainBackupOfflineCmp(BlackboxTestCase): self.extract_dir = None backup_file = self.backup(self.prov_dir) - self.extract_dir = tempfile.mkdtemp(dir=self.tempdir) - cmd = ("samba-tool domain backup restore --backup-file={f}" - " --targetdir={d} " - "--newservername=NEWSERVER").format(f=backup_file, - d=self.extract_dir) - self.check_output(cmd) + self.extract_dir = self.restore(backup_file) # attrs that are altered by the restore process ignore_attrs = ["servicePrincipalName", "lastLogonTimestamp", @@ -167,6 +182,27 @@ class DomainBackupOfflineCmp(BlackboxTestCase): return target + def join(self, backend): + target = tempfile.mkdtemp(dir=self.tempdir) + + join_cmd = "samba-tool domain join {domain} DC " +\ + "--server {server} " +\ + "--realm {realm} " +\ + "--username {username}%{password} " +\ + "--targetdir {target} " +\ + "--backend-store {backend} " +\ + "--option=\"vfs objects=dfs_samba4 acl_xattr fake_acls xattr_tdb\"" + join_cmd = join_cmd.format(server=os.environ["DC_SERVER"], + domain=os.environ["DOMAIN"], + realm=os.environ["REALM"], + username=os.environ["USERNAME"], + password=os.environ["PASSWORD"], + target=target, + backend=backend) + self.check_output(join_cmd) + + return target + def backup(self, prov_dir): # Run the backup and check we got one backup tar file cmd = ("samba-tool domain backup offline --targetdir={prov_dir} " @@ -183,6 +219,17 @@ class DomainBackupOfflineCmp(BlackboxTestCase): backup_file = os.path.join(prov_dir, tar_files[0]) return backup_file + def restore(self, backup_file): + # Restore from a backup file + extract_dir = tempfile.mkdtemp(dir=self.tempdir) + cmd = ("samba-tool domain backup restore --backup-file={f}" + " --targetdir={d} " + "--newservername=NEWSERVER").format(f=backup_file, + d=extract_dir) + self.check_output(cmd) + + return extract_dir + def tearDown(self): # Remove temporary directories shutil.rmtree(self.prov_dir) diff --git a/selftest/knownfail.d/bug-14669 b/selftest/knownfail.d/bug-14669 new file mode 100644 index 00000000000..83f8cbfe456 --- /dev/null +++ b/selftest/knownfail.d/bug-14669 @@ -0,0 +1 @@ +^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_join_restore diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 258c9122edc..e16e64fb2f0 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -884,7 +884,7 @@ for env in ["ad_dc_backup", smbv1_disabled_testenv]: planoldpythontestsuite(env + ":local", "samba.tests.domain_backup", extra_args=['-U"$USERNAME%$PASSWORD"']) -planoldpythontestsuite("none", +planoldpythontestsuite("ad_dc", "samba.tests.domain_backup_offline") # Encrypted secrets # ensure default provision (ad_dc) and join (vampire_dc) -- 2.31.1.362.g311531c9de From a20bee8fe5ff295705849d6e64bdfa763e8123c1 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 24 May 2021 12:59:59 +1200 Subject: [PATCH 10/12] dsdb: Add next_free_rid() function to allocate a RID without modifying the database If used to generate SIDs for objects, care should be taken, as the possibility for having duplicate objectSIDs can arise. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit cc98e03e7a0f2bf7a1ace2950fe6500f53640c1b) --- python/samba/samdb.py | 105 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/python/samba/samdb.py b/python/samba/samdb.py index d903babb406..d13c5e3b7a2 100644 --- a/python/samba/samdb.py +++ b/python/samba/samdb.py @@ -1285,6 +1285,111 @@ schemaUpdateNow: 1 '''return a new RID from the RID Pool on this DSA''' return dsdb._dsdb_allocate_rid(self) + def next_free_rid(self): + '''return the next free RID from the RID Pool on this DSA. + + :note: This function is not intended for general use, and care must be + taken if it is used to generate objectSIDs. The returned RID is not + formally reserved for use, creating the possibility of duplicate + objectSIDs. + ''' + rid, _ = self.free_rid_bounds() + return rid + + def free_rid_bounds(self): + '''return the low and high bounds (inclusive) of RIDs that are + available for use in this DSA's current RID pool. + + :note: This function is not intended for general use, and care must be + taken if it is used to generate objectSIDs. The returned range of + RIDs is not formally reserved for use, creating the possibility of + duplicate objectSIDs. + ''' + # Get DN of this server's RID Set + server_name_dn = ldb.Dn(self, self.get_serverName()) + res = self.search(base=server_name_dn, + scope=ldb.SCOPE_BASE, + attrs=["serverReference"]) + try: + server_ref = res[0]["serverReference"] + except KeyError: + raise ldb.LdbError( + ldb.ERR_NO_SUCH_ATTRIBUTE, + "No RID Set DN - " + "Cannot find attribute serverReference of %s " + "to calculate reference dn" % server_name_dn) from None + server_ref_dn = ldb.Dn(self, server_ref[0].decode("utf-8")) + + res = self.search(base=server_ref_dn, + scope=ldb.SCOPE_BASE, + attrs=["rIDSetReferences"]) + try: + rid_set_refs = res[0]["rIDSetReferences"] + except KeyError: + raise ldb.LdbError( + ldb.ERR_NO_SUCH_ATTRIBUTE, + "No RID Set DN - " + "Cannot find attribute rIDSetReferences of %s " + "to calculate reference dn" % server_ref_dn) from None + rid_set_dn = ldb.Dn(self, rid_set_refs[0].decode("utf-8")) + + # Get the alloc pools and next RID of this RID Set + res = self.search(base=rid_set_dn, + scope=ldb.SCOPE_BASE, + attrs=["rIDAllocationPool", + "rIDPreviousAllocationPool", + "rIDNextRID"]) + + uint32_max = 2**32 - 1 + uint64_max = 2**64 - 1 + + try: + alloc_pool = int(res[0]["rIDAllocationPool"][0]) + except KeyError: + alloc_pool = uint64_max + if alloc_pool == uint64_max: + raise ldb.LdbError(ldb.ERR_OPERATIONS_ERROR, + "Bad RID Set %s" % rid_set_dn) + + try: + prev_pool = int(res[0]["rIDPreviousAllocationPool"][0]) + except KeyError: + prev_pool = uint64_max + try: + next_rid = int(res[0]["rIDNextRID"][0]) + except KeyError: + next_rid = uint32_max + + # If we never used a pool, set up our first pool + if prev_pool == uint64_max or next_rid == uint32_max: + prev_pool = alloc_pool + next_rid = prev_pool & uint32_max + + next_rid += 1 + + # Now check if our current pool is still usable + prev_pool_lo = prev_pool & uint32_max + prev_pool_hi = prev_pool >> 32 + if next_rid > prev_pool_hi: + # We need a new pool, check if we already have a new one + # Otherwise we return an error code. + if alloc_pool == prev_pool: + raise ldb.LdbError(ldb.ERR_OPERATIONS_ERROR, + "RID pools out of RIDs") + + # Now use the new pool + prev_pool = alloc_pool + prev_pool_lo = prev_pool & uint32_max + prev_pool_hi = prev_pool >> 32 + next_rid = prev_pool_lo + + if next_rid < prev_pool_lo or next_rid > prev_pool_hi: + raise ldb.LdbError(ldb.ERR_OPERATIONS_ERROR, + "Bad RID chosen %d from range %d-%d" % + (next_rid, prev_pool_lo, prev_pool_hi)) + + return next_rid, prev_pool_hi + def normalize_dn_in_domain(self, dn): '''return a new DN expanded by adding the domain DN -- 2.31.1.362.g311531c9de From 5b24f5065708157a2474261eec9a5bc1e7014fa7 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Mon, 24 May 2021 16:46:28 +1200 Subject: [PATCH 11/12] python/tests/dsdb: Add tests for RID allocation functions BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 7c7cad81844950c3efe9a540a47b9d4e1ce1b2a1) --- python/samba/tests/dsdb.py | 305 ++++++++++++++++++++++++++++++++++++- 1 file changed, 304 insertions(+), 1 deletion(-) diff --git a/python/samba/tests/dsdb.py b/python/samba/tests/dsdb.py index 33cfcc12271..f1d0557743e 100644 --- a/python/samba/tests/dsdb.py +++ b/python/samba/tests/dsdb.py @@ -50,13 +50,316 @@ class DsdbTests(TestCase): base_dn = self.samdb.domain_dn() - self.account_dn = "cn=" + user_name + ",cn=Users," + base_dn + self.account_dn = "CN=" + user_name + ",CN=Users," + base_dn self.samdb.newuser(username=user_name, password=user_pass, description=user_description) # Cleanup (teardown) self.addCleanup(delete_force, self.samdb, self.account_dn) + # Get server reference DN + res = self.samdb.search(base=ldb.Dn(self.samdb, + self.samdb.get_serverName()), + scope=ldb.SCOPE_BASE, + attrs=["serverReference"]) + # Get server reference + self.server_ref_dn = ldb.Dn( + self.samdb, res[0]["serverReference"][0].decode("utf-8")) + + # Get RID Set DN + res = self.samdb.search(base=self.server_ref_dn, + scope=ldb.SCOPE_BASE, + attrs=["rIDSetReferences"]) + rid_set_refs = res[0] + self.assertIn("rIDSetReferences", rid_set_refs) + rid_set_str = rid_set_refs["rIDSetReferences"][0].decode("utf-8") + self.rid_set_dn = ldb.Dn(self.samdb, rid_set_str) + + def get_rid_set(self, rid_set_dn): + res = self.samdb.search(base=rid_set_dn, + scope=ldb.SCOPE_BASE, + attrs=["rIDAllocationPool", + "rIDPreviousAllocationPool", + "rIDUsedPool", + "rIDNextRID"]) + return res[0] + + def test_ridalloc_next_free_rid(self): + # Test RID allocation. We assume that RID + # pools allocated to us are continguous. + self.samdb.transaction_start() + try: + orig_rid_set = self.get_rid_set(self.rid_set_dn) + self.assertIn("rIDAllocationPool", orig_rid_set) + self.assertIn("rIDPreviousAllocationPool", orig_rid_set) + self.assertIn("rIDUsedPool", orig_rid_set) + self.assertIn("rIDNextRID", orig_rid_set) + + # Get rIDNextRID value from RID set. + next_rid = int(orig_rid_set["rIDNextRID"][0]) + + # Check the result of next_free_rid(). + next_free_rid = self.samdb.next_free_rid() + self.assertEqual(next_rid + 1, next_free_rid) + + # Check calling it twice in succession gives the same result. + next_free_rid2 = self.samdb.next_free_rid() + self.assertEqual(next_free_rid, next_free_rid2) + + # Ensure that the RID set attributes have not changed. + rid_set2 = self.get_rid_set(self.rid_set_dn) + self.assertEqual(orig_rid_set, rid_set2) + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_no_ridnextrid(self): + self.samdb.transaction_start() + try: + # Delete the rIDNextRID attribute of the RID set, + # and set up previous and next pools. + prev_lo = 1000 + prev_hi = 1999 + next_lo = 3000 + next_hi = 3999 + msg = ldb.Message() + msg.dn = self.rid_set_dn + msg["rIDNextRID"] = ldb.MessageElement([], + ldb.FLAG_MOD_DELETE, + "rIDNextRID") + msg["rIDPreviousAllocationPool"] = ( + ldb.MessageElement(str((prev_hi << 32) | prev_lo), + ldb.FLAG_MOD_REPLACE, + "rIDPreviousAllocationPool")) + msg["rIDAllocationPool"] = ( + ldb.MessageElement(str((next_hi << 32) | next_lo), + ldb.FLAG_MOD_REPLACE, + "rIDAllocationPool")) + self.samdb.modify(msg) + + # Ensure that next_free_rid() returns the start of the next pool + # plus one. + next_free_rid3 = self.samdb.next_free_rid() + self.assertEqual(next_lo + 1, next_free_rid3) + + # Check the result of allocate_rid() matches. + rid = self.samdb.allocate_rid() + self.assertEqual(next_free_rid3, rid) + + # Check that the result of next_free_rid() has now changed. + next_free_rid4 = self.samdb.next_free_rid() + self.assertEqual(rid + 1, next_free_rid4) + + # Check the range of available RIDs. + free_lo, free_hi = self.samdb.free_rid_bounds() + self.assertEqual(rid + 1, free_lo) + self.assertEqual(next_hi, free_hi) + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_no_free_rids(self): + self.samdb.transaction_start() + try: + # Exhaust our current pool of RIDs. + pool_lo = 2000 + pool_hi = 2999 + msg = ldb.Message() + msg.dn = self.rid_set_dn + msg["rIDPreviousAllocationPool"] = ( + ldb.MessageElement(str((pool_hi << 32) | pool_lo), + ldb.FLAG_MOD_REPLACE, + "rIDPreviousAllocationPool")) + msg["rIDAllocationPool"] = ( + ldb.MessageElement(str((pool_hi << 32) | pool_lo), + ldb.FLAG_MOD_REPLACE, + "rIDAllocationPool")) + msg["rIDNextRID"] = ( + ldb.MessageElement(str(pool_hi), + ldb.FLAG_MOD_REPLACE, + "rIDNextRID")) + self.samdb.modify(msg) + + # Ensure that calculating the next free RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.next_free_rid() + + self.assertEqual("RID pools out of RIDs", err.exception.args[1]) + + # Ensure we can still allocate a new RID. + self.samdb.allocate_rid() + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_new_ridset(self): + self.samdb.transaction_start() + try: + # Test what happens with RID Set values set to zero (similar to + # when a RID Set is first created, except we also set + # rIDAllocationPool to zero). + msg = ldb.Message() + msg.dn = self.rid_set_dn + msg["rIDPreviousAllocationPool"] = ( + ldb.MessageElement("0", + ldb.FLAG_MOD_REPLACE, + "rIDPreviousAllocationPool")) + msg["rIDAllocationPool"] = ( + ldb.MessageElement("0", + ldb.FLAG_MOD_REPLACE, + "rIDAllocationPool")) + msg["rIDNextRID"] = ( + ldb.MessageElement("0", + ldb.FLAG_MOD_REPLACE, + "rIDNextRID")) + self.samdb.modify(msg) + + # Ensure that calculating the next free RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.next_free_rid() + + self.assertEqual("RID pools out of RIDs", err.exception.args[1]) + + # Set values for the next pool. + pool_lo = 2000 + pool_hi = 2999 + msg = ldb.Message() + msg.dn = self.rid_set_dn + msg["rIDAllocationPool"] = ( + ldb.MessageElement(str((pool_hi << 32) | pool_lo), + ldb.FLAG_MOD_REPLACE, + "rIDAllocationPool")) + self.samdb.modify(msg) + + # Ensure the next free RID value is equal to the next pool's lower + # bound. + next_free_rid5 = self.samdb.next_free_rid() + self.assertEqual(pool_lo, next_free_rid5) + + # Check the range of available RIDs. + free_lo, free_hi = self.samdb.free_rid_bounds() + self.assertEqual(pool_lo, free_lo) + self.assertEqual(pool_hi, free_hi) + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_move_to_new_pool(self): + self.samdb.transaction_start() + try: + # Test moving to a new pool from the previous pool. + pool_lo = 2000 + pool_hi = 2999 + new_pool_lo = 4500 + new_pool_hi = 4599 + msg = ldb.Message() + msg.dn = self.rid_set_dn + msg["rIDPreviousAllocationPool"] = ( + ldb.MessageElement(str((pool_hi << 32) | pool_lo), + ldb.FLAG_MOD_REPLACE, + "rIDPreviousAllocationPool")) + msg["rIDAllocationPool"] = ( + ldb.MessageElement(str((new_pool_hi << 32) | new_pool_lo), + ldb.FLAG_MOD_REPLACE, + "rIDAllocationPool")) + msg["rIDNextRID"] = ( + ldb.MessageElement(str(pool_hi - 1), + ldb.FLAG_MOD_REPLACE, + "rIDNextRID")) + self.samdb.modify(msg) + + # We should have remained in the previous pool. + next_free_rid6 = self.samdb.next_free_rid() + self.assertEqual(pool_hi, next_free_rid6) + + # Check the range of available RIDs. + free_lo, free_hi = self.samdb.free_rid_bounds() + self.assertEqual(pool_hi, free_lo) + self.assertEqual(pool_hi, free_hi) + + # Allocate a new RID. + rid2 = self.samdb.allocate_rid() + self.assertEqual(next_free_rid6, rid2) + + # We should now move to the next pool. + next_free_rid7 = self.samdb.next_free_rid() + self.assertEqual(new_pool_lo, next_free_rid7) + + # Check the new range of available RIDs. + free_lo2, free_hi2 = self.samdb.free_rid_bounds() + self.assertEqual(new_pool_lo, free_lo2) + self.assertEqual(new_pool_hi, free_hi2) + + # Ensure that allocate_rid() matches. + rid3 = self.samdb.allocate_rid() + self.assertEqual(next_free_rid7, rid3) + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_no_ridsetreferences(self): + self.samdb.transaction_start() + try: + # Delete the rIDSetReferences attribute. + msg = ldb.Message() + msg.dn = self.server_ref_dn + msg["rIDSetReferences"] = ( + ldb.MessageElement([], + ldb.FLAG_MOD_DELETE, + "rIDSetReferences")) + self.samdb.modify(msg) + + # Ensure calculating the next free RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.next_free_rid() + + enum, estr = err.exception.args + self.assertEqual(ldb.ERR_NO_SUCH_ATTRIBUTE, enum) + self.assertIn("No RID Set DN - " + "Cannot find attribute rIDSetReferences of %s " + "to calculate reference dn" % self.server_ref_dn, + estr) + + # Ensure allocating a new RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.allocate_rid() + + enum, estr = err.exception.args + self.assertEqual(ldb.ERR_ENTRY_ALREADY_EXISTS, enum) + self.assertIn("No RID Set DN - " + "Failed to add RID Set %s - " + "Entry %s already exists" % + (self.rid_set_dn, self.rid_set_dn), + estr) + finally: + self.samdb.transaction_cancel() + + def test_ridalloc_no_rid_set(self): + self.samdb.transaction_start() + try: + # Set the rIDSetReferences attribute to not point to a RID Set. + fake_rid_set_str = self.account_dn + msg = ldb.Message() + msg.dn = self.server_ref_dn + msg["rIDSetReferences"] = ( + ldb.MessageElement(fake_rid_set_str, + ldb.FLAG_MOD_REPLACE, + "rIDSetReferences")) + self.samdb.modify(msg) + + # Ensure calculating the next free RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.next_free_rid() + + enum, estr = err.exception.args + self.assertEqual(ldb.ERR_OPERATIONS_ERROR, enum) + self.assertIn("Bad RID Set " + fake_rid_set_str, estr) + + # Ensure allocating a new RID fails. + with self.assertRaises(ldb.LdbError) as err: + self.samdb.allocate_rid() + + enum, estr = err.exception.args + self.assertEqual(ldb.ERR_OPERATIONS_ERROR, enum) + self.assertIn("Bad RID Set " + fake_rid_set_str, estr) + finally: + self.samdb.transaction_cancel() + def test_get_oid_from_attrid(self): oid = self.samdb.get_oid_from_attid(591614) self.assertEqual(oid, "1.2.840.113556.1.4.1790") -- 2.31.1.362.g311531c9de From fb9bde39c3578a6d71dbb4e2e7064c906bb173d0 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 27 May 2021 15:35:35 +1200 Subject: [PATCH 12/12] netcmd: Use next_free_rid() function to calculate a SID for restoring a backup This means we won't get errors if the DC doesn't have a rIDNextRID attribute, but we will still error if there is no RID Set or if all its pools are exhausted. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14669 Signed-off-by: Joseph Sutton Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 59d293b60608172ae61551c642d13d3b215924e4) --- python/samba/netcmd/domain_backup.py | 57 +++++++--------------------- selftest/knownfail.d/bug-14669 | 1 - 2 files changed, 14 insertions(+), 44 deletions(-) delete mode 100644 selftest/knownfail.d/bug-14669 diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index 3a1bb924a9a..a629b31d70f 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -61,50 +61,21 @@ from samba.ndr import ndr_pack # This ensures that the restored DC's SID won't clash with any other RIDs # already in use in the domain def get_sid_for_restore(samdb, logger): - # Find the DN of the RID set of the server - res = samdb.search(base=ldb.Dn(samdb, samdb.get_serverName()), - scope=ldb.SCOPE_BASE, attrs=["serverReference"]) - server_ref_dn = ldb.Dn(samdb, str(res[0]['serverReference'][0])) - res = samdb.search(base=server_ref_dn, - scope=ldb.SCOPE_BASE, - attrs=['rIDSetReferences']) - rid_set_dn = ldb.Dn(samdb, str(res[0]['rIDSetReferences'][0])) - - # Get the alloc pools and next RID of the RID set - res = samdb.search(base=rid_set_dn, - scope=ldb.SCOPE_SUBTREE, - expression="(rIDNextRID=*)", - attrs=['rIDAllocationPool', - 'rIDPreviousAllocationPool', - 'rIDNextRID']) - - # Decode the bounds of the RID allocation pools + # Allocate a new RID without modifying the database. This should be safe, + # because we acquire the RID master role after creating an account using + # this RID during the restore process. Acquiring the RID master role + # creates a new RID pool which we will fetch RIDs from, so we shouldn't get + # duplicates. try: - rid = int(res[0].get('rIDNextRID')[0]) - except IndexError: - logger.info("The RID pool for this DC is not initalized " - "(e.g. it may be a fairly new DC).") - logger.info("To initialize it, create a temporary user on this DC " - "(you can delete it later).") - raise CommandError("Cannot create backup - " - "please initialize this DC's RID pool first.") - - def split_val(num): - high = (0xFFFFFFFF00000000 & int(num)) >> 32 - low = 0x00000000FFFFFFFF & int(num) - return low, high - pool_l, pool_h = split_val(res[0].get('rIDPreviousAllocationPool')[0]) - npool_l, npool_h = split_val(res[0].get('rIDAllocationPool')[0]) - - # Calculate next RID based on pool bounds - if rid == npool_h: - raise CommandError('Out of RIDs, finished AllocPool') - if rid == pool_h: - if pool_h == npool_h: - raise CommandError('Out of RIDs, finished PrevAllocPool.') - rid = npool_l - else: - rid += 1 + rid = samdb.next_free_rid() + except LdbError as err: + logger.info("A SID could not be allocated for restoring the domain. " + "Either no RID Set was found on this DC, " + "or the RID Set was not usable.") + logger.info("To initialise this DC's RID pools, obtain a RID Set from " + "this domain's RID master, or run samba-tool dbcheck " + "to fix the existing RID Set.") + raise CommandError("Cannot create backup", err) # Construct full SID sid = dom_sid(samdb.get_domain_sid()) diff --git a/selftest/knownfail.d/bug-14669 b/selftest/knownfail.d/bug-14669 deleted file mode 100644 index 83f8cbfe456..00000000000 --- a/selftest/knownfail.d/bug-14669 +++ /dev/null @@ -1 +0,0 @@ -^samba.tests.domain_backup_offline.samba.tests.domain_backup_offline.DomainBackupOfflineCmp.test_domain_backup_offline_join_restore -- 2.31.1.362.g311531c9de