From 5c93f464726112a686790e04c6df6b9ce634c0a0 Mon Sep 17 00:00:00 2001 From: Tim Beale 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Ralph Boehme Reviewed-by: Andrew Bartlett (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 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 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett (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 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 Reviewed-by: Ralph Boehme Reviewed-by: Andrew Bartlett Autobuild-User(master): Andrew Bartlett 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 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 Reviewed-by: Douglas Bagnall Reviewed-by: Gary Lockyer (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 . +# +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 ") +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 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 Signed-off-by: Tim Beale Reviewed-by: Douglas Bagnall Reviewed-by: Gary Lockyer (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