Quantcast

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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Hi,

We few weeks ago we've discussed SID history and id-mapping -
https://lists.samba.org/archive/samba-technical/2017-February/118771.html.

Attached is a proposed initial fix for the issue, which focuses on
avoiding wrong results.

The fix finds the domain of the SID by resolving a SID with same domain
component and an RID of 513 (domain users), which hopefully never gets
migrated.

We've discussed other means such as smb.conf stuff or netsamlogon - I
think those methods can come on top of this method, because if they
don't work we should always fall back to something. The added resolving
doesn't cost much because it's in the same round-trip.

The key thing about this fix is that doesn't try to translate sid->xid
in any possible case (such as when old domain is gone and forgotten), it
just avoids getting the *wrong* result. As such, it's a good minimal fix
that can be applied to stable versions. For master, we can add the
smb.conf-based stuff, that will support more cases.

Review appreciated.
Thanks,
Uri.

sidhist-master.patch.txt (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 03/20/2017 06:49 AM, Uri Simchoni via samba-technical wrote:

> Hi,
>
> We few weeks ago we've discussed SID history and id-mapping -
> https://lists.samba.org/archive/samba-technical/2017-February/118771.html.
>
> Attached is a proposed initial fix for the issue, which focuses on
> avoiding wrong results.
>
> The fix finds the domain of the SID by resolving a SID with same domain
> component and an RID of 513 (domain users), which hopefully never gets
> migrated.
>
> We've discussed other means such as smb.conf stuff or netsamlogon - I
> think those methods can come on top of this method, because if they
> don't work we should always fall back to something. The added resolving
> doesn't cost much because it's in the same round-trip.
>
> The key thing about this fix is that doesn't try to translate sid->xid
> in any possible case (such as when old domain is gone and forgotten), it
> just avoids getting the *wrong* result. As such, it's a good minimal fix
> that can be applied to stable versions. For master, we can add the
> smb.conf-based stuff, that will support more cases.
>
> Review appreciated.
> Thanks,
> Uri.
>

Ping...

Correction to the above text - the fix doesn't try to translate sid->xid
in *every* possible case, it just avoids getting wrong results.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Uri,

On Mon, Mar 20, 2017 at 06:49:54AM +0200, Uri Simchoni via samba-technical wrote:

> We few weeks ago we've discussed SID history and id-mapping -
> https://lists.samba.org/archive/samba-technical/2017-February/118771.html.
>
> Attached is a proposed initial fix for the issue, which focuses on
> avoiding wrong results.
>
> The fix finds the domain of the SID by resolving a SID with same domain
> component and an RID of 513 (domain users), which hopefully never gets
> migrated.
>
> We've discussed other means such as smb.conf stuff or netsamlogon - I
> think those methods can come on top of this method, because if they
> don't work we should always fall back to something. The added resolving
> doesn't cost much because it's in the same round-trip.
>
> The key thing about this fix is that doesn't try to translate sid->xid
> in any possible case (such as when old domain is gone and forgotten), it
> just avoids getting the *wrong* result. As such, it's a good minimal fix
> that can be applied to stable versions. For master, we can add the
> smb.conf-based stuff, that will support more cases.
>
> Review appreciated.
> Thanks,
> 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.

We're leaving the decision whether to provide mapping for unresolved SIDs to the
idmap backend, which can lead to interesting issues. Eg, with the following
config:


        idmap config * : backend = autorid
        idmap config * : range = 1000000-1999999

        idmap config FOO : backend = rid
        idmap config FOO : range = 2000000 - 2999999

a sid2xid request for a SID from domain FOO that doesn't exist in the domain
may end up in the default idmap backend where autorid would happily assign a
range and provide a mapping.

I tried to address this issue in an earlier fix
d5af3f3b6565da624fe6f6e4cbea818392c0c68f for bug 11961. Unfortunately the fix
was not quite correct (to say the least...) and also introduced a similar
problem with idmap_rid: bug 12597.

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:

<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/winbindd-lookupsids-sids2xids>

> From 3fcf7130a086cb3c6bc3dc444dd1010d8078ae29 Mon Sep 17 00:00:00 2001
> From: Uri Simchoni <[hidden email]>
> Date: Fri, 17 Mar 2017 23:58:35 +0200
> Subject: [PATCH 4/4] winbindd-sids2xids: reliably determine idmap backend name
>
> For SIDs which belong to domains, determine the backend name
> by the domain name of "domain users" SID from the same domain.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=12702
>
> Signed-off-by: Uri Simchoni <[hidden email]>
> ---
>  source3/winbindd/wb_sids2xids.c | 61 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
> index 9da08d2..ea0d9c7 100644
> --- a/source3/winbindd/wb_sids2xids.c
> +++ b/source3/winbindd/wb_sids2xids.c
> @@ -207,6 +207,11 @@ static bool wb_add_domain_users_to_lookup(TALLOC_CTX *ctx,
>  }
>  
>  static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type);
> +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);
>  static struct wbint_TransIDArray *wb_sids2xids_extract_for_domain_index(
>   TALLOC_CTX *mem_ctx, const struct wbint_TransIDArray *src,
>   uint32_t domain_index);
> @@ -238,7 +243,8 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
>  
>   for (i=0; i<state->num_non_cached; i++) {
>   struct dom_sid dom_sid;
> - struct lsa_DomainInfo *info;
> + const char *dom_name = wb_get_idmap_backend_name(
> +    domains, names, state->non_cached, i);
>   struct lsa_TranslatedName *n = &names->names[i];
>   struct wbint_TransID *t = &state->ids.ids[i];
>   int domain_index;
> @@ -246,11 +252,10 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
>   sid_copy(&dom_sid, &state->non_cached[i]);
>   sid_split_rid(&dom_sid, &t->rid);
>  
> - info = &domains->domains[n->sid_index];
>   t->type = lsa_SidType_to_id_type(n->sid_type);
>  
>   domain_index = init_lsa_ref_domain_list(
> - state, &state->idmap_doms, info->name.string, &dom_sid);
> +    state, &state->idmap_doms, dom_name, &dom_sid);
>   if (domain_index == -1) {
>   tevent_req_oom(req);
>   return;
> @@ -309,6 +314,56 @@ static enum id_type lsa_SidType_to_id_type(const enum lsa_SidType sid_type)
>   return type;
>  }
>  
> +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().

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
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?

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

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

