[PATCHES] winbindd: fix sid->xid for SID History SIDs

[Ralph Böhme]

On Tue, Mar 28, 2017 at 03:00:10PM +0300, Uri Simchoni wrote:
> On 03/28/2017 01:54 PM, Ralph Böhme wrote:
> > Hi Uri,
> > mostly lgtm, just one issue, see below.
> > 
> > Fwiw, I'm currently working on another issue in sids2xids. Not really related
> > but I'm mentioning it here as you're currently having fun with the same area of
> > code.
> > 
> Yeah, I figured others are working there, with recent patches by Metze
> and Volker. Another reason for me for trying to keep it small (beside
> the backporting).
> 
> Another thing in TODO list is offline operation, which involves *not*
> resolving anything initially, if we're certain we know the id-mapping
> backend (and then resolving only if the backend requests us to do so).
> Are you guys working on anything similar?

Not directly.

> > Imho we should just fail early if a SID can't be resolved and refuse to provide
> > a UNIX id mapping, I have a WIP branch that does this here:
> > 
> 
> That's totally fine by me - I have no experience with autorid, and
> figured its ability to map anything is a feature, not a bug, so I wanted
> to keep it that way. Changing that is easy, I'll submit a V2.

no, no, please don't, just leave it broken for now. :)

> >> +static const char *
> >> +wb_get_idmap_backend_name(const struct lsa_RefDomainList *domains,
> >> +			  const struct lsa_TransNameArray *names,
> >> +			  const struct dom_sid *sids,
> >> +			  uint32_t idx)
> >> +{
> >> +	struct dom_sid sid = sids[idx];
> >> +	struct lsa_TranslatedName *n = &names->names[idx];
> >> +	struct lsa_DomainInfo *info = NULL;
> >> +	if (!is_domain_principal_sid(&sid)) {
> >> +		/* "special" SID */
> >> +		goto ret;
> >> +	}
> >> +
> >> +	if (sid_check_is_in_our_sam(&sid)) {
> >> +		/* our domain - the lookup is always
> >> +		 * correct for ID mapping.
> >> +		 */
> >> +		goto ret;
> >> +	}
> >> +
> >> +	sid_split_rid(&sid, NULL);
> >> +	sid_append_rid(&sid, DOMAIN_RID_USERS);
> >> +
> >> +	for (idx = 0; idx < names->count; ++idx) {
> >> +		if (!dom_sid_equal(&sid, &sids[idx])) {
> >> +			continue;
> >> +		}
> >> +
> >> +		/* has the DC been able to resolve
> >> +		 * "domain users" for same domain?
> >> +		 */
> >> +		n = &names->names[idx];
> >> +		if (lsa_SidType_to_id_type(n->sid_type) ==
> >> +		    ID_TYPE_NOT_SPECIFIED) {
> >> +			/* default id-mapping backend */
> >> +			return "";
> >> +		}
> >> +
> >> +		goto ret;
> >> +	}
> > 
> > doesn't this add O(n^2) overhead for finding the domain SID? When mapping a
> > token with a large number of groups this may be noticable.
> > 
> > Maybe add_sid_to_array_unique() should be add_sid_to_array_front_unique().
> > 
> 
> I was thinking of having add_sid_to_array_back_unique(), because the
> code assumes that if a SID is added, it's added to the back. I didn't
> consider the O^2.
> 
> I suppose I can make a pass and construct an array with just the -513
> records (hopefully just one or two of them), and then use that.

ah, another one: why would be using the domain users SID anyway? Why not just
the domain SID?

Cheerio!
-slow