Domain join fails when a large number (140) of SRV records exist for "_ldap._tcp.dc._msdcs.<DOMAIN>". Standard DNS tools ("dig", "nslookup") resolve the full SRV set correctly, but the join operation errors out. Switching the DNS backend from Samba internal DNS to "bind9-dlz" makes the issue disappear and the domain join succeeds. * Samba AD DC with **Samba internal DNS** (problem occurs) * Same setup with **bind9-dlz** (problem disappears)
I did some debugging about this and found out some explanation. Short explanation: Samba DNS server never set the truncated (TC flag) on big DNS answers and windows seems to reject (or doesn't handle) DNS UDP packets over 4192 bytes, setting a "Malformed Packet" error. It works bith bind9-dlz because bind does set the truncated flag, which allows windows to upgrade the connection to TCP and get the whole response. Bind9-dlz behavior: - Packet <= 4096 bytes: Send UDP - Packet > 4096 bytes: Send small UDP packet (< 512 bytes) with TC flag set. Client is expected to retry as TCP for full answer. I don't know if client may specify the limit (4096). When looking into the code, I think the fix should be around dns_process_recv function, which is where the dns packet is serialized in NDR format. I'd be willing to make the patch but only way I see it is by editting ndr_push_dns_name_packet, to track serialized size record after record, allowing to break at 512/4096 bytes. But this code is autogenerated from IDL, I don't know if it is possible (or expected) to add custom behavior inside these functions.
can you give the patch from bug 15536 a try?
The patch is already included in the Samba version I tested (4.22.6). It also appears to be intended for the Samba DNS client, not the server.
(In reply to Andréas Leroux from comment #1) > But this code is autogenerated from IDL, I don't know if it is possible (or expected) to add custom behavior inside these functions It is possible (but perhaps not the right thing here; I will get to that). This shows the pattern -- copy the generated code into the tree in one commit, modify in others: git log -p -n2 ee1b8ae04b10306c059174a5b4b637b080fe23fd But isn't it simpler to check the length afterwards, in the callers? In dns_udp_call_process_done() if call->out.length > threshold then you set the bit and the length. In dns_tcp_call_process_done() you don't.
(In reply to Douglas Bagnall from comment #4) > In dns_udp_call_process_done() if call->out.length > threshold then you set the bit and the length. I thought of that yes, but if we truncate roughly at the threshold, it would most certainly truncate inside an answer record and end up with an invalid DNS packet. Which is why I thought about ndr_push_dns_name_packet to keep track of the current length, record after record, and be able to truncate at the latest record ending before the threshold.
(In reply to Andréas Leroux from comment #5) Right, though we could provide an empty answer section in UDP with the truncated bit. https://blog.apnic.net/2024/02/27/dns-and-udp-truncation/ suggests that this works universally outside of one or two rogue ISPs.
Created attachment 18816 [details] dns-udp-truncation You're right, I made this patch accordingly. It does fix the initial issue of domain join with a lot of controllers. That said, I'm not entirely satisfied with the current implementation. I've hardcoded the threshold to 4096 bytes. But I've seen it should be a client setting with EDNS0 though I couldn't find it in samba codebase. If this is not handled for now we could stick to this hardcoded threshold, moving it to an explicit constant though. And then the `bool over_udp` in dns_process_recv, instead of leaving UDP specific behavior in dns_udp_call_process_done. But I didn't want to duplicate the drop behavior from dns_process_recv. I'm open to suggestions on the patch, and I'll open a gitlab PR if you're good with it.
Yeah, we don't seem to do RFC6891/EDNS0 at all. It seems like 1232 is regarded as a safe size, and 1400 as a standard almost-safe size. But if we are ignoring EDNS0, we should in theory stick to the original 512 byte threshold. So I guess the eventual logic is is there EDNS0? no -> use 512 yes -> use MIN(requested threshold, our threshold) but for now maybe we go for 1232. I suppose a lower threshold will affect a lot more domains but not many more individual requests, so we probably don't need to worry about day-to-day performance. Do you know what other workloads than join that might be affected? The patch is nice and simple. I am in two minds about whether it should go there or in dns_udp_call_process_done() but whatever. The other things are 1. /* */ comments, not // comments, and it should probably say more about what is happening. 2. maybe we should TALLOC_FREE(out->data) before regenerating the packet. It will be freed soon enough already, but we know by definition it it large-ish. 3. We want some kind of tests.
I agree with the 1232 threshold. I've applied the changes requested by your previous comment and made the MR on the gitlab https://gitlab.com/samba-team/samba/-/merge_requests/4406