Broken idmap interface design
idra at samba.org
Thu Apr 19 19:23:04 GMT 2007
On Thu, 2007-04-19 at 12:11 -0500, Gerald (Jerry) Carter wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> Jeremy Allison wrote:
> >> Requiring the idmap_tdb code (or idmap_rid) to issues a
> >> winbindd client call is wrong and a layering violation. The
> >> caller should specify the SID type which is exactly what
> >> the WINBINDD_SID_TO_UID, et. al. calls used to do.
> > Indeed. Looking at this interface cold after ignoring
> > it for a while I think the SID_TYPE enum needs to be
> > present as input on all calls into a "map SID to XXX".
> Agreed. I'm still looking at what would be the minimal
> appropriate fix. Simo and I have a call later this afternoon
> to chat about the current state and how to move forward.
Ok, we cleared out the problem in the call.
Jerry is right, we have a layering violation.
Let me explain why we had the lookup_sid() call in idmap_new_mapping().
Basically we decided that decision on whether to allocate or not a new
mapping was to be done into idmap. The problem was that to avoid
malicious ID consumption I decided to add a call to verify if the SID
was a valid existing SID before going on and asking the alloc backend to
give as an uid.
The lookup_sid() purpose was never really that of retrieving the SID
type. The type can be easily passed by the caller, and in fact that is
what we will do for 3.0.25
The error was in putting the verification inside idmap (causing the
layering violation). Now what we will do is to make the verification,
_before_ calling into idmap.
Winbindd is the gateway to idmap anyway so we can trust nobody else
should be able to inject calls with bogus requests just to deplete our
uid space. The only side effect I can see is that, this way, winbindd
will have to verify the validity of a SID for each idmap request as
winbindd has no knowledge of whether the mapping already exist (no
verification required) or it will be allocated just at that point
(verification absolutely required).
If this will turn out to be a performance problem we will find out how
to cope with that (maybe via a cache or something).
Also the multiple SIDS to IDS call will not be exposed in 3.0.25, we
will delay it to 3.0.26, as Jerry want to enhance the interface before
releasing it. This interface was introduced to solve some performances
issues for users with many many many SIDs attached when caches are empty
(we've seen PACs with thousands of SIDs for a single user :-/).
For idmap_rid that was just copying and pasting without thinking (I
already feel an electric shock here* :), I should never have put such
calls down there, even with the current design, that was my fault.
* If you don't get this joke don't worry, someone here does :)
Samba Team GPL Compliance Officer
email: idra at samba.org
More information about the samba-technical