Bug 11392 - Joining a Huawai storage fails: empty CLDAP ping answer
Joining a Huawai storage fails: empty CLDAP ping answer
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.2.2
All All
: P5 normal
: 4.7
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-08 10:14 UTC by Arvid Requate
Modified: 2017-08-04 06:59 UTC (History)
5 users (show)

See Also:


Attachments
0001-s4-dsdb-netlogon-allow-missing-ntver-in-cldap-ping.patch (1.14 KB, patch)
2015-07-08 10:14 UTC, Arvid Requate
no flags Details
0001-s4-torture-ldap-Test-netlogon-without-NtVer.patch (2.39 KB, patch)
2017-06-21 13:49 UTC, Arvid Requate
no flags Details
patches with knownfail entries (4.21 KB, patch)
2017-06-22 04:18 UTC, Andrew Bartlett
no flags Details
s4-dsdb-netlogon-allow-missing-ntver-in-cldap-ping.patch (4.35 KB, patch)
2017-06-22 11:55 UTC, Arvid Requate
no flags Details
additional patch for master (1.45 KB, patch)
2017-07-25 02:28 UTC, Andrew Bartlett
no flags Details
patch cherry-picked from master for 4.7 (6.38 KB, patch)
2017-07-31 21:57 UTC, Andrew Bartlett
garming: review+
Details
patch cherry-picked from master for 4.6 (6.38 KB, patch)
2017-07-31 22:03 UTC, Andrew Bartlett
garming: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate 2015-07-08 10:14:39 UTC
Created attachment 11241 [details]
0001-s4-dsdb-netlogon-allow-missing-ntver-in-cldap-ping.patch

Seen in the wild: A Huawai storage array performs the initial CLDAP ping without NtVer in the filter. Samba doesn't send a searchResEntry in this case, only the searchResResult (success).

[MS-ADTS] sect. 6.3.3.2 "Domain Controller Response to an LDAP Ping" specifies that in this case the DC should answer normally with a NETLOGON_SAM_LOGON_RESPONSE_NT40 structure.

The attached patch fixes the problem, verified with examples/misc/cldap.pl as well as with that Huawai device.
Comment 1 Andrew Bartlett 2015-07-08 21:35:16 UTC
In addition to this patch, the testsuite needs to be modified to cover the same case, so we don't regress here.

Once that is done, I'll be very glad to ask for a second review and get this into master.

Thanks!
Comment 2 Karolin Seeger 2016-12-06 11:25:48 UTC
ping

Is anyone working on the test?
It's a pitty that the patch doesn't get upstream...
Comment 3 Arvid Requate 2016-12-06 16:35:15 UTC
> Is anyone working on the test?

What is expected here, create a torture test for each misbehaving corner case?
Comment 4 Andrew Bartlett 2016-12-06 18:57:00 UTC
(In reply to Arvid Requate from comment #3)
Yes.
Comment 5 Andrew Bartlett 2017-01-03 19:55:14 UTC
(In reply to Andrew Bartlett from comment #4)
To be clear, I will happily accept tests that operate on the netlogon RootDSE attribute over LDAP, not CLDAP, as the handlers are combined.  

This should make it much, much easier to write tests in (say) python using our ldb bindings.
Comment 6 Arvid Requate 2017-06-21 13:49:04 UTC
Created attachment 13300 [details]
0001-s4-torture-ldap-Test-netlogon-without-NtVer.patch

The patch adds test to ldap.netlogon-udp.
Comment 7 Andrew Bartlett 2017-06-22 04:18:17 UTC
Created attachment 13302 [details]
patches with knownfail entries

Here are the two patches but with a knownfail and the test before the patch (that way we know that the patch fixes things).

However, you assert that the response in an NETLOGON_SAM_LOGON_RESPONSE_NT40 but you don't prove that, and the code looks like it would set version = 0xffffffff and so match all the bits. 

Can you extend the test to show the response structure, and set the default of *version to NETLOGON_NT_VERSION_1?

I suggest parsing the "netlogon" attribute with ndr_pull_struct_blob with 
ndr_pull_NETLOGON_SAM_LOGON_RESPONSE_NT40.  The run it against windows to be sure.

Sorry for not giving you this feedback earlier.
Comment 8 Arvid Requate 2017-06-22 11:55:58 UTC
Created attachment 13303 [details]
s4-dsdb-netlogon-allow-missing-ntver-in-cldap-ping.patch

> However, you assert that the response in an NETLOGON_SAM_LOGON_RESPONSE_NT40
> but you don't prove that, and the code looks like it would set version =
> 0xffffffff and so match all the bits. 
> 
> Can you extend the test to show the response structure, and set the default of
> *version to NETLOGON_NT_VERSION_1?
> 
> I suggest parsing the "netlogon" attribute with ndr_pull_struct_blob with
> ndr_pull_NETLOGON_SAM_LOGON_RESPONSE_NT40.  The run it against windows to
> be sure.


Yes, I had already implement that and you are right, I saw that it returned NETLOGON_NT_VERSION_5EX, not NETLOGON_NT_VERSION_1. Since my original patch was enough to satisfy that particular NAS device I preferred to be careful and not change anything more than that.

Anyway, the attached patch adjusts the behaviour to return NETLOGON_NT_VERSION_1 by default, if the CLDAP filter didn't ask for any specific NtVer. The torture test has been adjusted accordingly. I also included the selftest/knownfail.d/huawei changes recommended by you for autobuild.
Comment 9 Andrew Bartlett 2017-07-24 10:08:01 UTC
(In reply to Arvid Requate from comment #8)

Sorry for the slow reply. 

Have you run this against windows?  

I'll do that tomorrow, and if it passes then I'll push it to master. 

Thanks for your patience on this!
Comment 10 Andrew Bartlett 2017-07-25 02:28:46 UTC
Created attachment 13427 [details]
additional patch for master

I tested against Windows 2012R2, as as you note, the response is a NETLOGON_NT_VERSION_5.  We do need to match Windows here, even if the client is more relaxed about what it accepts. 

Can you confirm the client accepts the response with this additional patch?

Then we can get this into 4.7.

Thanks,
Comment 11 Arvid Requate 2017-07-25 13:15:10 UTC
I don't have access to the customer device, sorry. I guess it would accept the answer, it was unhappy because it didn't receive any searchResEntry.
Comment 12 Andrew Bartlett 2017-07-31 21:57:09 UTC
Created attachment 13438 [details]
patch cherry-picked from master for 4.7
Comment 13 Andrew Bartlett 2017-07-31 22:03:36 UTC
Created attachment 13439 [details]
patch cherry-picked from master for 4.6

Patch is also good for 4.6
Comment 14 Karolin Seeger 2017-08-01 06:11:51 UTC
(In reply to Andrew Bartlett from comment #13)
Pushed to autobuild-v4-{7,6}-test.
Comment 15 Karolin Seeger 2017-08-04 06:59:13 UTC
(In reply to Karolin Seeger from comment #14)
Pushed to both branches.
Closing out bug report.

Thanks!