Recent changes to autorid (was Re: [SCM] Samba Shared Repository - branch master updated)

[Michael Adam]

On 2014-04-28 at 00:47 +0200, Michael Adam wrote:
> On 2014-04-27 at 22:02 +0200, Christian Ambach wrote:
> > Am 25.04.14 20:36, schrieb Simo:
> > >On Fri, 2014-04-25 at 17:53 +0200, Michael Adam wrote:
> > >>
> > >>+#define IDMAP_AUTORID_ALLOC_RESERVED 500
> > >
> > >but I think this fiddling and making
> > >strange exceptions with autorid is a bad idea.
> >
> > I somehow agree with Simo.
> 
> I had some good reasons for doing these changes
> and doing them this way, but on the other hand I
> certainly understand the bad feelings about it.
> 
> I will write a detailed explanation tomorrow
> (it is too late now).

Ok, here we go:

The change that's met criticism is the introduction
of a reserved area of a few (500) IDs at the top of
the ALLOC range and a special treatment of the list
of well-known SIDs that allocates these starting
from the top of the range, moving down.

The reason for this change is the desire to have
the mappings as deterministic as possible.

A problem with the well knowns when compared to
other sids is that they don't share a common
domain sid, but are rather a set of special domain
SIDs (S-1-0, S-1-1, S-1-3, ...) with just a few
valid rids, and also e.g. S-1-5 which is of course
also the initial part of Builtin S-1-5-32 and
of the domain sids S-1-5-21-...
So an algorithmic treatment with the rid algorith
would be difficult at best, and definitiely a waste
of ranges. So a special treatment was considered needed.

The first attempt to make these determinstic was
the addition of the function idmap_autorid_preallocate_wellknown()
which is called from the initialization routine.
This simply allocates the well-kowns in the ALLOC
range, traversing a hard coded list. So the order
of allocation determines the resulting mappings.

For a fresh install, this makes the mappings for
these sids deterministic for a given configuration.
(At least with my recent changes to wrap this
into a transaction..)
But for a DB that is upgraded, this may be different:
If the old code had not used well-known allocation yet,
but the alloc range had been used (as a source of
IDs for group mapping), after the upgrade the well-knowns
would be allocated starting at a higher GID than for the
fresh install.

In order to make this more deterministic, various
approaches were discussed (admittedly not on the list
but directly with one major user of autorid..).
One proposal was to use a separate special range
and have a fixed calculation wellknown-sid<->xID
in this range. But this was dismissed because one could
also not guarantee a certain range for this, when it
comes to upgrades. And it is very fragile since a careless
change of the order of the sids in the code would break
the DB in an upgrade and cause access-problems at least.

So we came up with this idea:
We stick to the alloc range.
But instead of using the regular gid HWM counter, we add a
reserved sub-range at the top, which is filled starting
at the top and counting down. The mapping is still
determined by the order of the hard-coded list, but
we achieve what I think is the best balance between
determinism and maximum upgrade-compatibility and robustness
because the mappings are stored as regular alloc-mappings:

- Case from above: if an older code did not map well-knowns,
  the db upgraded from that code will have the exact
  same well-known mappings as a fresh install with the
  same configuration.

- If older code already allocated (some or all of) the
  well-known sids, the upgraded code will honour these
  and only allocate the missing ones in the upper subrange.

- If for some reason the 500 reserved IDs are not
  enough, tose sids exceeding the 500 will simply be mapped
  using the regular ALLOC mecahnism (below the top 500).

I agree that the code may appear strange at first glance,
but the change is in fact rather clear and minimal imho:
I have first reworked the code so that the flow is now
rather clear:

sid-to-id:
  if sid-is-for-alloc:
    if sid-is-special (wellknown):
      if special allocation in top range works:
        done
      end
      # fall through to regular alloc..
    end
    regular alloc using HWM counters
  else:
    rid-based mapping
  end
end

Thanks to anyone who has read this far... ;-)


All that being said, I understand that the change might
still be considered problematic. And I sitll hold up
the offer to withdraw the decisive patches for further
discussion.

(Of course, I agree with Christian that this needs
 some documentation in code and manpage. I will provide
 those if we choose to keep the mechanism. Also then
 we should of course add Christian's typo-fix.)

Opinions?

Cheers - Michael




> But I certainly offer to
> revert the changes (that is the two patches that
> add the special treatment for wellknowns) for now
> if that is desired. (I think the other patches
> are reasonable independent improvements of the code.)
> 
> Cheers - Michael
> 
> > Adding more and more exceptions and special
> > handlings to autorid throws away the originally simple idea behind it
> > and creates a more and more complex module. You now need to look at many
> > code paths to understand how the different types of SIDs are treated and
> > most of them miss the explanatory comments why some SIDs are treated
> > differently. We definitely need more documentation here: both in the
> > code and in the manpage.
> > What is the rationale behind this change? Making sure all current
> > well-knowns are place right next to each other and when Microsoft adds
> > new well-knowns in the future they also be placed beside the existing
> > ones? How can you make sure that 500 will be enough?
> > 
> > Looking at the changes, I corrected a typo in a new DEBUG message,
> > please see attachment.
> > 
> > Cheers,
> > Christian
> > 
> 
> > From 6c81cbfe73bb6901611540cc22eb0a6da70b2034 Mon Sep 17 00:00:00 2001
> > From: Christian Ambach <ambi at samba.org>
> > Date: Sun, 27 Apr 2014 21:54:40 +0200
> > Subject: [PATCH] autorid: fix a typo
> > 
> > Signed-off-by: Christian Ambach <ambi at samba.org>
> > ---
> >  source3/winbindd/idmap_autorid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/source3/winbindd/idmap_autorid.c b/source3/winbindd/idmap_autorid.c
> > index 37612c2..2cafcb5 100644
> > --- a/source3/winbindd/idmap_autorid.c
> > +++ b/source3/winbindd/idmap_autorid.c
> > @@ -434,7 +434,7 @@ static NTSTATUS idmap_autorid_sid_to_id_alloc_action(
> >  			return ret;
> >  		}
> >  
> > -		DEBUG(10, ("Sepecial sid %s not mapped. falling back to "
> > +		DEBUG(10, ("Special sid %s not mapped. falling back to "
> >  			   "regular allocation\n",
> >  			   sid_string_dbg(ctx->map->sid)));
> >  	}
> > -- 
> > 1.8.3.2
> > 
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140428/a83463a5/attachment.pgp>