[PATCH] Check idmap config with testparm
Michael Adam
obnox at samba.org
Thu Dec 8 07:42:37 UTC 2016
On 2016-12-07 at 18:43 +0100, Andreas Schneider wrote:
> Hello,
>
> you might know I work for a Distributor and fix winbind issues there every
> day.
> I see so many invalid idmap configurations, I think 70% of the configs are
> wrong or invalid.
>
> In addition our documentation for ID mapping really sucks!
Hmm, I take this a little bit as a personal affront.
Let me reply with a similar non-diplomatic statement:
People should learn to read! :-)
Have you read the section about "idmap config DOMAIN : OPTION" in
"man smb.conf" and the backend specific manpages?
Among other things, smb.conf clearly states:
"The first three of these [idmap_tdb, idmap_tdb2, idmap_ldap]
create mappings of their own using internal unixid counters and
store the mappings in a database. These are suitable for use in
the default idmap configuration."
As well as:
"The configured ranges must be mutually disjoint."
Also, for further examples, reading the manpages of idmap_rid,
I see:
"One usually needs to define a writeable default idmap range,
using a backend like tdb or ldap that can create unix ids."
Looking at idmap_ad:
"the ad backend does not work as the default idmap backend, but
one has to configure it separately for each domain for which
one wants to use it, using disjoint ranges."
Enough examples. The doc is cetainly not perfect, but
saying it sucks just proves not having read it, imho.
> So I had a call with Marc and he started to improve it. See the User
> documentation in the Wiki.
>
> While trying to chase down a winbindd bug the last days I read all the changes
> last year and stumbled upon Volkers nice lp_wi_scan_global_parametrics()
> function again. So I decided it is time to check the idmap config in testparm.
This is an excellent idea!
(Don't rely on reading capabilities is always the safe bet... ;-)
> So here we go ...
>
>
> <config>
> idmap config * : backend = rid
> idmap config * : range = 1000000-1999999
>
> # Winbind domain idmap
> idmap config EARTH : backend = rid
> idmap config EARTH : range = 100000000-199999999
>
> idmap config MARS : backend = rid
> idmap config MARS : range = 200000000-299999999
>
> idmap config VENUS : backend = rid
> idmap config VENUS : range = 150000000-399999999
> </config>
>
> <console>
> bin/testparm smb.conf.ads > /dev/null
> Load smb config files from smb.conf.ads
>
> ERROR: Do not use the 'rid' backend for the default backend (idmap config *)!
>
> ERROR: The idmap range for the domain MARS overlaps with the range of VENUS
Note that iirc, with Volker's recent work on idmap_ad, it
is not forbidden any more to have overlapping idmap ranges!
At least you should be able to have multiple ad backend
configs with the same range...
> Server role: ROLE_DOMAIN_MEMBER
>
> Press enter to see a dump of your service definitions
> </console>
>
> Review and push of the attached patchset is much appreciated!
Good approach, but please don't push yet.
See comments inline below:
> From cf9872f8f0ab5e5712290cc602cfc9f60351f442 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/3] s3-testparm: Fix trailing whitespaces
>
> Signed-off-by: Andreas Schneider <asn 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
RB: me
> From 8e49a3a63d0cca6f380ee4f0de96c4f83be9cb9e 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/3] s3-testparm: Print error if 'rid' is used for the default
> backend
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/utils/testparm.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index c330ad16959..4dbdedc1591 100644
> --- a/source3/utils/testparm.c
> +++ b/source3/utils/testparm.c
> @@ -67,6 +67,8 @@ static int do_global_checks(void)
> int ret = 0;
> SMB_STRUCT_STAT st;
> const char *socket_options;
> + const char *idmap_backend;
> + bool ok;
>
> if (lp_security() >= SEC_DOMAIN && !lp_encrypt_passwords()) {
> fprintf(stderr, "ERROR: in 'security=domain' mode the "
> @@ -313,6 +315,14 @@ static int do_global_checks(void)
> fprintf(stderr, "'algorithmic rid base' must be even.\n\n");
> }
>
> + idmap_backend = lp_idmap_default_backend();
> + ok = strequal(idmap_backend, "rid");
> + if (ok) {
> + ret = 1;
> + fprintf(stderr, "ERROR: Do not use the 'rid' backend for the "
> + "default backend (idmap config *)!\n\n");
> + }
> +
> #ifndef HAVE_DLOPEN
> if (lp_preload_modules()) {
> fprintf(stderr, "WARNING: 'preload modules = ' set while loading "
> --
> 2.11.0
Why only rid?
It is correct that having 'rid' in default is probably
the most serious, because you will immediately have
collisions when more than one domain is treated
(and *this* needs to be documented, patch pending...),
but the only modules that should be used as default
are: tdb, tdb2, ldap, autorid, hash.
rid, ad, rfc2307, nss, script should only be used
in explicit configs.
> From b28d64945ccabab9430791f17a589254c81216e2 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/3] s3-testparm: Print an error if we have overlapping idmap
> config
As mentioned, hasn't overlapping ranges at least fo the AD
backend recently been made explicitly supported?
Cheers - Michael
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/utils/testparm.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/source3/utils/testparm.c b/source3/utils/testparm.c
> index 4dbdedc1591..017ff1c88e1 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,127 @@ 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)) {
> + fprintf(stderr,
> + "ERROR: The idmap range for the domain "
> + "%s overlaps with the range of "
> + "%s!\n\n",
> + c->domain_name,
> + x->domain_name);
> + 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.
> @@ -323,6 +446,11 @@ static int do_global_checks(void)
> "default backend (idmap config *)!\n\n");
> }
>
> + ok = do_idmap_check();
> + if (!ok) {
> + ret = 1;
> + }
> +
> #ifndef HAVE_DLOPEN
> if (lp_preload_modules()) {
> fprintf(stderr, "WARNING: 'preload modules = ' set while loading "
> --
> 2.11.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 163 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20161208/b108983c/signature.sig>
More information about the samba-technical
mailing list