> Cheerio!
> -slow
>

V2 to follow...

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Uri,

>> The fix finds the domain of the SID by resolving a SID with same domain
>> component and an RID of 513 (domain users), which hopefully never gets
>> migrated.

I think we should better try to resolve the domain sid, instead
of relying on RID 513.

And we should only do that if we don't know about the domain yet.

>> We've discussed other means such as smb.conf stuff or netsamlogon - I
>> think those methods can come on top of this method, because if they
>> don't work we should always fall back to something. The added resolving
>> doesn't cost much because it's in the same round-trip.
>>
>> The key thing about this fix is that doesn't try to translate sid->xid
>> in any possible case (such as when old domain is gone and forgotten), it
>> just avoids getting the *wrong* result. As such, it's a good minimal fix
>> that can be applied to stable versions. For master, we can add the
>> smb.conf-based stuff, that will support more cases.
>>
>> Review appreciated.
>> Thanks,
>> 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.
I think this is related...

I'm wondering if your fixes would also fix Uri's problem.

At least we should carefully think about this and have one
combined and tested patchset.

Otherwise both of you have tested something that won't reflect the reality.

Uri, can you run a command like this:
bin/rpcclient -UW4EDOM-L4\\administrator%A1b2C3d4
w2008r2-133.w4edom-l4.base -c 'lookupsids
S-1-5-21-278041429-3399921908-1452754838-66666
S-1-5-21-278041429-3399921908-1452754838
S-1-5-21-278041429-3399921908-1452754837-77777
S-1-5-21-278041429-3399921908-1452754837 S-1-5-32-66666 S-1-5-32
S-1-5-32-544' -d 10

That tries to resolve the primary sid of a user, the sid history value
and both domain sids and invalid sids in both domains at the same time
(in various order combinations)?
I guess that will help a lot to see the answers from a Windows DC in that
case.

Thanks!
metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi metze,

On Tue, Mar 28, 2017 at 02:11:20PM +0200, Stefan Metzmacher wrote:
> >> The fix finds the domain of the SID by resolving a SID with same domain
> >> component and an RID of 513 (domain users), which hopefully never gets
> >> migrated.
>
> I think we should better try to resolve the domain sid, instead
> of relying on RID 513.

yup.

