search

nils-ohlmeier / sipsak

SIP swiss army knife
GNU General Public License v2.0
140 stars 37 forks source link

Overhaul handling of SRV DNS responses #81

Open alexdowad opened 2 years ago

alexdowad commented 2 years ago

@nils-ohlmeier, please don't merge this immediately... I am just opening this PR to allow for code review.

The new DNS test suite does (indirectly) use c-ares, but does not issue any DNS requests, except for localhost... the only function from c-ares which is used is ares_expand_name, which just operates on a buffer in memory and does not touch the network or have any other side effects. This does mean, however, that the tests can only run on a host which has c-ares installed. Probably I should add a check in check_dns.c to see if c-ares is installed.

I tried running the unit tests under Valgrind and it didn't detect any memory leaks, use of uninitialized memory, buffer overruns, etc.

When I am working on the PHP interpreter, I build it with ubsan enabled, and it has helped me to catch bugs. I notice that you are using -Wall -fstack-protector for building the tests, which is good, but I would also love to add ubsan. (-fsanitize=undefined -fno-sanitize-recover)

The behavior of CNAME is changed a bit here. In the previous code, if ca_tmpname matched the name in a CNAME record, ca_tmpname would be replaced by the value of the CNAME record... but even if it didn't match, the value of the CNAME record would still replace ca_tmpname, and if the value of the CNAME record was an IPv4 address, it would immediately be returned as the IP address to use.

Now, CNAMEs only have an effect if their name matches the value of a SRV record.

Do you think it is possible that a DNS server might first return a CNAME record and then follow it with the matching SRV record? If so, I need to modify my code to account for that possibility. Right now, it needs to see the SRV record before the CNAME, or else the CNAME will not have any effect.

alexdowad commented 2 years ago

OK, I see that the CI checks can't pass if a test suite only runs on hosts with c-ares.

Just amended that so the new test suite still runs and 'passes', but doesn't actually test anything, on hosts without c-ares.

alexdowad commented 2 years ago

Hmm, I was just thinking more about CNAMES...

I can imagine a scenario where we do a SRV lookup for a.b.c and the DNS server responds with just a CNAME redirecting us from a.b.c to something.else. If that happens, would the right thing to do be: 1) do a recursive SRV lookup for something.else, or 2) do an A/AAAA lookup for something.else and use the resulting IP address?

nils-ohlmeier commented 2 years ago

I can imagine a scenario where we do a SRV lookup for a.b.c and the DNS server responds with just a CNAME redirecting us from a.b.c to something.else. If that happens, would the right thing to do be: 1) do a recursive SRV lookup for something.else, or 2) do an A/AAAA lookup for something.else and use the resulting IP address?

Good question. I'm not a DNS expert. But I don't think that CNAMES can redirect to anything else then A/AAAA.