[PATCH] Check idmap config with testparm

Jeremy Allison jra at samba.org
Mon Dec 12 19:18:03 UTC 2016


On Mon, Dec 12, 2016 at 10:15:47AM +0100, Andreas Schneider wrote:
> On Monday, 12 December 2016 09:37:07 CET Michael Adam wrote:
> > On 2016-12-08 at 10:46 -0800, Jeremy Allison wrote:
> > > On Thu, Dec 08, 2016 at 09:48:25AM +0100, Volker Lendecke wrote:
> > > > On Thu, Dec 08, 2016 at 07:58:40AM +0000, Rowland Penny wrote:
> > > > > Hi Volker, Could you explain for the idiots amongst us (i.e. me), just
> > > > > how this is supposed to work ?
> > > > 
> > > > The AD backend just reads the SFU attributes in Active Directory.
> > > > This is completely controlled by the administrator of the domains. I
> > > > have several customers with a global unix id allocation policy but
> > > > where the unix ids are spread across multiple domains in a more or
> > > > less random fashion. Globally unix ids are guaranteed to be unique,
> > > > but you can't tell from a range assignment which domain they belong
> > > > to. What winbind with overlapping ranges now does is just try all of
> > > > the domains until a mapping is found. There is no guarantee of a
> > > > particular order in which domains are tried, we depend on the AD
> > > > administration to gurantee uniqueness. This is for unixid2sid,
> > > > sid2unixid is simple, there we can just find the domain from the sid.
> > > 
> > > It's nice that we can do this, but I would certainly
> > > consider this under "Advanced" usage :-).
> > 
> > It may be, but if you think about it, for the AD backend
> > this may actually be a very common and reasonable config.
> > 
> > > For most regular users - a blanket "don't have overlapping
> > > ranges" would be very good advice !
> > 
> > True. But complaining with "ERROR" about a supported
> > (albeit advanced) config may not be the right thing..
> 
> Here is an updated patchset. For imapd_ad overlapping configs it will issue a 
> NOTE that they overlap, for all other combnation it will print an error.
> 
> 
> Michael and I also implemented that winbind fails if an invalid idmap default 
> backend is specified.
> 
> 
> If nobody pushes it earlier or objects, I will push it tomorrow.

../source3/winbindd/winbindd.c: In function ‘main’:
../source3/winbindd/winbindd.c:1699:8: error: ‘i’ undeclared (first use in this function)
   for (i = 0; i < ARRAY_SIZE(invalid_backends); i++) {
        ^
../source3/winbindd/winbindd.c:1699:8: note: each undeclared identifier is reported only once for each function it appears in
../source3/winbindd/winbindd.c:1700:9: error: declaration of ‘ok’ shadows a previous local [-Werror=shadow]
    bool ok;
         ^
../source3/winbindd/winbindd.c:1541:7: error: shadowed declaration is here [-Werror=shadow]
  bool ok;

Fixed it up and pushed.. :-).

> Thanks,
> 
> 
> 
> 	Andreas
> 
> 
> 
> -- 
> Andreas Schneider                   GPG-ID: CC014E3D
> Samba Team                             asn at samba.org
> www.samba.org

