[PATCH] Create a 'binddns dir' for files used by the bind_dlz module and named

Andreas Schneider asn at samba.org
Wed Sep 6 09:27:38 UTC 2017


On Tuesday, 5 September 2017 12:31:20 CEST Andreas Schneider via samba-
technical wrote:
> On Monday, 4 September 2017 21:22:39 CEST Andrew Bartlett wrote:
> > Andreas,
> 
> Andrew,
> 
> > I know this won't make you very happy, but I think this is a 4.8 patch
> > at this point.  You can of course patch Fedora packages, but I fear
> > further dragons, given the fight it has given so far, and while parts
> > of the DLZ mode are tested (thankfully!) the whole integration is not
> > verified in make test.

Hi,

> would you accept it if Marc and I would do manual testing and fix remaining
> issues.
> 
> This means upgrading from 4.6 to 4.7 with my patchset and check if it works
> seemlessly.
> Switching backends etc.
> 
> I do not feel very happy with the current code and giving named broad access
> to keytab and AD partitions.
> 
> Also when switching from bind_dlz to the internal DNS we should remove files
> which give the named group full access to AD.

Jeremy pushed the last patchset from this thread to master.

Yesterday I asked Marc for help. He tested the feature and we discussed 
several aspects, especially security concerns, like file and directory 
permission.
We fixed some issues we found during extensive testing and we improved the 
messages samba-tool and samba_upgradedns print so that the user knows what he 
has to do.
We also found some things we need to fix in the documentation in the wiki.


The attached patchset addresses the remaining issues. Marc will answer and add 
the test plan we created and he followed.

I hope this gives you the confidence in the changes that we can include them 
in 4.7.

Please review carefully.


Thanks,



	Andreas


P.S.: The internel DNS server doesn't work in 4.7 and master
      https://bugzilla.samba.org/show_bug.cgi?id=13019

-- 
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             asn at samba.org
www.samba.org
-------------- next part --------------
>From 29d573d0149c2097a4f4103780a308f49e32f2a4 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 5 Sep 2017 14:18:44 +0200
Subject: [PATCH 1/9] wafsamba: Do not chmod already existing dirs on install

This might break backward compatibility.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 buildtools/wafsamba/wafsamba.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/buildtools/wafsamba/wafsamba.py b/buildtools/wafsamba/wafsamba.py
index 57913af2bd7..f91adca1a0c 100644
--- a/buildtools/wafsamba/wafsamba.py
+++ b/buildtools/wafsamba/wafsamba.py
@@ -900,8 +900,6 @@ def INSTALL_DIR(bld, path, chmod=0o755):
             except OSError, e:
                 if not os.path.isdir(path):
                     raise Utils.WafError("Cannot create the folder '%s' (error: %s)" % (path, e))
-        else:
-            os.chmod(path, chmod)
 Build.BuildContext.INSTALL_DIR = INSTALL_DIR
 
 def INSTALL_DIRS(bld, destdir, dirs, chmod=0o755):
-- 
2.14.1


>From f7c9dcd2a6efa385196528dd023c4bd3465b9aeb Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 5 Sep 2017 20:36:47 +0200
Subject: [PATCH 2/9] samba:provision: Give a hint to copy the krb5.conf and
 not symlink it

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 python/samba/provision/__init__.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/python/samba/provision/__init__.py b/python/samba/provision/__init__.py
index f820f6ab675..8a8c033105f 100644
--- a/python/samba/provision/__init__.py
+++ b/python/samba/provision/__init__.py
@@ -2200,6 +2200,9 @@ def provision(logger, session_info, smbconf=None,
                          realm=names.realm)
         logger.info("A Kerberos configuration suitable for Samba AD has been "
                     "generated at %s", paths.krb5conf)
+        logger.info("Merge the contents of this file with your system "
+                    "krb5.conf or replace it with this one. Do not create a "
+                    "symlink!")
 
         if serverrole == "active directory domain controller":
             create_dns_update_list(lp, logger, paths)
-- 
2.14.1


