From ebe42a83b0f63336dbb000fccecce91545c37940 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 16:13:05 +1300 Subject: [PATCH 1/4] 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 96d6c0a2cea0a988ffe34326315cbc0b55274a20 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 18 Mar 2021 10:52:52 +1300 Subject: [PATCH 2/4] 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 99f0283e3f77a1d23bc9e2f27cd73f627f21563b Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 16:22:40 +1300 Subject: [PATCH 3/4] 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 a3dc7fb454f..b10846f2345 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -1028,6 +1028,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))) @@ -1040,22 +1044,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 681645c5ecc0a3628d7532e1a9a773a5b6df1ec2 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 16 Mar 2021 22:20:21 +1300 Subject: [PATCH 4/4] 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 b10846f2345..ba3d12c4952 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -278,7 +278,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) @@ -502,7 +503,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: @@ -568,7 +570,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, @@ -860,7 +863,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() @@ -948,7 +952,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') @@ -973,7 +978,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) @@ -1025,7 +1030,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 @@ -1080,7 +1086,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) @@ -1092,7 +1099,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