Broken idmap interface design

[James Peach]

On 19/04/2007, at 7:49 AM, Gerald (Jerry) Carter wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Simo,
>
>> On Thu, 2007-04-19 at 23:30 +1000, Luke Howard wrote:
>>> Sorry to jump in here, one thing I'd like to see
>>> in idmap_ad is support for using the Global Catalog. Shouldn't
>>> be too hard. Thoughts?
>>
>> Well IIRC rfc2307 attributes are not exposed via GC by
>> default, so to use it we must have fallback code in place.
>> Not that hard, but I guess this is more of a 3.0.26 feature.
>> I am working only to stabilize the code for offline
>> usage right now.
>
> It's actually worse than that.  The idmap interface is
> badly broken.  I hate to say this, but the calls into
> winbindd from the idmap child has to go.  I know how you
> arrived at the design assumptions.
>
> You designed the unixids_to_sids() and sids_to_unixids()
> with the assumption that the idmap plugin would have
> knowledge about the SID type.  I didn't catch this
> because the backend I'm using for primary testing operates
> similarly to idmap_ad and can obtain the SID type based
> on LDAP searches.  This is ok for something like idmap_ad
> which can get the information.  But the general and
> default case is idmap_tdb (or even idmap_ldap).

There's two cases for mapping from SIDs to UGIDs.

First case is where full SIDs are explicitly stored in the directory  
or have a static partitioning (like Unix Users and Unix Groups). In  
this case, you don't need to be told what kind of ID the caller is  
looking for, because the SID is globally unique, and must only map to  
one ID.

Second case is where the SID is dynamically generated from other  
information in the directory. In this case, knowing the type of ID in  
advance helps a lot and can prevent you generating bogus SIDs.

I take it that it is this second case that is problematic?

> 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.

I don't see any winbind client calls in idmap_tdb.c. Could you spell  
this out a bit more?

> Right now I'm going to do several things in order to get
> the code to a release point.
>
> (a) Remove WINBINDD_SIDS_TO_XIDS from winbindd_nss.h to
>    prevent us from having to support the broken call in
>    future releases.  The existing idmap_methods API will
>    not change but will become solely an internal interface
>    used by winbindd.
>
> (b) Overload the id_map.xid.type to be specified by the caller
>    and not filled by the idmap backend.

So existing idmap modules will need to be changed?

--
James Peach | jpeach at samba.org