[PATCH] Allow GetDCNameEx to be called for arbitrary sites and trusted domains

[Garming Sam via samba-technical]

Hi,

While looking at automatic site coverage (which has already went
upstream), I noticed that our DC location calls in NETLOGON are
particularly bad. GetDCNameEx only returned WERR_NO_SUCH_DOMAIN when you
asked for a site that the DC did not exist in. Furthermore, it did not
bother checking the domain, which meant that as long as you supplied a
valid site name in both domains, you could get a DC in the wrong domain
returned.

In order to remedy a large amount of the behaviour, I've implemented a
winbind forward call which triggers the dsgetdcname calls (using CLDAP
and DNS) in the winbind DC locator process. This allows arbitrary sites
to be queried for, and by doing so, the trusted domain case also works too.

There are a number of other errors in the RPC call which I have had to
fix, including:

- Failing to forward the error code from GetDCNameEx2 to GetDCNameEx
when it is called internally.
- Returning a more appropriate client site name (and avoid a fallback
that would be wrong in trusted domains)
- Handling of NULL and empty string parameters

There appears to still be issues with querying using a NETBIOS domain
name (including trusted domains), due to winbind sometimes falling back
to NETBIOS queries which have no site-awareness. I've noticed this
against Windows and seen it once in testenv, but the particular test I
expected to fail seems to consistently pass in make test. I've marked
the questionable test in flapping, which may warrant more inspection
later on.

Please review and push.


Cheers,

Garming

[Garming Sam via samba-technical]

New version. Fixed some error handling.

Post by Garming Sam via samba-technical
Hi,
While looking at automatic site coverage (which has already went
upstream), I noticed that our DC location calls in NETLOGON are
particularly bad. GetDCNameEx only returned WERR_NO_SUCH_DOMAIN when you
asked for a site that the DC did not exist in. Furthermore, it did not
bother checking the domain, which meant that as long as you supplied a
valid site name in both domains, you could get a DC in the wrong domain
returned.
In order to remedy a large amount of the behaviour, I've implemented a
winbind forward call which triggers the dsgetdcname calls (using CLDAP
and DNS) in the winbind DC locator process. This allows arbitrary sites
to be queried for, and by doing so, the trusted domain case also works too.
There are a number of other errors in the RPC call which I have had to
- Failing to forward the error code from GetDCNameEx2 to GetDCNameEx
when it is called internally.
- Returning a more appropriate client site name (and avoid a fallback
that would be wrong in trusted domains)
- Handling of NULL and empty string parameters
There appears to still be issues with querying using a NETBIOS domain
name (including trusted domains), due to winbind sometimes falling back
to NETBIOS queries which have no site-awareness. I've noticed this
against Windows and seen it once in testenv, but the particular test I
expected to fail seems to consistently pass in make test. I've marked
the questionable test in flapping, which may warrant more inspection
later on.
Please review and push.
Cheers,
Garming

[Stefan Metzmacher via samba-technical]

Hi Garming,

can you please have a look at
https://git.samba.org/?p=samba.git;a=commitdiff;h=373da95b0b72ec7db8463a
https://git.samba.org/?p=samba.git;a=commitdiff;h=023bd2d15d5e9599c59281
https://git.samba.org/?p=samba.git;a=commitdiff;h=8022b63f6cfadc58d6711e

We use that pattern for a few async calls in already.
dcesrv_netr_LogonSamLogon_base_call(), dcesrv_netr_LogonControl_base_call(),
dcesrv_lsa_LookupSids_base_call(),
dcesrv_lsa_LookupNames_base_call()

I think we should use that pattern also for GetDCName* too.

Also wb_irpc_GetDCName_done() should be moved after wb_irpc_GetDCName(),
that makes it much easier to read and matches the pattern we always use
for async programming, e.g.

subreq = wb_lookupsids_send(msg,
server_event_context(),
sids, req->in.sids->num_sids);
if (subreq == NULL) {
return NT_STATUS_NO_MEMORY;
}
tevent_req_set_callback(subreq, wb_irpc_lsa_LookupSids3_done,
state);
msg->defer_reply = true;

return NT_STATUS_OK;
}

