[PATCH] Check if the idmap_hash range is big enough
Michael Adam
obnox at samba.org
Fri Mar 3 10:04:57 UTC 2017
On 2017-02-23 at 17:55 +0100, Andreas Schneider wrote:
> On Tuesday, 21 February 2017 10:16:21 CET Michael Adam wrote:
> > On 2017-02-21 at 09:32 +0100, Andreas Schneider wrote:
> > > On Tuesday, 21 February 2017 02:10:17 CET Michael Adam wrote:
> > > > On 2017-02-20 at 11:51 +0100, Andreas Schneider wrote:
> > > > > On Friday, 17 February 2017 18:44:34 CET Michael Adam wrote:
> > > > > > > - idmap config * : range = 1000-4000000000
> > > > > > > + idmap config * : range = 100000-4000000000
> > > > > >
> > > > > > If you want to catch as much as possible of a domain
> > > > > > that gets hashed to 0, then the lower bound needs to
> > > > > > be as low as possible, hence the 1000. But then,
> > > > > > the first 1000 rids in a domain will be used as well,
> > > > > > and hence why not skip this first range entirely
> > > > > > and start at 500000 ? ;-)
> > > > >
> > > > > Ok, lets start there. I think we should suggest 525000 that is big
> > > > > enough
> > > > > and easy to deal with for our users.
> > > > >
> > > > > > So:
> > > > > >
> > > > > > - the idmap hash module, when used for "idmap config *",
> > > > > >
> > > > > > ideally should have the full range of
> > > > > > 0 - 2147483648 which is not quite possible (at the low
> > > > > > end at least)...
> > > > >
> > > > > The best is to start with 500000. 1000 is normally the start for local
> > > > > users.
> > > >
> > > > Now what? 500000 or 525000 ? :-)
> > > >
> > > > > See attached patchset.
> > > > >
> > > > > Andreas
> > > > >
> > > > > From c0f379a680613fdb28a23d0cf2e3ed9ace260fd7 Mon Sep 17 00:00:00 2001
> > > > > From: Andreas Schneider <asn at samba.org>
> > > > > Date: Wed, 15 Feb 2017 08:55:24 +0100
> > > > > Subject: [PATCH 1/2] docs: Improve the idmap_hash manpage
> > > > >
> > > > > BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
> > > > >
> > > > > Signed-off-by: Andreas Schneider <asn at samba.org>
> > > > > ---
> > > > >
> > > > > docs-xml/manpages/idmap_hash.8.xml | 7 ++++++-
> > > > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/docs-xml/manpages/idmap_hash.8.xml
> > > > > b/docs-xml/manpages/idmap_hash.8.xml index 9f4f1d1933c..a9230498efe
> > > > > 100644
> > > > > --- a/docs-xml/manpages/idmap_hash.8.xml
> > > > > +++ b/docs-xml/manpages/idmap_hash.8.xml
> > > > > @@ -24,6 +24,11 @@
> > > > >
> > > > > to support a local name mapping files if enabled via the
> > > > > "winbind normalize names" and "winbind nss
> > > > > info"
> > > > > parameters in smb.conf.
> > > > >
> > > > > + The module divides the range into subranges for each domain that
> > > > > is
> > > > > being + handled by the idmap config.
> > > > > + Each range has a size of roughly 525000 IDs (20 bit). This means
> > > > > + that the range for multiple domains needs to be large enough! So
> a
> > > > > good value + is normally '100000-4000000' or even bigger.
> > > >
> > > > That's not the main point.
> > > > It's not that you need a couple of those ranges of size ~ 525000
> > > > in order to accomodate a few domains. You need *them all*
> > > > because each domain has a fixed absolute range associated to it
> > > > by the hashing algorithm, and you don't know a priori which domain
> > > > will come by...
> > > >
> > > > So no, the above range is not normally a good value,
> > > > since those almost 8 ranges out of the more than 4000
> > > > ranges that exist, are likely not among those needed
> > > > for the domains that enter the system...
> > > >
> > > > (apart from this, the low id of 100000 seems to contradict your
> > > > mention of 525000 above...)
> > >
> > > You should look at patch version v4.
> >
> > Ah, I had only seen v3. :-)
> > v4 is of course more consistent.
> >
> > > You said a domain needs 524288 ids for allocation. So 525000 is easier for
> > > a user, that's why I choose that. If the text is wrong, could you please
> > > suggest a text instead of letting me do the guesswork here?
> >
> > Well, I am not actually letting you do guesswork.
> > I'm only saying what I see as problematic. And if
> > that is not clear you can also look at the code
> > instead of guessing, right? ... ;-)
> >
> > I would also propose a text if I had a good one.
> > The unresolvable dilemma here is that this module
> > is utterly messed up, and NO configuration that
> > we can come up with will be perfect, for reasons
> > I detailed in an earlier mail.
> >
> > I guess any good and honest text will have to
> > explain the formulas used by the module, so that
> > the reader can understand what the module does.
> > It will also contain the sentence "DO NOT USE THIS MODULE!"...
> >
> > (See below...)
> >
> > > > > </para>
> > > > >
> > > > > </refsynopsisdiv>
> > > > >
> > > > > @@ -53,7 +58,7 @@
> > > > >
> > > > > <programlisting>
> > > > > [global]
> > > > > idmap config * : backend = hash
> > > > >
> > > > > - idmap config * : range = 1000-4000000000
> > > > > + idmap config * : range = 100000-4000000000
> > > > >
> > > > > winbind nss info = hash
> > > > > winbind normalize names = yes
> > > >
> > > > Again, this misses the main point, because the hash ranges
> > > > are determined absolutely, and not relative to configured
> > > > idmap ranges:
> > > >
> > > > Yeah, it's right that a range this small can't even accomodate
> > > > a single domain, but even if we are just big enough for one
> > > > range, this is likely not an entire range (but starting in the
> > > > middle of one range and ending in the middle of the next one),
> > > > and even if one full range is included the likelyhood that
> > > > it will be this one range that is used by the domain of users
> > > > logging on to the samba server is extremely low...
> > > >
> > > > I am really sorry to be coming across so negatively.
> > > > I would like to have a better answer, but currently
> > > > I only know what is NOT sufficient or completely good... :-/
> > >
> > > I don't see or understand where we are going here. I don't understand what
> > > you want.
> >
> > Well. You want to change the manpage and testparm.
> > I am reviewing your changes since I want the changes
> > going in to be correct, and I want them to be an
> > improvement over what we currently have...
> >
> > What I personally want or would like to do myself is
> > to remove the idmap_hash module altogether. But
> > unfortunately that does not seem to be feasible,
> > since it is used out there.
> >
> > > If you do not want to change/fix the manpage or add a check in idmap_hash
> > > module itself then please tell me and I can stop wasting time on a
> > > patchset
> > > which is not accepted upstream.
> > >
> > > If you want to fix the documentation please suggest a text for the
> > > manpage.
> >
> > Let me try to explain what the module does. This is not yet a
> > polished text, but may serve as a basis:
> >
> > =========================================================================
> > The idmap_hash module calculates a Unix ID for a given SID as
> > follows:
> >
> > - Write the SID as DOMAINSID-RID.
> > - The module calculates a 12-bit hash value of the DOMAINSID,
> > i.e. a value hash(DOMAINSID) between 0 and 4095.
> > - The unix-ID for SID is then calculated as
> >
> > unix-id(SID) = hash(DOMAINSID) * 0x080000 + (RID % 0x080000)
> >
> > (Note 0x080000 == 524288 and 4095 == 0x0FFF.)
> >
> >
> > Hence:
> >
> > - Each domain has its predefined fixed range of
> >
> > hash(DOMAINSID)*0x080000 -- (hash(domainsid)*0x080000 + 524287)
> >
> > - The overall required range to be able to map all SIDs is
> >
> > 0 -- 4096 * 524288 - 1 = 2147483647
> >
> > This leads to a few issues:
> >
> > - Any range smaller than 0 - 2147483647 will filter some SIDs.
> > - Since we can not start the range at 0, some SIDs can *never*
> > be mapped.
> > - Some domain SIDs will be mapped to the same range.
> > - RIDs will wrap around, i.e. DOMSID-RID and
> > DOMSID-(RID+524288) will be mapped to the same ID.
> >
> > Hence the recommendation is:
> >
> > DO NOT USE THIS MODULE!
> >
> > If you have to use it, then make the range as big as possible.
> > I would say start as low as you can afford, i.e. 1000 or 10000.
> > That way, you'll at least catch some IDs of those domains
> > that are unfortunate enough to fall into hash value 0...
> > (Note to Andreas: If you want to start at 520000 instead,
> > completely filtering hash value 0 domains, that is a point of
> > view as well, which comes closer to not using the module at all...)
> >
> > All in all, I can only repeat:
> >
> > DO NOT USE THIS MODULE!
> >
> > =====================================================================
> >
> > Does that make it more clear?
> >
> > Cheers - Michael
>
>
> Some modifications, here is the latest patchset.
Thanks!
The first 2 patches are RB+me,
but the range check is wrong imho, since it only
checks the difference. But e.g.:
21474836478-4294967295 would be big enough,
but not a single SID would get mappad, as you
explained in the manpage text... :-)
Cheers - Michael
> From 4372b37db8ed6a249682253a5a1712a824e71d9e Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Wed, 15 Feb 2017 08:55:24 +0100
> Subject: [PATCH 1/3] docs: Improve the idmap_hash manpage
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> docs-xml/manpages/idmap_hash.8.xml | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/docs-xml/manpages/idmap_hash.8.xml b/docs-xml/manpages/idmap_hash.8.xml
> index 9f4f1d1933c..311319d806b 100644
> --- a/docs-xml/manpages/idmap_hash.8.xml
> +++ b/docs-xml/manpages/idmap_hash.8.xml
> @@ -13,17 +13,35 @@
>
> <refnamediv>
> <refname>idmap_hash</refname>
> - <refpurpose>Samba's idmap_hash Backend for Winbind</refpurpose>
> + <refpurpose>DO NOT USE THIS BACKEND</refpurpose>
> </refnamediv>
>
> <refsynopsisdiv>
> <title>DESCRIPTION</title>
> - <para>The idmap_hash plugin implements a hashing algorithm used to map
> + <para>DO NOT USE THIS PLUGIN
> +
> + The idmap_hash plugin implements a hashing algorithm used to map
> SIDs for domain users and groups to 31-bit uids and gids, respectively.
> This plugin also implements the nss_info API and can be used
> to support a local name mapping files if enabled via the
> "winbind normalize names" and "winbind nss info"
> parameters in smb.conf.
> + The module divides the range into subranges for each domain that is being
> + handled by the idmap config.
> +
> + The module needs the complete UID and GID range to be able to map all
> + SIDs. The lowest value for the range should be the smallest ID
> + available in the system. This is normally 1000. The highest ID should
> + be set to 2147483647.
> +
> + A smaller range will lead to issues because of the hashing algorithm
> + used. The overall range to map all SIDs is 0 - 2147483647. Any range
> + smaller than 0 - 2147483647 will filter some SIDs. As we can normally
> + only start with 1000, we are not able to map 1000 SIDs. This already
> + can lead to issues. The smaller the range the less SIDs can be mapped.
> +
> + We do not recommend to use this plugin. It will be removed in a future
> + release of Samba.
> </para>
> </refsynopsisdiv>
>
> @@ -53,7 +71,7 @@
> <programlisting>
> [global]
> idmap config * : backend = hash
> - idmap config * : range = 1000-4000000000
> + idmap config * : range = 1000-2147483647
>
> winbind nss info = hash
> winbind normalize names = yes
> --
> 2.11.0
>
>
> From 8ed47c1ce277469dfd45fefb982749e0b38fe1e3 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Tue, 21 Feb 2017 14:51:08 +0100
> Subject: [PATCH 2/3] idmap_hash: Add a deprecation message
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/winbindd/idmap_hash/idmap_hash.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/source3/winbindd/idmap_hash/idmap_hash.c b/source3/winbindd/idmap_hash/idmap_hash.c
> index 743b0ec4ff8..36cc0f1e354 100644
> --- a/source3/winbindd/idmap_hash/idmap_hash.c
> +++ b/source3/winbindd/idmap_hash/idmap_hash.c
> @@ -112,6 +112,10 @@ static NTSTATUS idmap_hash_initialize(struct idmap_domain *dom)
> size_t num_domains = 0;
> int i;
>
> + DBG_ERR("The idmap_hash module is deprecated and should not be used. "
> + "Please migrate to a different plugin. This module will be "
> + "removed in a future version of Samba\n");
> +
> if (!strequal(dom->name, "*")) {
> DBG_ERR("Error: idmap_hash configured for domain '%s'. "
> "But the hash module can only be used for the default "
> --
> 2.11.0
>
>
> From eedb8a6cba243da7719ceb1a987eb059889f145b Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Mon, 20 Feb 2017 11:44:22 +0100
> Subject: [PATCH 3/3] idmap_hash: Make sure the idmap range is big enough
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12582
>
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
> source3/winbindd/idmap_hash/idmap_hash.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/source3/winbindd/idmap_hash/idmap_hash.c b/source3/winbindd/idmap_hash/idmap_hash.c
> index 36cc0f1e354..a14c8d87015 100644
> --- a/source3/winbindd/idmap_hash/idmap_hash.c
> +++ b/source3/winbindd/idmap_hash/idmap_hash.c
> @@ -123,6 +123,14 @@ static NTSTATUS idmap_hash_initialize(struct idmap_domain *dom)
> return NT_STATUS_INVALID_PARAMETER;
> }
>
> + if ((dom->high_id - dom->low_id) < 2000000000U) {
> + DBG_ERR("Error: The idmap_hash range configured for domain "
> + "'%s' is too small! Please consult the manpage of "
> + "the idmap_hash module.\n",
> + dom->name);
> + return NT_STATUS_INVALID_PARAMETER;
> + }
> +
> /* If the domain SID hash table has been initialized, assume
> that we completed this function previously */
>
> --
> 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/20170303/997c9aec/signature.sig>
More information about the samba-technical
mailing list