> From 1ed5ff38328f42478b4e8eafdb6782872de3df34 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 7 Dec 2016 17:03:22 +0100
> Subject: [PATCH 1/4] s3-testparm: Fix trailing whitespaces
> 
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Reviewed-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/utils/testparm.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index e1d66ce68e4..c330ad16959 100644
> --- a/source3/utils/testparm.c
> +++ b/source3/utils/testparm.c
> @@ -1,21 +1,21 @@
> -/* 
> +/*
>     Unix SMB/CIFS implementation.
>     Test validity of smb.conf
>     Copyright (C) Karl Auer 1993, 1994-1998
>  
>     Extensively modified by Andrew Tridgell, 1995
>     Converted to popt by Jelmer Vernooij (jelmer at nl.linux.org), 2002
> -   
> +
>     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/>.
>  */
> @@ -324,7 +324,7 @@ static int do_global_checks(void)
>  		fprintf(stderr, "ERROR: passdb backend must have a value or be "
>  				"left out\n\n");
>  	}
> -	
> +
>  	if (lp_os_level() > 255) {
>  		fprintf(stderr, "WARNING: Maximum value for 'os level' is "
>  				"255!\n\n");
> @@ -336,7 +336,7 @@ static int do_global_checks(void)
>  	}
>  
>  	return ret;
> -}   
> +}
>  
>  /**
>   * per-share logic tests
> @@ -491,7 +491,7 @@ static void do_per_share_checks(int s)
>  	 */
>  	lp_set_cmdline("log level", "2");
>  
> -	pc = poptGetContext(NULL, argc, argv, long_options, 
> +	pc = poptGetContext(NULL, argc, argv, long_options,
>  			    POPT_CONTEXT_KEEP_FIRST);
>  	poptSetOtherOptionHelp(pc, "[OPTION...] <config-file> [host-name] [host-ip]");
>  
> @@ -504,7 +504,7 @@ static void do_per_share_checks(int s)
>  
>  	setup_logging(poptGetArg(pc), DEBUG_STDERR);
>  
> -	if (poptPeekArg(pc)) 
> +	if (poptPeekArg(pc))
>  		config_file = poptGetArg(pc);
>  
>  	cname = poptGetArg(pc);
> -- 
> 2.11.0
> 
> 
> From 76b82579598dd1cca162bf4e280435e7ef772636 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 7 Dec 2016 17:44:25 +0100
> Subject: [PATCH 2/4] s3-testparm: Print error if the default backend is
>  incorrect
> 
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Reviewed-by: Michael Adam <obnox at samba.org>
> ---
>  source3/utils/testparm.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index c330ad16959..e0cfaf14d9f 100644
> --- a/source3/utils/testparm.c
> +++ b/source3/utils/testparm.c
> @@ -313,6 +313,32 @@ static int do_global_checks(void)
>  		fprintf(stderr, "'algorithmic rid base' must be even.\n\n");
>  	}
>  
> +	if (lp_server_role() != ROLE_STANDALONE) {
> +		const char *default_backends[] = {
> +			"tdb", "tdb2", "ldap", "autorid", "hash"
> +		};
> +		const char *idmap_backend;
> +		bool valid_backend = false;
> +		uint32_t i;
> +		bool ok;
> +
> +		idmap_backend = lp_idmap_default_backend();
> +
> +		for (i = 0; i < ARRAY_SIZE(default_backends); i++) {
> +			ok = strequal(idmap_backend, default_backends[i]);
> +			if (ok) {
> +				valid_backend = true;
> +			}
> +		}
> +
> +		if (!valid_backend) {
> +			ret = 1;
> +			fprintf(stderr, "ERROR: Do not use the '%s' backend "
> +					"as the default idmap backend!\n\n",
> +					idmap_backend);
> +		}
> +	}
> +
>  #ifndef HAVE_DLOPEN
>  	if (lp_preload_modules()) {
>  		fprintf(stderr, "WARNING: 'preload modules = ' set while loading "
> -- 
> 2.11.0
> 
> 
> From d79621499bcd06762dbd0ed01d24ec6ec9173234 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 7 Dec 2016 18:19:53 +0100
> Subject: [PATCH 3/4] s3-testparm: Print an error if we have overlapping idmap
>  config
> 
> Except if both backends are 'ad'.
> 
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Reviewed-by: Michael Adam <obnox at samba.org>
> ---
>  source3/utils/testparm.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
> 
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index e0cfaf14d9f..3e80c39cf9d 100644
> --- a/source3/utils/testparm.c
> +++ b/source3/utils/testparm.c
> @@ -36,6 +36,8 @@
>  #include "popt_common.h"
>  #include "lib/param/loadparm.h"
>  
> +#include <regex.h>
> +
>  /*******************************************************************
>   Check if a directory exists.
>  ********************************************************************/
> @@ -57,6 +59,145 @@ static bool directory_exist_stat(const char *dname,SMB_STRUCT_STAT *st)
>  	return ret;
>  }
>  
> +struct idmap_config {
> +	const char *domain_name;
> +	const char *backend;
> +	uint32_t high;
> +	uint32_t low;
> +};
> +
> +struct idmap_domains {
> +	struct idmap_config *c;
> +	uint32_t count;
> +	uint32_t size;
> +};
> +
> +static bool lp_scan_idmap_found_domain(const char *string,
> +				       regmatch_t matches[],
> +				       void *private_data)
> +{
> +	bool ok = false;
> +
> +	if (matches[1].rm_so == -1) {
> +		fprintf(stderr, "Found match, but no name - invalid idmap config");
> +		return false;
> +	}
> +	if (matches[1].rm_eo <= matches[1].rm_so) {
> +		fprintf(stderr, "Invalid match - invalid idmap config");
> +		return false;
> +	}
> +
> +	{
> +		struct idmap_domains *d = private_data;
> +		struct idmap_config *c = &d->c[d->count];
> +		regoff_t len = matches[1].rm_eo - matches[1].rm_so;
> +		char domname[len + 1];
> +
> +		if (d->count >= d->size) {
> +			return false;
> +		}
> +
> +		memcpy(domname, string + matches[1].rm_so, len);
> +		domname[len] = '\0';
> +
> +		c->domain_name = talloc_strdup_upper(d->c, domname);
> +		if (c->domain_name == NULL) {
> +			return false;
> +		}
> +		c->backend = talloc_strdup(d->c, lp_idmap_backend(domname));
> +		if (c->backend == NULL) {
> +			return false;
> +		}
> +
> +		ok = lp_idmap_range(domname, &c->low, &c->high);
> +		if (!ok) {
> +			fprintf(stderr,
> +				"ERROR: Invalid idmap range for domain "
> +				"%s!\n\n",
> +				c->domain_name);
> +			return false;
> +		}
> +
> +		d->count++;
> +	}
> +
> +	return false; /* Keep scanning */
> +}
> +
> +static bool do_idmap_check(void)
> +{
> +	struct idmap_domains *d;
> +	uint32_t i;
> +	bool ok = false;
> +	int rc;
> +
> +	d = talloc_zero(talloc_tos(), struct idmap_domains);
> +	if (d == NULL) {
> +		return false;
> +	}
> +	d->count = 0;
> +	d->size = 32;
> +
> +	d->c = talloc_array(d, struct idmap_config, d->size);
> +	if (d->c == NULL) {
> +		goto done;
> +	}
> +
> +	rc = lp_wi_scan_global_parametrics("idmapconfig\\(.*\\):backend",
> +					   2,
> +					   lp_scan_idmap_found_domain,
> +					   d);
> +	if (rc != 0) {
> +		fprintf(stderr,
> +			"FATAL: wi_scan_global_parametrics failed: %d",
> +			rc);
> +	}
> +
> +	for (i = 0; i < d->count; i++) {
> +		struct idmap_config *c = &d->c[i];
> +		uint32_t j;
> +
> +		for (j = 0; j < d->count && j != i; j++) {
> +			struct idmap_config *x = &d->c[j];
> +
> +			if ((c->low >= x->low && c->low <= x->high) ||
> +			    (c->high >= x->low && c->high <= x->high)) {
> +				/* Allow overlapping ranges for idmap_ad */
> +				ok = strequal(c->backend, x->backend);
> +				if (ok) {
> +					ok = strequal(c->backend, "ad");
> +					if (ok) {
> +						fprintf(stderr,
> +							"NOTE: The idmap_ad "
> +							"range for the domain "
> +							"%s overlaps with the "
> +							"range of %s.\n\n",
> +							c->domain_name,
> +							x->domain_name);
> +						continue;
> +					}
> +				}
> +
> +				fprintf(stderr,
> +					"ERROR: The idmap range for the domain "
> +					"%s (%s) overlaps with the range of "
> +					"%s (%s)!\n\n",
> +					c->domain_name,
> +					c->backend,
> +					x->domain_name,
> +					x->backend);
> +				ok = false;
> +				goto done;
> +			}
> +		}
> +	}
> +
> +	ok = true;
> +done:
> +	TALLOC_FREE(d);
> +	return ok;
> +}
> +
>  /***********************************************
>   Here we do a set of 'hard coded' checks for bad
>   configuration settings.
> @@ -337,6 +478,11 @@ static int do_global_checks(void)
>  					"as the default idmap backend!\n\n",
>  					idmap_backend);
>  		}
> +
> +		ok = do_idmap_check();
> +		if (!ok) {
> +			ret = 1;
> +		}
>  	}
>  
>  #ifndef HAVE_DLOPEN
> -- 
> 2.11.0
> 
> 
> From ca490b24d56774aede66e8975216013cb09bcaa8 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 12 Dec 2016 10:05:39 +0100
> Subject: [PATCH 4/4] s3:winbind: Do not start with an invalid default idmap
>  backend
> 
> Pair-Programmed-With: Michael Adam <obnox at samba.org>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/winbindd/winbindd.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/source3/winbindd/winbindd.c b/source3/winbindd/winbindd.c
> index 778ee808d0c..e42f0ab4557 100644
> --- a/source3/winbindd/winbindd.c
> +++ b/source3/winbindd/winbindd.c
> @@ -1689,6 +1689,26 @@ int main(int argc, const char **argv)
>  		exit(1);
>  	}
>  
> +	{
> +		const char *idmap_backend;
> +		const char *invalid_backends[] = {
> +			"ad", "rfc2307", "rid",
> +		};
> +
> +		idmap_backend = lp_idmap_default_backend();
> +		for (i = 0; i < ARRAY_SIZE(invalid_backends); i++) {
> +			bool ok;
> +
> +			ok = strequal(idmap_backend, invalid_backends[i]);
> +			if (ok) {
> +				DBG_ERR("FATAL: Invalid idmap backend %s "
> +					"configured as the default backend!\n",
> +					idmap_backend);
> +				exit(1);
> +			}
> +		}
> +	}
> +
>  	ok = directory_create_or_exist(lp_lock_directory(), 0755);
>  	if (!ok) {
>  		DEBUG(0, ("Failed to create directory %s for lock files - %s\n",
> -- 
> 2.11.0
> 




More information about the samba-technical mailing list