static void wb_irpc_lsa_LookupSids3_done(struct tevent_req *subreq)
{
struct wb_irpc_lsa_LookupSids3_state *state =
tevent_req_callback_data(subreq,
struct wb_irpc_lsa_LookupSids3_state);
struct lsa_RefDomainList *domains = NULL;
struct lsa_TransNameArray *names = NULL;
NTSTATUS status;
uint32_t i;

status = wb_lookupsids_recv(subreq, state->msg,
&domains, &names);


I also think we should have a bug report for that and backport it to
4.8.

It seems that some of the patches fix bugs in a former patch,
these should be squashed to make it easier to understand.

Thanks!
metze

Post by Garming Sam via samba-technical
New version. Fixed some error handling.

Post by Garming Sam via samba-technical
Hi,
While looking at automatic site coverage (which has already went
upstream), I noticed that our DC location calls in NETLOGON are
particularly bad. GetDCNameEx only returned WERR_NO_SUCH_DOMAIN when you
asked for a site that the DC did not exist in. Furthermore, it did not
bother checking the domain, which meant that as long as you supplied a
valid site name in both domains, you could get a DC in the wrong domain
returned.
In order to remedy a large amount of the behaviour, I've implemented a
winbind forward call which triggers the dsgetdcname calls (using CLDAP
and DNS) in the winbind DC locator process. This allows arbitrary sites
to be queried for, and by doing so, the trusted domain case also works too.
There are a number of other errors in the RPC call which I have had to
- Failing to forward the error code from GetDCNameEx2 to GetDCNameEx
when it is called internally.
- Returning a more appropriate client site name (and avoid a fallback
that would be wrong in trusted domains)
- Handling of NULL and empty string parameters
There appears to still be issues with querying using a NETBIOS domain
name (including trusted domains), due to winbind sometimes falling back
to NETBIOS queries which have no site-awareness. I've noticed this
against Windows and seen it once in testenv, but the particular test I
expected to fail seems to consistently pass in make test. I've marked
the questionable test in flapping, which may warrant more inspection
later on.
Please review and push.
Cheers,
Garming

[Garming Sam via samba-technical]

Hi metze,

I've changed the patches to follow the calling format you've suggested
(and squashed some patches in the process). Hopefully it's much closer
to what you expect.


Cheers,

Garming

Post by Stefan Metzmacher via samba-technical
Hi Garming,
can you please have a look at
https://git.samba.org/?p=samba.git;a=commitdiff;h=373da95b0b72ec7db8463a
https://git.samba.org/?p=samba.git;a=commitdiff;h=023bd2d15d5e9599c59281
https://git.samba.org/?p=samba.git;a=commitdiff;h=8022b63f6cfadc58d6711e
We use that pattern for a few async calls in already.
dcesrv_netr_LogonSamLogon_base_call(), dcesrv_netr_LogonControl_base_call(),
dcesrv_lsa_LookupSids_base_call(),
dcesrv_lsa_LookupNames_base_call()
I think we should use that pattern also for GetDCName* too.
Also wb_irpc_GetDCName_done() should be moved after wb_irpc_GetDCName(),
that makes it much easier to read and matches the pattern we always use
for async programming, e.g.
subreq = wb_lookupsids_send(msg,
server_event_context(),
sids, req->in.sids->num_sids);
if (subreq == NULL) {
return NT_STATUS_NO_MEMORY;
}
tevent_req_set_callback(subreq, wb_irpc_lsa_LookupSids3_done,
state);
msg->defer_reply = true;
return NT_STATUS_OK;
}
static void wb_irpc_lsa_LookupSids3_done(struct tevent_req *subreq)
{
struct wb_irpc_lsa_LookupSids3_state *state =
tevent_req_callback_data(subreq,
struct wb_irpc_lsa_LookupSids3_state);
struct lsa_RefDomainList *domains = NULL;
struct lsa_TransNameArray *names = NULL;
NTSTATUS status;
uint32_t i;
status = wb_lookupsids_recv(subreq, state->msg,
&domains, &names);
I also think we should have a bug report for that and backport it to
4.8.
It seems that some of the patches fix bugs in a former patch,
these should be squashed to make it easier to understand.
Thanks!
metze

Post by Garming Sam via samba-technical
New version. Fixed some error handling.

Post by Garming Sam via samba-technical
Hi,
While looking at automatic site coverage (which has already went
upstream), I noticed that our DC location calls in NETLOGON are
particularly bad. GetDCNameEx only returned WERR_NO_SUCH_DOMAIN when you
asked for a site that the DC did not exist in. Furthermore, it did not
bother checking the domain, which meant that as long as you supplied a
valid site name in both domains, you could get a DC in the wrong domain
returned.
In order to remedy a large amount of the behaviour, I've implemented a
winbind forward call which triggers the dsgetdcname calls (using CLDAP
and DNS) in the winbind DC locator process. This allows arbitrary sites
to be queried for, and by doing so, the trusted domain case also works too.
There are a number of other errors in the RPC call which I have had to
- Failing to forward the error code from GetDCNameEx2 to GetDCNameEx
when it is called internally.
- Returning a more appropriate client site name (and avoid a fallback
that would be wrong in trusted domains)
- Handling of NULL and empty string parameters
There appears to still be issues with querying using a NETBIOS domain
name (including trusted domains), due to winbind sometimes falling back
to NETBIOS queries which have no site-awareness. I've noticed this
against Windows and seen it once in testenv, but the particular test I
expected to fail seems to consistently pass in make test. I've marked
the questionable test in flapping, which may warrant more inspection
later on.
Please review and push.
Cheers,
Garming

[Stefan Metzmacher via samba-technical]

Hi Garming,

Post by Garming Sam via samba-technical
I've changed the patches to follow the calling format you've suggested
(and squashed some patches in the process). Hopefully it's much closer
to what you expect.

Yes, much better, but a few little cosmetic things:

- Can you please inline winbind_forward_GetDCName()
it's easier to have this all in dcesrv_netr_DsRGetDCName_base_call()

- Please add TALLOC_FREE(subreq); after
status = dcerpc_winbind_DsGetDcName_recv()

- It would be good to add a "finished:" label before
"if (state->_r.dcex2 != NULL) {" in
dcesrv_netr_DsRGetDCName_base_done()
and use early "goto finished" in order to reduce the
indentation levels.

- Rename irpc_GetDCName_state into wb_irpc_GetDCName_state

- Move the forward declaration of wb_irpc_GetDCName_done()
after the 'struct wb_irpc_GetDCName_state' definition.

Now a few logic things:

- Can we really safely dereference state->r.out.info[0]->
in dcesrv_netr_DsRGetDCName_base_done() if result is not
NT_STATUS_OK?

- dc_unc = talloc_asprintf(state->dce_call, uses the wrong
memory context, it should be state->r.out.info[0] (if we can assume a
valid talloc pointer) or state->mem_ctx.

- Don't we need to check the result of samdb_client_site_name()
in dcesrv_netr_DsRGetDCName_base_call() ?

- Is "netlogon: Resolve calls to GetDCNameEx2 within the same
NETLOGON domain" really needed? "The return will have the DNS domain,
which is not quite as nice, but it does not seem to violate any
assumptions" sounds risky...

Thanks!
metze

[Garming Sam via samba-technical]

Post by Stefan Metzmacher via samba-technical
- Can you please inline winbind_forward_GetDCName()
it's easier to have this all in dcesrv_netr_DsRGetDCName_base_call()
- Please add TALLOC_FREE(subreq); after
status = dcerpc_winbind_DsGetDcName_recv()
- It would be good to add a "finished:" label before
"if (state->_r.dcex2 != NULL) {" in
dcesrv_netr_DsRGetDCName_base_done()
and use early "goto finished" in order to reduce the
indentation levels.
- Rename irpc_GetDCName_state into wb_irpc_GetDCName_state
- Move the forward declaration of wb_irpc_GetDCName_done()
after the 'struct wb_irpc_GetDCName_state' definition.
- Can we really safely dereference state->r.out.info[0]->
in dcesrv_netr_DsRGetDCName_base_done() if result is not
NT_STATUS_OK?

It only dereferences info if the NTSTATUS of
dcerpc_winbind_DsGetDcName_recv is ok. winbind is supposed to return
DOMAIN_CONTROLLER_NOT_FOUND at this top level. If you want, I can
double-check the info before dereferencing it.

Post by Stefan Metzmacher via samba-technical
- dc_unc = talloc_asprintf(state->dce_call, uses the wrong
memory context, it should be state->r.out.info[0] (if we can assume a
valid talloc pointer) or state->mem_ctx.

I didn't have state->mem_ctx before, so yes, I will change that.

Post by Stefan Metzmacher via samba-technical
- Don't we need to check the result of samdb_client_site_name()
in dcesrv_netr_DsRGetDCName_base_call() ?

Not really. It can be NULL, and that doesn't mean anything bad has
happened. Returning NULL to the RPC call is also quite expected.

Post by Stefan Metzmacher via samba-technical
- Is "netlogon: Resolve calls to GetDCNameEx2 within the same
NETLOGON domain" really needed? "The return will have the DNS domain,
which is not quite as nice, but it does not seem to violate any
assumptions" sounds risky...

So, in general, when you specify the domain name as a NETBIOS name,
Windows seems to return the DC UNC in NETBIOS form (and this follows the
same pattern with DNS names). All this patch means is that we end up
returning DNS names (for DCs outside our site), but if you specify the
flag to return a particular format, everything is as expected. We don't
seem to follow this convention even when we didn't go to winbind. We
seem to need this patch, because against Windows, we must be getting
slightly different results via our DC locator calls and without it, we
get strange intermittent failures even while querying within the same
domain. I haven't really got the time to look at why this might be
happening.

I'll try to fix the other cosmetic issues.

Thanks,

Garming

[Garming Sam via samba-technical]

Hi,

I think I've addressed everything. There's also an additional patch
marked SQUASH that checks info and info[0] is non-NULL if you think
that's appropriate. I've also modified some of the comments slightly to
indicate that NETBIOS names actually do work (even before the patch
where I clobber it), but not always, to which I'm still not at all sure why.

Cheers,

Garming

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
- Can we really safely dereference state->r.out.info[0]->
in dcesrv_netr_DsRGetDCName_base_done() if result is not
NT_STATUS_OK?

It only dereferences info if the NTSTATUS of
dcerpc_winbind_DsGetDcName_recv is ok. winbind is supposed to return
DOMAIN_CONTROLLER_NOT_FOUND at this top level. If you want, I can
double-check the info before dereferencing it.

Post by Stefan Metzmacher via samba-technical
- dc_unc = talloc_asprintf(state->dce_call, uses the wrong
memory context, it should be state->r.out.info[0] (if we can assume a
valid talloc pointer) or state->mem_ctx.

I didn't have state->mem_ctx before, so yes, I will change that.

Post by Stefan Metzmacher via samba-technical
- Don't we need to check the result of samdb_client_site_name()
in dcesrv_netr_DsRGetDCName_base_call() ?

Not really. It can be NULL, and that doesn't mean anything bad has
happened. Returning NULL to the RPC call is also quite expected.

Post by Stefan Metzmacher via samba-technical
- Is "netlogon: Resolve calls to GetDCNameEx2 within the same
NETLOGON domain" really needed? "The return will have the DNS domain,
which is not quite as nice, but it does not seem to violate any
assumptions" sounds risky...

So, in general, when you specify the domain name as a NETBIOS name,
Windows seems to return the DC UNC in NETBIOS form (and this follows the
same pattern with DNS names). All this patch means is that we end up
returning DNS names (for DCs outside our site), but if you specify the
flag to return a particular format, everything is as expected. We don't
seem to follow this convention even when we didn't go to winbind. We
seem to need this patch, because against Windows, we must be getting
slightly different results via our DC locator calls and without it, we
get strange intermittent failures even while querying within the same
domain. I haven't really got the time to look at why this might be
happening.
I'll try to fix the other cosmetic issues.
Thanks,
Garming

[Stefan Metzmacher via samba-technical]

Post by Garming Sam via samba-technical
Hi,
I think I've addressed everything. There's also an additional patch
marked SQUASH that checks info and info[0] is non-NULL if you think
that's appropriate.

In addition to the check of the 'status' variable, which just means
the response was correctly transmitted from the server (winbindd) to the
client (the netlogon server), we need to check the 'result' variable
before looking at other r->out.* elements at all.

Something like this (please don't try to safe lines by using '||' for
the if statements):

if (!NT_STATUS_IS_OK(result)) {
DBG_NOTICE("DC location via winbind returned - %s\n",
nt_errstr(result));
state->r.out.result = result;
goto finished;
}

Shouldn't we use something other than WERR_NO_SUCH_DOMAIN?
This is more an internal error.

+ if (!NT_STATUS_IS_OK(status)) {
+ DBG_NOTICE("DC location via winbind failed - %s\n",
+ nt_errstr(status));
+ state->r.out.result = WERR_NO_SUCH_DOMAIN;
+ goto finished;
+ }

info and info[0] is non-NULL should also be checked in a separate if
statement, with a DBG_ERR or DBG_WARNING message indicating something
like an internal error.

I also noticed that wb_irpc_GetDCName_done() should return the result of
wb_dsgetdcname_recv() as application result using:

state->r->out.result = status; and
irpc_send_reply(state->msg, NT_STATUS_OK);

and the memory context passed to wb_dsgetdcname_recv() should be
state->dcinfo (if it's a valid talloc pointer) or state->msg

Post by Garming Sam via samba-technical
I've also modified some of the comments slightly to
indicate that NETBIOS names actually do work (even before the patch
where I clobber it), but not always, to which I'm still not at all sure why.
Cheers,
Garming

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
- Can we really safely dereference state->r.out.info[0]->
in dcesrv_netr_DsRGetDCName_base_done() if result is not
NT_STATUS_OK?

It only dereferences info if the NTSTATUS of
dcerpc_winbind_DsGetDcName_recv is ok. winbind is supposed to return
DOMAIN_CONTROLLER_NOT_FOUND at this top level. If you want, I can
double-check the info before dereferencing it.

Post by Stefan Metzmacher via samba-technical
- dc_unc = talloc_asprintf(state->dce_call, uses the wrong
memory context, it should be state->r.out.info[0] (if we can assume a
valid talloc pointer) or state->mem_ctx.

I didn't have state->mem_ctx before, so yes, I will change that.

Post by Stefan Metzmacher via samba-technical
- Don't we need to check the result of samdb_client_site_name()
in dcesrv_netr_DsRGetDCName_base_call() ?

Not really. It can be NULL, and that doesn't mean anything bad has
happened. Returning NULL to the RPC call is also quite expected.

Ok.

Post by Garming Sam via samba-technical

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
- Is "netlogon: Resolve calls to GetDCNameEx2 within the same
NETLOGON domain" really needed? "The return will have the DNS domain,
which is not quite as nice, but it does not seem to violate any
assumptions" sounds risky...

So, in general, when you specify the domain name as a NETBIOS name,
Windows seems to return the DC UNC in NETBIOS form (and this follows the
same pattern with DNS names). All this patch means is that we end up
returning DNS names (for DCs outside our site), but if you specify the
flag to return a particular format, everything is as expected.

To me the patch looks like it's forcing the dns name even if the client
asked for a netbios name (DS_IS_FLAT_NAME) explicitly.

Post by Garming Sam via samba-technical

Post by Garming Sam via samba-technical
We don't seem to follow this convention even when we didn't go to winbind. We
seem to need this patch, because against Windows, we must be getting
slightly different results via our DC locator calls and without it, we
get strange intermittent failures even while querying within the same
domain. I haven't really got the time to look at why this might be
happening.

I think we should better understand what's going on here.

metze

[Garming Sam via samba-technical]

Post by Stefan Metzmacher via samba-technical
+ if (!NT_STATUS_IS_OK(status)) {
+ DBG_NOTICE("DC location via winbind failed - %s\n",
+ nt_errstr(status));
+ state->r.out.result = WERR_NO_SUCH_DOMAIN;
+ goto finished;
+ }
info and info[0] is non-NULL should also be checked in a separate if
statement, with a DBG_ERR or DBG_WARNING message indicating something
like an internal error.
I also noticed that wb_irpc_GetDCName_done() should return the result of
state->r->out.result = status; and
irpc_send_reply(state->msg, NT_STATUS_OK);
and the memory context passed to wb_dsgetdcname_recv() should be
state->dcinfo (if it's a valid talloc pointer) or state->msg

Right, I see, you want me to embed the error within the response, so as
to not confuse top level errors. It seems that in other cases in the
file wb_*_recv isn't supposed to fail, and I was just following the
pattern of the others.

Post by Stefan Metzmacher via samba-technical
So, in general, when you specify the domain name as a NETBIOS name,
Windows seems to return the DC UNC in NETBIOS form (and this follows the
same pattern with DNS names). All this patch means is that we end up
returning DNS names (for DCs outside our site), but if you specify the
flag to return a particular format, everything is as expected.
To me the patch looks like it's forcing the dns name even if the client
asked for a netbios name (DS_IS_FLAT_NAME) explicitly.

The flags from the original request are passed down to winbind, who will
correctly interpret them. The only thing I modify is the inbound flags
(which are separate from the outbound ones e.g. DS_RETURN_FLAT_NAME),
which would only affect the default format of the response.


In regards to why it doesn't work with NETBIOS, I noticed this log
(searching for a Windows DC):

/home/ubuntu/samba/bin/smbd: 10.9.0.11 (ipv4:10.9.0.11:55217) connect to
service IPC$ initially as user SAMDOM\Administrator (uid=0, gid=100)
(pid 15027)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.default-first-site-nae._sites.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: resolve_lmhosts: Attempting lmhosts
lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: resolve_wins: WINS server resolution
selected and no WINS servers listed.
/home/ubuntu/samba/bin/winbindd: name_resolve_bcast: Attempting
broadcast lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( 10.9.0.12 )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.10 ( 10.9.0.10 )
/home/ubuntu/samba/bin/winbindd: No nmbd found

source3/libsmb/clidgram.c: nbt_getdc_send

        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }

So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.


Cheers,

Garming

[Garming Sam via samba-technical]

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
I also noticed that wb_irpc_GetDCName_done() should return the result of
state->r->out.result = status; and
irpc_send_reply(state->msg, NT_STATUS_OK);
and the memory context passed to wb_dsgetdcname_recv() should be
state->dcinfo (if it's a valid talloc pointer) or state->msg

Right, I see, you want me to embed the error within the response, so as
to not confuse top level errors. It seems that in other cases in the
file wb_*_recv isn't supposed to fail, and I was just following the
pattern of the others.

Addressed this now hopefully.

Post by Garming Sam via samba-technical
The flags from the original request are passed down to winbind, who will
correctly interpret them. The only thing I modify is the inbound flags
(which are separate from the outbound ones e.g. DS_RETURN_FLAT_NAME),
which would only affect the default format of the response.

You should also note that there are some tests for DS_RETURN_FLAT_NAME
in my patchset, which are asserting that the short trust name is returned.

Cheers,

Garming

Post by Garming Sam via samba-technical
In regards to why it doesn't work with NETBIOS, I noticed this log
/home/ubuntu/samba/bin/smbd: 10.9.0.11 (ipv4:10.9.0.11:55217) connect to
service IPC$ initially as user SAMDOM\Administrator (uid=0, gid=100)
(pid 15027)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.default-first-site-nae._sites.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: resolve_lmhosts: Attempting lmhosts
lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: resolve_wins: WINS server resolution
selected and no WINS servers listed.
/home/ubuntu/samba/bin/winbindd: name_resolve_bcast: Attempting
broadcast lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( 10.9.0.12 )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.10 ( 10.9.0.10 )
/home/ubuntu/samba/bin/winbindd: No nmbd found
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.
Cheers,
Garming

[Garming Sam via samba-technical]

Hi,

Using Volker's patches, the "No nmbd found" disappears, but the site
aware location is wrong (meaning the RPC call must fail in these cases).
It seems that the underlying dsgetdcname call does not respect the site
name parameter in winbind when using NETBIOS names. I've also noticed
that although I test the winbind forwarding and having different
domains, it doesn't actually test being in a different site (partly
because we don't have any DCs like that in selftest currently). With my
full patchset, making the query using a trust NETBIOS domain name AND a
specified site may fail if there is more than one site (or it
arbitrarily picks a DC whose site differs). Compared with the original
behaviour at least, that's significantly better, and all the DNS domains
should work as well as the single domain case with NETBIOS (where it's
currently remedied at the RPC layer).

To fix the trusted domain case, either dsgetdcname needs to use the
response from discover_dc_netbios to retry the query with the DNS realm
(this is on top of Volker's patches). Or it needs to do some other
mapping using information winbind might know. I've got a number of other
projects that I need to be working on, so I can't really look into this
further. I would really like to push this current patchset for now
(assuming you don't have any further objections), so as to fix most of
the cases just by implementing the forwarding behaviour, and hopefully
there's enough info that I've gathered to go on to fix the edge cases
around trusted domains (and presumably undo the RPC layer NETBIOS fix I
made).

Cheers,

Garming

Post by Garming Sam via samba-technical

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
I also noticed that wb_irpc_GetDCName_done() should return the result of
state->r->out.result = status; and
irpc_send_reply(state->msg, NT_STATUS_OK);
and the memory context passed to wb_dsgetdcname_recv() should be
state->dcinfo (if it's a valid talloc pointer) or state->msg

Right, I see, you want me to embed the error within the response, so as
to not confuse top level errors. It seems that in other cases in the
file wb_*_recv isn't supposed to fail, and I was just following the
pattern of the others.

Addressed this now hopefully.

Post by Garming Sam via samba-technical
The flags from the original request are passed down to winbind, who will
correctly interpret them. The only thing I modify is the inbound flags
(which are separate from the outbound ones e.g. DS_RETURN_FLAT_NAME),
which would only affect the default format of the response.

You should also note that there are some tests for DS_RETURN_FLAT_NAME
in my patchset, which are asserting that the short trust name is returned.
Cheers,
Garming

Post by Garming Sam via samba-technical
In regards to why it doesn't work with NETBIOS, I noticed this log
/home/ubuntu/samba/bin/smbd: 10.9.0.11 (ipv4:10.9.0.11:55217) connect to
service IPC$ initially as user SAMDOM\Administrator (uid=0, gid=100)
(pid 15027)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.default-first-site-nae._sites.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: resolve_lmhosts: Attempting lmhosts
lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: resolve_wins: WINS server resolution
selected and no WINS servers listed.
/home/ubuntu/samba/bin/winbindd: name_resolve_bcast: Attempting
broadcast lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( 10.9.0.12 )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.10 ( 10.9.0.10 )
/home/ubuntu/samba/bin/winbindd: No nmbd found
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.
Cheers,
Garming

[Volker Lendecke via samba-technical]

Hello!

Just FYI: I have a big (unfinished) patchset in preparation that
rewrites source3/libsmb/dsgetdcname to be fully async and a lot more
correct and flexible and that rely on the patches you are working upon
now.

With best regards,

Volker Lendecke

Post by Garming Sam via samba-technical
Hi,
Using Volker's patches, the "No nmbd found" disappears, but the site
aware location is wrong (meaning the RPC call must fail in these cases).
It seems that the underlying dsgetdcname call does not respect the site
name parameter in winbind when using NETBIOS names. I've also noticed
that although I test the winbind forwarding and having different
domains, it doesn't actually test being in a different site (partly
because we don't have any DCs like that in selftest currently). With my
full patchset, making the query using a trust NETBIOS domain name AND a
specified site may fail if there is more than one site (or it
arbitrarily picks a DC whose site differs). Compared with the original
behaviour at least, that's significantly better, and all the DNS domains
should work as well as the single domain case with NETBIOS (where it's
currently remedied at the RPC layer).
To fix the trusted domain case, either dsgetdcname needs to use the
response from discover_dc_netbios to retry the query with the DNS realm
(this is on top of Volker's patches). Or it needs to do some other
mapping using information winbind might know. I've got a number of other
projects that I need to be working on, so I can't really look into this
further. I would really like to push this current patchset for now
(assuming you don't have any further objections), so as to fix most of
the cases just by implementing the forwarding behaviour, and hopefully
there's enough info that I've gathered to go on to fix the edge cases
around trusted domains (and presumably undo the RPC layer NETBIOS fix I
made).
Cheers,
Garming

Post by Garming Sam via samba-technical

Post by Garming Sam via samba-technical

Post by Stefan Metzmacher via samba-technical
I also noticed that wb_irpc_GetDCName_done() should return the result of
state->r->out.result = status; and
irpc_send_reply(state->msg, NT_STATUS_OK);
and the memory context passed to wb_dsgetdcname_recv() should be
state->dcinfo (if it's a valid talloc pointer) or state->msg

Right, I see, you want me to embed the error within the response, so as
to not confuse top level errors. It seems that in other cases in the
file wb_*_recv isn't supposed to fail, and I was just following the
pattern of the others.

Addressed this now hopefully.

Post by Garming Sam via samba-technical
The flags from the original request are passed down to winbind, who will
correctly interpret them. The only thing I modify is the inbound flags
(which are separate from the outbound ones e.g. DS_RETURN_FLAT_NAME),
which would only affect the default format of the response.

You should also note that there are some tests for DS_RETURN_FLAT_NAME
in my patchset, which are asserting that the short trust name is returned.
Cheers,
Garming

Post by Garming Sam via samba-technical
In regards to why it doesn't work with NETBIOS, I noticed this log
/home/ubuntu/samba/bin/smbd: 10.9.0.11 (ipv4:10.9.0.11:55217) connect to
service IPC$ initially as user SAMDOM\Administrator (uid=0, gid=100)
(pid 15027)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.default-first-site-nae._sites.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: dns_send_req: Failed to resolve
_ldap._tcp.dc._msdcs.samdom (Success)
/home/ubuntu/samba/bin/winbindd: ads_dns_lookup_srv: Failed to send DNS
query (NT_STATUS_UNSUCCESSFUL)
/home/ubuntu/samba/bin/winbindd: resolve_lmhosts: Attempting lmhosts
lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: resolve_wins: WINS server resolution
selected and no WINS servers listed.
/home/ubuntu/samba/bin/winbindd: name_resolve_bcast: Attempting
broadcast lookup for name samdom<0x1c>
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( 10.9.0.12 )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.12 ( )
/home/ubuntu/samba/bin/winbindd: Got a positive name query response from
10.9.0.10 ( 10.9.0.10 )
/home/ubuntu/samba/bin/winbindd: No nmbd found
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.
Cheers,
Garming

[Stefan Metzmacher via samba-technical]

Hi Garming,

Post by Garming Sam via samba-technical
Using Volker's patches, the "No nmbd found" disappears, but the site
aware location is wrong (meaning the RPC call must fail in these cases).
It seems that the underlying dsgetdcname call does not respect the site
name parameter in winbind when using NETBIOS names. I've also noticed
that although I test the winbind forwarding and having different
domains, it doesn't actually test being in a different site (partly
because we don't have any DCs like that in selftest currently). With my
full patchset, making the query using a trust NETBIOS domain name AND a
specified site may fail if there is more than one site (or it
arbitrarily picks a DC whose site differs). Compared with the original
behaviour at least, that's significantly better, and all the DNS domains
should work as well as the single domain case with NETBIOS (where it's
currently remedied at the RPC layer).
To fix the trusted domain case, either dsgetdcname needs to use the
response from discover_dc_netbios to retry the query with the DNS realm
(this is on top of Volker's patches). Or it needs to do some other
mapping using information winbind might know. I've got a number of other
projects that I need to be working on, so I can't really look into this
further. I would really like to push this current patchset for now
(assuming you don't have any further objections), so as to fix most of
the cases just by implementing the forwarding behaviour, and hopefully
there's enough info that I've gathered to go on to fix the edge cases
around trusted domains (and presumably undo the RPC layer NETBIOS fix I
made).

sorry for the delay. I hope to have another look at this tomorrow
and will push (at least some of the patches).

metze

[Garming Sam via samba-technical]

Ping?

Post by Stefan Metzmacher via samba-technical
Hi Garming,

Post by Garming Sam via samba-technical
Using Volker's patches, the "No nmbd found" disappears, but the site
aware location is wrong (meaning the RPC call must fail in these cases).
It seems that the underlying dsgetdcname call does not respect the site
name parameter in winbind when using NETBIOS names. I've also noticed
that although I test the winbind forwarding and having different
domains, it doesn't actually test being in a different site (partly
because we don't have any DCs like that in selftest currently). With my
full patchset, making the query using a trust NETBIOS domain name AND a
specified site may fail if there is more than one site (or it
arbitrarily picks a DC whose site differs). Compared with the original
behaviour at least, that's significantly better, and all the DNS domains
should work as well as the single domain case with NETBIOS (where it's
currently remedied at the RPC layer).
To fix the trusted domain case, either dsgetdcname needs to use the
response from discover_dc_netbios to retry the query with the DNS realm
(this is on top of Volker's patches). Or it needs to do some other
mapping using information winbind might know. I've got a number of other
projects that I need to be working on, so I can't really look into this
further. I would really like to push this current patchset for now
(assuming you don't have any further objections), so as to fix most of
the cases just by implementing the forwarding behaviour, and hopefully
there's enough info that I've gathered to go on to fix the edge cases
around trusted domains (and presumably undo the RPC layer NETBIOS fix I
made).

sorry for the delay. I hope to have another look at this tomorrow
and will push (at least some of the patches).
metze

[Andrew Bartlett via samba-technical]

On Thu, 2018-04-26 at 14:33 +1200, Garming Sam via samba-technical

Ping?

G'Day Metze,

Can you please indicate your intentions here? We need to finish this
up so we have a few less balls in the air.

I'm happy with the changes (they are a drastic improvement) and would
like to go over them with Garming for a formal review once I'm in the
office tomorrow.

Thanks,

Andrew Bartlett

[Stefan Metzmacher via samba-technical]

Hi Andrew,

Post by Andrew Bartlett via samba-technical
Can you please indicate your intentions here? We need to finish this
up so we have a few less balls in the air.
I'm happy with the changes (they are a drastic improvement) and would
like to go over them with Garming for a formal review once I'm in the
office tomorrow.

Please consider looking at
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-reviewE01

The only thing I'd like to avoid is the "forward_with_dns_name" change
https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=b1f44053a8519257a3afa817088e67f843a12fcb
I'm not yet convinced that's better to have that instead of leaving it.

But I won't block you on the rest, while I may fix up some stuff later...

metze

[Andrew Bartlett via samba-technical]

Post by Stefan Metzmacher via samba-technical
Hi Andrew,

Post by Andrew Bartlett via samba-technical
Can you please indicate your intentions here? We need to finish this
up so we have a few less balls in the air.
I'm happy with the changes (they are a drastic improvement) and would
like to go over them with Garming for a formal review once I'm in the
office tomorrow.

Please consider looking at
https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-reviewE01
The only thing I'd like to avoid is the "forward_with_dns_name" change
https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=b1f44053a8519257a3afa817088e67f843a12fcb
I'm not yet convinced that's better to have that instead of leaving it.
But I won't block you on the rest, while I may fix up some stuff later...

Thanks metze. I really appreciate that.

Andrew Bartlett

[Volker Lendecke via samba-technical]

Post by Garming Sam via samba-technical
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.

Well, this code path could go away very, pending review and
discussion. See my patchset with the unexpected pipe in
source4/nbt_server. Can you take a look there?

Thanks, Volker

[Volker Lendecke via samba-technical]

Post by Volker Lendecke via samba-technical

Post by Garming Sam via samba-technical
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.

Well, this code path could go away very, pending review and

"very soon" of course :-)

Volker

[Garming Sam via samba-technical]

Post by Volker Lendecke via samba-technical

Post by Garming Sam via samba-technical
source3/libsmb/clidgram.c: nbt_getdc_send
        state->nmbd_pid = pidfile_pid(lp_pid_directory(), "nmbd");
        if (state->nmbd_pid == 0) {
                DEBUG(3, ("No nmbd found\n"));
                tevent_req_nterror(req, NT_STATUS_NOT_SUPPORTED);
                return tevent_req_post(req, ev);
        }
So it seems that it relies on nmbd running in order to make any further
identification. This seems to be the expected code path, so selftest
must have some additional records or some other method that circumvents
this. I'm also not confident about the site-awareness of the above calls
that are being triggered. It is possible that someone actually has to do
the NETBIOS name to DNS name conversion at some layer to get this right.
The goal for me was to mostly just plumb the call and ensure the single
domain case works as expected.

Well, this code path could go away very, pending review and
discussion. See my patchset with the unexpected pipe in
source4/nbt_server. Can you take a look there?
Thanks, Volker

I will try my patchset on top and see if that's enough to return the
right answer. I suspect the site might still be wrong, but at least that
should be one less issue.

Thanks,

Garming