Quantcast

[PATCH] winbindd: error handling in rpc_try_lookup_sids3()

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

[PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
Hi!

Attached is another winbindd fix:
<https://bugzilla.samba.org/show_bug.cgi?id=12728>

Please review & push if ok. Thanks!

Cheerio!
-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Fri, Mar 31, 2017 at 10:53:30PM +0200, Ralph Böhme via samba-technical wrote:
> Hi!
>
> Attached is another winbindd fix:
> <https://bugzilla.samba.org/show_bug.cgi?id=12728>
>
> Please review & push if ok. Thanks!

Can I ask a question about this one please ?

NT_STATUS_SOME_NOT_MAPPED == NT_STATUS(0x107)
NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)

and:

#define NT_STATUS_IS_ERR(x) (unlikely((NT_STATUS_V(x) & 0xc0000000) == 0xc0000000))

So looking at the NT_STATUS_IS_ERR check we have:

NT_STATUS_IS_ERR(NT_STATUS_SOME_NOT_MAPPED) == false
NT_STATUS_IS_ERR(NT_STATUS_NONE_MAPPED) == true

i.e. Only NT_STATUS_NONE_MAPPED would return an
error here.

Can you explain why the change now treats
NT_STATUS_NONE_MAPPED as equal to NT_STATUS_SOME_NOT_MAPPED ?

Should I go look in the MS-LSA doc ? :-).

Also, the function below:

rpc_lookup_sids() has exactly the same logic as that
in rpc_try_lookup_sids3(). Should we do the same
change there ? If not, why not ?

Feel free to tell me to go read MS-LSA :-).

Cheers,

        Jeremy.

> From 2bd779b0f7b20ff7fcf5c0fb57509bcce053a62d Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <[hidden email]>
> Date: Sun, 26 Mar 2017 08:22:13 +0200
> Subject: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()
>
> NT_STATUS_NONE_MAPPED and NT_STATUS_SOME_NOT_MAPPED should not be
> treated as fatal error. We should continue processing the results and
> not bail out.
>
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728
>
> Signed-off-by: Ralph Boehme <[hidden email]>
> ---
>  source3/winbindd/winbindd_rpc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/source3/winbindd/winbindd_rpc.c b/source3/winbindd/winbindd_rpc.c
> index 3dd4f77..3763738 100644
> --- a/source3/winbindd/winbindd_rpc.c
> +++ b/source3/winbindd/winbindd_rpc.c
> @@ -981,9 +981,14 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
>   if (!NT_STATUS_IS_OK(status)) {
>   return status;
>   }
> - if (NT_STATUS_IS_ERR(result)) {
> - return result;
> +
> + if (!NT_STATUS_EQUAL(result, NT_STATUS_NONE_MAPPED) &&
> +    !NT_STATUS_EQUAL(result, NT_STATUS_SOME_NOT_MAPPED)) {
> + if (NT_STATUS_IS_ERR(result)) {
> + return result;
> + }
>   }
> +
>   if (sids->num_sids != lsa_names2.count) {
>   return NT_STATUS_INVALID_NETWORK_RESPONSE;
>   }
> @@ -1010,7 +1015,7 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
>   return NT_STATUS_INVALID_NETWORK_RESPONSE;
>   }
>   }
> - return result;
> + return NT_STATUS_OK;
>  }
>  
>  NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
> --
> 2.9.3
>


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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:

> On Fri, Mar 31, 2017 at 10:53:30PM +0200, Ralph Böhme via samba-technical wrote:
> > Hi!
> >
> > Attached is another winbindd fix:
> > <https://bugzilla.samba.org/show_bug.cgi?id=12728>
> >
> > Please review & push if ok. Thanks!
>
> Can I ask a question about this one please ?
>
> NT_STATUS_SOME_NOT_MAPPED == NT_STATUS(0x107)
> NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)
>
> and:
>
> #define NT_STATUS_IS_ERR(x) (unlikely((NT_STATUS_V(x) & 0xc0000000) == 0xc0000000))
>
> So looking at the NT_STATUS_IS_ERR check we have:
>
> NT_STATUS_IS_ERR(NT_STATUS_SOME_NOT_MAPPED) == false
> NT_STATUS_IS_ERR(NT_STATUS_NONE_MAPPED) == true
>
> i.e. Only NT_STATUS_NONE_MAPPED would return an
> error here.

yes, but it shouldn't.