>From a224c9078e96dc101f9ca1fb586d584f5552b6c4 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Thu, 10 Aug 2017 15:04:08 +0200
Subject: [PATCH 3/9] dynconfig: Fix location of the default 'binddns dir'

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 dynconfig/wscript | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/dynconfig/wscript b/dynconfig/wscript
index fee37eaaf5f..54977e42bd4 100644
--- a/dynconfig/wscript
+++ b/dynconfig/wscript
@@ -174,6 +174,12 @@ dynconfig = {
          'OPTION':    '--with-privatedir',
          'HELPTEXT':  'Where to put sam.ldb and other private files',
     },
+    'BINDDNS_DIR' : {
+         'STD-PATH':  '${PREFIX}/bind-dns',
+         'FHS-PATH':  '${LOCALSTATEDIR}/lib/samba/bind-dns',
+         'OPTION':    '--with-bind-dns-dir',
+         'HELPTEXT':  'bind-dns config directory',
+    },
     'LOCKDIR' : {
          'STD-PATH':  '${LOCALSTATEDIR}/lock',
          'FHS-PATH':  '${LOCALSTATEDIR}/lock/samba',
@@ -192,12 +198,6 @@ dynconfig = {
          'OPTION':    '--with-statedir',
          'HELPTEXT':  'Where to put persistent state files',
     },
-    'BINDDNS_DIR' : {
-         'STD-PATH':  '${LOCALSTATEDIR}/lib',
-         'FHS-PATH':  '${LOCALSTATEDIR}/lib/samba/bind-dns',
-         'OPTION':    '--with-bind-dns-dir',
-         'HELPTEXT':  'bind-dns config directory',
-    },
     'CACHEDIR' : {
          'STD-PATH':  '${LOCALSTATEDIR}/cache',
          'FHS-PATH':  '${LOCALSTATEDIR}/cache/samba',
-- 
2.14.1


>From 9e5f0a6ee214a6600de1f4ffd338b55903f4f1a0 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 22 Aug 2017 17:10:01 +0200
Subject: [PATCH 4/9] s4:bind_dlz: Try the 'binddns dir' first

The directory is normally empty if you did not provision or call
samba_upgradedns for the bind_dlz module.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source4/dns_server/dlz_bind9.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/source4/dns_server/dlz_bind9.c b/source4/dns_server/dlz_bind9.c
index 75a9ce0f648..5079257c9fe 100644
--- a/source4/dns_server/dlz_bind9.c
+++ b/source4/dns_server/dlz_bind9.c
@@ -682,9 +682,9 @@ _PUBLIC_ isc_result_t dlz_create(const char *dlzname,
 	}
 
 	if (state->options.url == NULL) {
-		state->options.url = lpcfg_private_path(state,
-							state->lp,
-							"dns/sam.ldb");
+		state->options.url = talloc_asprintf(state,
+						     "%s/dns/sam.ldb",
+						     lpcfg_binddns_dir(state->lp));
 		if (state->options.url == NULL) {
 			result = ISC_R_NOMEMORY;
 			goto failed;
@@ -693,7 +693,7 @@ _PUBLIC_ isc_result_t dlz_create(const char *dlzname,
 		if (!file_exist(state->options.url)) {
 			state->options.url = talloc_asprintf(state,
 							     "%s/dns/sam.ldb",
-							     lpcfg_binddns_dir(state->lp));
+							     lpcfg_private_dir(state->lp));
 			if (state->options.url == NULL) {
 				result = ISC_R_NOMEMORY;
 				goto failed;
@@ -1324,7 +1324,7 @@ _PUBLIC_ isc_boolean_t dlz_ssumatch(const char *signer, const char *name, const
 
 	keytab_file = talloc_asprintf(tmp_ctx,
 				      "%s/dns.keytab",
-				      lpcfg_private_dir(state->lp));
+				      lpcfg_binddns_dir(state->lp));
 	if (keytab_file == NULL) {
 		state->log(ISC_LOG_ERROR, "samba_dlz: Out of memory!");
 		talloc_free(tmp_ctx);
@@ -1334,7 +1334,7 @@ _PUBLIC_ isc_boolean_t dlz_ssumatch(const char *signer, const char *name, const
 	if (!file_exist(keytab_file)) {
 		keytab_file = talloc_asprintf(tmp_ctx,
 					      "%s/dns.keytab",
-					      lpcfg_binddns_dir(state->lp));
+					      lpcfg_private_dir(state->lp));
 		if (keytab_file == NULL) {
 			state->log(ISC_LOG_ERROR, "samba_dlz: Out of memory!");
 			talloc_free(tmp_ctx);
-- 
2.14.1


>From b3c7789401829da3c7b59f36dbe1e10f0be2d4e0 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 6 Sep 2017 07:23:57 +0200
Subject: [PATCH 5/9] python:provision: Change the group of the 'binddns dir'
 too

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 python/samba/provision/__init__.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/python/samba/provision/__init__.py b/python/samba/provision/__init__.py
index 8a8c033105f..07c24795477 100644
--- a/python/samba/provision/__init__.py
+++ b/python/samba/provision/__init__.py
@@ -2238,6 +2238,14 @@ def provision(logger, session_info, smbconf=None,
 
         # chown the dns.keytab in the bind-dns directory
         if paths.bind_gid is not None:
+            try:
+                os.chmod(paths.binddns_dir, 0770)
+                os.chown(paths.binddns_dir, -1, paths.bind_gid)
+            except OSError:
+                if not os.environ.has_key('SAMBA_SELFTEST'):
+                    logger.info("Failed to chown %s to bind gid %u",
+                                paths.binddns_dir, paths.bind_gid)
+
             try:
                 os.chmod(bind_dns_keytab_path, 0640)
                 os.chown(bind_dns_keytab_path, -1, paths.bind_gid)
-- 
2.14.1


>From d112febcb782a23ed988ff3b8b2207e2bb5b5028 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 6 Sep 2017 07:25:04 +0200
Subject: [PATCH 6/9] python:provision: Do not change the owner of the
 sam.ldb.d dir

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 python/samba/provision/sambadns.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/python/samba/provision/sambadns.py b/python/samba/provision/sambadns.py
index d4cb93a89ea..c95583162e7 100644
--- a/python/samba/provision/sambadns.py
+++ b/python/samba/provision/sambadns.py
@@ -864,9 +864,6 @@ def create_samdb_copy(samdb, logger, paths, names, domainsid, domainguid):
     # Give bind read/write permissions dns partitions
     if paths.bind_gid is not None:
         try:
-            os.chown(samldb_dir, -1, paths.bind_gid)
-            os.chmod(samldb_dir, 0750)
-
             for dirname, dirs, files in os.walk(dns_dir):
                 for d in dirs:
                     dpath = os.path.join(dirname, d)
-- 
2.14.1


>From 9f65bdae0a398ce9aff09e771303f932bcc2509c Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 6 Sep 2017 10:06:40 +0200
Subject: [PATCH 7/9] samba_upgradedns: Change the group of the 'binddns dir'
 too

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source4/scripting/bin/samba_upgradedns | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/source4/scripting/bin/samba_upgradedns b/source4/scripting/bin/samba_upgradedns
index 2582da0f6bc..db3ef5c6d65 100755
--- a/source4/scripting/bin/samba_upgradedns
+++ b/source4/scripting/bin/samba_upgradedns
@@ -505,6 +505,13 @@ if __name__ == '__main__':
 
             # chown the dns.keytab in the bind-dns directory
             if paths.bind_gid is not None:
+                try:
+                    os.chmod(paths.binddns_dir, 0o770)
+                    os.chown(paths.binddns_dir, -1, paths.bind_gid)
+                except OSError:
+                    if not os.environ.has_key('SAMBA_SELFTEST'):
+                        logger.info("Failed to chown %s to bind gid %u",
+                                    paths.binddns_dir, paths.bind_gid)
                 try:
                     os.chmod(bind_dns_keytab_path, 0640)
                     os.chown(bind_dns_keytab_path, -1, paths.bind_gid)
-- 
2.14.1


>From 3a7e77800384d9651c7efa2dd712949220252a75 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Wed, 6 Sep 2017 07:25:40 +0200
Subject: [PATCH 8/9] samba_upgradedns: Print better hints after we migrated
 the config

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source4/scripting/bin/samba_upgradedns | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/source4/scripting/bin/samba_upgradedns b/source4/scripting/bin/samba_upgradedns
index db3ef5c6d65..3369bcfed93 100755
--- a/source4/scripting/bin/samba_upgradedns
+++ b/source4/scripting/bin/samba_upgradedns
@@ -442,6 +442,12 @@ if __name__ == '__main__':
 
     # Special stuff for DLZ backend
     if opts.dns_backend == "BIND9_DLZ":
+        config_migration = False
+
+        if (paths.private_dir != paths.binddns_dir and
+            os.path.isfile(os.path.join(paths.private_dir, "named.conf"))):
+            config_migration = True
+
         # Check if dns-HOSTNAME account exists and create it if required
         secrets_msgs = ldbs.secrets.search(expression='(samAccountName=dns-%s)' % hostname, attrs=['secret'])
         msg = ldbs.sam.search(base=domaindn, scope=ldb.SCOPE_DEFAULT,
@@ -537,9 +543,15 @@ if __name__ == '__main__':
 
         cleanup_obsolete_dns_files(paths)
 
-        logger.info("See %s for an example configuration include file for BIND", paths.namedconf)
-        logger.info("and %s for further documentation required for secure DNS "
-                    "updates", paths.namedtxt)
+        if config_migration:
+            logger.info("ATTENTION: The BIND configuration and keytab has been moved to: %s",
+                        paths.binddns_dir)
+            logger.info("           Please update your BIND configuration accordingly.")
+        else:
+            logger.info("See %s for an example configuration include file for BIND", paths.namedconf)
+            logger.info("and %s for further documentation required for secure DNS "
+                        "updates", paths.namedtxt)
+
     elif opts.dns_backend == "SAMBA_INTERNAL":
         # Check if dns-HOSTNAME account exists and delete it if required
         try:
-- 
2.14.1


>From ca8dc553acbdae49a2adbd387be9bd9cee48b485 Mon Sep 17 00:00:00 2001
From: Andreas Schneider <asn at samba.org>
Date: Tue, 5 Sep 2017 11:47:27 +0200
Subject: [PATCH 9/9] samba_upgradedns: When we setup the internal dns cleanup
 bind-dns dir

Make sure to remove everything from the bind-dns directory to avoid
possible security issues with the named group having write access to all
AD partions

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12957

Signed-off-by: Andreas Schneider <asn at samba.org>
---
 source4/scripting/bin/samba_upgradedns | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/source4/scripting/bin/samba_upgradedns b/source4/scripting/bin/samba_upgradedns
index 3369bcfed93..261d8a1922d 100755
--- a/source4/scripting/bin/samba_upgradedns
+++ b/source4/scripting/bin/samba_upgradedns
@@ -553,6 +553,23 @@ if __name__ == '__main__':
                         "updates", paths.namedtxt)
 
     elif opts.dns_backend == "SAMBA_INTERNAL":
+        # Make sure to remove everything from the bind-dns directory to avoid
+        # possible security issues with the named group having write access
+        # to all AD partions
+        cleanup_remove_file(os.path.join(paths.binddns_dir, "dns.keytab"))
+        cleanup_remove_file(os.path.join(paths.binddns_dir, "named.conf"))
+        cleanup_remove_file(os.path.join(paths.binddns_dir, "named.conf.update"))
+        cleanup_remove_file(os.path.join(paths.binddns_dir, "named.txt"))
+
+        cleanup_remove_dir(os.path.dirname(paths.dns))
+
+        try:
+            os.chmod(paths.private_dir, 0o700)
+            os.chown(paths.private_dir, -1, 0)
+        except:
+            logger.warn("Failed to restore owner and permissions for %s",
+                        (paths.private_dir))
+
         # Check if dns-HOSTNAME account exists and delete it if required
         try:
             dn_str = 'samAccountName=dns-%s,CN=Principals' % hostname
-- 
2.14.1



More information about the samba-technical mailing list