Attachment 14538 Details for Bug 13621 – Backport of fixes for 4.9

Backport of fixes for 4.9

v4.9-backport-patch.txt (text/plain), 35.89 KB, created by Tim Beale on 2018-10-24 01:14:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Beale

Created: 2018-10-24 01:14:23 UTC

Size: 35.89 KB

patch

obsolete

>From 5c93f464726112a686790e04c6df6b9ce634c0a0 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Mon, 17 Sep 2018 15:36:21 +1200
>Subject: [PATCH 01/10] netcmd: Add --site option when restoring a domain
>
>Restoring a backup only worked if the Default-First-Site-Name site was
>still present. When the new restored DC account is created, it was
>trying to add the new server's DN under CN=Default-First-Site-Name.
>However, if the original domain was setup using a different site, then
>the restore would fail because the DN didn't exist.
>
>When running the restore command, you should be able to specify the
>site that you want the new/restored DC to be in (same as during a
>DC 'join'). Passing the correct --site argument is one way to avoid
>this problem. (A subsequent patch will further improve the tool so it
>can work around non-default sites automatically).
>
>Note we also need to pass the site through to where the new DNS entries
>get registered (in the rename case).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit e1f255a4d54b59924295ea875fdef62ccebb8811)
>---
> python/samba/netcmd/domain_backup.py | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
>diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py
>index 5f18e81..bdcac2c 100644
>--- a/python/samba/netcmd/domain_backup.py
>+++ b/python/samba/netcmd/domain_backup.py
>@@ -35,7 +35,7 @@ from samba.netcmd import Option, CommandError
> from samba.dcerpc import misc, security
> from samba import Ldb
> from fsmo import cmd_fsmo_seize
>-from samba.provision import make_smbconf
>+from samba.provision import make_smbconf, DEFAULTSITE
> from samba.upgradehelpers import update_krbtgt_account_password
> from samba.remove_dc import remove_dc
> from samba.provision import secretsdb_self_join
>@@ -289,6 +289,7 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>                help="set IPv4 ipaddress"),
>         Option("--host-ip6", type="string", metavar="IP6ADDRESS",
>                help="set IPv6 ipaddress"),
>+        Option("--site", help="Site to add the new server in", type=str),
>     ]
> 
>     takes_optiongroups = {
>@@ -297,7 +298,7 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>     }
> 
>     def register_dns_zone(self, logger, samdb, lp, ntdsguid, host_ip,
>-                          host_ip6):
>+                          host_ip6, site):
>         '''
>         Registers the new realm's DNS objects when a renamed domain backup
>         is restored.
>@@ -324,7 +325,7 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
> 
>         # Add the DNS objects for the new realm (note: the backup clone already
>         # has the root server objects, so don't add them again)
>-        fill_dns_data_partitions(samdb, domainsid, names.sitename, domaindn,
>+        fill_dns_data_partitions(samdb, domainsid, site, domaindn,
>                                  forestdn, dnsdomain, dnsforest, hostname,
>                                  host_ip, host_ip6, domainguid, ntdsguid,
>                                  dnsadmins_sid, add_root=False)
>@@ -355,7 +356,8 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>         samdb.transaction_commit()
> 
>     def run(self, sambaopts=None, credopts=None, backup_file=None,
>-            targetdir=None, newservername=None, host_ip=None, host_ip6=None):
>+            targetdir=None, newservername=None, host_ip=None, host_ip6=None,
>+            site=DEFAULTSITE):
>         if not (backup_file and os.path.exists(backup_file)):
>             raise CommandError('Backup file not found.')
>         if targetdir is None:
>@@ -407,7 +409,7 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>         ncs = [str(r) for r in res[0].get('namingContexts')]
> 
>         creds = credopts.get_credentials(lp)
>-        ctx = DCJoinContext(logger, creds=creds, lp=lp,
>+        ctx = DCJoinContext(logger, creds=creds, lp=lp, site=site,
>                             forced_local_samdb=samdb,
>                             netbios_name=newservername)
>         ctx.nc_list = ncs
>@@ -445,7 +447,7 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>         # know the new DC's IP address)
>         if is_rename:
>             self.register_dns_zone(logger, samdb, lp, ctx.ntds_guid,
>-                                   host_ip, host_ip6)
>+                                   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)
>-- 
>2.7.4
>
>
>From 8666cda1bdeea6e3b8a9e57ff1a87a8d3fe09322 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 18 Sep 2018 17:23:48 +1200
>Subject: [PATCH 02/10] tests: Add test-case for restore into non-default site
>
>Add a test-case that exercises the new '--site' restore option and
>ensures the restored DC gets added to the correct site.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit ad69aaf7e13435111fc990954ff0bc81ed5325c5)
>---
> python/samba/tests/domain_backup.py | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
>diff --git a/python/samba/tests/domain_backup.py b/python/samba/tests/domain_backup.py
>index 2df360f..9ea0106 100644
>--- a/python/samba/tests/domain_backup.py
>+++ b/python/samba/tests/domain_backup.py
>@@ -27,6 +27,7 @@ from samba.auth import system_session
> from samba import Ldb, dn_from_dns_name
> from samba.netcmd.fsmo import get_fsmo_roleowner
> import re
>+from samba import sites
> 
> 
> def get_prim_dom(secrets_path, lp):
>@@ -148,6 +149,32 @@ class DomainBackupBase(SambaToolCmdTest, TestCaseInTempDir):
>         # assert that we don't find user secrets in the DB
>         self.check_restored_database(lp, expect_secrets=False)
> 
>+    def _test_backup_restore_into_site(self):
>+        """Does a backup and restores into a non-default site"""
>+
>+        # create a new non-default site
>+        sitename = "Test-Site-For-Backups"
>+        sites.create_site(self.ldb, self.ldb.get_config_basedn(), sitename)
>+        self.addCleanup(sites.delete_site, self.ldb,
>+                        self.ldb.get_config_basedn(), sitename)
>+
>+        # restore the backup DC into the site we just created
>+        backup_file = self.create_backup()
>+        self.restore_backup(backup_file, ["--site=" + sitename])
>+
>+        lp = self.check_restored_smbconf()
>+        restored_ldb = self.check_restored_database(lp)
>+
>+        # check the restored DC was added to the site we created, i.e. there's
>+        # an entry matching the new DC sitting underneath the site DN
>+        site_dn = "CN={0},CN=Sites,{1}".format(sitename,
>+                                               restored_ldb.get_config_basedn())
>+        match_server = "(&(objectClass=server)(cn={0}))".format(self.new_server)
>+        res = restored_ldb.search(site_dn, scope=ldb.SCOPE_SUBTREE,
>+                                  expression=match_server)
>+        self.assertTrue(len(res) == 1,
>+                        "Failed to find new DC under site")
>+
>     def create_smbconf(self, settings):
>         """Creates a very basic smb.conf to pass to the restore tool"""
> 
>@@ -372,6 +399,9 @@ class DomainBackupOnline(DomainBackupBase):
>     def test_backup_restore_no_secrets(self):
>         self._test_backup_restore_no_secrets()
> 
>+    def test_backup_restore_into_site(self):
>+        self._test_backup_restore_into_site()
>+
> 
> class DomainBackupRename(DomainBackupBase):
> 
>@@ -399,6 +429,9 @@ class DomainBackupRename(DomainBackupBase):
>     def test_backup_restore_no_secrets(self):
>         self._test_backup_restore_no_secrets()
> 
>+    def test_backup_restore_into_site(self):
>+        self._test_backup_restore_into_site()
>+
>     def test_backup_invalid_args(self):
>         """Checks that rename commands with invalid args are rejected"""
> 
>-- 
>2.7.4
>
>
>From 6e11ad058d9717a1e298dd6bd88e2e02d0ed3185 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 18 Sep 2018 14:54:51 +1200
>Subject: [PATCH 03/10] netcmd: Re-create default site for backup-restore (if
> missing)
>
>Normally when a new DC joins a domain, samba-tool works out the new
>DC's site automatically. However, it does this by querying the existing
>DC using CLDAP. In the restore case, there is no DC running. We could
>still query the DB on disk and work out the correct site based on the
>new DC's IP, however:
>- comparing between the CN=Subnet DNs and an IP-address string seems
>  like it'd be non-trivial to write, and
>- in the lab-domain rename case, chances are the user will want a
>  completely different subnet to what's already in the DB.
>
>The restore command now has a --site option so the user can specify an
>appropriate site for the restored DC. This patch makes the restore
>command work by default (i.e. without a --site option) even if the
>default Default-First-Site-Name doesn't exist. Basically the solution is
>to just check Default-First-Site-Name exists and create it if it
>doesn't. As the recommended workflow is to use the restored DC as a
>temporary seed that you'll later throw away, this approach seems
>acceptable. Subsequent DCs will then be joined to the running restored
>DC, so an appropriate site will be determined using CLDAP. The only
>side-effect is potentially an extra Site object.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit ce57a800c9bed7e6876cdc0baf3a2d5fdc879ecf)
>---
> python/samba/netcmd/domain_backup.py | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
>diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py
>index bdcac2c..d18ea60 100644
>--- a/python/samba/netcmd/domain_backup.py
>+++ b/python/samba/netcmd/domain_backup.py
>@@ -45,6 +45,7 @@ from samba.provision import guess_names, determine_host_ip, determine_host_ip6
> from samba.provision.sambadns import (fill_dns_data_partitions,
>                                       get_dnsadmins_sid,
>                                       get_domainguid)
>+from samba import sites
> 
> 
> # work out a SID (based on a free RID) to use when the domain gets restored.
>@@ -355,9 +356,23 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>         chk.check_database(controls=controls, attrs=attrs)
>         samdb.transaction_commit()
> 
>+    def create_default_site(self, samdb, logger):
>+        '''Creates the default site, if it doesn't already exist'''
>+
>+        sitename = DEFAULTSITE
>+        search_expr = "(&(cn={0})(objectclass=site))".format(sitename)
>+        res = samdb.search(samdb.get_config_basedn(), scope=ldb.SCOPE_SUBTREE,
>+                           expression=search_expr)
>+
>+        if len(res) == 0:
>+            logger.info("Creating default site '{0}'".format(sitename))
>+            sites.create_site(samdb, samdb.get_config_basedn(), sitename)
>+
>+        return sitename
>+
>     def run(self, sambaopts=None, credopts=None, backup_file=None,
>             targetdir=None, newservername=None, host_ip=None, host_ip6=None,
>-            site=DEFAULTSITE):
>+            site=None):
>         if not (backup_file and os.path.exists(backup_file)):
>             raise CommandError('Backup file not found.')
>         if targetdir is None:
>@@ -401,6 +416,13 @@ class cmd_domain_backup_restore(cmd_fsmo_seize):
>         samdb_path = os.path.join(private_dir, 'sam.ldb')
>         samdb = SamDB(url=samdb_path, session_info=system_session(), lp=lp)
> 
>+        if site is None:
>+            # There's no great way to work out the correct site to add the
>+            # restored DC to. By default, add it to Default-First-Site-Name,
>+            # creating the site if it doesn't already exist
>+            site = self.create_default_site(samdb, logger)
>+            logger.info("Adding new DC to site '{0}'".format(site))
>+
>         # Create account using the join_add_objects function in the join object
>         # We need namingContexts, account control flags, and the sid saved by
>         # the backup process.
>-- 
>2.7.4
>
>
>From 4ce24218d0521afb28c68cf75c83a4222789113a Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Tue, 18 Sep 2018 16:30:15 +1200
>Subject: [PATCH 04/10] selftest: Change backup testenvs to use non-default
> site
>
>Previously (i.e. up until the last patch) the backup/restore commands
>only worked if the Default-First-Site-Name site was present. If this
>site didn't exist, then the various restore testenvs would fail to
>start. This is now fixed, but this patch changes the backupfrom testenv
>so that it uses a non-default site. This will detect the problem if it
>is ever re-introduced.
>
>To do this we need to change provision_ad_dc() so the
>extra_provision_options can be specified as an argument. (Note that Perl
>treats undef the same as an empty array).
>
>By default, the restore will add the new DC into the
>Default-First-Site-Name site. This means the backupfromdc and restored
>testenvs will now have different sites, so we need to update the ldapcmp
>filters to exclude site-specific attributes.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit 696fa6a1e6c615a992a3016ff32405b864b62eec)
>---
> selftest/target/Samba4.pm               | 15 +++++++++------
> testprogs/blackbox/ldapcmp_restoredc.sh |  3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
>index 0095089..a21ada8 100755
>--- a/selftest/target/Samba4.pm
>+++ b/selftest/target/Samba4.pm
>@@ -1833,7 +1833,8 @@ sub read_config_h($)
> 
> sub provision_ad_dc($$$$$$)
> {
>-	my ($self, $prefix, $hostname, $domain, $realm, $smbconf_args) = @_;
>+	my ($self, $prefix, $hostname, $domain, $realm, $smbconf_args,
>+		$extra_provision_options) = @_;
> 
> 	my $prefix_abs = abs_path($prefix);
> 
>@@ -1944,7 +1945,6 @@ sub provision_ad_dc($$$$$$)
> 	copy = print1
> ";
> 
>-	my $extra_provision_options = undef;
> 	push (@{$extra_provision_options}, "--backend-store=mdb");
> 	print "PROVISIONING AD DC...\n";
> 	my $ret = $self->provision($prefix,
>@@ -2508,7 +2508,7 @@ sub setup_ad_dc
> 	}
> 
> 	my $env = $self->provision_ad_dc($path, "addc", "ADDOMAIN",
>-					 "addom.samba.example.com", "");
>+					 "addom.samba.example.com", "", undef);
> 	unless ($env) {
> 		return undef;
> 	}
>@@ -2535,7 +2535,7 @@ sub setup_ad_dc_no_nss
> 	}
> 
> 	my $env = $self->provision_ad_dc($path, "addc_no_nss", "ADNONSSDOMAIN",
>-					 "adnonssdom.samba.example.com", "");
>+					 "adnonssdom.samba.example.com", "", undef);
> 	unless ($env) {
> 		return undef;
> 	}
>@@ -2566,7 +2566,7 @@ sub setup_ad_dc_no_ntlm
> 
> 	my $env = $self->provision_ad_dc($path, "addc_no_ntlm", "ADNONTLMDOMAIN",
> 					 "adnontlmdom.samba.example.com",
>-					 "ntlm auth = disabled");
>+					 "ntlm auth = disabled", undef);
> 	unless ($env) {
> 		return undef;
> 	}
>@@ -2597,8 +2597,11 @@ sub setup_backupfromdc
> 	       return "UNKNOWN";
> 	}
> 
>+	my $provision_args = ["--site=Backup-Site"];
>+
> 	my $env = $self->provision_ad_dc($path, "backupfromdc", "BACKUPDOMAIN",
>-					 "backupdom.samba.example.com", "");
>+					 "backupdom.samba.example.com", "",
>+					 $provision_args);
> 	unless ($env) {
> 		return undef;
> 	}
>diff --git a/testprogs/blackbox/ldapcmp_restoredc.sh b/testprogs/blackbox/ldapcmp_restoredc.sh
>index 51951ba..d7a51ae 100755
>--- a/testprogs/blackbox/ldapcmp_restoredc.sh
>+++ b/testprogs/blackbox/ldapcmp_restoredc.sh
>@@ -55,6 +55,9 @@ ldapcmp_with_orig() {
>     # these are just differences between provisioning a domain and joining a DC
>     IGNORE_ATTRS="$IGNORE_ATTRS,localPolicyFlags,operatingSystem,displayName"
> 
>+    # the restored DC may use a different side compared to the original DC
>+    IGNORE_ATTRS="$IGNORE_ATTRS,serverReferenceBL,msDS-IsDomainFor"
>+
>     LDAPCMP_CMD="$PYTHON $BINDIR/samba-tool ldapcmp"
>     $LDAPCMP_CMD $DB1_PATH $DB2_PATH --two --filter=$IGNORE_ATTRS $BASE_DN_OPTS
> }
>-- 
>2.7.4
>
>
>From f61e6c20654d69845ddc6d9ff071a7a917d12628 Mon Sep 17 00:00:00 2001
>From: David Mulder <dmulder@suse.com>
>Date: Thu, 28 Jun 2018 09:01:59 -0600
>Subject: [PATCH 05/10] python: Allow forced signing via smb.SMB()
>
>Signed-off-by: David Mulder <dmulder@suse.com>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit 4c7348e44d10ca519dd1322fd40b12c69e17a8e6)
>
>Back-ported as a dependency required for:
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>---
> source4/libcli/pysmb.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/source4/libcli/pysmb.c b/source4/libcli/pysmb.c
>index a53e30b..45ff9a0 100644
>--- a/source4/libcli/pysmb.c
>+++ b/source4/libcli/pysmb.c
>@@ -601,7 +601,7 @@ static PyObject *py_smb_new(PyTypeObject *type, PyObject *args, PyObject *kwargs
> 	PyObject *py_creds = Py_None;
> 	PyObject *py_lp = Py_None;
> 	const char *kwnames[] = { "hostname", "service", "creds", "lp",
>-				  "ntlmv2_auth", "use_spnego", NULL };
>+				  "ntlmv2_auth", "use_spnego", "sign", NULL };
> 	const char *hostname = NULL;
> 	const char *service = NULL;
> 	PyObject *smb;
>@@ -612,11 +612,12 @@ static PyObject *py_smb_new(PyTypeObject *type, PyObject *args, PyObject *kwargs
> 	struct smbcli_session_options session_options;
> 	uint8_t ntlmv2_auth = 0xFF;
> 	uint8_t use_spnego = 0xFF;
>+	PyObject *sign = Py_False;
> 
>-	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "zz|OObb",
>+	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "zz|OObbO",
> 					 discard_const_p(char *, kwnames),
> 					 &hostname, &service, &py_creds, &py_lp,
>-					 &ntlmv2_auth, &use_spnego)) {
>+					 &ntlmv2_auth, &use_spnego, &sign)) {
> 		return NULL;
> 	}
> 
>@@ -658,6 +659,9 @@ static PyObject *py_smb_new(PyTypeObject *type, PyObject *args, PyObject *kwargs
> 	if (use_spnego != 0xFF) {
> 		options.use_spnego = use_spnego;
> 	}
>+	if (PyObject_IsTrue(sign)) {
>+		options.signing = SMB_SIGNING_REQUIRED;
>+	}
> 
> 	status = do_smb_connect(spdata, spdata, hostname, service,
> 				&options,
>-- 
>2.7.4
>
>
>From 6cad13a435cbc7a6c171eba9079d4401bf382946 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Wed, 26 Sep 2018 17:01:03 +1200
>Subject: [PATCH 06/10] netcmd: Make sure SMB connection is signed when backing
> up sysvol
>
>i.e. protect the client against man-in-the-middle attacks by default.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit 0122f45f053ecc545950c31bf1fb33fba143478c)
>---
> python/samba/netcmd/domain_backup.py | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/python/samba/netcmd/domain_backup.py b/python/samba/netcmd/domain_backup.py
>index d18ea60..bbc7224 100644
>--- a/python/samba/netcmd/domain_backup.py
>+++ b/python/samba/netcmd/domain_backup.py
>@@ -239,7 +239,7 @@ class cmd_domain_backup_online(samba.netcmd.Command):
> 
>         # Grab the remote DC's sysvol files and bundle them into a tar file
>         sysvol_tar = os.path.join(tmpdir, 'sysvol.tar.gz')
>-        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds)
>+        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds, sign=True)
>         backup_online(smb_conn, sysvol_tar, remote_sam.get_domain_sid())
> 
>         # remove the default sysvol files created by the clone (we want to
>@@ -762,7 +762,7 @@ class cmd_domain_backup_rename(samba.netcmd.Command):
>         # use the old realm) backed here, as well as default files generated
>         # for the new realm as part of the clone/join.
>         sysvol_tar = os.path.join(tmpdir, 'sysvol.tar.gz')
>-        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds)
>+        smb_conn = smb.SMB(server, "sysvol", lp=lp, creds=creds, sign=True)
>         backup_online(smb_conn, sysvol_tar, remote_sam.get_domain_sid())
> 
>         # connect to the local DB (making sure we use the new/renamed config)
>-- 
>2.7.4
>
>
>From 8ed66769cd18a448e4373a09164e78e673972790 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Thu, 27 Sep 2018 09:46:41 +1200
>Subject: [PATCH 07/10] s3/smbd: Server responds incorrectly if no SMB protocol
> chosen
>
>The SMBnegprot response from the server contains the DialectIndex of the
>selected protocol from the client's request message. Currently, if no
>protocol is selected, the server is responding with a DialectIndex=zero,
>which is a valid index (PROTOCOL_CORE by default). The Windows spec, and
>historically the code, should return DialectIndex=0xffff if no protocol
>is chosen. The following commit changed it recently (presumably
>inadvertently), so that it now returns DialectIndex=zero.
>
>06940155f315529c5b5 s3:smbd: Fix size types in reply_negprot()
>
>This results in somewhat confusing error messages on the client side:
>ERROR(runtime): uncaught exception - (3221225997, 'The transport
>connection has been reset.')
>
>or, when signing is configured as mandatory:
>smbXcli_negprot: SMB signing is mandatory and the selected protocol
>level (1) doesn't support it.
>ERROR(runtime): uncaught exception - (3221225506, '{Access Denied} A
>process has requested access to an object but has not been granted those
>access rights.')
>
>This patch restores the old behaviour of returning 0xffff.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Pair-Programmed-With: Ralph Boehme <slow@samba.org>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>(cherry picked from commit 378706266496ce79c1887fe96ab3b15f56770244)
>---
> source3/smbd/negprot.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/source3/smbd/negprot.c b/source3/smbd/negprot.c
>index 27366ea..2d5edf1 100644
>--- a/source3/smbd/negprot.c
>+++ b/source3/smbd/negprot.c
>@@ -28,6 +28,13 @@
> #include "auth/gensec/gensec.h"
> #include "../libcli/smb/smb_signing.h"
> 
>+/*
>+ * MS-CIFS, 2.2.4.52.2 SMB_COM_NEGOTIATE Response:
>+ * If the server does not support any of the listed dialects, it MUST return a
>+ * DialectIndex of 0XFFFF
>+ */
>+#define NO_PROTOCOL_CHOSEN	0xffff
>+
> extern fstring remote_proto;
> 
> static void get_challenge(struct smbXsrv_connection *xconn, uint8_t buff[8])
>@@ -748,7 +755,7 @@ void reply_negprot(struct smb_request *req)
> 
> 		DBG_NOTICE("No protocol supported !\n");
> 		reply_outbuf(req, 1, 0);
>-		SSVAL(req->outbuf, smb_vwv0, choice);
>+		SSVAL(req->outbuf, smb_vwv0, NO_PROTOCOL_CHOSEN);
> 
> 		ok = srv_send_smb(xconn, (char *)req->outbuf,
> 				  false, 0, false, NULL);
>-- 
>2.7.4
>
>
>From d121241ca987e25ed25d5bb3a8f95c711f9e5230 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Thu, 27 Sep 2018 09:53:24 +1200
>Subject: [PATCH 08/10] libcli: Add debug message if fail to negoatiate SMB
> protocol
>
>Currently if the client and server can't negotiate an SMB protocol, you
>just get the followiing error on the client-side, which doesn't tell you
>much.
>ERROR(runtime): uncaught exception - (3221225667, 'The network responded
>incorrectly.')
>
>This patch adds a debug message to help highlight what's actually going
>wrong.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13621
>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Ralph Boehme <slow@samba.org>
>Reviewed-by: Andrew Bartlett <abartlet@samba.org>
>
>Autobuild-User(master): Andrew Bartlett <abartlet@samba.org>
>Autobuild-Date(master): Fri Sep 28 11:25:29 CEST 2018 on sn-devel-144
>
>(cherry picked from commit 34cbd89fec836f5de0cb5ba3f289b1f4ae00c5d7)
>---
> libcli/smb/smbXcli_base.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
>index ad1b67b..d94b4d8 100644
>--- a/libcli/smb/smbXcli_base.c
>+++ b/libcli/smb/smbXcli_base.c
>@@ -4369,6 +4369,7 @@ static void smbXcli_negprot_smb1_done(struct tevent_req *subreq)
> 	}
> 
> 	if (conn->protocol == PROTOCOL_NONE) {
>+		DBG_ERR("No compatible protocol selected by server.\n");
> 		tevent_req_nterror(req, NT_STATUS_INVALID_NETWORK_RESPONSE);
> 		return;
> 	}
>-- 
>2.7.4
>
>
>From ac125fb772e4e541111ae928642f45128b4fcf62 Mon Sep 17 00:00:00 2001
>From: Tim Beale <timbeale@catalyst.net.nz>
>Date: Fri, 28 Sep 2018 12:35:35 +1200
>Subject: [PATCH 09/10] 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@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>(cherry picked from commit dec3eda1f74f5bf7ea91c1be3d5dfd832e9672b9)
>---
> selftest/knownfail.d/attr_from_server         |   5 +
> source4/dsdb/tests/python/attr_from_server.py | 150 ++++++++++++++++++++++++++
> source4/selftest/tests.py                     |   5 +
> 3 files changed, 160 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..1a0112d
>--- /dev/null
>+++ b/source4/dsdb/tests/python/attr_from_server.py
>@@ -0,0 +1,150 @@
>+# -*- 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,{0}".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={0},{1}".format(os.environ["SERVER"], servers_dn)
>+        ntds_conn = "CN=Test-NTDS-Conn,CN=NTDS Settings,{0}".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,{0}".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 = int(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 3a07eee..ce13c5a 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -144,6 +144,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 05538e2a55fc00832672e56103df852150be7ff2 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 12 Sep 2018 14:48:04 -0500
>Subject: [PATCH 10/10] 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@samba.org>
>Signed-off-by: Tim Beale <timbeale@catalyst.net.nz>
>Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>(cherry picked from commit 4092b369aeeb7058d78b8d6f41dbbc6d69203ecc)
>---
> 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
>

Flags:

timbeale: review? (gary)
gary: review+

Actions: View

Attachments on bug 13621: 14538