> Can you explain why the change now treats
> NT_STATUS_NONE_MAPPED as equal to NT_STATUS_SOME_NOT_MAPPED ?

Because it *is* equal. :) The only difference is that with NT_STATUS_NONE_MAPPED
count will be 0 and with NT_STATUS_SOME_NOT_MAPPED count will be at least 1.

Because even with NT_STATUS_NONE_MAPPED we get a lsa_TransNameArray that we
should process and return to our caller. Eg, running this against a Windows 2016
server:

$ sudo ./bin/rpcclient -U Administrator%Passw0rd 10.10.11.101 -c "lookupsids S-1-5-21-666" -d 10
---8<---
     lsa_LookupSids: struct lsa_LookupSids
        in: struct lsa_LookupSids
            handle                   : *
                handle: struct policy_handle
                    handle_type              : 0x00000000 (0)
                    uuid                     : f8df0a26-ee79-4c1f-91e2-b56fb8269b44
            sids                     : *
                sids: struct lsa_SidArray
                    num_sids                 : 0x00000001 (1)
                    sids                     : *
                        sids: ARRAY(1)
                            sids: struct lsa_SidPtr
                                sid                      : *
                                    sid                      : S-1-5-21-666

...

        out: struct lsa_LookupSids
            domains                  : *
                domains                  : *
                    domains: struct lsa_RefDomainList
                        count                    : 0x00000000 (0)
                        domains                  : NULL
                        max_size                 : 0x00000000 (0)
            names                    : *
                names: struct lsa_TransNameArray
                    count                    : 0x00000001 (1)
                    names                    : *
                        names: ARRAY(1)
                            names: struct lsa_TranslatedName
                                sid_type                 : SID_NAME_UNKNOWN (8)
                                name: struct lsa_String
                                    length                   : 0x0018 (24)
                                    size                     : 0x001a (26)
                                    string                   : *
                                        string                   : 'S-1-5-21-666'
                                sid_index                : 0xffffffff (4294967295)
            count                    : *
                count                    : 0x00000000 (0)
            result                   : NT_STATUS_NONE_MAPPED
LSA_LOOKUPSIDS returned status: 'NT_STATUS_OK', result: 'NT_STATUS_NONE_MAPPED', mapped count = 0'
---8<---

So we got a lsa_TransNameArray and we must treat this the same way we handle a
failed mapping in the NT_STATUS_SOME_NOT_MAPPED case.

> Should I go look in the MS-LSA doc ? :-).

3.1.4.9 at the very end:

If LookupLevel is LsapLookupWksta, and the return code can be identified as an
error value other than STATUS_NONE_MAPPED, ReferencedDomains and the Names
fields in the TranslatedNames structure MUST NOT be returned.

> Also, the function below:
>
> rpc_lookup_sids() has exactly the same logic as that
> in rpc_try_lookup_sids3(). Should we do the same
> change there ? If not, why not ?

Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
dcerpc_lsa_lookup_sids_noalloc().

Let's see what happens when I try to fix those as well... :)

Cheerio!
-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
What about changing
     NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)
to
     NT_STATUS_NONE_MAPPED == NT_STATUS(0x73)
to not be an error (like NT_STATUS_SOME_NOT_MAPPED)?

On 4/1/17 08:21, Ralph Böhme via samba-technical wrote:

> On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
>> On Fri, Mar 31, 2017 at 10:53:30PM +0200, Ralph Böhme via samba-technical wrote:
>>> Hi!
>>>
>>> Attached is another winbindd fix:
>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__bugzilla.samba.org_show-5Fbug.cgi-3Fid-3D12728&d=DwIDaQ&c=y2w-uYmhgFWijp_IQN0DhA&r=Ru1enaR19oK8d7j6CTyYvKyHRw2ZzjrSu2PJyn0fKAQ&m=O33NyuJ35pSENWKa-5d_aJBTvKCueC85OiaFLTxDUd0&s=cYKQBp3QB0b__fr72QrTT-ZxgX7vAGhTS22iyY9QjOc&e= >
>>>
>>> Please review & push if ok. Thanks!
>> Can I ask a question about this one please ?
>>
>> NT_STATUS_SOME_NOT_MAPPED == NT_STATUS(0x107)
>> NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)
>>
>> and:
>>
>> #define NT_STATUS_IS_ERR(x) (unlikely((NT_STATUS_V(x) & 0xc0000000) == 0xc0000000))
>>
>> So looking at the NT_STATUS_IS_ERR check we have:
>>
>> NT_STATUS_IS_ERR(NT_STATUS_SOME_NOT_MAPPED) == false
>> NT_STATUS_IS_ERR(NT_STATUS_NONE_MAPPED) == true
>>
>> i.e. Only NT_STATUS_NONE_MAPPED would return an
>> error here.
> yes, but it shouldn't.
>
>> Can you explain why the change now treats
>> NT_STATUS_NONE_MAPPED as equal to NT_STATUS_SOME_NOT_MAPPED ?
> Because it *is* equal. :) The only difference is that with NT_STATUS_NONE_MAPPED
> count will be 0 and with NT_STATUS_SOME_NOT_MAPPED count will be at least 1.
>
> Because even with NT_STATUS_NONE_MAPPED we get a lsa_TransNameArray that we
> should process and return to our caller. Eg, running this against a Windows 2016
> server:
>
> $ sudo ./bin/rpcclient -U Administrator%Passw0rd 10.10.11.101 -c "lookupsids S-1-5-21-666" -d 10
> ---8<---
>       lsa_LookupSids: struct lsa_LookupSids
>          in: struct lsa_LookupSids
>              handle                   : *
>                  handle: struct policy_handle
>                      handle_type              : 0x00000000 (0)
>                      uuid                     : f8df0a26-ee79-4c1f-91e2-b56fb8269b44
>              sids                     : *
>                  sids: struct lsa_SidArray
>                      num_sids                 : 0x00000001 (1)
>                      sids                     : *
>                          sids: ARRAY(1)
>                              sids: struct lsa_SidPtr
>                                  sid                      : *
>                                      sid                      : S-1-5-21-666
>
> ...
>
>          out: struct lsa_LookupSids
>              domains                  : *
>                  domains                  : *
>                      domains: struct lsa_RefDomainList
>                          count                    : 0x00000000 (0)
>                          domains                  : NULL
>                          max_size                 : 0x00000000 (0)
>              names                    : *
>                  names: struct lsa_TransNameArray
>                      count                    : 0x00000001 (1)
>                      names                    : *
>                          names: ARRAY(1)
>                              names: struct lsa_TranslatedName
>                                  sid_type                 : SID_NAME_UNKNOWN (8)
>                                  name: struct lsa_String
>                                      length                   : 0x0018 (24)
>                                      size                     : 0x001a (26)
>                                      string                   : *
>                                          string                   : 'S-1-5-21-666'
>                                  sid_index                : 0xffffffff (4294967295)
>              count                    : *
>                  count                    : 0x00000000 (0)
>              result                   : NT_STATUS_NONE_MAPPED
> LSA_LOOKUPSIDS returned status: 'NT_STATUS_OK', result: 'NT_STATUS_NONE_MAPPED', mapped count = 0'
> ---8<---
>
> So we got a lsa_TransNameArray and we must treat this the same way we handle a
> failed mapping in the NT_STATUS_SOME_NOT_MAPPED case.
>
>> Should I go look in the MS-LSA doc ? :-).
> 3.1.4.9 at the very end:
>
> If LookupLevel is LsapLookupWksta, and the return code can be identified as an
> error value other than STATUS_NONE_MAPPED, ReferencedDomains and the Names
> fields in the TranslatedNames structure MUST NOT be returned.
>
>> Also, the function below:
>>
>> rpc_lookup_sids() has exactly the same logic as that
>> in rpc_try_lookup_sids3(). Should we do the same
>> change there ? If not, why not ?
> Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
> dcerpc_lsa_lookup_sids_noalloc().
>
> Let's see what happens when I try to fix those as well... :)
>
> Cheerio!
> -slow
>


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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Sat, Apr 01, 2017 at 08:53:04AM -0400, Jim Brown via samba-technical wrote:
> What about changing
>     NT_STATUS_NONE_MAPPED == NT_STATUS(0xc0000073)
> to
>     NT_STATUS_NONE_MAPPED == NT_STATUS(0x73)
> to not be an error (like NT_STATUS_SOME_NOT_MAPPED)?

No, we can't do that. Anything with 0xC0000000 has
to be mapped as an error.

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Sat, Apr 01, 2017 at 02:21:14PM +0200, Ralph Böhme wrote:

> On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> > Also, the function below:
> >
> > rpc_lookup_sids() has exactly the same logic as that
> > in rpc_try_lookup_sids3(). Should we do the same
> > change there ? If not, why not ?
>
> Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
> dcerpc_lsa_lookup_sids_noalloc().
>
> Let's see what happens when I try to fix those as well... :)
ok, so how about this one? Fixes all lookupsids and lookupsids3 callers, adding
a new macro NT_STATUS_LOOKUP_ERR. It just passed a private autobuild.