> >> We've discussed other means such as smb.conf stuff or netsamlogon - I
> >> think those methods can come on top of this method, because if they
> >> don't work we should always fall back to something. The added resolving
> >> doesn't cost much because it's in the same round-trip.
> >>
> >> The key thing about this fix is that doesn't try to translate sid->xid
> >> in any possible case (such as when old domain is gone and forgotten), it
> >> just avoids getting the *wrong* result. As such, it's a good minimal fix
> >> that can be applied to stable versions. For master, we can add the
> >> smb.conf-based stuff, that will support more cases.
> >>
> >> Review appreciated.
> >> Thanks,
> >> 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.
>
> I think this is related...
>
> I'm wondering if your fixes would also fix Uri's problem.

not quite. Uri is fixing something that is cause by lookupsid() returning a
name.

I'm trying to address problems that arise if the lookupsid() fais in the first
place. So my patches won't fix what Uri is trying to address at all.

> At least we should carefully think about this and have one
> combined and tested patchset.

Tests would be nice. I'll add tests to my patches later on. Would it be possible
to have tests for the sid-history case as well? Please, please, please.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Am 28.03.2017 um 14:18 schrieb Ralph Böhme:

> Hi metze,
>
> On Tue, Mar 28, 2017 at 02:11:20PM +0200, Stefan Metzmacher wrote:
>>>> The fix finds the domain of the SID by resolving a SID with same domain
>>>> component and an RID of 513 (domain users), which hopefully never gets
>>>> migrated.
>>
>> I think we should better try to resolve the domain sid, instead
>> of relying on RID 513.
>
> yup.
>
>>>> We've discussed other means such as smb.conf stuff or netsamlogon - I
>>>> think those methods can come on top of this method, because if they
>>>> don't work we should always fall back to something. The added resolving
>>>> doesn't cost much because it's in the same round-trip.
>>>>
>>>> The key thing about this fix is that doesn't try to translate sid->xid
>>>> in any possible case (such as when old domain is gone and forgotten), it
>>>> just avoids getting the *wrong* result. As such, it's a good minimal fix
>>>> that can be applied to stable versions. For master, we can add the
>>>> smb.conf-based stuff, that will support more cases.
>>>>
>>>> Review appreciated.
>>>> Thanks,
>>>> 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.
>>
>> I think this is related...
>>
>> I'm wondering if your fixes would also fix Uri's problem.
>
> not quite. Uri is fixing something that is cause by lookupsid() returning a
> name.
>
> I'm trying to address problems that arise if the lookupsid() fais in the first
> place. So my patches won't fix what Uri is trying to address at all.
I mean in both cases we have the bug that we get the
domain name wrong and use the wrong idmap backend.

>> At least we should carefully think about this and have one
>> combined and tested patchset.
>
> Tests would be nice. I'll add tests to my patches later on. Would it be possible
> to have tests for the sid-history case as well? Please, please, please.

I have some wip code for sIDHistory, but that's not ready for master.

I mainly meant manual tests.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 03/28/2017 03:11 PM, Stefan Metzmacher wrote:

> Hi Uri,
>
>>> The fix finds the domain of the SID by resolving a SID with same domain
>>> component and an RID of 513 (domain users), which hopefully never gets
>>> migrated.
>
> I think we should better try to resolve the domain sid, instead
> of relying on RID 513.
>
> And we should only do that if we don't know about the domain yet.
>
>>> We've discussed other means such as smb.conf stuff or netsamlogon - I
>>> think those methods can come on top of this method, because if they
>>> don't work we should always fall back to something. The added resolving
>>> doesn't cost much because it's in the same round-trip.
>>>
>>> The key thing about this fix is that doesn't try to translate sid->xid
>>> in any possible case (such as when old domain is gone and forgotten), it
>>> just avoids getting the *wrong* result. As such, it's a good minimal fix
>>> that can be applied to stable versions. For master, we can add the
>>> smb.conf-based stuff, that will support more cases.
>>>
>>> Review appreciated.
>>> Thanks,
>>> 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.
>
> I think this is related...
>
> I'm wondering if your fixes would also fix Uri's problem.
>
> At least we should carefully think about this and have one
> combined and tested patchset.
>
> Otherwise both of you have tested something that won't reflect the reality.
>
> Uri, can you run a command like this:
> bin/rpcclient -UW4EDOM-L4\\administrator%A1b2C3d4
> w2008r2-133.w4edom-l4.base -c 'lookupsids
> S-1-5-21-278041429-3399921908-1452754838-66666
> S-1-5-21-278041429-3399921908-1452754838
> S-1-5-21-278041429-3399921908-1452754837-77777
> S-1-5-21-278041429-3399921908-1452754837 S-1-5-32-66666 S-1-5-32
> S-1-5-32-544' -d 10
>
> That tries to resolve the primary sid of a user, the sid history value
> and both domain sids and invalid sids in both domains at the same time
> (in various order combinations)?
> I guess that will help a lot to see the answers from a Windows DC in that
> case.
>
> Thanks!
> metze
>
One of my DCs was moved somewhere, can't find it right now, will sort
this out tomorrow. So meanwhile I queried just one DC, in various
combinations - all provided the same results.

