pdns
pdns copied to clipboard
PowerDNS
auth udp: use stubDoResolve for ALIAS
Short description
Implements @Habbie's "alternate outcome" from #13039.
Somebody please tell me what is wrong with this :)
Checklist
I have:
- [x] read the CONTRIBUTING.md document
- [x] compiled this code
- [x] tested this code
- [ ] included documentation (including possible behaviour changes)
- [ ] documented the code
- [ ] added or modified regression test(s)
- [ ] added or modified unit test(s)
Pull Request Test Coverage Report for Build 11049952298
Details
- 25 of 54 (46.3%) changed or added relevant lines in 2 files are covered.
- 53 unchanged lines in 14 files lost coverage.
- Overall coverage increased (+0.1%) to 64.7%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| pdns/stubresolver.cc | 1 | 5 | 20.0% |
| pdns/packethandler.cc | 24 | 49 | 48.98% |
| <!-- | Total: | 25 | 54 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| pdns/auth-main.cc | 1 | 61.47% |
| pdns/packethandler.cc | 1 | 72.31% |
| modules/gpgsqlbackend/gpgsqlbackend.cc | 1 | 88.62% |
| pdns/tcpreceiver.cc | 1 | 66.92% |
| pdns/sstuff.hh | 2 | 56.83% |
| pdns/backends/gsql/gsqlbackend.hh | 2 | 96.57% |
| pdns/misc.cc | 2 | 63.66% |
| pdns/stubresolver.cc | 3 | 75.58% |
| pdns/tsigverifier.cc | 3 | 77.22% |
| pdns/axfr-retriever.cc | 3 | 67.07% |
| <!-- | Total: | 53 |
| Totals | |
|---|---|
| Change from base Build 11049514595: | 0.1% |
| Covered Lines: | 124664 |
| Relevant Lines: | 161993 |
💛 - Coveralls
this emits A+RRSIG for me, very nice. Did not verify the signature but I'm confident.
Somebody please tell me what is wrong with this :)
haven't found anything yet! We'll need tests though
Somebody please tell me what is wrong with this :)
haven't found anything yet! We'll need tests though
I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!
I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!
oh. I didn't even check if we had those :D I'll have a look, testing the DNSSEC bits would probably be nice now.
this emits A+RRSIG for me, very nice. Did not verify the signature but I'm confident.
Does that mean that this PR also solves the problem of signing ALIAS responses?
Does that mean that this PR also solves the problem of signing ALIAS responses?
sure feels that way. Please test and give us your opinion :)
I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!
the existing tests are good besides, indeed, the DNSSEC thing. We test plenty of DNSSEC in regression-tests/ but not so much in .auth-py/ currently. I'm not sure what's the easiest path is here. Perhaps just start with +DO on a query from test_ALIAS.py and see if an RRSIG appears.
I like the idea of dnssec and ALIAS.
There is however something you cannot ignore. Using stubDoResolve will make ALIAS much more sensitive to exhausting all available backends.
There are similarities wit LUA but the big difference with ALIAS is it's legacy. ALIAS is deployed in many places and is hand out to customers. Hopefully this didn't happen for LUA records ;)
Using stubDoResolve will make ALIAS much more sensitive to exhausting all available backends.
Yeah that was also my concern. But I guess one can a) get more backends and b) employ ratelimiting in front and also behind the auth.
Given that the diff to packethandler itself is quite small, I wonder if we can give users the choice between 'old' (dnsproxy) and 'new' (stubDoResolve)
I wonder if we can give users the choice
Seems possible, but code and test complexity will go up.
I can type it in, if we know we want this.
Seems possible, but code and test complexity will go up.
understood
I can type it in, if we know we want this.
if people have existing deployments where users have the ability to enter ALIASed names (which I'm sure they do), I'm afraid @mind04's concerns are relevant. I did not think about existing users enough, before.
What exactly is the problem with stubDoResolve and the Backends? Why does resolving a domain requires a backend?
What exactly is the problem with stubDoResolve and the Backends? Why does resolving a domain requires a backend?
in this PR, the resolving happens synchronously inside the distributor thread, which has a database connection open
in this PR, the resolving happens synchronously inside the distributor thread, which has a database connection open
"synchronously" sounds bad. Was the old code also synchronously or asynchronous?
"synchronously" sounds bad. Was the old code also synchronously or asynchronous?
The old code was asynchronous, which is why doing DNSSEC there was so hard it never happened.
That's bad. Having a zone with
*.example.com. ALIAS zone.with.nonresponding.nameservers.
and someone doing <random>.example.com with a few queries per second can cause a DoS attack.
Do we have the same problem already if the incoming queries use TCP?
Do we have the same problem already if the incoming queries use TCP?
No, only in the AXFR path.
How about simply caching the responses (and potentially even querying all alias targets ahead of time and on a regular basis to fill this cache)? This way, there's no DoS.