I don't really think the macro name is a good choice, but I couldn't come up
with something else? Suggestions? It could be used for LSA and SAMR SID and name
lookups.

Cheerio!
-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Sat, Apr 01, 2017 at 10:53:24PM +0200, Ralph Böhme wrote:

> On Sat, Apr 01, 2017 at 02:21:14PM +0200, Ralph Böhme wrote:
> > On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> > > Also, the function below:
> > >
> > > rpc_lookup_sids() has exactly the same logic as that
> > > in rpc_try_lookup_sids3(). Should we do the same
> > > change there ? If not, why not ?
> >
> > Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
> > dcerpc_lsa_lookup_sids_noalloc().
> >
> > Let's see what happens when I try to fix those as well... :)
>
> ok, so how about this one? Fixes all lookupsids and lookupsids3 callers, adding
> a new macro NT_STATUS_LOOKUP_ERR. It just passed a private autobuild.
>
> I don't really think the macro name is a good choice, but I couldn't come up
> with something else? Suggestions? It could be used for LSA and SAMR SID and name
> lookups.

I don't like it either, but it's not easy to think of
good names. RB+. Pushed, and we can update later if
we think of a better name :-).

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Sat, Apr 01, 2017 at 08:48:06PM -0700, Jeremy Allison wrote:

> On Sat, Apr 01, 2017 at 10:53:24PM +0200, Ralph Böhme wrote:
> > On Sat, Apr 01, 2017 at 02:21:14PM +0200, Ralph Böhme wrote:
> > > On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> > > > Also, the function below:
> > > >
> > > > rpc_lookup_sids() has exactly the same logic as that
> > > > in rpc_try_lookup_sids3(). Should we do the same
> > > > change there ? If not, why not ?
> > >
> > > Yes, there are more places where it's done wrong: winbindd_lookup_sids() and
> > > dcerpc_lsa_lookup_sids_noalloc().
> > >
> > > Let's see what happens when I try to fix those as well... :)
> >
> > ok, so how about this one? Fixes all lookupsids and lookupsids3 callers, adding
> > a new macro NT_STATUS_LOOKUP_ERR. It just passed a private autobuild.
> >
> > I don't really think the macro name is a good choice, but I couldn't come up
> > with something else? Suggestions? It could be used for LSA and SAMR SID and name
> > lookups.
>
> I don't like it either, but it's not easy to think of
> good names. RB+. Pushed, and we can update later if
> we think of a better name :-).

thanks!

Looks like this one never hit autobuild? Or failed?

Cheerio!
-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Sunday, 2 April 2017 17:11:34 CEST Ralph Böhme via samba-technical wrote:

> On Sat, Apr 01, 2017 at 08:48:06PM -0700, Jeremy Allison wrote:
> > On Sat, Apr 01, 2017 at 10:53:24PM +0200, Ralph Böhme wrote:
> > > On Sat, Apr 01, 2017 at 02:21:14PM +0200, Ralph Böhme wrote:
> > > > On Fri, Mar 31, 2017 at 04:08:36PM -0700, Jeremy Allison wrote:
> > > > > Also, the function below:
> > > > >
> > > > > rpc_lookup_sids() has exactly the same logic as that
> > > > > in rpc_try_lookup_sids3(). Should we do the same
> > > > > change there ? If not, why not ?
> > > >
> > > > Yes, there are more places where it's done wrong:
> > > > winbindd_lookup_sids() and dcerpc_lsa_lookup_sids_noalloc().
> > > >
> > > > Let's see what happens when I try to fix those as well... :)
> > >
> > > ok, so how about this one? Fixes all lookupsids and lookupsids3 callers,
> > > adding a new macro NT_STATUS_LOOKUP_ERR. It just passed a private
> > > autobuild.
> > >
> > > I don't really think the macro name is a good choice, but I couldn't
> > > come up with something else? Suggestions? It could be used for LSA and
> > > SAMR SID and name lookups.
> >
> > I don't like it either, but it's not easy to think of
> > good names. RB+. Pushed, and we can update later if
> > we think of a better name :-).
>
> thanks!
>
> Looks like this one never hit autobuild? Or failed?

I had the same lately, I pushed job to autobuild and after some time the build
simply vanished. No build error mail!



        Andreas

--
Andreas Schneider                   GPG-ID: CC014E3D
Samba Team                             [hidden email]
www.samba.org

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Mon, Apr 03, 2017 at 11:49:36AM -0700, Jeremy Allison wrote:

> Ralph, trying to push and I'm getting:
>
> [655(3442)/2083 at 1h12s] idmap.alloc(ad_member_rfc2307)
> Domain SAMBADOMAIN has SID S-1-5-21-3583314470-906957706-621714972
> id: 66666: no such user
> failed to call wbcStringToSid: WBC_ERR_INVALID_SID
> Could not lookup sid S-1-5-21-3583314470-906957706-621714972\66666
> Using non-existing SID S-1-5-21-3583314470-906957706-621714972-66666 to check no id allocation is done by the backend
> wbinfo returned: S-1-5-21-3583314470-906957706-621714972-66666 -> uid/gid 1166666
> UNEXPECTED(failure): idmap.alloc.wbinfo SID to xid returns unmapped for unknown SID(ad_member_rfc2307)
> REASON: Exception: Exception:
>
> Can you take a look (sorry).
sorry for the hassle! Was only running make test TESTS=wbinfo on the last
incarnation as I already have full private autobuilds running with additional
sids2xids patches on top.

The good thing is, that this actually found another hidden small bug. Patch 4/5
fixes it.

This one passed a full autobuild (hopefully I ran it with the correct sha...).

Please review & push if ok.

Cheerio!
-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
Am 04.04.2017 um 12:50 schrieb Ralph Böhme via samba-technical:

> On Mon, Apr 03, 2017 at 11:49:36AM -0700, Jeremy Allison wrote:
>> Ralph, trying to push and I'm getting:
>>
>> [655(3442)/2083 at 1h12s] idmap.alloc(ad_member_rfc2307)
>> Domain SAMBADOMAIN has SID S-1-5-21-3583314470-906957706-621714972
>> id: 66666: no such user
>> failed to call wbcStringToSid: WBC_ERR_INVALID_SID
>> Could not lookup sid S-1-5-21-3583314470-906957706-621714972\66666
>> Using non-existing SID S-1-5-21-3583314470-906957706-621714972-66666 to check no id allocation is done by the backend
>> wbinfo returned: S-1-5-21-3583314470-906957706-621714972-66666 -> uid/gid 1166666
>> UNEXPECTED(failure): idmap.alloc.wbinfo SID to xid returns unmapped for unknown SID(ad_member_rfc2307)
>> REASON: Exception: Exception:
>>
>> Can you take a look (sorry).
>
> sorry for the hassle! Was only running make test TESTS=wbinfo on the last
> incarnation as I already have full private autobuilds running with additional
> sids2xids patches on top.
>
> The good thing is, that this actually found another hidden small bug. Patch 4/5
> fixes it.
Please don't push this, it's the wrong fix.

The fundamental problem is that I didn't understood that
find_lookup_domain_from_sid() always returns the primary domain
for all remote domains when we discussed commit
9be918116e356c358ef77cc2933e471090088293.

We need to use find_domain_from_sid_noinit() to get a possible fallback
for the domain name if wb_lookupsid_recv() fails.
state->single_domains[state->single_sids_done] is most likely wrong.

metze


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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Tue, Apr 04, 2017 at 02:13:37PM +0200, Stefan Metzmacher wrote:

> Am 04.04.2017 um 12:50 schrieb Ralph Böhme via samba-technical:
> > On Mon, Apr 03, 2017 at 11:49:36AM -0700, Jeremy Allison wrote:
> >> Ralph, trying to push and I'm getting:
> >>
> >> [655(3442)/2083 at 1h12s] idmap.alloc(ad_member_rfc2307)
> >> Domain SAMBADOMAIN has SID S-1-5-21-3583314470-906957706-621714972
> >> id: 66666: no such user
> >> failed to call wbcStringToSid: WBC_ERR_INVALID_SID
> >> Could not lookup sid S-1-5-21-3583314470-906957706-621714972\66666
> >> Using non-existing SID S-1-5-21-3583314470-906957706-621714972-66666 to check no id allocation is done by the backend
> >> wbinfo returned: S-1-5-21-3583314470-906957706-621714972-66666 -> uid/gid 1166666
> >> UNEXPECTED(failure): idmap.alloc.wbinfo SID to xid returns unmapped for unknown SID(ad_member_rfc2307)
> >> REASON: Exception: Exception:
> >>
> >> Can you take a look (sorry).
> >
> > sorry for the hassle! Was only running make test TESTS=wbinfo on the last
> > incarnation as I already have full private autobuilds running with additional
> > sids2xids patches on top.
> >
> > The good thing is, that this actually found another hidden small bug. Patch 4/5
> > fixes it.
>
> Please don't push this, it's the wrong fix.
Well, I'd still argue it's the correct fix, just for a different problem, but
anyway. :)

