From d51b1bc9129fae1c182e8c84e760582f9770671c Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 12 Jul 2018 16:13:27 +1200 Subject: [PATCH 1/5] netcmd: Improve domain backup targetdir checks + Added check that specified targetdir is actually a directory (if it exists) + Deleted a redundant 'Creating targetdir' check that would never be hit + Move code into a separate function so we can reuse it for offline backups (which take a different set of parameters, but still have a targetdir) Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett Reviewed-by: Gary Lockyer (cherry picked from commit 4f532cc177cd1e95d8ccf8e69f50b315354df34c) Backported to v4.9 for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566 --- python/samba/netcmd/domain_backup.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index cfd9796..f7b8edb 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -139,6 +139,17 @@ def add_backup_marker(samdb, marker, value): samdb.modify(m) +def check_targetdir(logger, targetdir): + if targetdir is None: + raise CommandError('Target directory required') + + if not os.path.exists(targetdir): + logger.info('Creating targetdir %s...' % targetdir) + os.makedirs(targetdir) + elif not os.path.isdir(targetdir): + raise CommandError("%s is not a directory" % targetdir) + + def check_online_backup_args(logger, credopts, server, targetdir): # Make sure we have all the required args. u_p = {'user': credopts.creds.get_username(), @@ -147,12 +158,8 @@ def check_online_backup_args(logger, credopts, server, targetdir): raise CommandError("Creds required.") if server is None: raise CommandError('Server required') - if targetdir is None: - raise CommandError('Target directory required') - if not os.path.exists(targetdir): - logger.info('Creating targetdir %s...' % targetdir) - os.makedirs(targetdir) + check_targetdir(logger, targetdir) # For '--no-secrets' backups, this sets the Administrator user's password to a @@ -211,10 +218,6 @@ class cmd_domain_backup_online(samba.netcmd.Command): lp = sambaopts.get_loadparm() creds = credopts.get_credentials(lp) - if not os.path.exists(targetdir): - logger.info('Creating targetdir %s...' % targetdir) - os.makedirs(targetdir) - tmpdir = tempfile.mkdtemp(dir=targetdir) # Run a clone join on the remote -- 2.7.4 From 5fde4146d5f68f413f6d24c39c5618dc936b5e5a Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 9 Aug 2018 15:30:55 +1200 Subject: [PATCH 2/5] netcmd: domain backup didn't support prompting for password The online/rename backups only worked if you specified both the username and password in the actual command itself. If you just entered the username (expecting to be prompted for the password later), then the command was rejected. The problem was the order the code was doing things in. We were checking credopts.creds.get_password() *before* we'd called credopts.get_credentials(lp), whereas it should be the other way around. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 14077b6682d7dc1b16e1ccb42ef61e9f4c0a1715) --- python/samba/netcmd/domain_backup.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index f7b8edb..b16763a 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -150,10 +150,10 @@ def check_targetdir(logger, targetdir): raise CommandError("%s is not a directory" % targetdir) -def check_online_backup_args(logger, credopts, server, targetdir): +def check_online_backup_args(logger, creds, server, targetdir): # Make sure we have all the required args. - u_p = {'user': credopts.creds.get_username(), - 'pass': credopts.creds.get_password()} + u_p = {'user': creds.get_username(), + 'pass': creds.get_password()} if None in u_p.values(): raise CommandError("Creds required.") if server is None: @@ -212,12 +212,12 @@ class cmd_domain_backup_online(samba.netcmd.Command): logger = self.get_logger() logger.setLevel(logging.DEBUG) - # Make sure we have all the required args. - check_online_backup_args(logger, credopts, server, targetdir) - lp = sambaopts.get_loadparm() creds = credopts.get_credentials(lp) + # Make sure we have all the required args. + check_online_backup_args(logger, creds, server, targetdir) + tmpdir = tempfile.mkdtemp(dir=targetdir) # Run a clone join on the remote @@ -680,8 +680,11 @@ class cmd_domain_backup_rename(samba.netcmd.Command): logger = self.get_logger() logger.setLevel(logging.INFO) + lp = sambaopts.get_loadparm() + creds = credopts.get_credentials(lp) + # Make sure we have all the required args. - check_online_backup_args(logger, credopts, server, targetdir) + check_online_backup_args(logger, creds, server, targetdir) delete_old_dns = not keep_dns_realm new_dns_realm = new_dns_realm.lower() @@ -695,8 +698,6 @@ class cmd_domain_backup_rename(samba.netcmd.Command): tmpdir = tempfile.mkdtemp(dir=targetdir) # setup a join-context for cloning the remote server - lp = sambaopts.get_loadparm() - creds = credopts.get_credentials(lp) include_secrets = not no_secrets ctx = DCCloneAndRenameContext(new_base_dn, new_domain_name, new_dns_realm, logger=logger, -- 2.7.4 From 34293881b75c1b8c4d6867b2a6ef9ae6847f46cb Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 9 Aug 2018 15:34:51 +1200 Subject: [PATCH 3/5] netcmd: Fix kerberos option for domain backups The previous fix still didn't work if you specified --kerberos=yes (in which case the creds still doesn't have a password). credopts.get_credentials(lp) should be enough to ensure a user/password is set (it's all that the other commands seem to do). BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 8fb706c34682bf6dc6033963518c7eccffc3944f) --- python/samba/netcmd/domain_backup.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index b16763a..b2c8302 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -152,10 +152,6 @@ def check_targetdir(logger, targetdir): def check_online_backup_args(logger, creds, server, targetdir): # Make sure we have all the required args. - u_p = {'user': creds.get_username(), - 'pass': creds.get_password()} - if None in u_p.values(): - raise CommandError("Creds required.") if server is None: raise CommandError('Server required') -- 2.7.4 From c6249cb7a65c56edde0431e3b62a97602b91644c Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 9 Aug 2018 15:35:59 +1200 Subject: [PATCH 4/5] netcmd: Delete unnecessary function Minor code cleanup. The last 2 patches gutted this function, to the point where there's no longer any value in keeping it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit d2d039515119523192676b311d5997afd34f4c90) --- python/samba/netcmd/domain_backup.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index b2c8302..8e82cd7 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -150,14 +150,6 @@ def check_targetdir(logger, targetdir): raise CommandError("%s is not a directory" % targetdir) -def check_online_backup_args(logger, creds, server, targetdir): - # Make sure we have all the required args. - if server is None: - raise CommandError('Server required') - - check_targetdir(logger, targetdir) - - # For '--no-secrets' backups, this sets the Administrator user's password to a # randomly-generated value. This is similar to the provision behaviour def set_admin_password(logger, samdb, username): @@ -212,7 +204,10 @@ class cmd_domain_backup_online(samba.netcmd.Command): creds = credopts.get_credentials(lp) # Make sure we have all the required args. - check_online_backup_args(logger, creds, server, targetdir) + if server is None: + raise CommandError('Server required') + + check_targetdir(logger, targetdir) tmpdir = tempfile.mkdtemp(dir=targetdir) @@ -680,7 +675,11 @@ class cmd_domain_backup_rename(samba.netcmd.Command): creds = credopts.get_credentials(lp) # Make sure we have all the required args. - check_online_backup_args(logger, creds, server, targetdir) + if server is None: + raise CommandError('Server required') + + check_targetdir(logger, targetdir) + delete_old_dns = not keep_dns_realm new_dns_realm = new_dns_realm.lower() -- 2.7.4 From f27f1368030c25e318f50481a2332a9d9a4b1345 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Thu, 9 Aug 2018 16:20:10 +1200 Subject: [PATCH 5/5] netcmd: Fix --kerberos=yes and --no-secrets domain backups The --kerberos=yes and --no-secrets options didn't work in combination for domain backups. The problem was creds.get_username() might not necessarily match the kerberos user (such as in the selftest environment). If this was the case, then trying to reset the admin password failed (because the creds.get_username() didn't exist in the DB). Because the admin user always has a fixed RID, we can work out the administrator based on its object SID, instead of relying on the username in the creds. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Wed Aug 15 10:19:09 CEST 2018 on sn-devel-144 (cherry picked from commit f249bea1e0538300288e7cf1dcb6037c45f92276) --- python/samba/netcmd/domain_backup.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py index 8e82cd7..5f18e81 100644 --- a/python/samba/netcmd/domain_backup.py +++ b/python/samba/netcmd/domain_backup.py @@ -32,7 +32,7 @@ from samba.auth import system_session from samba.join import DCJoinContext, join_clone, DCCloneAndRenameContext from samba.dcerpc.security import dom_sid from samba.netcmd import Option, CommandError -from samba.dcerpc import misc +from samba.dcerpc import misc, security from samba import Ldb from fsmo import cmd_fsmo_seize from samba.provision import make_smbconf @@ -152,16 +152,25 @@ def check_targetdir(logger, targetdir): # For '--no-secrets' backups, this sets the Administrator user's password to a # randomly-generated value. This is similar to the provision behaviour -def set_admin_password(logger, samdb, username): +def set_admin_password(logger, samdb): """Sets a randomly generated password for the backup DB's admin user""" + # match the admin user by RID + domainsid = samdb.get_domain_sid() + match_admin = "(objectsid={}-{})".format(domainsid, + security.DOMAIN_RID_ADMINISTRATOR) + search_expr = "(&(objectClass=user){})".format(match_admin) + + # retrieve the admin username (just in case it's been renamed) + res = samdb.search(base=samdb.domain_dn(), scope=ldb.SCOPE_SUBTREE, + expression=search_expr) + username = str(res[0]['samaccountname']) + adminpass = samba.generate_random_password(12, 32) logger.info("Setting %s password in backup to: %s" % (username, adminpass)) logger.info("Run 'samba-tool user setpassword %s' after restoring DB" % username) - samdb.setpassword("(&(objectClass=user)(sAMAccountName=%s))" - % ldb.binary_encode(username), adminpass, - force_change_at_next_login=False, + samdb.setpassword(search_expr, adminpass, force_change_at_next_login=False, username=username) @@ -244,7 +253,7 @@ class cmd_domain_backup_online(samba.netcmd.Command): # ensure the admin user always has a password set (same as provision) if no_secrets: - set_admin_password(logger, samdb, creds.get_username()) + set_admin_password(logger, samdb) # Add everything in the tmpdir to the backup tar file backup_file = backup_filepath(targetdir, realm, time_str) @@ -756,7 +765,7 @@ class cmd_domain_backup_rename(samba.netcmd.Command): # ensure the admin user always has a password set (same as provision) if no_secrets: - set_admin_password(logger, samdb, creds.get_username()) + set_admin_password(logger, samdb) # Add everything in the tmpdir to the backup tar file backup_file = backup_filepath(targetdir, new_dns_realm, time_str) -- 2.7.4