[PATCH] Fix credentials for domain backup commands (Bug 13566)
Tim Beale
timbeale at catalyst.net.nz
Thu Aug 9 21:53:46 UTC 2018
Hi,
These patches fix some problems I noticed with the new domain backup
commands. Basically, the commands would only ever work if you specified
the administrator's password on the command line. Using credential
options like --kerberos=yes didn't work.
Clean CI run here:
https://gitlab.com/catalyst-samba/samba/pipelines/27488840
Thanks,
Tim
-------------- next part --------------
From f07cc37a7c2f89c5391011d11bdb7f2771e514c5 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 9 Aug 2018 15:30:55 +1200
Subject: [PATCH 1/4] 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 <timbeale at catalyst.net.nz>
---
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 05146c0..c251bc6 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -156,10 +156,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:
@@ -218,12 +218,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
@@ -686,8 +686,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()
@@ -701,8 +704,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 7fbf3c070d523c6146fe1f045be724f41baa0192 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 9 Aug 2018 15:34:51 +1200
Subject: [PATCH 2/4] 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 <timbeale at catalyst.net.nz>
---
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 c251bc6..cb00e46 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -158,10 +158,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 e5721754952d97ca78e25f2da4bf21dee2f2d558 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 9 Aug 2018 15:35:59 +1200
Subject: [PATCH 3/4] 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 <timbeale at catalyst.net.nz>
---
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 cb00e46..cf2327e 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -156,14 +156,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):
@@ -218,7 +210,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)
@@ -686,7 +681,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 7eccf7dc7ab8a2ffdd491f05bddb59ae5bcead73 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Thu, 9 Aug 2018 16:20:10 +1200
Subject: [PATCH 4/4] 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).
Probably a better option is to assume the admin's username is
'Administrator' rather than use creds.get_username().
Either way, this is not a crucial step in the backup process (it's more
of a courtesy to ensure the admin always has a password set). So
add a sanity-check that we're resetting the password for a valid user,
and don't fail the backup if we're not.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13566
Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
---
python/samba/netcmd/domain_backup.py | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py
index cf2327e..d4ae3d4 100644
--- a/python/samba/netcmd/domain_backup.py
+++ b/python/samba/netcmd/domain_backup.py
@@ -157,17 +157,31 @@ 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):
+# randomly-generated value. This is similar to the provision behaviour.
+def set_admin_password(logger, samdb):
"""Sets a randomly generated password for the backup DB's admin user"""
+ # assume the admin account is the same as what got created in the provision
+ username = "Administrator"
+
+ match_user = "(sAMAccountName={}))".format(ldb.binary_encode(username))
+ search_expr = "(&(objectClass=user){})".format(match_user)
+
+ # sanity-check the admin user exists. Setting the admin's password here is
+ # not strictly necessary (it's more of a courtesy), so the user can always
+ # do this step themselves, when they restore the backup
+ res = samdb.search(base=samdb.domain_dn(), scope=ldb.SCOPE_SUBTREE,
+ expression=search_expr)
+ if len(res) == 0:
+ logger.info("No {} user found in backup DB. "
+ "Please reset admin password manually".format(username))
+ return
+
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)
@@ -250,7 +264,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)
@@ -762,7 +776,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
More information about the samba-technical
mailing list