I'll resend this patchset once the below patchset, which is for a different bug,
is in master.

> The fundamental problem is that I didn't understood that
> find_lookup_domain_from_sid() always returns the primary domain
> for all remote domains when we discussed commit
> 9be918116e356c358ef77cc2933e471090088293.
>
> We need to use find_domain_from_sid_noinit() to get a possible fallback
> for the domain name if wb_lookupsid_recv() fails.
> state->single_domains[state->single_sids_done] is most likely wrong.

yup, and for that the proper fix is attached.

Original bug:
<https://bugzilla.samba.org/show_bug.cgi?id=11961>

Attempted fix: 9be918116e356c358ef77cc2933e471090088293

Another user found it doesn't work with a different idmap setup:
<https://bugzilla.samba.org/show_bug.cgi?id=12597>

(Hopefully) correct patch, including tests, attached. It did pass a private
autobuild.

-slow

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

Re: [PATCH] winbindd: error handling in rpc_try_lookup_sids3()

Samba - samba-technical mailing list
On Wed, Apr 05, 2017 at 07:47:02PM +0200, Ralph Böhme wrote:

> On Tue, Apr 04, 2017 at 02:13:37PM +0200, Stefan Metzmacher wrote:
> > Am 04.04.2017 um 12:50 schrieb Ralph Böhme via samba-technical:
> > > On Mon, Apr 03, 2017 at 11:49:36AM -0700, Jeremy Allison wrote:
> > >> Ralph, trying to push and I'm getting:
> > >>
> > >> [655(3442)/2083 at 1h12s] idmap.alloc(ad_member_rfc2307)
> > >> Domain SAMBADOMAIN has SID S-1-5-21-3583314470-906957706-621714972
> > >> id: 66666: no such user
> > >> failed to call wbcStringToSid: WBC_ERR_INVALID_SID
> > >> Could not lookup sid S-1-5-21-3583314470-906957706-621714972\66666
> > >> Using non-existing SID S-1-5-21-3583314470-906957706-621714972-66666 to check no id allocation is done by the backend
> > >> wbinfo returned: S-1-5-21-3583314470-906957706-621714972-66666 -> uid/gid 1166666
> > >> UNEXPECTED(failure): idmap.alloc.wbinfo SID to xid returns unmapped for unknown SID(ad_member_rfc2307)
> > >> REASON: Exception: Exception:
> > >>
> > >> Can you take a look (sorry).
> > >
> > > sorry for the hassle! Was only running make test TESTS=wbinfo on the last
> > > incarnation as I already have full private autobuilds running with additional
> > > sids2xids patches on top.
> > >
> > > The good thing is, that this actually found another hidden small bug. Patch 4/5
> > > fixes it.
> >
> > Please don't push this, it's the wrong fix.
>
> Well, I'd still argue it's the correct fix, just for a different problem, but
> anyway. :)
>
> I'll resend this patchset once the below patchset, which is for a different bug,
> is in master.
it is, so I'm adding the current version of this patchset. It just passed a
private autobuild.

> > The fundamental problem is that I didn't understood that
> > find_lookup_domain_from_sid() always returns the primary domain
> > for all remote domains when we discussed commit
> > 9be918116e356c358ef77cc2933e471090088293.
> >
> > We need to use find_domain_from_sid_noinit() to get a possible fallback
> > for the domain name if wb_lookupsid_recv() fails.
> > state->single_domains[state->single_sids_done] is most likely wrong.
>
> yup, and for that the proper fix is attached.
>
> Original bug:
> <https://bugzilla.samba.org/show_bug.cgi?id=11961>
>
> Attempted fix: 9be918116e356c358ef77cc2933e471090088293
>
> Another user found it doesn't work with a different idmap setup:
> <https://bugzilla.samba.org/show_bug.cgi?id=12597>
>
> (Hopefully) correct patch, including tests, attached. It did pass a private
> autobuild.
This is in master.

Cheerio!
-slow

bug12728-master.patch (5K) Download Attachment
Loading...