samba4.winbind.struct flapping test uncovers a real issue
Michael Adam
obnox at samba.org
Mon Oct 6 03:25:22 MDT 2014
Hi Andrew,
It is great that you looked at it and were able to
apparently hunt it down!
I am currently looking at the patches and doing
a few tests.
Will push them with some line-lenght adaption
if all goes well.
Cheers - Michael
On 2014-10-06 at 19:28 +1300, Andrew Bartlett wrote:
>
> Many of you would have been frustrated by failures like this one:
>
> [1433/1686 in 1h24m54s] samba4.winbind.struct(s3member:local)
> Running WINBINDD_DOMAIN_INFO (struct based)
> DOMAIN 'BUILTIN' => '' [ ]
> DOMAIN 'LOCALADMEMBER' => '' [ ]
> DOMAIN 'SAMBADOMAIN' => 'samba.example.com' [ PR AD NA ]
> UNEXPECTED(failure): samba4.winbind.struct.domain_info(s3member:local)
> REASON: _StringException: _StringException: ../source4/torture/winbind/struct_based.c:434: rep.data.domain_info.name was SAMBADOMAIN, expected torturedom00: Netbios domain name doesn't match
>
> I think I have finally found out why this happens. In our AD DC, our
> NETLOGON DsRGetDCNameEx2 implementation is frankly, fantasy. It returns
> it's own IP or 127.0.0.1 for essentially any input. So, we then contact our primary
> domain or localhost, thinking it is the trust, and then overwrite details of the
> trust. This could happen in the real-world if IPs were switched, or by
> a malicious actor intercepting traffic. (Hence my desire to enforce SMB
> signing).
>
> With the attached patches, I think I've addressed the flapping test, and it not, I've at least improved the error messages so we can dianose it better.
>
> See for example this from an earlier run, showing how we now give enough information to understand the real failure.
>
> [1432/1686 in 1h27m47s] samba4.winbind.pac(s4member:local)
> [1433/1686 in 1h27m47s] samba4.winbind.struct(s3member:local)
> Running WINBINDD_DOMAIN_INFO (struct based)
> DOMAIN 'BUILTIN' => '' [ ] [S-1-5-32]
> DOMAIN 'LOCALADMEMBER' => '' [ ] [S-1-5-21-1134438427-3415451756-3748806739]
> DOMAIN 'SAMBADOMAIN' => 'samba.example.com' [ PR AD NA ] [S-1-5-21-2713267906-3234219033-185379325]
> DOMAIN 'torturedom00' => 'torturedom00.samba.example.com' [ AD NA ] [S-1-5-21-97398-379795-10000]
> DOMAIN 'torturedom01' => 'torturedom01.samba.example.com' [ AD NA ] [S-1-5-21-97398-379795-10001]
> DOMAIN 'torturedom02' => 'torturedom02.samba.example.com' [ AD NA ] [S-1-5-21-97398-379795-10002]
> DOMAIN 'torturedom03' => 'torturedom03.samba.example.com' [ AD NA ] [S-0-0]
> UNEXPECTED(failure): samba4.winbind.struct.domain_info(s3member:local)
> REASON: _StringException: _StringException: ../source4/torture/winbind/struct_based.c:459: Expression `ok' failed: SID's doesn't match
>
> I still don't have a full explanation, but clearly not trusting the DC
> to assert it's own names is a good start. We should reject such
> connections, and try again with another DC, but we don't do that currently.
>
> Any thoughts? Should we review/push this much?
>
> This is part of my subdomain-wip4 branch that I'm still trying to sort
> out. I'm still chasing down the wbinfo -t issue there.
>
> Andrew Bartlett
>
> --
> Andrew Bartlett http://samba.org/~abartlet/
> Authentication Developer, Samba Team http://samba.org
> Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
>
>
> --
> Andrew Bartlett http://samba.org/~abartlet/
> Authentication Developer, Samba Team http://samba.org
> Samba Developer, Catalyst IT http://catalyst.net.nz/services/samba
>
> >From e7997cdb96d895d0b69be9ffb44a8281b4e3bc00 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Sun, 5 Oct 2014 16:00:47 +1300
> Subject: [PATCH 1/2] torture: Reorder torture_winbind_struct_domain_info tests
>
> This tries to ensure we get enough information to debug this
> intermittent failure.
>
> I think this may be a real failure, but it is hard to tell without more info.
>
> This patch prints out the full details of what the domain returned before
> doing the assertions.
>
> Andrew Bartlett
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> source4/torture/winbind/struct_based.c | 37 +++++++++++++++++-----------------
> 1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/source4/torture/winbind/struct_based.c b/source4/torture/winbind/struct_based.c
> index d47d068..ef27b05 100644
> --- a/source4/torture/winbind/struct_based.c
> +++ b/source4/torture/winbind/struct_based.c
> @@ -428,22 +428,6 @@ static bool torture_winbind_struct_domain_info(struct torture_context *torture)
>
> DO_STRUCT_REQ_REP(WINBINDD_DOMAIN_INFO, &req, &rep);
>
> - torture_assert_str_equal(torture,
> - rep.data.domain_info.name,
> - listd[i].netbios_name,
> - "Netbios domain name doesn't match");
> -
> - torture_assert_str_equal(torture,
> - rep.data.domain_info.alt_name,
> - listd[i].dns_name,
> - "DNS domain name doesn't match");
> -
> - sid = dom_sid_parse_talloc(torture, rep.data.domain_info.sid);
> - torture_assert(torture, sid, "Failed to parse SID");
> -
> - ok = dom_sid_equal(listd[i].sid, sid);
> - torture_assert(torture, ok, "SID's doesn't match");
> -
> if (rep.data.domain_info.primary) {
> flagstr = talloc_strdup_append(flagstr, "PR ");
> }
> @@ -462,10 +446,27 @@ static bool torture_winbind_struct_domain_info(struct torture_context *torture)
> flagstr = talloc_strdup_append(flagstr, "NA ");
> }
>
> - torture_comment(torture, "DOMAIN '%s' => '%s' [%s]\n",
> + torture_comment(torture, "DOMAIN '%s' => '%s' [%s] [%s]\n",
> rep.data.domain_info.name,
> rep.data.domain_info.alt_name,
> - flagstr);
> + flagstr,
> + rep.data.domain_info.sid);
> +
> + sid = dom_sid_parse_talloc(torture, rep.data.domain_info.sid);
> + torture_assert(torture, sid, "Failed to parse SID");
> +
> + ok = dom_sid_equal(listd[i].sid, sid);
> + torture_assert(torture, ok, "SID's doesn't match");
> +
> + torture_assert_str_equal(torture,
> + rep.data.domain_info.name,
> + listd[i].netbios_name,
> + "Netbios domain name doesn't match");
> +
> + torture_assert_str_equal(torture,
> + rep.data.domain_info.alt_name,
> + listd[i].dns_name,
> + "DNS domain name doesn't match");
> }
>
> return true;
> --
> 1.9.3
>
> >From 691ce0d1e3804ae989f578d460f974692507011c Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Sun, 5 Oct 2014 18:32:09 +1300
> Subject: [PATCH 2/2] winbindd: Do not overwrite domain list with conflicting
> info from a trusted domain
>
> This places less trust in our primary DC or trusted domain DC and refuses to update info that is conflicting
>
> This does not currently reject the connection to the DC, but only ensures it can only update missing information or to correct the case of the domain.
>
> Andrew Bartlett
>
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> ---
> source3/winbindd/winbindd_cm.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/source3/winbindd/winbindd_cm.c b/source3/winbindd/winbindd_cm.c
> index cecdc67..1df45c5 100644
> --- a/source3/winbindd/winbindd_cm.c
> +++ b/source3/winbindd/winbindd_cm.c
> @@ -2463,6 +2463,14 @@ no_dssetup:
> domain->active_directory = True;
>
> if (lsa_info->dns.name.string) {
> + if (!strequal(domain->name, lsa_info->dns.name.string)) {
> + DEBUG(0, ("set_dc_type_and_flags_connect: DC for domain %s "
> + "claimed it was a DC for domain %s, refusing to initialize\n",
> + domain->name, lsa_info->dns.name.string));
> + TALLOC_FREE(cli);
> + TALLOC_FREE(mem_ctx);
> + return;
> + }
> talloc_free(domain->name);
> domain->name = talloc_strdup(domain,
> lsa_info->dns.name.string);
> @@ -2472,6 +2480,14 @@ no_dssetup:
> }
>
> if (lsa_info->dns.dns_domain.string) {
> + if (domain->alt_name != NULL && !strequal(domain->alt_name, lsa_info->dns.dns_domain.string)) {
> + DEBUG(0, ("set_dc_type_and_flags_connect: DC for domain %s (%s) "
> + "claimed it was a DC for domain %s, refusing to initialize\n",
> + domain->alt_name, domain->name, lsa_info->dns.dns_domain.string));
> + TALLOC_FREE(cli);
> + TALLOC_FREE(mem_ctx);
> + return;
> + }
> talloc_free(domain->alt_name);
> domain->alt_name =
> talloc_strdup(domain,
> @@ -2499,6 +2515,16 @@ no_dssetup:
> }
>
> if (lsa_info->dns.sid) {
> + if (!is_null_sid(&domain->sid) && !dom_sid_equal(&domain->sid, lsa_info->dns.sid)) {
> + DEBUG(0, ("set_dc_type_and_flags_connect: DC for domain %s (%s) "
> + "claimed it was a DC for domain %s, refusing to initialize\n",
> + dom_sid_string(talloc_tos(), &domain->sid),
> + domain->name,
> + dom_sid_string(talloc_tos(), lsa_info->dns.sid)));
> + TALLOC_FREE(cli);
> + TALLOC_FREE(mem_ctx);
> + return;
> + }
> sid_copy(&domain->sid, lsa_info->dns.sid);
> }
> } else {
> @@ -2520,6 +2546,14 @@ no_dssetup:
> if (NT_STATUS_IS_OK(status) && NT_STATUS_IS_OK(result)) {
>
> if (lsa_info->account_domain.name.string) {
> + if (!strequal(domain->name, lsa_info->account_domain.name.string)) {
> + DEBUG(0, ("set_dc_type_and_flags_connect: DC for domain %s "
> + "claimed it was a DC for domain %s, refusing to initialize\n",
> + domain->name, lsa_info->account_domain.name.string));
> + TALLOC_FREE(cli);
> + TALLOC_FREE(mem_ctx);
> + return;
> + }
> talloc_free(domain->name);
> domain->name =
> talloc_strdup(domain,
> @@ -2527,6 +2561,16 @@ no_dssetup:
> }
>
> if (lsa_info->account_domain.sid) {
> + if (!is_null_sid(&domain->sid) && !dom_sid_equal(&domain->sid, lsa_info->account_domain.sid)) {
> + DEBUG(0, ("set_dc_type_and_flags_connect: DC for domain %s (%s) "
> + "claimed it was a DC for domain %s, refusing to initialize\n",
> + dom_sid_string(talloc_tos(), &domain->sid),
> + domain->name,
> + dom_sid_string(talloc_tos(), lsa_info->account_domain.sid)));
> + TALLOC_FREE(cli);
> + TALLOC_FREE(mem_ctx);
> + return;
> + }
> sid_copy(&domain->sid, lsa_info->account_domain.sid);
> }
> }
> --
> 1.9.3
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20141006/71059f79/attachment.pgp>
More information about the samba-technical
mailing list