See attached Python script and its output. I'll extend that to work vs
two DCs of both domains simultaneously- throw some of the combinations
on one and some on the other.

Thanks,
Uri.

resolve.py (1K) Download Attachment
res.txt (83K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 03/28/2017 11:08 PM, Uri Simchoni via samba-technical wrote:

> On 03/28/2017 03:11 PM, Stefan Metzmacher wrote:
>> Hi Uri,
>>
>>>> The fix finds the domain of the SID by resolving a SID with same domain
>>>> component and an RID of 513 (domain users), which hopefully never gets
>>>> migrated.
>>
>> I think we should better try to resolve the domain sid, instead
>> of relying on RID 513.
>>
>> And we should only do that if we don't know about the domain yet.
>>
>>>> We've discussed other means such as smb.conf stuff or netsamlogon - I
>>>> think those methods can come on top of this method, because if they
>>>> don't work we should always fall back to something. The added resolving
>>>> doesn't cost much because it's in the same round-trip.
>>>>
>>>> The key thing about this fix is that doesn't try to translate sid->xid
>>>> in any possible case (such as when old domain is gone and forgotten), it
>>>> just avoids getting the *wrong* result. As such, it's a good minimal fix
>>>> that can be applied to stable versions. For master, we can add the
>>>> smb.conf-based stuff, that will support more cases.
>>>>
>>>> Review appreciated.
>>>> Thanks,
>>>> 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.
>>
>> I think this is related...
>>
>> I'm wondering if your fixes would also fix Uri's problem.
>>
>> At least we should carefully think about this and have one
>> combined and tested patchset.
>>
>> Otherwise both of you have tested something that won't reflect the reality.
>>
>> Uri, can you run a command like this:
>> bin/rpcclient -UW4EDOM-L4\\administrator%A1b2C3d4
>> w2008r2-133.w4edom-l4.base -c 'lookupsids
>> S-1-5-21-278041429-3399921908-1452754838-66666
>> S-1-5-21-278041429-3399921908-1452754838
>> S-1-5-21-278041429-3399921908-1452754837-77777
>> S-1-5-21-278041429-3399921908-1452754837 S-1-5-32-66666 S-1-5-32
>> S-1-5-32-544' -d 10
>>
>> That tries to resolve the primary sid of a user, the sid history value
>> and both domain sids and invalid sids in both domains at the same time
>> (in various order combinations)?
>> I guess that will help a lot to see the answers from a Windows DC in that
>> case.
>>
>> Thanks!
>> metze
>>
> One of my DCs was moved somewhere, can't find it right now, will sort
> this out tomorrow. So meanwhile I queried just one DC, in various
> combinations - all provided the same results.
>
> See attached Python script and its output. I'll extend that to work vs
> two DCs of both domains simultaneously- throw some of the combinations
> on one and some on the other.
>
> Thanks,
> Uri.
>

I also added same queries to the DC of the other domain, and in
parallel, and with permutations, all yielded same result.

However, all the SIDs I asked about belonged to DOMAIN2 (primary SID or
sid history SID). When adding SIDs of DOMAIN1 to the mix, I got one
peculiar finding, namely that one of the DC's is unable to resolve sid's
which are in sid history of another domain, while the other DC can:

My domains:
uri-vgw-6:~ # wbinfo -n 'DOMAIN1\'
S-1-5-21-3293503978-489118715-2763867031 SID_DOMAIN (3)
uri-vgw-6:~ # wbinfo -n 'DOMAIN2\'
S-1-5-21-1387724271-3540671778-1971508351 SID_DOMAIN (3)

(DOMAIN1 is forest root)

The DC that can:

Ask DOMAIN1 about primary SID in DOMAIN2:
uri-vgw-6:~ # rpcclient dom1.domain1.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-1387724271-3540671778-1971508351-1115'
S-1-5-21-1387724271-3540671778-1971508351-1115 DOMAIN2\d1u1 (1)

Ask DOMAIN1 about sid-history in DOMAIN2:
uri-vgw-6:~ # rpcclient dom1.domain1.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-3293503978-489118715-2763867031-1106'
S-1-5-21-3293503978-489118715-2763867031-1106 DOMAIN2\d1u1 (1)

Ask DOMAIN1 about primary SID in DOMAIN1:
uri-vgw-6:~ # rpcclient dom1.domain1.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-3293503978-489118715-2763867031-1114'
S-1-5-21-3293503978-489118715-2763867031-1114 DOMAIN1\d1u8 (1)

Ask DOMAIN1 about sid-history in DOMAIN1:
uri-vgw-6:~ # rpcclient dom1.domain1.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-1387724271-3540671778-1971508351-1114'
S-1-5-21-1387724271-3540671778-1971508351-1114 DOMAIN1\d2u10 (1)
uri-vgw-6:~ #

Now same questions to DOMAIN2, last one returns unknown:

uri-vgw-6:~ # rpcclient dom2.domain2.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-1387724271-3540671778-1971508351-1115'
S-1-5-21-1387724271-3540671778-1971508351-1115 DOMAIN2\d1u1 (1)

uri-vgw-6:~ # rpcclient dom2.domain2.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-3293503978-489118715-2763867031-1106'
S-1-5-21-3293503978-489118715-2763867031-1106 DOMAIN2\d1u1 (1)

uri-vgw-6:~ # rpcclient dom2.domain2.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-3293503978-489118715-2763867031-1114'
S-1-5-21-3293503978-489118715-2763867031-1114 DOMAIN1\d1u8 (1)

uri-vgw-6:~ # rpcclient dom2.domain2.local -U
'administrator%activenas08!' -c 'lookupsids
S-1-5-21-1387724271-3540671778-1971508351-1114'
S-1-5-21-1387724271-3540671778-1971508351-1114 *unknown*\*unknown* (8)
uri-vgw-6:~ #


- There's a two-way trust between the DCs
- The one that *can* is 2008R2
- The one that can't is 2012R2

With respect to the sid-history fix though, what we know so far is that
a DC will give you the correct result or no result, and if we always
lookup in our domain, that's good enough IMHO.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi,

Here's a V2 of the patch set.

To address the concerns in the comments:
- Resolve domain sid instead of <dom-sid>-513 - done. Domain sids didn't
undergo "bulk resolving", so this is fixed too..

- Efficiency of finding domain name - done by just adding (exactly once)
the domain sid to the resolve list, instead of adding only if it's not
already there. This way we guarantee the domain sids are nicely tucked
at the end of the list, which narrows the search. This change may cause
a domain sid to appear twice in the lookup list. This works (on Windows
and Samba DCs), and IMHO, the extra complexity of avoiding it is not
worth it because it should be a rare-if-non-existent case to do
sid-to-xid for a domain sid.

- Shouldn't lookup domain name if we know it already - not handled.
Since the domain lookup is added to the bulk lookup, I don't believe
doing so would have a noticeable impact. I would prefer to keep the fix
small (for backports) and add such an optimization when the possible
gain is to avoid the lookup entirely.

- Tests - As Metze said, we can't really test it yet (except manually).
The one thing I'm not entirely comfortable with and would add a test for
is that assertion that looking up domain SID twice in the lookup list
works - I can add a wbinfo test for that to make sure we don't regress,
if the concept is acceptable.

Thanks,
Uri.

On 03/20/2017 06:49 AM, Uri Simchoni via samba-technical wrote:

> Hi,
>
> We few weeks ago we've discussed SID history and id-mapping -
> https://lists.samba.org/archive/samba-technical/2017-February/118771.html.
>
> Attached is a proposed initial fix for the issue, which focuses on
> avoiding wrong results.
>
> The fix finds the domain of the SID by resolving a SID with same domain
> component and an RID of 513 (domain users), which hopefully never gets
> migrated.
>
> We've discussed other means such as smb.conf stuff or netsamlogon - I
> think those methods can come on top of this method, because if they
> don't work we should always fall back to something. The added resolving
> doesn't cost much because it's in the same round-trip.
>
> The key thing about this fix is that doesn't try to translate sid->xid
> in any possible case (such as when old domain is gone and forgotten), it
> just avoids getting the *wrong* result. As such, it's a good minimal fix
> that can be applied to stable versions. For master, we can add the
> smb.conf-based stuff, that will support more cases.
>
> Review appreciated.
> Thanks,
> Uri.
>


sidhist-master-v2.patch.txt (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
> Hi,
>
> Here's a V2 of the patch set.

just fyi, I have this on my list.

-slow

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 04/07/2017 08:32 PM, Ralph Böhme wrote:
> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
>> Hi,
>>
>> Here's a V2 of the patch set.
>
> just fyi, I have this on my list.
>
> -slow
>
Thanks.
Here's a rebased patch set, with 4/6 adapted to the recent change in
wb_lookupsids_bulk(). Otherwise the same.

Uri.

sidhist-master-v3.patch.txt (20K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:

> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
> > On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
> >> Hi,
> >>
> >> Here's a V2 of the patch set.
> >
> > just fyi, I have this on my list.
> >
> > -slow
> >
> Thanks.
> Here's a rebased patch set, with 4/6 adapted to the recent change in
> wb_lookupsids_bulk(). Otherwise the same.
looks like this still has the O(N^2) performance issue I mentioned or did I miss
anything?

Besides that, attached is a simple fix for sids2xids on-top of my proposed
cleanup for wb_lookupsids [1] that should already address the SID-history if the
domain is still around.

Your patch could go on-top as a more complete patch for the case that the
"historic domain" is behind a one-way trust (where we don't have the domain in
our list of trusted domains).

Thoughts?

-slow

[1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>

MAYBE-bug12702-master.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On 04/10/2017 08:24 PM, Ralph Böhme wrote:

> On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:
>> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
>>> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
>>>> Hi,
>>>>
>>>> Here's a V2 of the patch set.
>>>
>>> just fyi, I have this on my list.
>>>
>>> -slow
>>>
>> Thanks.
>> Here's a rebased patch set, with 4/6 adapted to the recent change in
>> wb_lookupsids_bulk(). Otherwise the same.
>
> looks like this still has the O(N^2) performance issue I mentioned or did I miss
> anything?
>
> Besides that, attached is a simple fix for sids2xids on-top of my proposed
> cleanup for wb_lookupsids [1] that should already address the SID-history if the
> domain is still around.
>
> Your patch could go on-top as a more complete patch for the case that the
> "historic domain" is behind a one-way trust (where we don't have the domain in
> our list of trusted domains).
>
> Thoughts?
>
> -slow
>
> [1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>
>

Pushed the simple fix, I think for backports we can use just "no
match->default domain", or, if we want the bugfix to be based on
backported patches, only backport [1/6] of the recent simplification
patch set and the "don_sid_in_donain" fix.

Regarding my patch and O(N^2) - the search is O(NxM) where M is the
number of added domain sids in the lookup. Notice that the domain sids
are added, unconditionally, to the back of the resolving list, and the
subsequent search is over the added stuff only.

Please indicate if you'd like me to rebase this patch set on top of
recent commits or drop this - as far as I'm concerned, the main
objective of not polluting the id-mapping cache is achieved, and
anything else is just trying to get it right for more cases.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
On Wed, Apr 12, 2017 at 11:46:19AM +0300, Uri Simchoni wrote:

> On 04/10/2017 08:24 PM, Ralph Böhme wrote:
> > On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:
> >> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
> >>> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
> >>>> Hi,
> >>>>
> >>>> Here's a V2 of the patch set.
> >>>
> >>> just fyi, I have this on my list.
> >>>
> >>> -slow
> >>>
> >> Thanks.
> >> Here's a rebased patch set, with 4/6 adapted to the recent change in
> >> wb_lookupsids_bulk(). Otherwise the same.
> >
> > looks like this still has the O(N^2) performance issue I mentioned or did I miss
> > anything?
> >
> > Besides that, attached is a simple fix for sids2xids on-top of my proposed
> > cleanup for wb_lookupsids [1] that should already address the SID-history if the
> > domain is still around.
> >
> > Your patch could go on-top as a more complete patch for the case that the
> > "historic domain" is behind a one-way trust (where we don't have the domain in
> > our list of trusted domains).
> >
> > Thoughts?
> >
> > -slow
> >
> > [1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>
> >
>
> Pushed the simple fix, I think for backports we can use just "no
> match->default domain", or, if we want the bugfix to be based on
> backported patches, only backport [1/6] of the recent simplification
> patch set and the "don_sid_in_donain" fix.
backport for 4.5 and 4.6 attached. Can you test it?

> Regarding my patch and O(N^2) - the search is O(NxM) where M is the
> number of added domain sids in the lookup. Notice that the domain sids
> are added, unconditionally, to the back of the resolving list, and the
> subsequent search is over the added stuff only.
>
> Please indicate if you'd like me to rebase this patch set on top of
> recent commits or drop this - as far as I'm concerned, the main
> objective of not polluting the id-mapping cache is achieved, and
> anything else is just trying to get it right for more cases.

More correct is always good, but it comes at a cost: it makes the code slightly
more complicated. So given the prospect of moving the idmap backend decision to
be based on SIDs instead of domain names, which will also solve the more exotic
SID-history use-cases, maybe it makes sense to stop here.

-slow

bug12702-v45,v46.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Samba - samba-technical mailing list
Sorry for delaying my answer. This looks reasonable, and I'll manually
test it in a couple of days.

One remark: The find_domain_from_sid_noinit() in the prerequisite patch
is kind of against where we're heading, in that we don't want to rely on
that and get inconsistent results based on whether the domain has been
discovered already or not. In master, this is justified because it moved
from another place (i.e. it was not introduced by a the fix to this bug,
it was part of you dis-entangling lookupsids). Here we just add a new
lookup. The bug could well be fixed without the fallback. I don't mind
if it stays with the prerequisite patch as-is, to make things common, I
just don't want it to appear like we're headed in different directions...

Thanks,
Uri.

On 04/19/2017 12:15 AM, Ralph Böhme via samba-technical wrote:

> On Wed, Apr 12, 2017 at 11:46:19AM +0300, Uri Simchoni wrote:
>> On 04/10/2017 08:24 PM, Ralph Böhme wrote:
>>> On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:
>>>> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
>>>>> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Here's a V2 of the patch set.
>>>>>
>>>>> just fyi, I have this on my list.
>>>>>
>>>>> -slow
>>>>>
>>>> Thanks.
>>>> Here's a rebased patch set, with 4/6 adapted to the recent change in
>>>> wb_lookupsids_bulk(). Otherwise the same.
>>>
>>> looks like this still has the O(N^2) performance issue I mentioned or did I miss
>>> anything?
>>>
>>> Besides that, attached is a simple fix for sids2xids on-top of my proposed
>>> cleanup for wb_lookupsids [1] that should already address the SID-history if the
>>> domain is still around.
>>>
>>> Your patch could go on-top as a more complete patch for the case that the
>>> "historic domain" is behind a one-way trust (where we don't have the domain in
>>> our list of trusted domains).
>>>
>>> Thoughts?
>>>
>>> -slow
>>>
>>> [1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>
>>>
>>
>> Pushed the simple fix, I think for backports we can use just "no
>> match->default domain", or, if we want the bugfix to be based on
>> backported patches, only backport [1/6] of the recent simplification
>> patch set and the "don_sid_in_donain" fix.
>
> backport for 4.5 and 4.6 attached. Can you test it?
>
>> Regarding my patch and O(N^2) - the search is O(NxM) where M is the
>> number of added domain sids in the lookup. Notice that the domain sids
>> are added, unconditionally, to the back of the resolving list, and the
>> subsequent search is over the added stuff only.
>>
>> Please indicate if you'd like me to rebase this patch set on top of
>> recent commits or drop this - as far as I'm concerned, the main
>> objective of not polluting the id-mapping cache is achieved, and
>> anything else is just trying to get it right for more cases.
>
> More correct is always good, but it comes at a cost: it makes the code slightly
> more complicated. So given the prospect of moving the idmap backend decision to
> be based on SIDs instead of domain names, which will also solve the more exotic
> SID-history use-cases, maybe it makes sense to stop here.
>
> -slow
>


Loading...