[Patches] Expanded group memberships on boundaries of outgoing trusts (bugs #13299, #13300, #13307)

[Andrew Bartlett]

On Tue, 2018-02-27 at 17:54 +0100, Stefan Metzmacher via samba-
technical wrote:
> Hi,
> 
> here are patches, which implement the required group membership
> expanding on an AD DC, when we got the users token from a trusted domain.
> 
> In order to have tests for this, I've modified
> "samba-tool group {add,remove}members" to accept a sid string
> instead of a user/group name.
> 
> Under the hood, a member is specified as
> '<SID=S-1-5-21-222-333-444-555>' and the new fpo_attr module,
> creates a foreignSecurityPrincipal object on the fly.
> 
> In order to match the Windows behaviour, it's no longer
> possible to create foreignSecurityPrincipal objects directly.
> 
> While being there I had to fix a bug in the "extended_dn_store"
> module, where we allowed linked attributes to deleted objects,
> which should only be allowed for non-linked attributes with
> DN syntax.
> 
> In order to protect administrators from believing, we would
> support for "selective authentication" (CROSS_ORIGANIZATION) trusts,
> I added code to ignore these in winbindd and the kdc for now.
> 
> I have working code to also implement SID-Filtering and also
> selective authentication, but the patches need more cleanup,
> which is more likely something for the 4.9 release.
> 
> With the attached patches (which I'd like to see in 4.8),
> we will be in a very good shape and support a lot of trust
> setups as AD DC. I'll write up a WHATSNEW section about the
> whole trust situation in the next days.
> 
> Please review and push:-)

I'll aim this at the perf testing rig, but I already have some
concerns:

The extended dn stuff is pretty performance sensitive (we need to
ensure for replication that we always end up in the 'don't parse the DN
case', so I will want to check those patches.

I'm also concerned about:

> From 3dea1ea7d5c6c8bc95440944a7daa8336f520098 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Thu, 22 Feb 2018 23:24:59 +0100
> Subject: [PATCH 08/26] dsdb:unique_object_sids: remove
>  unique_object_sids_init() and get the domain_sid during the request
> 
> samdb_domain_sid() already has an effective cache, there's no need to
> cache it again.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13300
> 
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---

Yes, we have a cache, but the O(n) behaviour isn't great (at times past
it did feature clearly in profiles, thankfully we have avoided it in a
few key cases), so I wouldn't say it is effective.  

That is why that code tried for a startup-time cache.

Finally (and I ask this question genuinely), is this really best
described as a last-moment bug fix?  I know incredibly well the
pressure to get features in rather than wait 6 months, but we do have
the feature deadline rules for a reason.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba